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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.08.14 at 05:02, <tiejun.chen@xxxxxxxxx> wrote:
> I tried to figure out solution as you suggestion but I'd like show my 
> draft design before post anything to review since please give some 
> suggestions here:
> 
> 1. In the xen/include/xen/iommu.h file,
> 
> struct iommu_ops {
>       ...
>       int (*get_device_reserved_memory)(struct list_head 
> *dev_reserved_memory);
> 
> 2. In the xen/drivers/passthrough/vtd/iommu.c file,
> 
> extern int get_device_acpi_reserved_memory(struct list_head 
> *dev_reserved_memory);
> 
> const struct iommu_ops intel_iommu_ops = {
>       ...
>       .get_device_reserved_memory = get_device_acpi_reserved_memory,
> 
> 3. In the xen/drivers/passthrough/vtd/dmar.c file,
> 
> struct list_head devices_reserved_memory = LIST_HEAD_INIT ( 
> devices_reserved_memory );
> int get_device_acpi_reserved_memory(struct list_head *dev_reserved_memory)
> {
>      static unsigned int device_reserved_memory_entries = 0;
>      static unsigned int check_done = 0;
>      struct acpi_rmrr_unit *rmrru;
>      struct device_acpi_reserved_memory *darm = NULL;
> 
>      dev_reserved_memory = &devices_reserved_memory;
> 
>      if ( check_done )
>          return device_reserved_memory_entries;
>      else
>      {
>          list_for_each_entry(rmrru, &acpi_rmrr_units, list)
>          {
>              darm = xzalloc(struct device_acpi_reserved_memory);
>              if ( !darm )
>                  return -ENOMEM;
> 
>              darm->base_address = rmrru->base_address;
>              darm->end_address = rmrru->end_address;
>              list_add(&darm->list, &devices_reserved_memory);
>              device_reserved_memory_entries++;
>          }
>      }
> 
>      check_done = 1;
> 
>      return device_reserved_memory_entries;
> }
> 
> 4. In the xen/include/asm-x86/acpi.h file,
> 
> +struct device_acpi_reserved_memory {
> +    struct list_head list;
> +    u64    base_address;
> +    u64    end_address;
> +};
> 
> 
> Here a couple of questions:
> 
> 1. Here I introduce this struct device_acpi_reserved_memory to avoid 
> exposing that existing structure and list acpi_rmrr_units
> 
> struct acpi_rmrr_unit {
>      struct dmar_scope scope;
>      struct list_head list;
>      u64    base_address;
>      u64    end_address;
>      u16    segment;
>      u8     allow_all:1;
> };
> 
> Because:
> 
> 1> Actually we just need two fields, base_address and end_address.
> 2> If reuse that structure, we still have to change some head files to 
> make sure we can use this in other files like I did in original patch #1 
> you don't like.
> 
> So what is your idea?
> 
> 2. Based on your isolation policy, I don't expose acpi_rmrr_units 
> directly. Instead, I will copy this to another list, 
> devices_reserved_memory as I show above.
> 
> Is this reasonable and expected?

This still allocates another instance of structures to create a second
linked list. Did you consider get_device_reserved_memory() to take
a callback function instead?

Jan


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