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

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

On 2014/8/29 17:18, Jan Beulich wrote:
>>>> On 29.08.14 at 05:02, <tiejun.chen@intel.com> 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