Re: [PATCH v1 06/14] xen/arm: Add support for PCI ecam operations

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 15 Sep 2021, Rahul Singh wrote:
> > On 15 Sep 2021, at 12:06 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> > wrote:
> > On Tue, 14 Sep 2021, Rahul Singh wrote:
> >>>> +        return NULL;
> >>>> +
> >>>> +    busn -= cfg->busn_start;
> >>>> +    base = cfg->win + (busn << cfg->ops->bus_shift);
> >>>> +
> >>>> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + 
> >>>> where;
> >>>> +}
> >>> 
> >>> I understand that the arm32 part is not implemented and not part of this
> >>> series, that's fine. However if the plan is that arm32 will dynamically
> >>> map each bus individually, then I imagine this function will have an
> >>> ioremap in the arm32 version. Which means that we also need an
> >>> unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
> >>> would be a NOP today for arm64, but I think it makes sense to have it if
> >>> we want the API to be generic.
> >> 
> >> As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t 
> >> see any use case to unmap the 
> >> bus dynamically. We can add the support for per_bus_mapping for ARM32 once 
> >> we implement arm32 part.
> >> Let me know your view on this.
> > 
> > In the patch titled "xen/arm: PCI host bridge discovery within XEN on
> > ARM" there is the following in-code comment:
> > 
> > * On 64-bit systems, we do a single ioremap for the whole config space
> > * since we have enough virtual address range available.  On 32-bit, we
> > * ioremap the config space for each bus individually.
> > *
> > * As of now only 64-bit is supported 32-bit is not supported.
> > 
> > 
> > So I take it that on arm32 we don't have enough virtual address range
> > available, therefore we cannot ioremap the whole range. Instead, we'll
> > have to ioremap the config space of each bus individually.
> 
> Yes you are right my understand is also same.
> > 
> > I assumed that the idea was to call ioremap and iounmap dynamically,
> > otherwise the total amount of virtual address range required would still
> > be the same.
> 
> As per my understanding for 32-bit we need per_bus mapping as we don’t have 
> enough virtual address space in one chunk
> but we can have virtual address space in different chunk.

Interesting. I would have assumed that the sum of all the individual
smaller ioremaps would still be equal to one big ioremap. Maybe for
Linux is different, but I don't think that many smaller ioremaps would
buy us very much in Xen because it is the total ioremap virtual space
that is too small. Or am I missing something?


> I am not sure if we need to map/unmap the virtual address space for each 
> read/write call. 
> I just checked the Linux code[1]  and there also mapping is done once not for 
> each read/write call.

So my guess is that for arm32 we would have to resort to dynamic
map/unmap for each read/write call, unless there is a trick with the
individual smaller ioremaps that I haven't spotted (e.g. maybe something
doesn't get mapped that way?)

That said, given that we are uncertain about this and the arm32
implementation is nowhere close, I think that we are OK to continue like
this for this series. Maybe you could add a couple of sentences to the
in-code comment so that if somebody wants to jump in and implement
arm32 support they would know where to start.