On 30.05.2023 11:12, Roger Pau Monné wrote:
> On Tue, May 30, 2023 at 10:45:09AM +0200, Jan Beulich wrote:
>> 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.
>
> Maybe I'm getting myself confused, but if you add something like:
>
> for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain;
> ; d = dom_xen )
>
> Such loop would need to be avoided for domUs, so my suggestion was to
> add the assert in order to remind us that the loop would need
> adjusting if we ever add domU support. But maybe you had already
> plans to restrict the loop to dom0 only.
Not really, no, but at the bottom of the loop I also have
if ( !is_hardware_domain(d) )
break;
}
(still mis-formatted in the v2 patch). I.e. restricting to Dom0 goes
only as far as the 2nd loop iteration.
Jan
