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

["wangyanan (Y)" <wangyanan55@huawei.com>]

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