| Message ID | 1393435114-26374-3-git-send-email-jjherne@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 26.02.2014 18:18, 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> This patch is *still* present despite your previous response that you incorporated my review comments. You are adding a custom s390x API here rather than reusing the generic QOM API as requested. I don't even read further in such cases - like I pointed out, a pure resend is not helping address review feedback! Andreas
On 02/28/2014 11:30 AM, Andreas Färber wrote: > Am 26.02.2014 18:18, 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> > > This patch is *still* present despite your previous response that you > incorporated my review comments. You are adding a custom s390x API here > rather than reusing the generic QOM API as requested. I don't even read > further in such cases - like I pointed out, a pure resend is not helping > address review feedback! > > Andreas > Andreas, My apologies, I am not trying to ruffle any feathers and I certainly have not intended to make you feel like I've ignored your review comments. I believe, if you look at patches 3 and 4 you will find that I've address the majority of your comments. I've even completed the work that adds the link property as you have suggested. The reason the ipi_array still exists in my patch set is because I was under the impression that we needed it to still exist. This belief stemmed from a misunderstanding I had related to an earlier e-mail discussion. I apologize for getting that detail wrong. I had assumed we would make use of the link property when possible but still keep the array around too. At this point, I only have one concern about removing the array. ipi_states is accessed from some high frequency code paths dealing with interrupt handling. I have not yet looked into it myself, but I trust I would find that retrieving the link property value would not significantly affect performance? Thank you for taking the time to read this patch set. Please let me know if you have any more concerns. I'm happy to work with you to address them. P.S. Sorry for taking so long to respond, I was away on vacation for a few days and I was not checking mail.
On 02/28/2014 11:30 AM, Andreas Färber wrote: > Am 26.02.2014 18:18, 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> > > This patch is *still* present despite your previous response that you > incorporated my review comments. You are adding a custom s390x API here > rather than reusing the generic QOM API as requested. I don't even read > further in such cases - like I pointed out, a pure resend is not helping > address review feedback! > > Andreas > Just a quick follow on note. Even though the link property has been added by this patch set, I just realized this part of the code is not yet complete. I'll work up a complete patch just to address this issue and post it separate from Hotplug. I think that makes the most sense because this is really a qom patch.
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c index 00807e7..d47fecf 100644 --- a/hw/s390x/s390-virtio.c +++ b/hw/s390x/s390-virtio.c @@ -55,12 +55,13 @@ static VirtIOS390Bus *s390_bus; static S390CPU **ipi_states; uint8_t *storage_keys; -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 1f4ca4f..79f47b4 100644 --- a/target-s390x/cpu.h +++ b/target-s390x/cpu.h @@ -372,6 +372,7 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type, extern uint8_t *storage_keys; +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);