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

[wangyanan (Y)]

On 2021/5/19 21:26, wangyanan (Y) wrote:
> Hi Drew,
>
> On 2021/5/19 16:27, Andrew Jones wrote:
> > On Tue, May 18, 2021 at 09:05:39PM +0200, Andrew Jones wrote:
> > > 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?
> > The more I think about this, the more I think we're in a bit of 
> > pickle and
> > need Peter Maydell to chime in. While we may want to make our -smp 
> > command
> > line option parsing more strict in order to bring some sanity to it, if
> > we do, then we'll break existing command lines, which, while may be
> > specifying useless inputs, have always gotten away with it. We probably
> > can't just change that now without forcing the user to opt into it.
> > Maybe we need to add another -smp parameter like 'strict' that has to
> > be set to 'on' in order to get this new behavior.
> >
> > Peter, do you have some suggestions for this? A summary of the problem
> > we'd like to solve is as follows:
> >
> >   We'd like to start describing CPU topology to guests when provided
> >   topology information with the '-smp ...' command line option. 
> > Currently,
> >   a user may provide nearly whatever it wants on that command line 
> > option
> >   and not get an error, even though the guest will not get a topology
> >   description. When building the topology its important to know what
> >   the user actually wants, so we're proposing to require both sockets
> >   and cores be given if one of them is given. Also, since we don't yet
> >   support hot plug for AArch64, we're proposing to enforce cpus == 
> > maxcpus.
> >
> > Is it fine to make those changes to the parsing for 6.1 and later? 
> > (Note,
> > mach-virt will override the default smp_parse with its own, so this is
> > mach-virt specific.) Or, should we only do this if a new parameter is
> > also given, e.g. 'strict'. Something like
> >
> >    -smp strict=on,cpus=4,sockets=2,cores=2
> >
> > would be needed by users who want to describe cpu topologies. Without
> > a strict description, then they get what they get today for their
> > DT/ACPI topology description, nothing.
> From my point of view, I like the idea of a new parameter like 
> "strict=on/off".
> I will explain the reason below but maybe I have missed something, so 
> I also
> hope for some suggestions from Peter. :)
>
> 1) We don't need to worry about breaking any existing -smp command lines
> including the rare and strange ones any more, since we will only have 
> more
> strict requirement for the new provided cmdlines with "strict=on" and 
> only
> generate topology description to guest with these new cmdlines provided.
>
> 2) This will provide an option for users to decide whether to enable 
> the feature
> or not. Furthermore, this feature can also work on older machine 
> types, if a user
> want to make use of cpu topology exposure to guest on older machines 
> and is
> also sure it won't affect the application's behavior, then he can read 
> the Doc and
> properly provided a -smp cmdline with "strict=on" to boot a VM.
>
> 3) We don't need to bother guessing different formats of -smp command 
> lines
> in parsing. If the new parameter is not specified or "strict=off" is 
> provided, we
> totally follow the rules in smp_parse() and disable the topology 
> exposure. And if
> "strict=on" is provided, we enable the topology exposure and enforce 
> completely
> detailed configuration like "-smp strict=on,cpus=4,sockets=2,cores=2".
IMO, threads should also be required here.
Libvirt requires all of them if one of sockets/cores/threads is provided.
So if we hope to be consistent with Libvirt, the required configuration
should at least "-smp strict=on,cpus=4,sockets=2,cores=2,threads=1".

Thanks,
Yanan
> But maxcpus will be optional, it will default to cpus if not provided. 
> We also ensure
> it matches cpus if provided, given that cpu hotplug is not available yet.
>
> Thanks,
> Yanan
> > Thanks,
> > drew
> >
> > .