Re: [Xen-devel] [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

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

On 2014/9/1 18:29, Jan Beulich wrote:
> > > > On 01.09.14 at 11:44, <tiejun.chen@xxxxxxxxx> wrote:
> > On 2014/8/29 17:18, Jan Beulich wrote:
> > > This still allocates another instance of structures to create a second
> > > linked list. Did you consider get_device_reserved_memory() to take
> > Do you mean we still use this existing type combo, acpi_rmrr_units and
> > acpi_rmrr_units?
> >
> > > a callback function instead?
> > But we should do something like this,
> >
> > 1. .get_device_reserved_memory = get_drm_all,
> > 2.  static int get_drm_all(struct list_head *dev_reserved_memory)
> >       {
> >           return (get_drm_callback(dev_reserved_memory));
> >       }
> >
> > 3. get_drm_callback = get_device_acpi_reserved_memory;
> > 4.  static int get_device_acpi_reserved_memory(struct list_head
> > *dev_reserved_memory)
> >       {
> >         ...
> >         dev_reserved_memory = &acpi_rmrr_units;
> >         ...
> >       }
> >
> > Then while calling the hypercall,
> >
> > struct list_head *dev_reserved_memory = NULL;
> > nr_entries = ops->get_device_reserved_memory(dev_reserved_memory);
> > if (!nr_entries)
> >         list_for_each_entry( darm, dev_reserved_memory, list )
> >         {
> >                 xxx.start_pfn = ...;
> >                 xxx.nr_pages = ...;
> >                 if ( copy_to_guest_offset(buffer, i, &xxx, 1) )
> >                 ...
> >         }
> Clearly not: The callback ought to be used _while_ processing the
> hypercall. And of course the callback shouldn't be used to retrieve
> &acpi_rmrr_units, but to report back to the calling entity the
> individual regions.
Jan,

I see you're reviewing other patches in v5 so really appreciate your 
comments.
But I will address those comments until here I can implement this 
callback mechanism as you expect. Because some comments from other 
patches may need to rebase on this better way. So I hope I can finish 
your callback mechanism firstly to avoid bring you potential duplicated 
faults :)
So could you take a look at the follows?

xen/vtd: add one iommu ops to expose device reserved
 memory

We need this interface to expose device reserved memory
safely in common place.

Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
---
 xen/drivers/passthrough/vtd/dmar.c  | 40 
+++++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/vtd/iommu.c | 14 +++++++++++++
 xen/include/asm-x86/iommu.h         |  3 +++
 xen/include/xen/iommu.h             |  1 +
 4 files changed, 58 insertions(+)

diff --git a/xen/drivers/passthrough/vtd/dmar.c 
b/xen/drivers/passthrough/vtd/dmar.c
index 1152c3a..f46aee2 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -567,6 +567,44 @@ out:
     return ret;
 }

+extern get_device_reserved_memory_t get_drm_callback;
+struct xen_mem_reserved_device_memory 
*get_device_acpi_reserved_memory(unsigned int *nr_entries)
+{
+    struct acpi_rmrr_unit *rmrru;
+    static struct xen_mem_reserved_device_memory *rmrrm = NULL;
+    static unsigned int drm_entries = 0;
+    static unsigned int check_done = 0;
+    unsigned int i = 0;
+
+    *nr_entries = drm_entries;
+    if ( check_done )
+        return rmrrm;
+
+    list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+        drm_entries++;
+
+    if ( drm_entries )
+    {
+        rmrrm = xzalloc_array(struct xen_mem_reserved_device_memory,
+                              drm_entries);
+        if ( !rmrrm )
+            return NULL;
+
+        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+        {
+            rmrrm[i].start_pfn = rmrru->base_address >> PAGE_SHIFT;
+            rmrrm[i].nr_pages = PAGE_ALIGN(rmrru->end_address -
+                                           rmrru->base_address) /
+                                           PAGE_SIZE;
+            i++;
+        }
+    }
+
+    check_done = 1;
+
+    return rmrrm;
+}
+
 static int __init
 acpi_parse_one_rmrr(struct acpi_dmar_header *header)
 {
@@ -678,6 +716,8 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
         }
     }

+    get_drm_callback = get_device_acpi_reserved_memory;
+
     return ret;
 }

diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index 042b882..43ff443 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2461,6 +2461,19 @@ static void vtd_dump_p2m_table(struct domain *d)
     vtd_dump_p2m_table_level(hd->arch.pgd_maddr, 
agaw_to_level(hd->arch.agaw), 0, 0);
 }

+struct xen_mem_reserved_device_memory *dummy_get_drm_callback(unsigned 
int *nr_entries)
+{
+    *nr_entries = 0;
+    return NULL;
+}
+
+get_device_reserved_memory_t get_drm_callback = dummy_get_drm_callback;
+
+struct xen_mem_reserved_device_memory *get_drm_all(unsigned int 
*nr_entries)
+{
+    return (get_drm_callback(nr_entries));
+}
+
 const struct iommu_ops intel_iommu_ops = {
     .init = intel_iommu_domain_init,
     .hwdom_init = intel_iommu_hwdom_init,
@@ -2486,6 +2499,7 @@ const struct iommu_ops intel_iommu_ops = {
     .iotlb_flush = intel_iommu_iotlb_flush,
     .iotlb_flush_all = intel_iommu_iotlb_flush_all,
     .dump_p2m_table = vtd_dump_p2m_table,
+    .get_device_reserved_memory = get_drm_all,
 };

 /*
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index e7a65da..aead1d7 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -15,6 +15,8 @@
 #ifndef __ARCH_X86_IOMMU_H__
 #define __ARCH_X86_IOMMU_H__

+#include <public/memory.h>
+
 #define MAX_IOMMUS 32

 /* Does this domain have a P2M table we can use as its IOMMU pagetable? */
@@ -32,6 +34,7 @@ int iommu_supports_eim(void);
 int iommu_enable_x2apic_IR(void);
 void iommu_disable_x2apic_IR(void);

+typedef struct xen_mem_reserved_device_memory* 
(*get_device_reserved_memory_t)(unsigned int *nr_entries);
 #endif /* !__ARCH_X86_IOMMU_H__ */
 /*
  * Local variables:
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 8eb764a..8806ef6 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -149,6 +149,7 @@ struct iommu_ops {
     void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, 
unsigned int value);
     unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int 
reg);
     int (*setup_hpet_msi)(struct msi_desc *);
+    struct xen_mem_reserved_device_memory* 
(*get_device_reserved_memory)(unsigned int *nr_entries);
 #endif /* CONFIG_X86 */
     void (*suspend)(void);
     void (*resume)(void);
--


Then when call the hypercall,

+    case XENMEM_reserved_device_memory_map:
+    {
+        struct xen_mem_reserved_device_memory *xmrdm = NULL;
+        struct xen_mem_reserved_device_memory_map xmrdmm;
+        XEN_GUEST_HANDLE(xen_mem_reserved_device_memory_t) buffer;
+        XEN_GUEST_HANDLE_PARAM(xen_mem_reserved_device_memory_t) 
buffer_param;
+        const struct iommu_ops *ops = iommu_get_ops();
+        unsigned int nr_entries = 0;
+        unsigned int i = 0;
+
+        xmrdm = ops->get_device_reserved_memory(&nr_entries);
+        if ( !nr_entries )
+            return -ENOENT;
+        if ( nr_entries < 0 )
+            return -EFAULT;
+
+        if ( copy_from_guest(&xmrdmm, arg, 1) )
+            return -EFAULT;
+
+        if ( xmrdmm.nr_entries < nr_entries )
+        {
+            xmrdmm.nr_entries = nr_entries;
+            if ( copy_to_guest(arg, &xmrdmm, 1) )
+                return -EFAULT;
+            return -ENOBUFS;
+        }
+
+        buffer_param = guest_handle_cast(xmrdmm.buffer,
+                                         xen_mem_reserved_device_memory_t);
+        buffer = guest_handle_from_param(buffer_param,
+                                         xen_mem_reserved_device_memory_t);
+        if ( !guest_handle_okay(buffer, xmrdmm.nr_entries) )
+            return -EFAULT;
+
+        for ( i = 0; i < nr_entries; i++ )
+        {
+            if ( copy_to_guest_offset(buffer, i, xmrdm + i, 1) )
+                return -EFAULT;
+        }
+
+        xmrdmm.nr_entries = i;
+
+        if ( copy_to_guest(arg, &xmrdmm, 1) )
+                return -EFAULT;
+
+        return 0;
+    }
+



Thanks
Tiejun

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