Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

> Thanks for this; a few more comments...
Thanks for your time.

> > @@ -1577,9 +1578,15 @@ int iommu_do_pci_domctl(
> >           seg = machine_sbdf >> 16;
> >           bus = PCI_BUS(machine_sbdf);
> >           devfn = PCI_DEVFN2(machine_sbdf);
> > +        flag = domctl->u.assign_device.flag;
> > +        if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED )
> This is not a blocker, but a stylistic comment: I would have inverted
> the bitmask here, as that's conceptually what you're checking.  I
> won't make this a blocker for going in.
What about this?

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 6e23fc6..17a4206 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1579,7 +1579,7 @@ int iommu_do_pci_domctl(
         bus = PCI_BUS(machine_sbdf);
         devfn = PCI_DEVFN2(machine_sbdf);
         flag = domctl->u.assign_device.flag;
-        if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED )
+        if ( flag & ~XEN_DOMCTL_DEV_RDM_MASK )
         {
             ret = -EINVAL;
             break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index bca25c9..07549a4 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -480,6 +480,7 @@ struct xen_domctl_assign_device {
     } u;
     /* IN */
 #define XEN_DOMCTL_DEV_RDM_RELAXED      1
+#define XEN_DOMCTL_DEV_RDM_MASK         0x1
     uint32_t  flag;   /* flag of assigned device */
 };
 typedef struct xen_domctl_assign_device xen_domctl_assign_device_t;

> > @@ -1898,7 +1899,14 @@ static int intel_iommu_add_device(u8 devfn, struct 
> > pci_dev *pdev)
> >                PCI_BUS(bdf) == pdev->bus &&
> >                PCI_DEVFN2(bdf) == devfn )
> >           {
> > -            ret = rmrr_identity_mapping(pdev->domain, 1, rmrr);
> > +            /*
> > +             * Here means we're add a device to the hardware domain
> > +             * so actually RMRR is always reserved on e820 so either
> > +             * of flag is fine for hardware domain and here we'd like
> > +             * to pass XEN_DOMCTL_DEV_RDM_RELAXED.
> > +             */
> Sorry I didn't give feedback on your comment before.  I don't find it
> clear enough; I'd suggest something like this:
>
> "iommu_add_device() is only called for the hardware domain (see
> xen/drivers/passthrough/pci.c:pci_add_device()).  Since RMRRs are
> always reserved in the e820 map for the hardware domain, there
> shouldn't be a conflict."
Loos good and thanks.

> I also said that if we went with anything other than STRICT that we'd
> need to check to make sure that the domain really was the hardware
> domain before proceeding, in case the assumption that pdev->domain ==
> hardware_domain ever changed.  (Perhaps with an ASSERT -- Jan, what do
> you think?)
Sounds reasonable.

> Also, passing in RELAXED in locations where the flag is completely
> ignored (such as when removing mappings) doesn't really make any
> sense.
>
> On the whole I think it would be better if you removed the RELAXED
> flag for both removals and for hardware domains.
Just as I said in another email I agreed your STRICT way.

Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel