Re: [Xen-devel] [PATCH v8 4/8] xen/arm: Use AFF1 when translating ICC_SGI1R_EL1 to cpumask

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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 <baozich@xxxxxxxxx>
> >>
> >> The old unsigned long type of vcpu_mask can only express 64 cpus at the
> >> most, which might not be enough for the guest which used vGICv3. We
> >> introduce a new struct sgi_target for the target cpu list of SGI, which
> >> holds the affinity path information (only level 1 at the moment). For
> >> GICv2 that has no affinity level, we can just set the corresponding
> >> fields to be 0.
> >>
> >> Signed-off-by: Chen Baozi <baozich@xxxxxxxxx>
> >> ---
> >>  xen/arch/arm/vgic-v2.c            |  7 +++---
> >>  xen/arch/arm/vgic-v3.c            | 10 +++++----
> >>  xen/arch/arm/vgic.c               | 45 
> >> +++++++++++++++++----------------------
> >>  xen/include/asm-arm/gic_v3_defs.h |  3 +++
> >>  xen/include/asm-arm/vgic.h        |  7 +++++-
> >>  5 files changed, 38 insertions(+), 34 deletions(-)
> >>
> >> 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.

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

Ian.


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