From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH V5 09/10] xen/arm: make domain_max_vcpus return value from vgic_ops Date: Sun, 31 May 2015 19:21:19 +0100 Message-ID: <556B511F.8060400@citrix.com> References: <1432984051-10838-1-git-send-email-cbz@baozis.org> <1432984051-10838-10-git-send-email-cbz@baozis.org> <556B0E3E.30106@citrix.com> <906BA87F-69AA-4D25-A264-5B6DD3B01C63@gmail.com> <556B4D5D.6000503@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Yz7rR-0005PB-5x for xen-devel@lists.xenproject.org; Sun, 31 May 2015 18:21:25 +0000 In-Reply-To: <556B4D5D.6000503@citrix.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: Julien Grall , Chen Baozi Cc: xen-devel@lists.xenproject.org, Chen Baozi , Campbell Ian List-Id: xen-devel@lists.xenproject.org On 31/05/15 19:05, Julien Grall wrote: > Hi Chen, > > On 31/05/2015 16:31, Chen Baozi wrote: >>> On May 31, 2015, at 21:35, Julien Grall >>> wrote: >>>> + >>>> #define NR_GIC_LOCAL_IRQS NR_LOCAL_IRQS >>>> #define NR_GIC_SGI 16 >>>> #define MAX_RDIST_COUNT 4 >>>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h >>>> index 2f413e1..1157b04 100644 >>>> --- a/xen/include/asm-arm/vgic.h >>>> +++ b/xen/include/asm-arm/vgic.h >>>> @@ -110,6 +110,8 @@ struct vgic_ops { >>>> struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int >>>> irq); >>>> /* vGIC sysreg emulation */ >>>> int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr >>>> hsr); >>>> + /* Get the maximum number of vCPU supported */ >>>> + int (*get_max_vcpus)(void); >>> >>> Why did you create a function? The value never change so a field >>> value would have been better=85 >> >> But is it appropriate that we define a field value in a struct called >> vgic_ops? I >> thought =91XXX_ops=92 refers to a struct that is made up by function >> points. (FIXME) > > Well if the only issue if the name, please rename the structure. IHMO, > the name is fine for me, I will let Ian & Stefano decides about it. > > As the vGIC is always supporting N cpus and will never change, it's > pointless to use a function (thinking about the "cost" vs access an > field). We have plenty of other structures with mixed function pointers and data. A "const unsigned int max_vcpus;" should absolutely be used in a case like this. ~Andrew