Re: [Qemu-arm] [Qemu-devel] [PATCH] hw/arm/virt: gicv3: use all target-list bits

[Andrew Jones]

On Thu, Jun 23, 2016 at 12:15:59PM +0100, Peter Maydell wrote:
> On 21 June 2016 at 19:58, Andrew Jones <address@hidden> wrote:
> > Signed-off-by: Andrew Jones <address@hidden>
> 
> I think this commit message could be improved...it's both
> very short and a bit off the mark.
> 
> > ---
> >  hw/arm/virt.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index c5c125e9204a0..53f545921003c 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1271,6 +1271,16 @@ static void machvirt_init(MachineState *machine)
> >          }
> >          cpuobj = object_new(object_class_get_name(oc));
> >
> > +        /* Adjust MPIDR per the GIC's target-list size. */
> > +        if (gic_version == 3) {
> > +            CPUState *cs = CPU(cpuobj);
> > +            uint8_t Aff1 = cs->cpu_index / 16;
> > +            uint8_t Aff0 = cs->cpu_index % 16;
> > +
> > +            object_property_set_int(cpuobj, (Aff1 << ARM_AFF1_SHIFT) | 
> > Aff0,
> > +                                    "mp-affinity", NULL);
> > +        }
> 
> We still don't have support in KVM for telling the CPU what
> affinity to use, so these may get overridden later if KVM's
> idea of affinity and ours differ. I guess that's no different
> to what we have today, though.
> 
> I think it would be better to:
>  * use the loop index 'n' rather than fishing the cpu_index
>    out of the CPUState.
>  * do this regardless of GIC version (if it's GICv2 we only
>    have 8 CPUs max anyway)
>  * comment it as "Create our CPUs in clusters of 16; this suits
>    the GICv3's target list limitations, and matches how KVM
>    assigns them"
>  * for 32-bit, set the mp-affinity in the same arrangement the
>    kernel does for KVM, which is clusters of 4 CPUs

Actually this depends on the host. KVM will use all 8 tlist bits
with ARM guests, when running on an AArch64 host. As a guest doesn't
really have to worry about clusters for shared cache, then ignoring
the 4 per cluster requirement is likely fine (is that a hard
requirement anyway?), but it could confuse a guest.

So we can either
a) play it safe and always use clusters of 4 for ARM guests, and
   KVM will get "fixed" when we start managing the guest's MPIDR
   from userspace, or
b) use 8 here, as TCG always has, and KVM does for AArch32 guests.
   This might be less safe, but also improves SGI efficiency.

Thanks,
drew


>  * note also in the comment that for KVM these will be overridden
>    by the hard-coded topology in the kernel when the CPU is
>    realized
> 
> [is changing mp-affinity a migration compat break?]
> 
> thanks
> -- PMM
>