|
| From: | Pierre Morel |
| Subject: | Re: [PATCH v19 01/21] s390x/cpu topology: add s390 specifics to CPU topology |
| Date: | Tue, 18 Apr 2023 15:52:28 +0200 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 |
On 4/18/23 14:38, Nina Schoetterl-Glausch wrote:
On Tue, 2023-04-18 at 12:01 +0200, Pierre Morel wrote:On 4/18/23 10:53, Nina Schoetterl-Glausch wrote:On Mon, 2023-04-03 at 18:28 +0200, Pierre Morel wrote:S390 adds two new SMP levels, drawers and books to the CPU topology. The S390 CPU have specific topology features like dedication and entitlement to give to the guest indications on the host vCPUs scheduling and help the guest take the best decisions on the scheduling of threads on the vCPUs. Let us provide the SMP properties with books and drawers levels and S390 CPU with dedication and entitlement, Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> Reviewed-by: Thomas Huth <thuth@redhat.com> ---[...]diff --git a/qapi/machine-common.json b/qapi/machine-common.json new file mode 100644 index 0000000000..73ea38d976 --- /dev/null +++ b/qapi/machine-common.json @@ -0,0 +1,22 @@ +# -*- Mode: Python -*- +# vim: filetype=python +# +# This work is licensed under the terms of the GNU GPL, version 2 or later. +# See the COPYING file in the top-level directory. + +## +# = Machines S390 data types +## + +## +# @CpuS390Entitlement: +# +# An enumeration of cpu entitlements that can be assumed by a virtual +# S390 CPU +# +# Since: 8.1 +## +{ 'enum': 'CpuS390Entitlement', + 'prefix': 'S390_CPU_ENTITLEMENT', + 'data': [ 'horizontal', 'low', 'medium', 'high' ] }You can get rid of the horizontal value now that the entitlement is ignored if the polarization is vertical.Right, horizontal is not used, but what would you like? - replace horizontal with 'none' ? - add or substract 1 when we do the conversion between enum string and value ?Yeah, I would completely drop it because it is a meaningless value and adjust the conversion to the cpu value accordingly.frankly I prefer to keep horizontal here which is exactly what is given in the documentation for entitlement = 0Not sure what you mean with this.
I mean: Extract from the PoP: ---- The following values are used: PP Meaning 0 The one or more CPUs represented by the TLE are horizontally polarized. 1 The one or more CPUs represented by the TLE are vertically polarized. Entitlement is low. 2 The one or more CPUs represented by the TLE are vertically polarized. Entitlement is medium. 3 The one or more CPUs represented by the TLE are vertically polarized. Entitlement is high. ----Also I find that using an enum to systematically add/subtract a value is for me weird.
so I really prefer to keep "horizontal", "low", "medium", "high" event "horizontal" will never appear.
A mater of taste, it does not change anything to the functionality or the API.
[...]diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index b10a8541ff..57165fa3a0 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -37,6 +37,7 @@ #ifndef CONFIG_USER_ONLY #include "sysemu/reset.h" #endif +#include "hw/s390x/cpu-topology.h"#define CR0_RESET 0xE0UL#define CR14_RESET 0xC2000000UL; @@ -259,6 +260,12 @@ static gchar *s390_gdb_arch_name(CPUState *cs) static Property s390x_cpu_properties[] = { #if !defined(CONFIG_USER_ONLY) DEFINE_PROP_UINT32("core-id", S390CPU, env.core_id, 0), + DEFINE_PROP_INT32("socket-id", S390CPU, env.socket_id, -1), + 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),I would define an entitlement PropertyInfo in qdev-properties-system.[ch], then one can use e.g. -device z14-s390x-cpu,core-id=11,entitlement=highDon't you think it is an enhancement we can do later?It's a user visible change, so no.
We could have kept both string and integer.
But it's not complicated, should be just:
const PropertyInfo qdev_prop_cpus390entitlement = {
.name = "CpuS390Entitlement",
.enum_table = &CpuS390Entitlement_lookup,
.get = qdev_propinfo_get_enum,
.set = qdev_propinfo_set_enum,
.set_default_value = qdev_propinfo_set_default_value_enum,
};
Plus a comment & build bug in qdev-properties-system.c
and
extern const PropertyInfo qdev_prop_cpus390entitlement;
#define DEFINE_PROP_CPUS390ENTITLEMENT(_n, _s, _f, _d) \
DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_cpus390entitlement, \
CpuS390Entitlement)
in qdev-properties-system.h
You need to change the type of env.entitlement and set the default to 1 for
medium
and that should be it.
OK, it does not change anything to the functionality but is a little bit more pretty.
on the command line and cpu hotplug. I think setting the default entitlement to medium here should be fine. [...]right, I had medium before and should not have change it. Anyway what ever the default is, it must be changed later depending on dedication.No, you can just set it to medium and get rid of the adjustment code. s390_topology_check will reject invalid changes and the default above is fine since dedication is false.
I do not want a default specification for the entitlement to depend on the polarization.
If we do as you propose, by horizontal polarization a default entitlement with dedication will be accepted but will be refused after the guest switched for vertical polarization.
So we need adjustment before the check in both cases.I find it easier and more logical if there is no default value than to have a default we need to overwrite.
| [Prev in Thread] | Current Thread | [Next in Thread] |