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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Julien!

On 23.11.21 18:12, Julien Grall wrote:
>
>
> 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.
While I do agree that MSI part and virtual bus topology are not belonging to 
this
patch I can't agree with the rest: we already have MMIO handlers for guest 
domains
and we introduce domain_vpci_get_num_mmio_handlers which must also account
on guests and stay consistent.
So, despite the patch has "hardware domain" in its name it doesn't mean we 
should
break guests here.
Thus I do think the above is still correct wrt this patch.
>
> Cheers,
>
Thank you,
Oleksandr