Re: [PATCH for Xen 4.5] xen/arm: Add support for GICv3 for domU

[Julien Grall <julien.grall@linaro.org>]

Hi Jan,

On 10/31/2014 01:37 PM, Jan Beulich wrote:
>>>> On 31.10.14 at 12:23, <julien.grall@linaro.org> wrote:
>> On 31/10/2014 09:02, Jan Beulich wrote:
>>>> +        domctl->u.configuredomain.gic_version = gic_version;
>>>> +
>>>> +        /* TODO: Make the copy generic for all ARCH domctl */
>>>> +        if ( __copy_to_guest(u_domctl, domctl, 1) )
>>>
>>> With just a single field needing copying, __copy_field_to_guest()
>>> would be quite a bit more efficient.
>>
>> The configuredomain structure contains only a field and I plan to rework 
>> this code for Xen 4.6 to make this copy generic within the function (see 
>> the TODO).
>>
>> Anyway, for this use case using __copy_field_to_guest is not more 
>> efficient for ARM.
> 
> But you don't say why. To me there's a difference between copying
> 1 byte or 128.

The cost of copying 128 bytes is negligible compare to the complexity of
raw_copy_to_guest. Furthermore this DOMCTL is not in an hot path (will
always been called once during domain boot).

Hence, this copy will be common to all ARCH domctl in Xen 4.6. I didn't
do the change know to avoid touching too much code.

> 
>>>> --- a/xen/include/public/domctl.h
>>>> +++ b/xen/include/public/domctl.h
>>>> @@ -68,6 +68,19 @@ struct xen_domctl_createdomain {
>>>>   typedef struct xen_domctl_createdomain xen_domctl_createdomain_t;
>>>>   DEFINE_XEN_GUEST_HANDLE(xen_domctl_createdomain_t);
>>>>
>>>> +#if defined(__arm__) || defined(__aarch64__)
>>>> +#define XEN_DOMCTL_CONFIG_GIC_DEFAULT   0
>>>> +#define XEN_DOMCTL_CONFIG_GIC_V2        1
>>>> +#define XEN_DOMCTL_CONFIG_GIC_V3        2
>>>> +/* XEN_DOMCTL_configure_domain */
>>>> +struct xen_domctl_configuredomain {
>>>
>>> The naming suggests that the #if really should be around just the
>>> gic_version field (with a dummy field in the #else case to be C89
>>> compatible, e.g. a zero width unnamed bitfield) and the
>>> corresponding #define-s above, ...
>>
>> It's a bit like xen_domctl_setvcpuextstate which is defined only for x86 
>> while the name seem pretty common.
> 
> That's a particularly bad example to compare with, as this is about
> CPU registers having got added after the ABI was defined. This
> (hopefully) shouldn't have many similar cases on other architectures.
> Plus, doing things in an odd way just because there's an odd
> precedent is always suspicious to me.
> 
>> I think we have to stay consistent in this header and not defining 
>> DOMCTL which is not used for a specific architecture.
> 
> Yes, I always advocate for consistency - provided what is there is
> a reasonable reference and was done properly.

Would renaming the structure name with "xen_arm_domctl_configuredomain"
would be sufficient for you?

Regards,

-- 
Julien Grall