Re: [PATCH v19 02/21] s390x/cpu topology: add topology entries on CPU hotplug

[Pierre Morel]

On 4/25/23 11:27, Nina Schoetterl-Glausch wrote:
> On Tue, 2023-04-25 at 10:45 +0200, Pierre Morel wrote:
> > On 4/24/23 17:32, Nina Schoetterl-Glausch wrote:
> > > On Fri, 2023-04-21 at 12:20 +0200, Pierre Morel wrote:
> > > > > On 4/20/23 10:59, Nina Schoetterl-Glausch wrote:
> > > > > > > On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:
[..]
> > >    #endif
> > >        DEFINE_PROP_END_OF_LIST()
> > >    };
> > >
> > > There are other ways to achieve the same, you could also
> > > implement get, set and set_default_value so that there is an additional
> > > "auto"/"uninitialized" value that is not in the enum.
> > > If you insist on having an additional state in the enum, name it "auto".
> > Yes, I think it is a better name.
> IMO using entitlement=auto doesn't make too much sense with the 
> set-cpu-topology command,
> because you can just leave if off or specify the entitlement you want directly.
> So there is no actual need to have a user visible auto value and no need to 
> have it in the enum.


This value is only usable but not required on input and is never 
displayed by the qapi.


> Then the only problem is adjusting the entitlement when doing dedicated=on on 
> the command line.
> (If you want that)

Even the exact usage of dedication depends on the administration entity 
it will give the guest the
knowledge of something like: "A real CPU is dedicated to this vCPU".

The people using the qapi interface or using QEMU hotplug can easily 
understand
the concept without going deeper with entitlement which once implemented 
will be,
quite more complex to deal with.


> So with my proposal there are only the low, medium and high values in the enum.
> In order to set the entitlement automatically when using the command line I 
> initialize
> the entitlement to -1, so we later know if it has been set via the command line 
> or not.
> But you cannot set -1 via the property because qdev_propinfo_get_enum expects a 
> string,
> which is why I do it in s390_cpu_initfn.
>
> I'm not sure if you can define entitlement as CpuS390Entitlement.
> I think I changed it to int when I was exploring different solutions
> and had to change it because of a type check. But what I proposed above doesn't 
> cause the same issue.
> DEFINE_PROP_CPUS390ENTITLEMENT could then also use CpuS390Entitlement.


I did not have the problem by using CpuS390Entitlement.


> So there are three possible solutions now:
> 1. My proposal above, which as automatic adjustment, but only the three 
> required values in the enum.
> 2. Don't do automatic adjustment, three enum values.
> 3. Automatic adjustment with auto value in the enum.
>
> I still favor 2. but the other ones aren't terrible.

Thank you to accept the solution 3, I understand your objections, but,
I really think that there is no need to bother the user with entitlement
while it will not get used in QEMU/KVM until the Linux scheduler is modified
for the host and for the guest to handle it.


Regards,

Pierre