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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.10.14 at 12:23, <julien.grall@xxxxxxxxxx> 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.

>>> --- 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.

Jan


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