From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH for Xen 4.5] xen/arm: Add support for GICv3 for domU Date: Fri, 31 Oct 2014 13:53:28 +0000 Message-ID: <54539458.3010901@linaro.org> References: <1414695092-20761-1-git-send-email-julien.grall@linaro.org> <54535E240200007800043DAC@mail.emea.novell.com> <54537126.5010401@linaro.org> <54539EA50200007800043EAE@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XkCe1-0008Jw-St for xen-devel@lists.xenproject.org; Fri, 31 Oct 2014 13:53:38 +0000 Received: by mail-wi0-f174.google.com with SMTP id d1so1387937wiv.1 for ; Fri, 31 Oct 2014 06:53:36 -0700 (PDT) In-Reply-To: <54539EA50200007800043EAE@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: ian.campbell@citrix.com, tim@xen.org, Vijaya Kumar K , Ian Jackson , stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org Hi Jan, On 10/31/2014 01:37 PM, Jan Beulich wrote: >>>> On 31.10.14 at 12:23, 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