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:
[..]
In the next version with entitlement being an enum it is right.
However, deleting this means that the default value for entitlement
depends on dedication.
If we have only low, medium, high and default for entitlement is medium.
If the user specifies the dedication true without specifying entitlement
we could force entitlement to high.
But we can not distinguish this from the user specifying dedication true
with a medium entitlement, which is wrong.
So three solution:
1) We ignore what the user say if dedication is specified as true
2) We specify that both dedication and entitlement must be specified if
dedication is true
3) We set an impossible default to distinguish default from medium
entitlement
For me the solution 3 is the best one, it is more flexible for the user.
Solution 1 is obviously bad.
Solution 2 forces the user to specify entitlement high and only high if
it specifies dedication true.
AFAIU, you prefer the solution 2, forcing user to specify both
dedication and entitlement to suppress a default value in the enum.
Why is it bad to have a default value in the enum that we do not use to
specify that the value must be calculated?
Yes, I'd prefer solution 2. I don't like adapting the internal state where only
the three values make sense for the user interface.
It also keeps things simple and requires less code.
I also don't think it's a bad thing for the user, as it's not a thing done manually often.
I'm also not a fan of a value being implicitly being changed even though it doesn't look
like it from the command.
However, what I really don't like is the additional state and naming it "horizontal",
No problem to use another name like "auto" as you propose later.
not so much the adjustment if dedication is switched to true without an entitlement given.
For the monitor command there is no problem, you currently have:
That is clear, the has_xxx does the job.
[..]
So you can just set it if (!has_entitlement).
There is also ways to set the value for cpus defined on the command line, e.g.:
Yes, thanks, I already said I find your proposition to use a
DEFINE_PROP_CPUS390ENTITLEMENT good and will use it.
diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h
index 0ac327ae60..41a605c5a7 100644
--- a/include/hw/qdev-properties-system.h
+++ b/include/hw/qdev-properties-system.h
@@ -22,6 +22,7 @@ extern const PropertyInfo qdev_prop_audiodev;
extern const PropertyInfo qdev_prop_off_auto_pcibar;
extern const PropertyInfo qdev_prop_pcie_link_speed;
extern const PropertyInfo qdev_prop_pcie_link_width;
+extern const PropertyInfo qdev_prop_cpus390entitlement;
#define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d) \
DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
@@ -73,5 +74,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
#define DEFINE_PROP_UUID_NODEFAULT(_name, _state, _field) \
DEFINE_PROP(_name, _state, _field, qdev_prop_uuid, QemuUUID)
+#define DEFINE_PROP_CPUS390ENTITLEMENT(_n, _s, _f) \
+ DEFINE_PROP(_n, _s, _f, qdev_prop_cpus390entitlement, int)
+
#endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 54541d2230..01308e0b94 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -135,7 +135,7 @@ struct CPUArchState {
int32_t book_id;
int32_t drawer_id;
bool dedicated;
- uint8_t entitlement; /* Used only for vertical polarization */
+ int entitlement; /* Used only for vertical polarization */
Isn't it better to use:
+ CpuS390Entitlement entitlement; /* Used only for vertical
polarization */
uint64_t cpuid;
#endif
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index d42493f630..db5c3d4fe6 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -1143,3 +1143,14 @@ const PropertyInfo qdev_prop_uuid = {
.set = set_uuid,
.set_default_value = set_default_uuid_auto,
};
+
+/* --- s390x cpu topology entitlement --- */
+
+QEMU_BUILD_BUG_ON(sizeof(CpuS390Entitlement) != sizeof(int));
+
+const PropertyInfo qdev_prop_cpus390entitlement = {
+ .name = "CpuS390Entitlement",
+ .enum_table = &CpuS390Entitlement_lookup,
+ .get = qdev_propinfo_get_enum,
+ .set = qdev_propinfo_set_enum,
+};
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index b8a292340c..1b3f5c61ae 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -199,8 +199,7 @@ static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
* is not dedicated.
* A dedicated CPU always receives a high entitlement.
*/
- if (env->entitlement >= S390_CPU_ENTITLEMENT__MAX ||
- env->entitlement == S390_CPU_ENTITLEMENT_HORIZONTAL) {
+ if (env->entitlement < 0) {
Here we can have:
+ if (env->entitlement == S390_CPU_ENTITLEMENT_AUTO) {
...
if (env->dedicated) {
env->entitlement = S390_CPU_ENTITLEMENT_HIGH;
} else {
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 57165fa3a0..dea50a3e06 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -31,6 +31,7 @@
#include "qapi/qapi-types-machine.h"
#include "sysemu/hw_accel.h"
#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
#include "fpu/softfloat-helpers.h"
#include "disas/capstone.h"
#include "sysemu/tcg.h"
@@ -248,6 +249,7 @@ static void s390_cpu_initfn(Object *obj)
cs->exception_index = EXCP_HLT;
#if !defined(CONFIG_USER_ONLY)
+ cpu->env.entitlement = -1;
Then we do not need this initialization if here under we define
DEFINE_PROP_CPUS390ENTITLEMENT differently
s390_cpu_init_sysemu(obj);
#endif
}
@@ -264,8 +266,7 @@ static Property s390x_cpu_properties[] = {
DEFINE_PROP_INT32("book-id", S390CPU, env.book_id, -1),
DEFINE_PROP_INT32("drawer-id", S390CPU, env.drawer_id, -1),
DEFINE_PROP_BOOL("dedicated", S390CPU, env.dedicated, false),
- DEFINE_PROP_UINT8("entitlement", S390CPU, env.entitlement,
- S390_CPU_ENTITLEMENT__MAX),
+ DEFINE_PROP_CPUS390ENTITLEMENT("entitlement", S390CPU, env.entitlement),
+ DEFINE_PROP_CPUS390ENTITLEMENT("entitlement", S390CPU, env.entitlement,
S390_CPU_ENTITLEMENT_AUTO),
#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.
