[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 30 May 2023 13:04:16 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=6mpXyUmSdrJG402BKaiJCtmB5duRnj++5v+WGT5Kx4I=; b=hmz+h1muIRzHNctBb3BFH1neKVl/7d5EuUetLv711My63r096k58OjefcZE2tpbx5KbyHvbM4+VQ89qhA35G2EJFMnHYKXTfzjcfmI+0kM11BLMf0rOmNCME0tTmTGcEIGcchQZjXK6V/loChbReyXcMOZkFtYDUm/cRzdhBMg7PPkba2Tm5p3MHrni3WV6XrnMedXsPtZitNeI8NJRFs23asFC8d6qoRYV/Ur5alw+5i/k87bEKqzaMnPOsXTGYm2OGYmpwRM/bK+1WwpSf67UaB9mQoUq6g9kppZpmZS0hDtJh3czGOScCpH8G35u4JtKS+dfFEJz5SdZrxhM/QQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Kcq0hqTH1Wlkr3/7lY+yDlfVv7YNlVhfrHbiJCqNWWccav3uB5iIQTf4n4grNf4qQmp0YTWL1vfia7c0g2Cr4aHsgyqcukCAWZWI/B1yJPi8sagOUkzsiziuNqkd9ebQerl1WL3yk6U/rzVUkRpTEKfbXQET78HNwJiI/J8HHIhrzXwO6Dy2Hy0ZXrRrkMuBiSozpbtrtRsfwoVzQJ90VsM4jfppHZUYj8kguMBUlc1STKdhlG/DU7ba9SCAGeZLcGnVzJ1Pym1wxOvn1RjtVAvkt13GS0TUS7Nvb2dv7qb79EUd4BPUPb+VqouHqfGYerFtlLoOyQX2wPGEfjtHBg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • Delivery-date: Tue, 30 May 2023 11:04:51 +0000
  • Ironport-data: A9a23:ZuE4O6uDzO5woBzNFKkRBOKAgefnVHxfMUV32f8akzHdYApBsoF/q tZmKW+OO/yOamL9f4wjOo++90IH6J7Qy9E2GgJoqn1kHylE+JbJXdiXEBz9bniYRiHhoOCLz O1FM4Wdc5pkJpP4jk3wWlQ0hSAkjclkfpKlVKiffHg3HVQ+IMsYoUoLs/YjhYJ1isSODQqIu Nfjy+XSI1bg0DNvWo4uw/vrRChH4rKq4Fv0gnRkPaoQ5AKFzyFPZH4iDfrZw0XQE9E88tGSH 44v/JnhlkvF8hEkDM+Sk7qTWiXmlZaLYGBiIlIPM0STqkAqSh4ai87XB9JFAatjsB2bnsgZ9 Tl4ncfYpTHFnEH7sL91vxFwS0mSNEDdkVPNCSDXXce7lyUqf5ZwqhnH4Y5f0YAwo45K7W9yG fMwbz4gRAyul72Nh+y0V9M1qPZkLZnGBdZK0p1g5Wmx4fcOZ7nmGv2PyfoGmTA6i4ZJAOrUY NcfZXx3dhPcbhZTO1ARTpUjgOOvgXq5eDpdwL6XjfNvvy6Pk0osgP60aIG9lt+iHK25mm6Co W3L5SLhCwwyP92D0zuVtHmrg4cjmAuiAdtJTeXjq6ICbFu7+HA5JRBNTXaCi6er1FOyWNt1K 349w397xUQ13AnxJjXnZDW6qnOZuh8XW/JLDvY3rgqKz8L88wufQ2QJUDNFQNgnr9MtAywn0 EeTmNHkDiApt6eaIVqG7audpz62PSkTLEcBaDUCQA9D5MPsyLzflTrKR9dnVaKw0Nv8HGiox yjQ9XBlwbIOkcQMyqO3u0jdhC6hrYTISQhz4RjLWmWi7UVyY4vNi5GU1GU3JM1odO6xJmRtd lBd8yRCxIji1a2wqRE=
  • Ironport-hdrordr: A9a23:hS0IPa78UNSJEMcvfQPXwVyBI+orL9Y04lQ7vn2ZFiY5TiXIra qTdaogviMc0AxhIE3Jmbi7WJVoMkmsjqKdgLNhdItKMzOW3FdAQLsN0WKm+UyYJ8SczJ8U6U 4DSdkYNDSYNzET4anHCUuDYrAdKbK8gcOVbJLlvhJQpHZRGsNdBmlCazqzIwlTfk1rFJA5HJ 2T6o5svDy7Y0kaacy9Gz0sQ/XDj8ejruOtXTc2QzocrCWehzKh77D3VzKC2A0Fbj9JybA+tU DYjg3C4Lm5uf3T8G6Q64aT1eUbpDLS8KoMOCW+sLlVFtwqsHfpWG1VYczMgNnympDt1L9lqq iPn/5qBbUI15qYRBDJnfKq4Xiq7N9m0Q6f9XaIxXTkusD3XzQ8Fo5Igp9YaALQ7w46sMh7y7 8j5RPsi3N7N2KzoM3G3am8azh60k6v5XYym+8aiHJSFYMYdb9KtIQauEdYCo0JEi724J0uVL AGNrCr2N9GNVeBK3zJtGhmx9KhGnw1AxedW0AH/siYySJfknx1x1YRgMYfgnAD/pQgTIQs3Z WyDo140LVVCsMGZ6N0A+kMBcOxF2zWWBrJdHmfJFz2fZt3SE4la6SHkIndyNvaCaDglqFC56 gpeGkoy1IPRw==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.