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

Re: [Xen-devel] [PATCH v3 3/5] xen, common: add the XEN_DOMCTL_memory_mapping hypercall



Hello Arianna,

Thank for the patch.

On 15/03/14 20:11, Arianna Avanzini wrote:
diff --git a/xen/arch/arm/cpu.c b/xen/arch/arm/cpu.c
index afc564f..28f2611 100644
--- a/xen/arch/arm/cpu.c
+++ b/xen/arch/arm/cpu.c
@@ -17,6 +17,8 @@

  #include <asm/processor.h>

+unsigned int paddr_bits __read_mostly = 40;
+

I would prefer a define for paddr_bits on ARM rather than creating a new variable. Something like #define paddr_bits PADDR_BITS in asm-arm/p2m.h

Furthermore, you should not hardcode 40, you must use PADDR_BITS.

[..]

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 7cf610a..b18fd46 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -818,6 +818,75 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)

[..]

+                if ( ret )
+                {
+                    printk(XENLOG_G_WARNING
+                           "memory_map: fail: dom%d gfn=%lx mfn=%lx\n",
+                           d->domain_id, gfn, mfn);
+                    if ( iomem_deny_access(d, mfn, mfn_end - 1) &&
+                         is_hardware_domain(current->domain) )
+                        printk(XENLOG_ERR
+                               "memory_map: failed to deny dom%d access "
+                               "to [%lx,%lx]\n",
+                               d->domain_id, mfn, mfn_end - 1);
+                }
+            }
+        }
+       else

Hard tab here. You should use space.

+        {
+            printk(XENLOG_G_INFO
+                   "memory_map: remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
+                   d->domain_id, gfn, mfn, nr_mfns);
+
+            add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
+            ret = iomem_deny_access(d, mfn, mfn_end - 1);
+            if ( ret && add )
+                ret = -EIO;
+            if ( ret && is_hardware_domain(current->domain) )
+                printk(XENLOG_ERR
+                       "memory_map: error %ld %s dom%d access to [%lx,%lx]\n",
+                       ret, add ? "removing" : "denying", d->domain_id,
+                       mfn, mfn_end - 1);
+       }
+    }
+    break;
+
      case XEN_DOMCTL_settimeoffset:
      {
          domain_set_time_offset(d, op->u.settimeoffset.time_offset_seconds);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 3b39c45..00e3bba 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -88,6 +88,10 @@ int p2m_populate_ram(struct domain *d, paddr_t start, 
paddr_t end);
   * address maddr. */
  int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
                       paddr_t end_gaddr, paddr_t maddr);
+int unmap_mmio_regions(struct domain *d,
+                       unsigned long start_gfn,
+                       unsigned long end_gfn,
+                       unsigned long mfn);

  int guest_physmap_add_entry(struct domain *d,
                              unsigned long gfn,
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 06e638f..4869a75 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -119,6 +119,8 @@

  #include <xen/types.h>

+extern unsigned int paddr_bits;
+
  struct cpuinfo_arm {
      union {
          uint32_t bits;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index f4e7253..91ef110 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -657,6 +657,16 @@ void p2m_flush(struct vcpu *v, struct p2m_domain *p2m);
  /* Flushes all nested p2m tables */
  void p2m_flush_nestedp2m(struct domain *d);

+/* Map MMIO regions in the p2m: start_gaddr and end_gaddr is the range
+ * in the guest physical address space to map, starting from the machine
+ * address maddr. */
+int map_mmio_regions(struct domain *d, paddr_t start_gaddr,
+                     paddr_t end_gaddr, paddr_t maddr);
+int unmap_mmio_regions(struct domain *d,
+                       unsigned long start_gfn,
+                       unsigned long end_gfn,
+                       unsigned long mfn);
+

The both functions are used on common code. I would prefer to define the prototype only once in common header (e.g in include/xen).

At the same time, it's not necessary to give the full addresses on argument. GFN, MFN is enough.

I think we already talk about it on V1. I said it wasn't urgent, but as the function is common now ... :)

Regards,

--
Julien Grall

_______________________________________________
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®.