[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.