Re: [PATCH 1/2] PCI: endpoint: Introduce 'get_bar' to map fixed address BARs in EPC

[Manivannan Sadhasivam]

On Thu, Jul 25, 2024 at 05:20:00PM +0900, Damien Le Moal wrote:
> On 7/25/24 17:06, Rick Wertenbroek wrote:
> > On Thu, Jul 25, 2024 at 7:33 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@xxxxxxxxxx> wrote:
> >>
> >> On Tue, Jul 23, 2024 at 06:48:27PM +0200, Niklas Cassel wrote:
> >>> Hello Rick,
> >>>
> >>> On Tue, Jul 23, 2024 at 06:06:26PM +0200, Rick Wertenbroek wrote:
> >>>>>
> >>>>> But like you suggested in the other mail, the right thing is to merge
> >>>>> alloc_space() and set_bar() anyway. (Basically instead of where EPF drivers
> >>>>> currently call set_bar(), the should call alloc_and_set_bar() (or whatever)
> >>>>> instead.)
> >>>>>
> >>>>
> >>>> Yes, if we merge both, the code will need to be in the EPC code
> >>>> (because of the set_bar), and then the pci_epf_alloc_space (if needed)
> >>>> would be called internally in the EPC code and not in the endpoint
> >>>> function code.
> >>>>
> >>>> The only downside, as I said in my other mail, is the very niche case
> >>>> where the contents of a BAR should be moved and remain unchanged when
> >>>> rebinding a given endpoint function from one controller to another.
> >>>> But this is not expected in any endpoint function currently, and with
> >>>> the new changes, the endpoint could simply copy the BAR contents to a
> >>>> local buffer and then set the contents in the BAR of the new
> >>>> controller.
> >>>> Anyways, probably no one is moving live functions between controllers,
> >>>> and if needed it still can be done, so no problem here...
> >>>
> >>> I think we need to wait for Mani's opinion, but I've never heard of anyone
> >>> doing so, and I agree with your suggested feature to copy the BAR contents
> >>> in case anyone actually changes the backing EPC controller to an EPF.
> >>> (You would need to stop the EPC to unbind + bind anyway, IIRC.)
> >>>
> >>
> >> Hi Rick/Niklas,
> >>
> >> TBH, I don't think currently we have an usecase for remapping the EPC to EPF. So
> >> we do not need to worry until the actual requirement comes.
> >>
> >> But I really like combining alloc() and set_bar() APIs. Something I wanted to do
> >> for so long but never got around to it. We can use the API name something like
> >> pci_epc_alloc_set_bar(). I don't like 'set' in the name, but I guess we need to
> >> have it to align with existing APIs.
> >>
> >> And regarding the implementation, the use of fixed address for BAR is not new.
> >> If you look into the pci-epf-mhi.c driver, you can see that I use a fixed BAR0
> >> location that is derived from the controller DT node (MMIO region).
> >>
> >> But I was thinking of moving this region to EPF node once we add DT support for
> >> EPF driver. Because, there can be many EPFs in a single endpoint and each can
> >> have upto 6 BARs. We cannot really describe each resource in the controller DT
> >> node.
> >>
> >> Given that you really have a usecase now for multiple BARs, I think it is best
> >> if we can add DT support for the EPF drivers and describe the BAR resources in
> >> each EPF node. With that, we can hide all the resource allocation within the EPC
> >> core without exposing any flag to the EPF drivers.
> >>
> >> So if the EPF node has a fixed location for BAR and defined in DT, then the new
> >> API pci_epf_alloc_set_bar() API will use that address and configure it
> >> accordingly. If not, it will just call pci_epf_alloc_space() internally to
> >> allocate the memory from coherent region and use it.
> >>
> >> Wdyt?
> >>
> >> - Mani
> >>
> >> --
> >> மணிவண்ணன் சதாசிவம்
> > 
> > Hello Mani, thank you for your feedback.
> > 
> > I don't know if the EPF node in the DT is the right place for the
> > following reasons. First, it describes the requirements of the EPF and
> > not the restrictions imposed by the EPC (so for example one function
> > requires the BAR to use a given physical address and this is described
> > in the DT EPF node, but the controller could use any address, it just
> > configures the controller to use the address the EPF wants, but in the
> > other case (e.g., on FPGA), the EPC can only handle a BAR at a given
> > physical address and no other address so this should be in the EPC
> > node). Second, it is very static, so the EPC relation EPF would be
> > bound in the DT, I prefer being able to bind-unbind from configfs so
> > that I can make changes without reboot (e.g., alternate between two or
> > more endpoint functions, which may have different BAR requirements and
> > configurations).
> > 
> > For the EPFs I really think it is good to keep the BAR requirements in
> > the code for the moment, this makes it easier to swap endpoint
> > functions and makes development easier as well (e.g., binding several
> > different EPFs without reboot of the SoC they run on. In my typical
> > tests I bind one function, turn-on the host, do tests, turn-off the
> > host, make changes to an EPF, scp the new .ko on the SoC, rebind,
> > turn-on the host, etc.). For example, I alternate between pci-epf-test
> > (6 bars) and pci-epf-nvme (1 bar) of different sizes.
> > 
> > However, I can see a world where both binding and configuring EPF from
> > DT and the way it is currently done (alloc/set bar in code) and bind
> > in configfs could exist together as each have their use cases. But
> > forcing the use of DT seems impractical.
> 
> I do not think using DT for configuring an EPF *ever* make sense. An init script
> on the EP platform can take care of whatever needs to be configured. DT is for
> and should be restricted to describing the HW, not things that can be configured
> at runtime using the HW information.
> 

No, MHI is a hardware entity. So atleast defining it in DT make sense. But as I
mentioned in reply to Rick, I haven't implemented it since it is really not
required now (MHI just needs a single BAR and right now the address is derived
from EPC node).

But for Rick's usecase, I agree with you/Rick that DT doesn't make sense. Thanks
for clearing it up.

> Doing it this way also makes the EPF code independent on the platform. E.g. if
> we ever add an EPF node in the DT, that same EPF would not work on an endpoint
> capable platform using UEFI. That is not acceptable for HW generic EPFs (e.g.
> nvme, virtio, etc).
> 

Well, those anyway cannot be defined in DT as they are software implementations
around hw.

- Mani

-- 
மணிவண்ணன் சதாசிவம்