Re: [Xen-devel] [PATCH V2 8/25] tools/libxl: Add a user configurable parameter to control vIOMMU attributes

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

On 2017年08月22日 21:19, Wei Liu wrote:
>> +=over 4
>> > +
>> > +=item B<KEY=VALUE>
>> > +
>> > +Possible B<KEY>s are:
>> > +
>> > +=over 4
>> > +
>> > +=item B<type="STRING">
>> > +
>> > +Currently there is only one valid type:
>> > +
>> > +(x86 only) "intel_vtd" means providing a emulated Intel VT-d to the guest.
>> > +
>> > +=item B<intremap=BOOLEAN>
>> > +
>> > +Specifies whether the vIOMMU should support interrupt remapping
>> > +and default 'true'.
>> > +
>> > +=item B<x2apic=BOOLEAN>
>> > +
>> > +Specifies whether the vIOMMU should support x2apic mode and default 
>> > 'true'.
>> > +Only valid for "intel_vtd".
> Why not expose base address and length as well?

"base address" and "length" of vIOMMU register region is low level
vIOMMU configuration. I am afraid users is vary hard to determine which
region is suitable for vIOMMU and doesn't conflict with other device model.

> 
>> +
>> > +libxl_viommu_info = Struct("viommu_info", [
>> > +    ("u", KeyedUnion(None, libxl_viommu_type, "type",
>> > +           [("intel_vtd", Struct(None, [
>> > +                 ("x2apic",     libxl_defbool)]))
>> > +           ])),
>> > +    ("intremap",        libxl_defbool),
>> > +    ("cap",             uint64),
>> > +    ("base_addr",       uint64),
>> > +    ("len",             uint64),
>> > +    ])
>> > +
>> >  libxl_domain_build_info = Struct("domain_build_info",[
>> >      ("max_vcpus",       integer),
>> >      ("avail_vcpus",     libxl_bitmap),
>> > @@ -506,6 +521,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>> >      # 65000 which is reserved by the toolstack.
>> >      ("device_tree",      string),
>> >      ("acpi",             libxl_defbool),
>> > +    ("viommu",           libxl_viommu_info),
> An array please, we shouldn't limit the number of viommus.
> 
>> >      ("u", KeyedUnion(None, libxl_domain_type, "type",
>> >                  [("hvm", Struct(None, [("firmware",         string),
>> >                                         ("bios",             
>> > libxl_bios_type),
>> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> > index 5c2bf17..11c4eb2 100644
>> > --- a/tools/xl/xl_parse.c
>> > +++ b/tools/xl/xl_parse.c
>> > @@ -17,6 +17,7 @@
>> >  #include <limits.h>
>> >  #include <stdio.h>
>> >  #include <stdlib.h>
>> > +#include <xen/domctl.h>
> Why is this needed?
> 
>> > +        if (libxl_defbool_is_default(viommu->intremap))
>> > +            libxl_defbool_set(&viommu->intremap, true);
>> > +
>> > +        if (!libxl_defbool_val(viommu->intremap)) {
>> > +            fprintf(stderr, "Cannot create one virtual VTD without 
>> > intremap\n");
>> > +            return 1;
>> > +        }
>> > +
>> > +        /* Set default values to unexposed fields */
>> > +        viommu->base_addr = VIOMMU_VTD_BASE_ADDR;
>> > +        viommu->len = VIOMMU_VTD_REGISTER_LEN;
>> > +
> You're doing this is xl. This is not right. The default value should be
> set from within libxl.
> 
> You should have a libxl_XXX_setdefault function for this type.

OK. will update.

-- 
Best regards
Tianyu Lan

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