Re: [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

Informing KVM of MPIDR is still on my TODO, and I'm getting closer
to having time to work on it.

Spoiler: I'm thinking about changing vcpu-id to the compressed mpidr.
We won't need it compressed if vcpu-id gets expanded to 64bits, which
I think Igor would like to do.

> idea of affinity and ours differ. I guess that's no different
> to what we have today, though.

Right, that was my thinking as well, the benefit is purely for
TCG.

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

Will do all the above. Thanks for the suggestions.

> 
> [is changing mp-affinity a migration compat break?]

Indeed this would introduce a guest visible change with a
migration from old to new. It seems we need to add a machine
property every time we add a guest visible change, which isn't
off by default and enabled with the cmdline, and then turn it
off for all older versions.

Thanks,
drew