Re: [PATCH v4 2/3] arm: Allow the user to specify the GIC version

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

Hi Ian,

On 07/07/15 15:39, Ian Campbell wrote:
> On Tue, 2015-07-07 at 15:08 +0100, Julien Grall wrote:
>> A platform may have a GIC compatible with previous version of the
>> device.
> 
> "...a GIC which is compatible with a previous version..."
> 
>>
>> This is allow to virtualize an unmodified OS on new hardware if the GIC
>> is compatible with older version.
> 
> "This is to allow virtualization of an unmodified..."
> 
> "...with the 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 only GICv2 and GICv3 are supported).
> 
> I think "Unless the user has specified in the configuration file..."

That sounds better.

[..]

>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 03a9205..8ac90c4 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -61,7 +61,40 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>      xc_config->nr_spis = nr_spis;
>>      LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
>>  
>> -    xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>> +    switch (d_config->b_info.arch_arm.gic_version) {
>> +    case LIBXL_GIC_VERSION_DEFAULT:
>> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>> +        break;
>> +    case LIBXL_GIC_VERSION_V2:
>> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2;
>> +        break;
>> +    case LIBXL_GIC_VERSION_V3:
>> +        xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3;
>> +        break;
>> +    default:
>> +        LOG(ERROR, "Unknown GIC version %s\n",
>> +            libxl_gic_version_to_string(d_config->b_info.arch_arm.gic_version));
> 
> libxl_gic_version_to_string will return NULL for a truly unknown value.
> Perhaps print %d version too/instead?

Will do.

> 
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index f799081..2b8a506 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -579,6 +579,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>>          goto out;
>>      }
>>  
>> +    ret = libxl__arch_domain_save_config(gc, d_config, xc_config);
> 
> 
>> +    if (ret < 0) {
>> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fail to save domain config");
> 
> Please use the shorter LOG* macros, and you don't need/want to log errno
> here since the function doesn't touch it.
> 
> Actually, the function logs on failure itself, no need to repeat it I
> think, just document "logs on failure" next to the prototype.

Ok.

> 
>> +        rc = ERROR_FAIL;
> 
> libxl__arch_domain_save_config already returns a libxl ERROR_*, so it
> should be stored into rc directly and propagated.

Ok.

> 
>> +        goto out;
>> +    }
>> +
>>      ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
>>      if (ret < 0) {
>>          LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain move fail");
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 1be3f8b..e825315 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -2250,6 +2250,18 @@ skip_vfb:
>>          }
>>      }
>>  
>> +    if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) {
>> +        libxl_gic_version v;
>> +
>> +        e = libxl_gic_version_from_string(buf, &v);
>> +        if (e) {
>> +            fprintf(stderr,
>> +                    "Unknown gic_version \"%s\" specified\n", buf);
>> +            exit(-ERROR_FAIL);
>> +        }
>> +        b_info->arch_arm.gic_version = v;
> 
> You can just pass &b_info....gic_version straight to the from_string
> function.

I was following some other example in xl_cmdimpl.c (see vendor_device).
Anyway, I will do the change.

Regards,

-- 
Julien Grall