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

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

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;
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_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

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.


Xen-devel mailing list



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