From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v2 12/15] xen/arm: gic: Store the necessary HW information per vGIC ... Date: Tue, 30 Jun 2015 15:00:17 +0100 Message-ID: <1435672817.21469.179.camel@citrix.com> References: <1435311269-3189-1-git-send-email-julien.grall@citrix.com> <1435311269-3189-13-git-send-email-julien.grall@citrix.com> <1435668976.21469.138.camel@citrix.com> <55929BCA.1060107@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z9w63-0001Ss-Ce for xen-devel@lists.xenproject.org; Tue, 30 Jun 2015 14:01:11 +0000 In-Reply-To: <55929BCA.1060107@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 Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On Tue, 2015-06-30 at 14:38 +0100, Julien Grall wrote: > Hi Ian, > > On 30/06/15 13:56, Ian Campbell wrote: > > On Fri, 2015-06-26 at 10:34 +0100, Julien Grall wrote: > >> ... in order to decouple the vGIC driver from the GIC driver. > >> > >> Each vGIC HW structure contains a boolean to indicate if the current GIC is > >> able to support this specific version of virtual GIC. > >> > >> Helpers have been introduced in order to help the GIC to setup correctly > >> the vGIC. The GIC will have to call them to announce the support of this > >> specific version. > >> > > > > "...to help the GIC correctly setup the vGIC" > > > > "...to announce support for this specific version" > > > > > >> Also drop fields that become unecessary in each global state. > > > > "unnecessary" > > I will fix it in the next version. > > >> @@ -228,6 +232,55 @@ static inline int vgic_allocate_spi(struct domain *d) > >> > >> extern void vgic_free_virq(struct domain *d, unsigned int virq); > >> > >> +struct vgic_v2_hw_config > >> +{ > >> + bool_t enabled; > >> + /* Distributor interface address */ > >> + paddr_t dbase; > >> + /* CPU interface address */ > >> + paddr_t cbase; > >> + /* Virtual CPU interface address */ > >> + paddr_t vbase; > >> +}; > >> + > >> +extern struct vgic_v2_hw_config vgic_v2_hw; > > > > My inclination is to call this either vgic_v2_hwdom(_config) (since it > > is vgic config for the hw dom) or to call it gic_v2_hw_config (since it > > contains config info of the physical gic which we happen to be going to > > use for vgic). > > > > I think given the expected usage the former makes more sense. > > I think that the 2 names don't work for the usage of the structure: > - vgic_v2_hwdom: vbase is used to map the guest CPU interface into the > virtual CPU interface > - gic_v2_hw_config: it gives the impression to be physical GIC specific > rather the virtual GIC. > > I would prefer to stick in vgic_v2_hw_config which show that we use it > for the virtual GIC. OK. > >> + > >> +static inline void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, > >> + paddr_t vbase) > >> +{ > >> + vgic_v2_hw.enabled = 1; > >> + vgic_v2_hw.dbase = dbase; > >> + vgic_v2_hw.cbase = cbase; > >> + vgic_v2_hw.vbase = vbase; > >> +} > > > > If you were to move this out of line into vgic-v2.c would that mean that > > vgic_v2_hw_config etc could be static to that file? > > No, we have to access the field enabled in domain_vgic_init to verify > the GIC is supporting the version of the vGIC. That's a shame. vgic_vN_init would have been the ideal place to test for this, which would have kept everything in one place, but you've just nuked that and I suppose don't want it coming back. It was the inclusion of into vgic.h which took me down the line of thinking of wanting to make this self contained, and in particular exposing struct rdist_region. Ian.