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

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

On 2014/8/28 15:44, Jan Beulich wrote:
>>>> On 28.08.14 at 09:19, <tiejun.chen@intel.com> wrote:
>> On 2014/8/28 15:09, Chen, Tiejun wrote:
>>> On 2014/8/28 14:50, Jan Beulich wrote:
>>>>>>> On 28.08.14 at 04:24, <tiejun.chen@intel.com> wrote:
>>>>> If you guys have no more comments, could I send a new series to review?
>>>>
>>>> You certainly can do so at any time, but as said before I didn't get
>>>> to looking at the current version yet; briefly having looked at the
>>>
>>> I knew this point so this is just why here I's like to ask if I can send
>>> new revision. I hope I can do better as you expect.
>>>
>>>> first two patches I'm already pretty convinced that the structuring
>>>> still isn't right (you shouldn't be exposing VT-d internals into
>>>
>>> If you have any comment, I think you can point inline, then I can take a
>>> look at that to improve or fix anything as you expect.
>>>
>>> As you know, I'm not familiar with Xen codes so sometimes I can't
>>> understand what you mean properly, even what you were saying. So I have
>>> to ask you to explain explicitly again.
>>>
>>>> arbitrary parts of the hypervisor, but rather introduce a new
>>
>> As I remember you or Andrew told me not to use acpi_rmrr_units directly,
>> instead I previously introduced a new array to store such RMRR info.
>
> Duplicating information for no reason. Did you check whether adding
> a new method to struct iommu_ops couldn't do what you want, at
> once retaining proper isolation _and_ not duplicating anything?
>

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?

3. If #1 and #2 are fine to you, the go the follows"

struct device_acpi_reserved_memory *darm;
int nr_entries = 0;
unsigned int i = 0;
struct list_head *dev_reserved_memory = NULL;
const struct iommu_ops *ops = iommu_get_ops();

if ( ops->get_device_reserved_memory )
{
	nr_entries = ops->get_device_reserved_memory(dev_reserved_memory);
	if ( !nr_entries )
		return -ENOENT;
	else if ( nr_entries < 0 )
                 return -EFAULT;
         }
         else
             return -ENOENT;
}

Any comments are appreciated.

Thanks
Tiejun