From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask Date: Wed, 17 Jun 2015 14:32:27 +0100 Message-ID: <558176EB.9060700@citrix.com> References: <1434097969-16793-1-git-send-email-cbz@baozis.org> <1434097969-16793-5-git-send-email-cbz@baozis.org> <1434546030.13744.369.camel@citrix.com> <55817278.7050409@citrix.com> <1434547151.13744.378.camel@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 1Z5DSj-0001Nm-O4 for xen-devel@lists.xenproject.org; Wed, 17 Jun 2015 13:33:05 +0000 In-Reply-To: <1434547151.13744.378.camel@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: Ian Campbell Cc: xen-devel@lists.xenproject.org, Chen Baozi , Chen Baozi List-Id: xen-devel@lists.xenproject.org On 17/06/15 14:19, Ian Campbell wrote: > On Wed, 2015-06-17 at 14:13 +0100, Julien Grall wrote: >> On 17/06/15 14:00, Ian Campbell wrote: >>> On Fri, 2015-06-12 at 16:32 +0800, Chen Baozi wrote: >>>> From: Chen Baozi >>>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c >>>> index 3be1a51..5949cf1 100644 >>>> --- a/xen/arch/arm/vgic-v2.c >>>> +++ b/xen/arch/arm/vgic-v2.c >>>> @@ -201,16 +201,17 @@ static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir) >>>> int virq; >>>> int irqmode; >>>> enum gic_sgi_mode sgi_mode; >>>> - unsigned long vcpu_mask = 0; >>>> + struct sgi_target target; >>>> >>>> + memset(&target, 0, sizeof(struct sgi_target)); >>> >>> I'd prefer explicit initialisation of the relevant fields please. Which >>> may mean setting aff1 to 0 somewhere at the top, with a suitable comment >>> as to why, and might involve setting target.list to zero in some other >>> cases below or via an explicit initialiser here. >> >> Well, only SGI_TARGET_LIST is caring about struct sgi_target (see >> vgic_to_sgi). I would only initialize it when it's required. > > Good point, and by keeping it a pointer you could even pass NULL in the > other cases, making this more obvious still. Good idea. >>> >>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>>> index 7b387b7..59bd98a 100644 >>>> --- a/xen/arch/arm/vgic.c >>>> +++ b/xen/arch/arm/vgic.c >>>> @@ -318,15 +318,14 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) >>>> } >>>> } >>>> >>>> -/* TODO: unsigned long is used to fit vcpu_mask.*/ >>>> int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, >>>> - unsigned long vcpu_mask) >>>> + const struct sgi_target *target) >>> >>> For a 3 byte struct perhaps we can pass by value instead of reference? >>> >>> I suppose it might eventually be 5 bytes, but even so... >>> >>>> @@ -334,29 +333,33 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int >>>> { >>>> case SGI_TARGET_LIST: >>>> perfc_incr(vgic_sgi_list); >>>> + base = target->aff1 << 4; >>>> + bitmap = target->list; >>>> + for_each_set_bit( i, &bitmap, sizeof(target->list) * 8 ) >>>> + { >>>> + vcpuid = base + i; >>>> + if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) ) >>> >>> What if d->vcpu[vcpuid] is NULL? (Was this a latent bug before, or am I >>> missing something?) >> >> I don't see any problem, if d->vcpu[vcpuid] is NULL there is no need to >> send an SGI as the VCPU is not present. > > But the code will, I think. I should have quoted a bit more, briefly it > is : > > + if ( d->vcpu[vcpuid] != NULL && !is_vcpu_online(d->vcpu[vcpuid]) ) > continue > + vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq); > > So if d->vcpu[vcpuid] == NULL it will try and send an SGI to it, won't > it? Hmmm, correct. I didn't read carefully the if, sorry. It should be it "d->vcpu[vcpuid] == NULL || !is_vcpu_online(d->vcpu[vcpuid])". And yes, this is a latent bug. Although, XEN_DOMCTL_max_vcpus will return -ENOMEM if it fail to allocate a VCPU and libxl will continue to create the domain. So no possibility for the guest to crash Xen. Regards, -- Julien Grall