RE: [RFC PATCH v2 5/6] hw/arm/virt-acpi-build: Add PPTT table

[Salil Mehta <salil.mehta@huawei.com>]

> From: Andrew Jones [mailto:drjones@redhat.com]
> Sent: Tuesday, May 18, 2021 8:06 PM
> To: Salil Mehta <salil.mehta@huawei.com>
> Cc: wangyanan (Y) <wangyanan55@huawei.com>; Peter Maydell
> <peter.maydell@linaro.org>; Michael S . Tsirkin <mst@redhat.com>; Wanghaibin
> (D) <wanghaibin.wang@huawei.com>; qemu-devel@nongnu.org; Shannon Zhao
> <shannon.zhaosl@gmail.com>; qemu-arm@nongnu.org; Alistair Francis
> <alistair.francis@wdc.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
> yangyicong <yangyicong@huawei.com>; yuzenghui <yuzenghui@huawei.com>; Igor
> Mammedov <imammedo@redhat.com>; zhukeqian <zhukeqian1@huawei.com>; lijiajie (H)
> <lijiajie11@huawei.com>; David Gibson <david@gibson.dropbear.id.au>; Linuxarm
> <linuxarm@huawei.com>; linuxarm@openeuler.org
> Subject: Re: [RFC PATCH v2 5/6] hw/arm/virt-acpi-build: Add PPTT table
> 
> On Tue, May 18, 2021 at 06:34:08PM +0000, Salil Mehta wrote:
> >  Those benefits, when vcpu pinning is used, are the same benefits
> > > as for the host, which already use PPTT tables to describe topology, even
> > > though hot plug isn't supported.
> >
> > yes sure, you mean pinning vcpus according to the cpu topology for performance?
> 
> Yup

Already Agreed :)

> > > Now, if you're saying we should only generate tables for smp.cpus, not
> >
> > Correct. This is what I thought we must be doing even now
> >
> > > smp.maxcpus, because hot plug isn't supported anyway, then I see your
> > > point. But, it'd be better to require smp.cpus == smp.maxcpus in our
> > > smp_parse function to do that, which we've never done before, so we may
> > > have trouble supporting existing command lines.
> >
> > I am trying to recall, if the vcpu Hotplug is not supported then can they
> > ever be different?
> >
> > cpus =  (threads * cores * sockets)
> >
> > static void smp_parse(MachineState *ms, QemuOpts *opts)
> > {
> >      [...]
> >
> >         if (sockets * cores * threads != ms->smp.max_cpus) {
> >             warn_report("Invalid CPU topology deprecated: "
> >                         "sockets (%u) * cores (%u) * threads (%u) "
> >                         "!= maxcpus (%u)",
> >                         sockets, cores, threads,
> >                         ms->smp.max_cpus);
> >         }
> >      [...]
> > }
> >
> > Although, above check does not exit(1) and just warns on detecting invalid
> > CPU topology. Not sure why?
> 
> Hmm, not sure what code you have there. I see this in
> hw/core/machine.c:smp_parse
> 
>         if (ms->smp.max_cpus < cpus) {
>             error_report("maxcpus must be equal to or greater than smp");
>             exit(1);
>         }
> 
>         if (sockets * cores * threads != ms->smp.max_cpus) {
>             error_report("Invalid CPU topology: "
>                          "sockets (%u) * cores (%u) * threads (%u) "
>                          "!= maxcpus (%u)",
>                          sockets, cores, threads,
>                          ms->smp.max_cpus);
>             exit(1);
>         }
> 
> >
> > Well if you think there are subtleties to support above implementation and
> > we cannot do it now then sure it is your call. :)
> 
> The problem is that -smp 4,maxcpus=8 doesn't error out today, even though
> it doesn't do anything. OTOH, -smp 4,cores=2 doesn't error out either, but
> we're proposing that it should. Maybe we can start erroring out when
> cpus != maxcpus until hot plug is supported?

Agreed, both don't make any sense if hotplug is not supported and ideally should
fail with error. We should block any such topology configuration.


Thanks
Salil