Re: [PATCH v2 14/15] arm: Allow the user to specify the GIC version

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

Hi Ian,

On 30/06/15 14:06, Ian Campbell wrote:
> On Fri, 2015-06-26 at 10:34 +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).
>                                 ^ly?

Yes. I will fixed on the next version.

[..]

>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index a3e0e2e..663eb2d 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -1688,6 +1688,33 @@ The default is B<en-us>.
>>  
>>  See L<qemu(1)> for more information.
>>  
>> +=head2 Architecture Specific options
>> +
>> +=head3 ARM
>> +
>> +=over 4
>> +
>> +=item B<gic_version="vN">
>> +
>> +Version of the GIC emulated for the guest. Currently, the following versions
>> +are supported:
>> +
>> +=over 4
>> +
>> +=item B<v2>
>> +
>> +Emulate a GICv2 hardware
>> +
>> +=item B<v3>
>> +
>> +Emulate a GICv3 hardware. Note that the GICv2 compatibility is not supported.
> 
> "Note that the emulated GIC does not support the GICv2 compatibility
> mode". (To avoid confusion with such compatibility used to provide the
> other option to the guest)

Good idea. I will use it.

> 
>> +
>> +=back
>> +
>> +Although, all the versions may not be supported on the host.
> 
> "This requires hardware compatibility with the requested version. Either
> natively or via hardware backwards compatibility support".
> 
> 
>> +
>> +=back
>> +
>>  =head1 SEE ALSO
>>  
>>  =over 4
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index 0a7913b..68bc954 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -200,6 +200,11 @@
>>  #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>>  
>>  /*
>> + * libxl_domain_build_info has the gic_version field.
>> + */
>> +#define LIBXL_HAVE_BUILDINFO_GIC_VERSION 1
> 
> Mention ARM in the comment and/or name?

Yes, I forgot to update it when I move the field within the "arm" structure.

[..]

>>  
>> +libxl_gic_version = Enumeration("gic_version", [
>> +    (0, "DEFAULT"),
>> +    (1, "2"),
>> +    (2, "3")
> 
> If you call these "V2" and "V3" then you can use
> libxl_gic_version_from_string to make the parsing much easier. If you
> subsequently add e.g. "v2m" then the parsing code in xl will also just
> work,

Will do.

> Better to line up the values with the gic version if possible too. Enums
> don't need to be contiguous. Or maybe use 0x20, 0x30 etc for the value,
> leaving space for gicv2m in 0x21 etc.

I will use 0x20, 0x30...

Regards,

-- 
Julien Grall