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

Re: [Xen-devel] [v8][PATCH 04/17] update the existing hypercall to support XEN_DOMCTL_set_rdm



On 2014/12/4 23:50, Jan Beulich wrote:
On 01.12.14 at 10:24, <tiejun.chen@xxxxxxxxx> wrote:
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -22,27 +22,66 @@ struct get_reserved_device_memory {
      unsigned int used_entries;
  };

-static int get_reserved_device_memory(xen_pfn_t start,
-                                      xen_ulong_t nr, void *ctxt)
+static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
+                                      u32 id, void *ctxt)
  {
      struct get_reserved_device_memory *grdm = ctxt;
+    struct domain *d;
+    unsigned int i;
+    u32 sbdf;
+    struct compat_reserved_device_memory rdm = {
+        .start_pfn = start, .nr_pages = nr
+    };

-    if ( grdm->used_entries < grdm->map.nr_entries )
-    {
-        struct compat_reserved_device_memory rdm = {
-            .start_pfn = start, .nr_pages = nr
-        };
+    if ( rdm.start_pfn != start || rdm.nr_pages != nr )
+        return -ERANGE;

-        if ( rdm.start_pfn != start || rdm.nr_pages != nr )
-            return -ERANGE;
+    d = rcu_lock_domain_by_any_id(grdm->map.domid);
+    if ( d == NULL )
+        return -ESRCH;

So why are you doing this in the call back (potentially many times)
instead of just once in compat_memory_op(), storing the pointer in
the context structure?

Okay.



-        if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries,
-                                     &rdm, 1) )
-            return -EFAULT;
+    if ( d )
+    {
+        if ( d->arch.hvm_domain.pci_force )

You didn't verify that the domain is a HVM/PVH one.

Is this, ASSERT(is_hvm_domain(grdm.domain)), correct?


+        {
+            if ( grdm->used_entries < grdm->map.nr_entries )
+            {
+                if ( __copy_to_compat_offset(grdm->map.buffer,
+                                             grdm->used_entries,
+                                             &rdm, 1) )
+                {
+                    rcu_unlock_domain(d);
+                    return -EFAULT;
+                }
+            }
+            ++grdm->used_entries;
+        }
+        else
+        {
+            for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ )
+            {
+                sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[i].seg,
+                                 d->arch.hvm_domain.pcidevs[i].bus,
+                                 d->arch.hvm_domain.pcidevs[i].devfn);
+                if ( sbdf == id )
+                {
+                    if ( grdm->used_entries < grdm->map.nr_entries )
+                    {
+                        if ( __copy_to_compat_offset(grdm->map.buffer,
+                                                     grdm->used_entries,
+                                                     &rdm, 1) )
+                        {
+                            rcu_unlock_domain(d);
+                            return -EFAULT;
+                        }
+                    }
+                    ++grdm->used_entries;

break;

Added.


Also trying to fold code identical on the if and else branches would
seem pretty desirable.

Sorry, I can't see what I'm missing.


@@ -319,9 +358,13 @@ int compat_memory_op(unsigned int cmd, 
XEN_GUEST_HANDLE_PARAM(void) compat)

              if ( !rc && grdm.map.nr_entries < grdm.used_entries )
                  rc = -ENOBUFS;
+
              grdm.map.nr_entries = grdm.used_entries;
-            if ( __copy_to_guest(compat, &grdm.map, 1) )
-                rc = -EFAULT;
+            if ( grdm.map.nr_entries )
+            {
+                if ( __copy_to_guest(compat, &grdm.map, 1) )
+                    rc = -EFAULT;
+            }

Why do you need this change?

If we have no any entries, why do we still copy that?


--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -904,17 +904,33 @@ int platform_supports_x2apic(void)

  int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
  {
-    struct acpi_rmrr_unit *rmrr;
+    struct acpi_rmrr_unit *rmrr, *rmrr_cur = NULL;
      int rc = 0;
+    unsigned int i;
+    u16 bdf;

-    list_for_each_entry(rmrr, &acpi_rmrr_units, list)
+    for_each_rmrr_device ( rmrr, bdf, i )
      {
-        rc = func(PFN_DOWN(rmrr->base_address),
-                  PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address),
-                  ctxt);
-        if ( rc )
-            break;
+        if ( rmrr != rmrr_cur )
+        {
+            rc = func(PFN_DOWN(rmrr->base_address),
+                      PFN_UP(rmrr->end_address) -
+                        PFN_DOWN(rmrr->base_address),
+                      PCI_SBDF(rmrr->segment, bdf),
+                      ctxt);
+
+            if ( unlikely(rc < 0) )
+                return rc;
+
+            /* Just go next. */
+            if ( !rc )
+                rmrr_cur = rmrr;
+
+            /* Now just return specific to user requirement. */
+            if ( rc > 0 )
+                return rc;

Nice that you check for that, but I can't see this case occurring
anymore. Did you lose some code? Also please don't write code

We have three scenarios here:

#1 rc < 0 means this is an error;
#2 rc = 0 means the tools caller don't know how many buffers it should construct, so we need to count all entries as 'nr_entries' to return. #3 rc > 0 means in some cases, we need to return some specific values, like 1 to indicate we're hitting some RMRR range. Currently, we use gfn to check this in case of memory populating, ept violation handler and mem_access.

more complicated than necessary. The above two if()s could be


+            if ( rc > 0 )
+                return rc;
+
+            rmrr_cur = rmrr;

--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -586,6 +586,11 @@ typedef struct xen_reserved_device_memory
xen_reserved_device_memory_t;
  DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t);

  struct xen_reserved_device_memory_map {
+    /*
+     * Domain whose reservation is being changed.
+     * Unprivileged domains can specify only DOMID_SELF.
+     */
+    domid_t        domid;
      /* IN/OUT */
      unsigned int nr_entries;
      /* OUT */

Your addition lacks an IN annotation.

Are you saying something for 'nr_entries'? But I didn't introduce anything to change the original usage. Anyway, I try to improve this,

    /*
     * IN: on call the number of entries which can be stored in buffer.
     * OUT: on return the number of entries which have been stored in
     * buffer. If on call the number is less the number of all necessary
     * entries, on return the number of entries which is needed.
     */



--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -31,6 +31,8 @@
  #define PCI_DEVFN2(bdf) ((bdf) & 0xff)
  #define PCI_BDF(b,d,f)  ((((b) & 0xff) << 8) | PCI_DEVFN(d,f))
  #define PCI_BDF2(b,df)  ((((b) & 0xff) << 8) | ((df) & 0xff))
+#define PCI_SBDF(s,bdf) (((s & 0xffff) << 16) | (bdf & 0xffff))
+#define PCI_SBDF2(s,b,df) (((s & 0xffff) << 16) | PCI_BDF2(b,df))

Missing several parentheses.


Okay,

#define PCI_SBDF(s,bdf) ((((s) & 0xffff) << 16) | ((bdf) & 0xffff))
#define PCI_SBDF2(s,b,df) ((((s) & 0xffff) << 16) | PCI_BDF2(b,df))


Thanks
Tiejun

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


 


Rackspace

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