Re: [PATCH v6 3/7] xen/arm: setup MMIO range trap handlers for hardware domain

[Julien Grall <julien@xen.org>]

On 23/11/2021 06:58, Oleksandr Andrushchenko wrote:
> Hi, Julien!

Hi Oleksandr,

> On 22.11.21 19:36, Julien Grall wrote:
>> On 18/11/2021 10:46, Oleksandr Andrushchenko wrote:
>>> On 18.11.21 09:27, Oleksandr Andrushchenko wrote:
>>>>>> +    unsigned int count;
>>>>>> +
>>>>>> +    if ( is_hardware_domain(d) )
>>>>>> +        /* For each PCI host bridge's configuration space. */
>>>>>> +        count = pci_host_get_num_bridges();
>>>>> This first part makes sense to me. But...
>>>>>
>>>>>> +    else
>>>>> ... I don't understand how the else is related to this commit. Can you clarify it?
>>>>>
>>>>>> +        /*
>>>>>> +         * There's a single MSI-X MMIO handler that deals with both PBA
>>>>>> +         * and MSI-X tables per each PCI device being passed through.
>>>>>> +         * Maximum number of supported devices is 32 as virtual bus
>>>>>> +         * topology emulates the devices as embedded endpoints.
>>>>>> +         * +1 for a single emulated host bridge's configuration space.
>>>>>> +         */
>>>>>> +        count = 1;
>>>>>> +#ifdef CONFIG_HAS_PCI_MSI
>>>>>> +        count += 32;
>>>>> Surely, this is a decision that is based on other factor in the vPCI code. So can use a define and avoid hardcoding the number?
>>>> Well, in the later series [1] this is defined via PCI_SLOT(~0) + 1 and there is no dedicated
>>>> constant for that. I can use the same here, e.g. s/32/PCI_SLOT(~0) + 1
>>
>> I would prefer if we introduce a new constant for that. This makes easier to update the code if we decide to increase the number of virtual devices.
>>
>> However, I am still not sure how the 'else' part is related to this commit. Can you please clarify it?
> Well, yes, this is too early for this patch to introduce some future knowledge, so I'll instead have:
> 
> unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
> {
>       if ( !has_vpci(d) )
>           return 0;
> 
>       if ( is_hardware_domain(d) )
>       {
>           int ret = pci_host_iterate_bridges_and_count(d, vpci_get_num_handlers_cb);
> 
>           return ret < 0 ? 0 : ret;
>       }
> 
>       /*
>        * This is a guest domain:
>        *
>        * 1 for a single emulated host bridge's configuration space.
>        */
>       return 1;

I am afraid that my question stands even with this approach. This patch 
is only meant to handle the hardware domain, therefore the change seems 
to be out of context.

I would prefer if this change is done separately.

Cheers,

-- 
Julien Grall