Re: [PATCH RFC v2] vPCI: account for hidden devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.05.2023 10:08, Roger Pau Monné wrote:
> On Thu, May 25, 2023 at 05:30:54PM +0200, Jan Beulich wrote:
>> On 25.05.2023 17:02, Roger Pau Monné wrote:
>>> On Thu, May 25, 2023 at 04:39:51PM +0200, Jan Beulich wrote:
>>>> On 24.05.2023 17:56, Roger Pau Monné wrote:
>>>>> On Wed, May 24, 2023 at 03:45:58PM +0200, Jan Beulich wrote:
>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>> @@ -218,6 +218,7 @@ static int modify_bars(const struct pci_
>>>>>>      struct vpci_header *header = &pdev->vpci->header;
>>>>>>      struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>>>>      struct pci_dev *tmp, *dev = NULL;
>>>>>> +    const struct domain *d;
>>>>>>      const struct vpci_msix *msix = pdev->vpci->msix;
>>>>>>      unsigned int i;
>>>>>>      int rc;
>>>>>> @@ -285,9 +286,11 @@ static int modify_bars(const struct pci_
>>>>>>  
>>>>>>      /*
>>>>>>       * Check for overlaps with other BARs. Note that only BARs that are
>>>>>> -     * currently mapped (enabled) are checked for overlaps.
>>>>>> +     * currently mapped (enabled) are checked for overlaps. Note also 
>>>>>> that
>>>>>> +     * for Dom0 we also need to include hidden, i.e. DomXEN's, devices.
>>>>>>       */
>>>>>> -    for_each_pdev ( pdev->domain, tmp )
>>>>>> +for ( d = pdev->domain; ; d = dom_xen ) {//todo
>>>>>
>>>>> Looking at this again, I think this is slightly more complex, as during
>>>>> runtime dom0 will get here with pdev->domain == hardware_domain OR
>>>>> dom_xen, and hence you also need to account that devices that have
>>>>> pdev->domain == dom_xen need to iterate over devices that belong to
>>>>> the hardware_domain, ie:
>>>>>
>>>>> for ( d = pdev->domain; ;
>>>>>       d = (pdev->domain == dom_xen) ? hardware_domain : dom_xen )
>>>>
>>>> Right, something along these lines. To keep loop continuation expression
>>>> and exit condition simple, I'll probably prefer
>>>>
>>>> for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain;
>>>>       ; d = dom_xen )
>>>
>>> LGTM.  I would add parentheses around the pdev->domain != dom_xen
>>> condition, but that's just my personal taste.
>>>
>>> We might want to add an
>>>
>>> ASSERT(pdev->domain == hardware_domain || pdev->domain == dom_xen);
>>>
>>> here, just to remind that this chunk must be revisited when adding
>>> domU support (but you can also argue we haven't done this elsewhere),
>>> I just feel here it's not so obvious we don't want do to this for
>>> domUs.
>>
>> I could add such an assertion, if only ...
>>
>>>>> And we likely want to limit this to devices that belong to the
>>>>> hardware_domain or to dom_xen (in preparation for vPCI being used for
>>>>> domUs).
>>>>
>>>> I'm afraid I don't understand this remark, though.
>>>
>>> This was looking forward to domU support, so that you already cater
>>> for pdev->domain not being hardware_domain or dom_xen, but we might
>>> want to leave that for later, when domU support is actually
>>> introduced.
>>
>> ... I understood why this checking doesn't apply to DomU-s as well,
>> in your opinion.
> 
> It's my understanding that domUs can never get hidden or read-only
> devices assigned, and hence there no need to check for overlap with
> devices assigned to dom_xen, as those cannot have any BARs mapped in
> a domU physmap.
> 
> So for domUs the overlap check only needs to be performed against
> devices assigned to pdev->domain.

I fully agree, but the assertion you suggested doesn't express that. Or
maybe I'm misunderstanding what you did suggest, and there was an
implication of some further if() around it.

Jan