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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

> On 15 Sep 2021, at 9:45 pm, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> 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.

I am ok with both ways adding comment in code to explain or implement the 
pci_ecam_add_bus(..) and pci_ecam_remove_bus() like Linux [1] and 
we can call those function in pci_read()/pci_write.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/ecam.c#n126


Regards,
Rahul