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

[Andrew Jones]

On Fri, Jun 24, 2016 at 06:03:21PM +0200, Andrew Jones wrote:
> 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.

Actually AArch32 guests would even use all 16 tlist bits on gicv3, if
there was a KVM host available to try it. So the (b) option shouldn't
be "use 8" it should be "don't treat 32-bit guests differently"

Thanks,
drew



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