On 2/7/23 12:27, Nina Schoetterl-Glausch wrote:
On Tue, 2023-02-07 at 10:59 +0100, Pierre Morel wrote:
On 2/6/23 19:34, Nina Schoetterl-Glausch wrote:
On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
...
[...]
/**
@@ -137,6 +225,21 @@ static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
(smp->books * smp->sockets * smp->cores)) %
smp->drawers;
}
Why are the changes below in this patch?
Because before thos patch we have only horizontal polarization.
Not really since you only enable topology in the next patch.
+
+ /*
+ * Machine polarity is set inside the global s390_topology structure.
+ * In the case the polarity is set as horizontal set the entitlement
Sorry here an error in the comment should be :
"In the case the polarity is NOT set as horizontal..."
+ * to POLARITY_VERTICAL_MEDIUM which is the better equivalent when
+ * machine polarity is set to vertical or POLARITY_VERTICAL_HIGH if
+ * the vCPU is dedicated.
+ */
+ if (s390_topology.polarity && !env->entitlement) {
It'd be more readable if you compared against enum values by name.
Right, I will change this to
if (s390_topology.polarity != S390_POLARITY_HORIZONTAL &&
env->entitlement == S390_ENTITLEMENT_UNSET) {
I don't see why you check s390_topology.polarity. If it is horizontal
then the value of the entitlement doesn't matter at all, so you can set it
to whatever.
Right, that is why it is done only for vertical polarization (sorry for
the wrong comment)
I'm saying you don't need to check it at all. You adjust the values for
vertical polarization, but you could just always do that since the values
don't matter at all if the polarization is horizontal.
OK right
All you want to do is enforce dedicated -> VERTICAL_HIGH, right?
So why don't you just add
+ if (cpu->env.dedicated && cpu->env.entitlement != POLARITY_VERTICAL_HIGH) {
+ error_setg(errp, "A dedicated cpu implies high entitlement");
+ return;
+ } >
to s390_topology_check?
Here it is to set the default in the case the values are not provided.
If no values are provided, they default to dedication=false and entitlement=medium,
as defined by patch 1, which are fine and don't need to be adjusted.
Right, I think I added this when working on modifying the attributes in
QAPI and rebased it in the wrong patch.
Here it has no sense.
But where you are right is that I should add a verification to the check
function.
+ if (env->dedicated) {
+ env->entitlement = POLARITY_VERTICAL_HIGH;
+ } else {
+ env->entitlement = POLARITY_VERTICAL_MEDIUM;
+ }
If it is horizontal, then setting the entitlement is pointless as it will be
reset to medium on PTF.
That is why the polarity is tested (sorry for the bad comment)
I said this because I'm fine with setting it pointlessly.
So the current polarization is vertical and a cpu is being hotplugged,
but setting the entitlement of the cpu being added is also pointless, because
it's determined by the dedication. That seems weird.
No it is not determined by the dedication, if there is no dedication the
3 vertical values are possible.
You set it to either high or medium based on the dedication. And for horizontal
polarization it obviously doesn't matter.
on PTF yes but not on hotplug, on hotplug all 3 values are possible.
As far as I understand you don't need this because the default values are fine.
You should add the check that if a dedicated cpu is hotplugged, then the entitlement
must be high, to patch 2 and that's it, no additional changes necessary.
Yes, right.
Regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
