Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[Cornelia Huck]

On Thu, 31 Aug 2017 18:03:52 +0200
Igor Mammedov  wrote:

> On Thu, 31 Aug 2017 16:35:13 +0200
> Cornelia Huck  wrote:
> 
> > On Wed, 30 Aug 2017 19:05:56 +0200
> > David Hildenbrand  wrote:
> >   
> > > Some time ago we discussed that using "id" as property name is not the
> > > right thing to do, as it is a reserved property for other devices.
> > > 
> > > Switch to the term "addr" instead, which matches the definition in the
> > > PoP called "CPU address". There is no such thing as cpu number, so
> > > rename env.cpu_num to env.cpu_addr.
> > > 
> > > We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr in sync.
> > > cpu->index was already implicitly used by e.g. cpu_exists(), so keeping
> > > both in sync seems to be the right thing to do.
> > > 
> > > cpu->index will now no longer automatically get set via
> > > cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed
> > > in sync.
> > > 
> > > Our new cpu property "addr" can be a static property. Range checks can
> > > be avoided by using the correct type and the "setting after realized"
> > > check is done implicitly.
> > > 
> > > AFAIK, s390x only supports cpu_add and not device_add for cpus. So we
> > > should be able to safely rename that property (no the "id" property
> > > could properly be used for device_add, which needs an artificial id for
> > > identification purposes).
> this patch seems to somewhat conflicting with
>  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06505.html
> that were supposed to go via machine tree and which I've respinned today
> to fix conflicts due just merged pull req.
> 
> Cornelia,
> 
> Could you put/merge it via s390x tree so that David and I work won't clash 
> again?

I can do that, sure. I can put your patch on top of David's.

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[Igor Mammedov]

On Thu, 31 Aug 2017 16:35:13 +0200
Cornelia Huck  wrote:

> On Wed, 30 Aug 2017 19:05:56 +0200
> David Hildenbrand  wrote:
> 
> > Some time ago we discussed that using "id" as property name is not the
> > right thing to do, as it is a reserved property for other devices.
> > 
> > Switch to the term "addr" instead, which matches the definition in the
> > PoP called "CPU address". There is no such thing as cpu number, so
> > rename env.cpu_num to env.cpu_addr.
> > 
> > We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr in sync.
> > cpu->index was already implicitly used by e.g. cpu_exists(), so keeping
> > both in sync seems to be the right thing to do.
> > 
> > cpu->index will now no longer automatically get set via
> > cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed
> > in sync.
> > 
> > Our new cpu property "addr" can be a static property. Range checks can
> > be avoided by using the correct type and the "setting after realized"
> > check is done implicitly.
> > 
> > AFAIK, s390x only supports cpu_add and not device_add for cpus. So we
> > should be able to safely rename that property (no the "id" property
> > could properly be used for device_add, which needs an artificial id for
> > identification purposes).  
this patch seems to somewhat conflicting with
 https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg06505.html
that were supposed to go via machine tree and which I've respinned today
to fix conflicts due just merged pull req.

Cornelia,

Could you put/merge it via s390x tree so that David and I work won't clash 
again?

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[Cornelia Huck]

On Thu, 31 Aug 2017 16:41:47 +0200
David Hildenbrand  wrote:

> On 31.08.2017 16:35, Cornelia Huck wrote:
> > On Wed, 30 Aug 2017 19:05:56 +0200
> > David Hildenbrand  wrote:
> >   
> >> Some time ago we discussed that using "id" as property name is not the
> >> right thing to do, as it is a reserved property for other devices.
> >>
> >> Switch to the term "addr" instead, which matches the definition in the
> >> PoP called "CPU address". There is no such thing as cpu number, so
> >> rename env.cpu_num to env.cpu_addr.
> >>
> >> We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr in sync.
> >> cpu->index was already implicitly used by e.g. cpu_exists(), so keeping
> >> both in sync seems to be the right thing to do.
> >>
> >> cpu->index will now no longer automatically get set via
> >> cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed
> >> in sync.
> >>
> >> Our new cpu property "addr" can be a static property. Range checks can
> >> be avoided by using the correct type and the "setting after realized"
> >> check is done implicitly.
> >>
> >> AFAIK, s390x only supports cpu_add and not device_add for cpus. So we
> >> should be able to safely rename that property (no the "id" property
> >> could properly be used for device_add, which needs an artificial id for
> >> identification purposes).  
> > 
> > I cannot parse the sentence in the brackets...  
> 
> Me too :)
> 
> ...So we should be able to safely rename that property. device_add will
> later need the reserved "id" property. Hotplugging a CPU would then look
> like this: "device_add host-s390-cpu id=cpu2 addr=2".

Yup, that's understandable :)

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[David Hildenbrand]

On 31.08.2017 16:35, Cornelia Huck wrote:
> On Wed, 30 Aug 2017 19:05:56 +0200
> David Hildenbrand  wrote:
> 
>> Some time ago we discussed that using "id" as property name is not the
>> right thing to do, as it is a reserved property for other devices.
>>
>> Switch to the term "addr" instead, which matches the definition in the
>> PoP called "CPU address". There is no such thing as cpu number, so
>> rename env.cpu_num to env.cpu_addr.
>>
>> We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr in sync.
>> cpu->index was already implicitly used by e.g. cpu_exists(), so keeping
>> both in sync seems to be the right thing to do.
>>
>> cpu->index will now no longer automatically get set via
>> cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed
>> in sync.
>>
>> Our new cpu property "addr" can be a static property. Range checks can
>> be avoided by using the correct type and the "setting after realized"
>> check is done implicitly.
>>
>> AFAIK, s390x only supports cpu_add and not device_add for cpus. So we
>> should be able to safely rename that property (no the "id" property
>> could properly be used for device_add, which needs an artificial id for
>> identification purposes).
> 
> I cannot parse the sentence in the brackets...

Me too :)

...So we should be able to safely rename that property. device_add will
later need the reserved "id" property. Hotplugging a CPU would then look
like this: "device_add host-s390-cpu id=cpu2 addr=2".

> 
>>
>> Signed-off-by: David Hildenbrand 
>> ---
>>  hw/s390x/s390-virtio-ccw.c |  2 +-
>>  target/s390x/cpu.c | 69 
>> --
>>  target/s390x/cpu.h |  5 ++--
>>  target/s390x/cpu_models.c  |  2 +-
>>  target/s390x/excp_helper.c |  2 +-
>>  target/s390x/helper.c  |  4 +--
>>  target/s390x/misc_helper.c |  4 +--
>>  target/s390x/translate.c   |  5 +---
>>  8 files changed, 28 insertions(+), 65 deletions(-)
> 
> ...the patch seems fine, though :)
> 


-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[Cornelia Huck]

On Wed, 30 Aug 2017 19:05:56 +0200
David Hildenbrand  wrote:

> Some time ago we discussed that using "id" as property name is not the
> right thing to do, as it is a reserved property for other devices.
> 
> Switch to the term "addr" instead, which matches the definition in the
> PoP called "CPU address". There is no such thing as cpu number, so
> rename env.cpu_num to env.cpu_addr.
> 
> We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr in sync.
> cpu->index was already implicitly used by e.g. cpu_exists(), so keeping
> both in sync seems to be the right thing to do.
> 
> cpu->index will now no longer automatically get set via
> cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed
> in sync.
> 
> Our new cpu property "addr" can be a static property. Range checks can
> be avoided by using the correct type and the "setting after realized"
> check is done implicitly.
> 
> AFAIK, s390x only supports cpu_add and not device_add for cpus. So we
> should be able to safely rename that property (no the "id" property
> could properly be used for device_add, which needs an artificial id for
> identification purposes).

I cannot parse the sentence in the brackets...

> 
> Signed-off-by: David Hildenbrand 
> ---
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  target/s390x/cpu.c | 69 
> --
>  target/s390x/cpu.h |  5 ++--
>  target/s390x/cpu_models.c  |  2 +-
>  target/s390x/excp_helper.c |  2 +-
>  target/s390x/helper.c  |  4 +--
>  target/s390x/misc_helper.c |  4 +--
>  target/s390x/translate.c   |  5 +---
>  8 files changed, 28 insertions(+), 65 deletions(-)

...the patch seems fine, though :)

[Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling

[David Hildenbrand]

Some time ago we discussed that using "id" as property name is not the
right thing to do, as it is a reserved property for other devices.

Switch to the term "addr" instead, which matches the definition in the
PoP called "CPU address". There is no such thing as cpu number, so
rename env.cpu_num to env.cpu_addr.

We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr in sync.
cpu->index was already implicitly used by e.g. cpu_exists(), so keeping
both in sync seems to be the right thing to do.

cpu->index will now no longer automatically get set via
cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed
in sync.

Our new cpu property "addr" can be a static property. Range checks can
be avoided by using the correct type and the "setting after realized"
check is done implicitly.

AFAIK, s390x only supports cpu_add and not device_add for cpus. So we
should be able to safely rename that property (no the "id" property
could properly be used for device_add, which needs an artificial id for
identification purposes).

Signed-off-by: David Hildenbrand 
---
 hw/s390x/s390-virtio-ccw.c |  2 +-
 target/s390x/cpu.c | 69 --
 target/s390x/cpu.h |  5 ++--
 target/s390x/cpu_models.c  |  2 +-
 target/s390x/excp_helper.c |  2 +-
 target/s390x/helper.c  |  4 +--
 target/s390x/misc_helper.c |  4 +--
 target/s390x/translate.c   |  5 +---
 8 files changed, 28 insertions(+), 65 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 03c88a524b..7754e3eaf9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -306,7 +306,7 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
 S390CPU *cpu = S390_CPU(dev);
 CPUState *cs = CPU(dev);
 
-name = g_strdup_printf("cpu[%i]", cpu->env.cpu_num);
+name = g_strdup_printf("cpu[%i]", cpu->env.cpu_addr);
 object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name,
  errp);
 g_free(name);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 7267b60d41..156589e921 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -36,6 +36,7 @@
 #include "trace.h"
 #include "qapi/visitor.h"
 #include "exec/exec-all.h"
+#include "hw/qdev-properties.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/hw.h"
 #include "sysemu/arch_init.h"
@@ -189,24 +190,26 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
**errp)
 }
 
 #if !defined(CONFIG_USER_ONLY)
-if (cpu->id >= max_cpus) {
-error_setg(, "Unable to add CPU: %" PRIi64
-   ", max allowed: %d", cpu->id, max_cpus - 1);
+if (cpu->env.cpu_addr >= max_cpus) {
+error_setg(, "Unable to add CPU: %" PRIu32 ", max allowed: %d",
+   cpu->env.cpu_addr, max_cpus - 1);
 goto out;
 }
 #endif
-if (cpu_exists(cpu->id)) {
-error_setg(, "Unable to add CPU: %" PRIi64
-   ", it already exists", cpu->id);
+if (cpu_exists(cpu->env.cpu_addr)) {
+error_setg(, "Unable to add CPU: %" PRIu32 ", it already exists",
+   cpu->env.cpu_addr);
 goto out;
 }
-if (cpu->id != scc->next_cpu_id) {
-error_setg(, "Unable to add CPU: %" PRIi64
-   ", The next available id is %" PRIi64, cpu->id,
+if (cpu->env.cpu_addr != scc->next_cpu_id) {
+error_setg(, "Unable to add CPU: %" PRIu32
+   ", the next available nr is %" PRIi64, cpu->env.cpu_addr,
scc->next_cpu_id);
 goto out;
 }
 
+/* sync cs->cpu_index and env->cpu_addr. The latter is needed for TCG. */
+cs->cpu_index = env->cpu_addr;
 cpu_exec_realizefn(cs, );
 if (err != NULL) {
 goto out;
@@ -216,7 +219,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
**errp)
 #if !defined(CONFIG_USER_ONLY)
 qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
 #endif
-env->cpu_num = cpu->id;
 s390_cpu_gdb_init(cs);
 qemu_init_vcpu(cs);
 #if !defined(CONFIG_USER_ONLY)
@@ -237,45 +239,6 @@ out:
 error_propagate(errp, err);
 }
 
-static void s390x_cpu_get_id(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
-S390CPU *cpu = S390_CPU(obj);
-int64_t value = cpu->id;
-
-visit_type_int(v, name, , errp);
-}
-
-static void s390x_cpu_set_id(Object *obj, Visitor *v, const char *name,
- void *opaque, Error **errp)
-{
-S390CPU *cpu = S390_CPU(obj);
-DeviceState *dev = DEVICE(obj);
-const int64_t min = 0;
-const int64_t max = UINT32_MAX;
-Error *err = NULL;
-int64_t value;
-
-if (dev->realized) {
-error_setg(errp, "Attempt to set property '%s' on '%s' after "
-   "it was realized", name, object_get_typename(obj));
-return;
-}
-
-visit_type_int(v, name, , );
-if (err) {
-error_propagate(errp, err);
-