Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 3/4] target/ppc: consolidate CPU device-tree id computation in helper

[David Gibson]

On Mon, May 22, 2017 at 03:13:25PM +0200, Greg Kurz wrote:
> On Mon, 22 May 2017 10:59:50 +0200
> Greg Kurz <address@hidden> wrote:
> 
> > On Mon, 22 May 2017 12:04:13 +1000
> > David Gibson <address@hidden> wrote:
> > 
> > > On Fri, May 19, 2017 at 12:32:20PM +0200, Greg Kurz wrote:  
> > > > For historical reasons, we compute CPU device-tree ids with a 
> > > > non-trivial
> > > > logic. This patch consolidate the logic in a single helper to be used
> > > > in various places where it is currently open-coded.
> > > > 
> > > > It is okay to get rid of DIV_ROUND_UP() because we're sure that the 
> > > > number
> > > > of threads per core in the guest cannot exceed the number of threads per
> > > > core in the host.    
> > > 
> > > However, your new logic still gives different answers in some cases.
> > > In particular when max_cpus is not a multiple of smp_threads.  Which
> > > is generally a bad idea, but allowed for older machine types for
> > > compatibility.   e.g. smp_threads=4, max_cpus=6 smt=8
> > > 
> 
> FWIW, this topology was never supported for pseries >= 2.7 since commit
> 94a94e4c4919 ("spapr: convert boot CPUs into CPU core devices", QEMU 2.7):
> 
> qemu-system-ppc64: max_cpus (6) must be multiple of threads (4)
> 
> The same happens goes with smp_cpus.

Yes, pre-2.7 is what I meant by older machine types.

> If we care for compat with pre-2.7 machine types (ie, no support for CPU
> hotplug),

We do.. RHEL 7.3 is still 2.6 based, for one thing.

> this topology isn't valid anymore since QEMU 2.9, with these
> commits:
> 
> 0c86d0fd92aa ("pseries: Always use core objects for CPU construction") which
> causes the following error if we only set max_cpus:
> 
> qemu-system-ppc64: This machine version does not support CPU hotplug

That patch has explicit provision for allowing the last core to have a
not-full complement of threads.

> 8149e2992f78 ("pseries: Enforce homogeneous threads-per-core") which
> causes the following error if we set smp_cpus (or smp_cpus == max_cpus):
> 
> qemu-system-ppc64: invalid nr-threads 2, must be 4

This one does indeed do what you say - but that's a bug :(.  It means
migration from older versions may be broken.

> So in the end, we already enforce max_cpus and smp_cpus to be multiple
> of smp_threads for all machine types. In this case...
> 
> > > Old logic:
> > >            DIV_ROUND_UP(6 * 8, 4)
> > >          = ⌈48 / 4⌉ = 12
> > > 
> > > New logic gives: ⌊6 / 4⌋ * 8 + (6 % 4)
> > >                = 1 * 8 + 2
> > >          = 10
> > >   
> > 
> > I now realize this two formulas are hardly reconcilable... this
> > probably means that this patch shouldn't try to consolidate the
> > logic in hw/ppc/spapr.c with the one in target/ppc/translate_init.c.
> 
> ... both formulas are equivalent, unless I'm missing something. Or
> do we really want to re-allow the funky topologies for older
> machines ?

Want to?  Not really.  Have to for compatibility?  Yes, absolutely.

I've just sent a patch to address this.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature