On Wed, 15 Sep 2021, Rahul Singh wrote: > > On 15 Sep 2021, at 12:06 am, Stefano Stabellini <sstabell...@kernel.org> > > 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.
