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

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

On 2014/8/29 17:18, Jan Beulich wrote:
> > > > 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
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) )
                ...
        }

Thanks
Tiejun

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