[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH RFC v2] vPCI: account for hidden devices
On Tue, May 30, 2023 at 11:44:52AM +0200, Jan Beulich wrote: > 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. Oh, right, and that would also exit the loop on the first iteration if the device is assigned to a domU, so it's all fine. Sorry for the noise then. Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |