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/28 15:44, Jan Beulich wrote:
> > > > On 28.08.14 at 09:19, <tiejun.chen@xxxxxxxxx> 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@xxxxxxxxx> 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

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