Re: [RFC 21/22] arm: Allow the user to specify the GIC version

[Julien Grall <julien.grall@citrix.com>]

On 05/06/15 13:42, Ian Campbell wrote:
> On Fri, 2015-05-08 at 14:29 +0100, Julien Grall wrote:
>> A platform may have a GIC compatible with previous version of the
>> device.
>>
>> This is allow to virtualize an unmodified OS on new hardware if the GIC
>> is compatible with older version.
>>
>> When a guest is created, the vGIC will emulate same version as the
>> hardware. Although, the user can specify in the configuration file the
>> preferred version (currently on GICv2 and GICv3 are supported).
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>>
>> ---
>>
>>     The hypervisor will check if the GIC is able to virtualize the
>>     version specified by the user (via the DOMCTL createdomain).
>>     If it's not compatible an error will be send on the Xen console
>>     which will make the error not obvious for user.
>>
>>     I'm wondering if we should expose to the toolstack the vGIC versions
>>     supported via a mecanism similar to xen_get_caps?
> 
> There may even be room in xencaps, although it isn't very scalable.
> 
> Another option might be to overwrite bad fields in the config struct
> with some sentinel to indicate they weren't usable. Not sure I like that
> though.

What about extending XENVER_get_features?

>>     I didn't add the documention because I wasn't sure in which section
>>     I should put it in the xl man.
> 
> Probably needs a new ARCHITECTURE: ARM section, and I bet a lot of stuff
> could move to ARCH: X86 (you don't have to do all that though).
> 
>> @@ -387,7 +393,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>      ("blkdev_start",    string),
>>  
>>      ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
>> -    
>> +
>> +    ("gic_version", libxl_gic_version),
> 
> For historical reasons the domain config is rather x86 centric. Fixing
> that is hard without breaking the API, but I think we should at least
> think about putting new arch specific stuff in a per-arch location.

Good idea.

> Just wrapping this in a struct named arm would be an ok start, we can
> add x86 when the next thing which should live there comes along.
> 
> I think having the fields always present, even for builds on arches
> where they don't make sense is ok, the alternative is to extend the IDL
> language to allow things to be made arch specific and generate suitable
> ifdefs...

I can give a look to see how hard it is to do it. Parameter such device
tree could go there too.

> 
> 
>> +
>>      ("device_model_version", libxl_device_model_version),
>>      ("device_model_stubdomain", libxl_defbool),
>>      # if you set device_model you must set device_model_version too
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 648ca08..b033c0b 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1298,6 +1298,18 @@ static void parse_config_data(const char *config_source,
>>          !xlu_cfg_get_string (config, "cpus_soft", &buf, 0))
>>          parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false);
>>  
>> +    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
> 
> Could we just make this an integer?

I choose the string solution over integer to not rule out the
possibility to expose to the user a specific extension of the GICvn
(such as V2M or ITS).

Regards,

-- 
Julien Grall