| Message ID | 1375366359-11553-6-git-send-email-jjherne@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 01.08.2013 16:12, schrieb Jason J. Herne: > From: "Jason J. Herne" <jjherne@us.ibm.com> > > Modify s390_cpu_addr2state to allow fetching state information for cpu addresses > above smp_cpus. Hotplug requires this capability. > > Also add s390_cpu_set_state function to allow modification of ipi_state entries > during hotplug. > > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com> > --- > hw/s390x/s390-virtio.c | 9 +++++---- > target-s390x/cpu.h | 2 +- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c > index 21e9124..5ad9cf3 100644 > --- a/hw/s390x/s390-virtio.c > +++ b/hw/s390x/s390-virtio.c > @@ -54,12 +54,13 @@ > static VirtIOS390Bus *s390_bus; > static S390CPU **ipi_states; > > -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state) > { > - if (cpu_addr >= smp_cpus) { > - return NULL; > - } > + ipi_states[cpu_addr] = state; > +} > > +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > +{ > return ipi_states[cpu_addr]; > } > This is what got us into the link<> discussion last time. If we do for (i = 0; i < ARRAY_SIZE(ipi_states); i++) { name = g_strdup_printf("cpu[%i]", i); object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU, &ipi_states[i], &err); } then we get said /machine/cpu[n] link<> properties, at a QMP level either returning nothing or the canonical path to the CPU object. On IRC I didn't get an answer of whether it was being done the above way because there is infrastructure missing, and a look at object.h now confirms that suspicion. CC'ing Anthony and Paolo. Since object_property_add_link() uses a NULL opaque, my idea would be to add a single setter hook argument passed through as opaque to object_set_link_property(), which would call it with the old and the new value. The purpose would be to avoid growing our own internal setter API, which is disjoint from the QMP qom-set we are targetting at. Regards, Andreas > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h > index 877eac7..62eb810 100644 > --- a/target-s390x/cpu.h > +++ b/target-s390x/cpu.h > @@ -377,7 +377,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type, > > uint8_t *s390_get_storage_keys(void); > void s390_alloc_storage_keys(ram_addr_t ram_size); > - > +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state); > S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); > void s390_add_running_cpu(S390CPU *cpu); > unsigned s390_del_running_cpu(S390CPU *cpu); >
On 01.08.2013, at 16:12, Jason J. Herne wrote: > From: "Jason J. Herne" <jjherne@us.ibm.com> > > Modify s390_cpu_addr2state to allow fetching state information for cpu addresses > above smp_cpus. Hotplug requires this capability. > > Also add s390_cpu_set_state function to allow modification of ipi_state entries > during hotplug. > > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com> > --- > hw/s390x/s390-virtio.c | 9 +++++---- > target-s390x/cpu.h | 2 +- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c > index 21e9124..5ad9cf3 100644 > --- a/hw/s390x/s390-virtio.c > +++ b/hw/s390x/s390-virtio.c > @@ -54,12 +54,13 @@ > static VirtIOS390Bus *s390_bus; > static S390CPU **ipi_states; > > -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state) > { > - if (cpu_addr >= smp_cpus) { > - return NULL; > - } > + ipi_states[cpu_addr] = state; We should still ensure that we don't access anything beyond the size of the array, no? Alex > +} > > +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > +{ > return ipi_states[cpu_addr]; > } > > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h > index 877eac7..62eb810 100644 > --- a/target-s390x/cpu.h > +++ b/target-s390x/cpu.h > @@ -377,7 +377,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type, > > uint8_t *s390_get_storage_keys(void); > void s390_alloc_storage_keys(ram_addr_t ram_size); > - > +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state); > S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); > void s390_add_running_cpu(S390CPU *cpu); > unsigned s390_del_running_cpu(S390CPU *cpu); > -- > 1.7.10.4 >
On 09/05/2013 08:01 AM, Andreas Färber wrote: > Am 01.08.2013 16:12, schrieb Jason J. Herne: >> From: "Jason J. Herne" <jjherne@us.ibm.com> >> >> Modify s390_cpu_addr2state to allow fetching state information for cpu addresses >> above smp_cpus. Hotplug requires this capability. >> >> Also add s390_cpu_set_state function to allow modification of ipi_state entries >> during hotplug. >> >> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com> >> --- >> hw/s390x/s390-virtio.c | 9 +++++---- >> target-s390x/cpu.h | 2 +- >> 2 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c >> index 21e9124..5ad9cf3 100644 >> --- a/hw/s390x/s390-virtio.c >> +++ b/hw/s390x/s390-virtio.c >> @@ -54,12 +54,13 @@ >> static VirtIOS390Bus *s390_bus; >> static S390CPU **ipi_states; >> >> -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) >> +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state) >> { >> - if (cpu_addr >= smp_cpus) { >> - return NULL; >> - } >> + ipi_states[cpu_addr] = state; >> +} >> >> +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) >> +{ >> return ipi_states[cpu_addr]; >> } >> > > This is what got us into the link<> discussion last time. If we do > > for (i = 0; i < ARRAY_SIZE(ipi_states); i++) { > name = g_strdup_printf("cpu[%i]", i); > object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU, > &ipi_states[i], &err); > } > > then we get said /machine/cpu[n] link<> properties, at a QMP level > either returning nothing or the canonical path to the CPU object. > > On IRC I didn't get an answer of whether it was being done the above way > because there is infrastructure missing, and a look at object.h now > confirms that suspicion. CC'ing Anthony and Paolo. > > Since object_property_add_link() uses a NULL opaque, my idea would be to > add a single setter hook argument passed through as opaque to > object_set_link_property(), which would call it with the old and the new > value. > > The purpose would be to avoid growing our own internal setter API, which > is disjoint from the QMP qom-set we are targetting at. > Ok, you lost me :). I must admit my understanding of QOM is still limited. Sorry for not keeping up. Why do we need this new hook? The link would contain a pointer to the correct ipi_states entry right? What are we using opaque for?
On 09/05/2013 08:01 AM, Andreas Färber wrote: > Am 01.08.2013 16:12, schrieb Jason J. Herne: >> From: "Jason J. Herne" <jjherne@us.ibm.com> >> ... > > This is what got us into the link<> discussion last time. If we do > > for (i = 0; i < ARRAY_SIZE(ipi_states); i++) { > name = g_strdup_printf("cpu[%i]", i); > object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU, > &ipi_states[i], &err); > } > > then we get said /machine/cpu[n] link<> properties, at a QMP level > either returning nothing or the canonical path to the CPU object. > > On IRC I didn't get an answer of whether it was being done the above way > because there is infrastructure missing, and a look at object.h now > confirms that suspicion. CC'ing Anthony and Paolo. > > Since object_property_add_link() uses a NULL opaque, my idea would be to > add a single setter hook argument passed through as opaque to > object_set_link_property(), which would call it with the old and the new > value. > > The purpose would be to avoid growing our own internal setter API, which > is disjoint from the QMP qom-set we are targetting at. I wrote the code, very close to how you suggested and it appears to be working when tested with qom-list. I'm still not certain why we need the ability to set the opaque of object_set_link_property()? For reference here is what I did: void s390_init_cpus(const char *cpu_model) { int i; char* name; if (cpu_model == NULL) { cpu_model = "host"; } ipi_states = g_malloc0(sizeof(S390CPU *) * max_cpus); for (i = 0; i < max_cpus; i++) { name = g_strdup_printf("cpu[%i]", i); object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU, (Object **)&ipi_states[i], NULL); } for (i = 0; i < smp_cpus; i++) { cpu_s390x_init(cpu_model); } } Yep, I know cpu_model is going away ;).
On Thu, 19 Sep 2013 16:19:46 -0400 "Jason J. Herne" <jjherne@linux.vnet.ibm.com> wrote: > On 09/05/2013 08:01 AM, Andreas Färber wrote: > > Am 01.08.2013 16:12, schrieb Jason J. Herne: > >> From: "Jason J. Herne" <jjherne@us.ibm.com> > >> > ... > > > > This is what got us into the link<> discussion last time. If we do > > > > for (i = 0; i < ARRAY_SIZE(ipi_states); i++) { > > name = g_strdup_printf("cpu[%i]", i); > > object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU, > > &ipi_states[i], &err); > > } > > > > then we get said /machine/cpu[n] link<> properties, at a QMP level > > either returning nothing or the canonical path to the CPU object. > > > > On IRC I didn't get an answer of whether it was being done the above way > > because there is infrastructure missing, and a look at object.h now > > confirms that suspicion. CC'ing Anthony and Paolo. > > > > Since object_property_add_link() uses a NULL opaque, my idea would be to > > add a single setter hook argument passed through as opaque to > > object_set_link_property(), which would call it with the old and the new > > value. > > > > The purpose would be to avoid growing our own internal setter API, which > > is disjoint from the QMP qom-set we are targetting at. > > I wrote the code, very close to how you suggested and it appears to be > working when tested with qom-list. I'm still not certain why we need > the ability to set the opaque of object_set_link_property()? > > For reference here is what I did: > > void s390_init_cpus(const char *cpu_model) > { > int i; > char* name; > > if (cpu_model == NULL) { > cpu_model = "host"; > } > > ipi_states = g_malloc0(sizeof(S390CPU *) * max_cpus); > > for (i = 0; i < max_cpus; i++) { > name = g_strdup_printf("cpu[%i]", i); > object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU, > (Object **)&ipi_states[i], NULL); > } > > for (i = 0; i < smp_cpus; i++) { > cpu_s390x_init(cpu_model); > } > } > > Yep, I know cpu_model is going away ;). Jason, do you have more information how cpu modeling or at least the parameterization will be managed in future? I'm close to finishing a patch that introduces S390 cpu models and need to know all this more precisely. Is there any document or discussion you can point me to. Thanks a lot. Michael >
On 09/19/2013 04:19 PM, Jason J. Herne wrote: > On 09/05/2013 08:01 AM, Andreas Färber wrote: >> Am 01.08.2013 16:12, schrieb Jason J. Herne: >>> From: "Jason J. Herne" <jjherne@us.ibm.com> >>> > ... >> >> This is what got us into the link<> discussion last time. If we do >> >> for (i = 0; i < ARRAY_SIZE(ipi_states); i++) { >> name = g_strdup_printf("cpu[%i]", i); >> object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU, >> &ipi_states[i], &err); >> } >> >> then we get said /machine/cpu[n] link<> properties, at a QMP level >> either returning nothing or the canonical path to the CPU object. >> >> On IRC I didn't get an answer of whether it was being done the above way >> because there is infrastructure missing, and a look at object.h now >> confirms that suspicion. CC'ing Anthony and Paolo. >> >> Since object_property_add_link() uses a NULL opaque, my idea would be to >> add a single setter hook argument passed through as opaque to >> object_set_link_property(), which would call it with the old and the new >> value. >> >> The purpose would be to avoid growing our own internal setter API, which >> is disjoint from the QMP qom-set we are targetting at. > > I wrote the code, very close to how you suggested and it appears to be > working when tested with qom-list. I'm still not certain why we need > the ability to set the opaque of object_set_link_property()? > > For reference here is what I did: > > void s390_init_cpus(const char *cpu_model) > { > int i; > char* name; > > if (cpu_model == NULL) { > cpu_model = "host"; > } > > ipi_states = g_malloc0(sizeof(S390CPU *) * max_cpus); > > for (i = 0; i < max_cpus; i++) { > name = g_strdup_printf("cpu[%i]", i); > object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU, > (Object **)&ipi_states[i], NULL); > } > > for (i = 0; i < smp_cpus; i++) { > cpu_s390x_init(cpu_model); > } > } > > Yep, I know cpu_model is going away ;). > Ping? :)
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c index 21e9124..5ad9cf3 100644 --- a/hw/s390x/s390-virtio.c +++ b/hw/s390x/s390-virtio.c @@ -54,12 +54,13 @@ static VirtIOS390Bus *s390_bus; static S390CPU **ipi_states; -S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state) { - if (cpu_addr >= smp_cpus) { - return NULL; - } + ipi_states[cpu_addr] = state; +} +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) +{ return ipi_states[cpu_addr]; } diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h index 877eac7..62eb810 100644 --- a/target-s390x/cpu.h +++ b/target-s390x/cpu.h @@ -377,7 +377,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type, uint8_t *s390_get_storage_keys(void); void s390_alloc_storage_keys(ram_addr_t ram_size); - +void s390_cpu_set_ipistate(uint16_t cpu_addr, S390CPU *state); S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); void s390_add_running_cpu(S390CPU *cpu); unsigned s390_del_running_cpu(S390CPU *cpu);