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 <drjo...@redhat.com> wrote:
> > > Signed-off-by: Andrew Jones <drjo...@redhat.com>
> >
> > 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
> >
