| Message ID | 1390405693-15696-1-git-send-email-jjherne@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 01/22/2014 10:48 AM, Jason J. Herne wrote: > From: "Jason J. Herne" <jjherne@us.ibm.com> > > Introduces global access to storage key data so we can set it for each cpu in > the S390 cpu initialization routine. > > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 3 +-- > hw/s390x/s390-virtio.c | 6 +++--- > hw/s390x/s390-virtio.h | 2 +- > target-s390x/cpu.h | 3 +++ > 4 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 733d988..62319b9 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args) > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *ram = g_new(MemoryRegion, 1); > int shift = 0; > - uint8_t *storage_keys; > int ret; > VirtualCssBus *css_bus; > > @@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args) > storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE); > > /* init CPUs */ > - s390_init_cpus(args->cpu_model, storage_keys); > + s390_init_cpus(args->cpu_model); > > if (kvm_enabled()) { > kvm_s390_enable_css_support(s390_cpu_addr2state(0)); > diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c > index 7adf92a..804483f 100644 > --- a/hw/s390x/s390-virtio.c > +++ b/hw/s390x/s390-virtio.c > @@ -53,6 +53,7 @@ > > static VirtIOS390Bus *s390_bus; > static S390CPU **ipi_states; > +uint8_t *storage_keys; > > S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) > { > @@ -176,7 +177,7 @@ void s390_init_ipl_dev(const char *kernel_filename, > qdev_init_nofail(dev); > } > > -void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys) > +void s390_init_cpus(const char *cpu_model) > { > int i; > > @@ -231,7 +232,6 @@ static void s390_init(QEMUMachineInitArgs *args) > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *ram = g_new(MemoryRegion, 1); > int shift = 0; > - uint8_t *storage_keys; > void *virtio_region; > hwaddr virtio_region_len; > hwaddr virtio_region_start; > @@ -273,7 +273,7 @@ static void s390_init(QEMUMachineInitArgs *args) > storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE); > > /* init CPUs */ > - s390_init_cpus(args->cpu_model, storage_keys); > + s390_init_cpus(args->cpu_model); > > /* Create VirtIO network adapters */ > s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390"); > diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h > index 5c405e7..c1cb042 100644 > --- a/hw/s390x/s390-virtio.h > +++ b/hw/s390x/s390-virtio.h > @@ -20,7 +20,7 @@ > typedef int (*s390_virtio_fn)(const uint64_t *args); > void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn); > > -void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys); > +void s390_init_cpus(const char *cpu_model); > void s390_init_ipl_dev(const char *kernel_filename, > const char *kernel_cmdline, > const char *initrd_filename, > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h > index 68b5ab7..b1432c7 100644 > --- a/target-s390x/cpu.h > +++ b/target-s390x/cpu.h > @@ -381,6 +381,9 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type, > { > } > #endif > + > +extern uint8_t *storage_keys; > + > S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); > void s390_add_running_cpu(S390CPU *cpu); > unsigned s390_del_running_cpu(S390CPU *cpu); > Ping.
On 22/01/14 16:48, Jason J. Herne wrote: > From: "Jason J. Herne" <jjherne@us.ibm.com> > > Introduces global access to storage key data so we can set it for each cpu in > the S390 cpu initialization routine. > > Signed-off-by: Jason J. Herne <jjherne@us.ibm.com> > --- > hw/s390x/s390-virtio-ccw.c | 3 +-- > hw/s390x/s390-virtio.c | 6 +++--- > hw/s390x/s390-virtio.h | 2 +- > target-s390x/cpu.h | 3 +++ > 4 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 733d988..62319b9 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args) > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *ram = g_new(MemoryRegion, 1); > int shift = 0; > - uint8_t *storage_keys; > int ret; > VirtualCssBus *css_bus; > > @@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args) > storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE); > > /* init CPUs */ > - s390_init_cpus(args->cpu_model, storage_keys); > + s390_init_cpus(args->cpu_model); > > if (kvm_enabled()) { > kvm_s390_enable_css_support(s390_cpu_addr2state(0)); > diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c > index 7adf92a..804483f 100644 > --- a/hw/s390x/s390-virtio.c > +++ b/hw/s390x/s390-virtio.c > @@ -53,6 +53,7 @@ > > static VirtIOS390Bus *s390_bus; > static S390CPU **ipi_states; > +uint8_t *storage_keys; This would add another global variable. I am find with this right now but somewhen in the future we might want to take care of storage keys in regard to migration as well. Could we add a container-device/memory? We could make it a child of the machine then? Dont know if that will work out with migration, though. Christian
On 02/06/2014 10:19 AM, Christian Borntraeger wrote: > On 22/01/14 16:48, Jason J. Herne wrote: >> From: "Jason J. Herne" <jjherne@us.ibm.com> >> >> Introduces global access to storage key data so we can set it for each cpu in >> the S390 cpu initialization routine. >> >> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com> >> --- >> hw/s390x/s390-virtio-ccw.c | 3 +-- >> hw/s390x/s390-virtio.c | 6 +++--- >> hw/s390x/s390-virtio.h | 2 +- >> target-s390x/cpu.h | 3 +++ >> 4 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c >> index 733d988..62319b9 100644 >> --- a/hw/s390x/s390-virtio-ccw.c >> +++ b/hw/s390x/s390-virtio-ccw.c >> @@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args) >> MemoryRegion *sysmem = get_system_memory(); >> MemoryRegion *ram = g_new(MemoryRegion, 1); >> int shift = 0; >> - uint8_t *storage_keys; >> int ret; >> VirtualCssBus *css_bus; >> >> @@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args) >> storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE); >> >> /* init CPUs */ >> - s390_init_cpus(args->cpu_model, storage_keys); >> + s390_init_cpus(args->cpu_model); >> >> if (kvm_enabled()) { >> kvm_s390_enable_css_support(s390_cpu_addr2state(0)); >> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c >> index 7adf92a..804483f 100644 >> --- a/hw/s390x/s390-virtio.c >> +++ b/hw/s390x/s390-virtio.c >> @@ -53,6 +53,7 @@ >> >> static VirtIOS390Bus *s390_bus; >> static S390CPU **ipi_states; >> +uint8_t *storage_keys; > > This would add another global variable. I am find with this right now > but somewhen in the future we might want to take care of storage keys > in regard to migration as well. > > Could we add a container-device/memory? We could make it > a child of the machine then? Dont know if that will work out with > migration, though. > > Christian Sorry for the delay in responding to this. I've been busy tracking down some kernel bugs. I'm not 100% sure what a storage key device would look like at the moment. However I agree that a device is the cleanest approach. I support sticking with the current implementation for now as it gets us cpu hotplug now, as opposed to later. I do have some concerns with respect to migration if storage keys are a separate device. I think that is a discussion for a different time though. Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :).
On 25/02/14 15:34, Jason J. Herne wrote:
> Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :).
Do you feel your series is ready for upstream, then yet please post the whole series.
Posting independent things is good, but I feel that the storage key rework makes more
sense if the followup patches make clear why.
Christian
Hi, Am 25.02.2014 20:15, schrieb Christian Borntraeger: > On 25/02/14 15:34, Jason J. Herne wrote: > >> Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :). > > Do you feel your series is ready for upstream, then yet please post the whole series. > Posting independent things is good, but I feel that the storage key rework makes more > sense if the followup patches make clear why. I had requested changes to that series that apparently I could not communicate in a form Jason could digest, and I have since been caught in downstream work and a backlog of other patches, not getting to writing the alternative myself yet nor will I the next few days. An outline of the idea as far as I remember was dropping the ipi array instead of refactoring it to dynamic allocation and - having discussed that a topology will not be needed - add them as cpu[n] child<s390-cpu> properties of /machine, allowing access via QOM property getters instead of some self-cooked solution. Open question was link<> or child<> property and, if link<>, whether some setter hook in QOM infrastructure may be needed to trigger the hot-add or whether QOM realize event will be sufficient. I'd still be interested in getting vCPU hotplug for s390x in 2.0, so maybe you can re-read my previous comments with a view to making -device and cpu-add work with minimum workarounds on your own? We still don't have model subclasses BTW, do we? Regards, Andreas
On 25/02/14 20:34, Andreas Färber wrote: > Hi, > > Am 25.02.2014 20:15, schrieb Christian Borntraeger: >> On 25/02/14 15:34, Jason J. Herne wrote: >> >>> Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :). >> >> Do you feel your series is ready for upstream, then yet please post the whole series. >> Posting independent things is good, but I feel that the storage key rework makes more >> sense if the followup patches make clear why. > > I had requested changes to that series that apparently I could not > communicate in a form Jason could digest, and I have since been caught > in downstream work and a backlog of other patches, not getting to > writing the alternative myself yet nor will I the next few days. I think posting the full series is the right thing to do. We already merged the sclp related changes, so the leftover patch(es) should be pretty small. Maybe the current state is already pretty close to what you want. > > An outline of the idea as far as I remember was dropping the ipi array > instead of refactoring it to dynamic allocation and - having discussed > that a topology will not be needed - add them as cpu[n] child<s390-cpu> > properties of /machine, allowing access via QOM property getters instead > of some self-cooked solution. Open question was link<> or child<> > property and, if link<>, whether some setter hook in QOM infrastructure > may be needed to trigger the hot-add or whether QOM realize event will > be sufficient. > > I'd still be interested in getting vCPU hotplug for s390x in 2.0, so > maybe you can re-read my previous comments with a view to making -device > and cpu-add work with minimum workarounds on your own? We still don't > have model subclasses BTW, do we? > > Regards, > Andreas >
On 02/25/2014 02:34 PM, Andreas Färber wrote: > Hi, > > Am 25.02.2014 20:15, schrieb Christian Borntraeger: >> On 25/02/14 15:34, Jason J. Herne wrote: >> >>> Christian, at one point you mentioned that it might be helpful to see this patch in the context of the rest of the hotplug patches. If you still feel this way let me know and I'll post the 4-patch series. If not, I still propose this one for s390-next. Thanks :). >> >> Do you feel your series is ready for upstream, then yet please post the whole series. >> Posting independent things is good, but I feel that the storage key rework makes more >> sense if the followup patches make clear why. > > I had requested changes to that series that apparently I could not > communicate in a form Jason could digest, and I have since been caught > in downstream work and a backlog of other patches, not getting to > writing the alternative myself yet nor will I the next few days. > Andreas, I believe I understood about 90% of what you were asking for. I had made many of the changes you requested. The last version of my patches can be seen here https://www.mail-archive.com/qemu-devel@nongnu.org/msg201796.html Most of the requested changes should be found within this set of patches. > An outline of the idea as far as I remember was dropping the ipi array > instead of refactoring it to dynamic allocation and - having discussed > that a topology will not be needed - add them as cpu[n] child<s390-cpu> > properties of /machine, allowing access via QOM property getters instead > of some self-cooked solution. Open question was link<> or child<> > property and, if link<>, whether some setter hook in QOM infrastructure > may be needed to trigger the hot-add or whether QOM realize event will > be sufficient. As per your original suggestion to try link<>, I wrote the code to do exactly that. Please see patch 6/8 in the above series. I have a version of these patches rebased for the latest master, but my test system is currently in use. I would like a chance to regression test them before I post them. I anticipate I should be able to do this today. At your option, you may opt to review the patches as posted at the above link. While some modifications have since been made, the overall design and function is the same. I look forward to your comments. Thank you for your time.
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 733d988..62319b9 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -80,7 +80,6 @@ static void ccw_init(QEMUMachineInitArgs *args) MemoryRegion *sysmem = get_system_memory(); MemoryRegion *ram = g_new(MemoryRegion, 1); int shift = 0; - uint8_t *storage_keys; int ret; VirtualCssBus *css_bus; @@ -112,7 +111,7 @@ static void ccw_init(QEMUMachineInitArgs *args) storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE); /* init CPUs */ - s390_init_cpus(args->cpu_model, storage_keys); + s390_init_cpus(args->cpu_model); if (kvm_enabled()) { kvm_s390_enable_css_support(s390_cpu_addr2state(0)); diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c index 7adf92a..804483f 100644 --- a/hw/s390x/s390-virtio.c +++ b/hw/s390x/s390-virtio.c @@ -53,6 +53,7 @@ static VirtIOS390Bus *s390_bus; static S390CPU **ipi_states; +uint8_t *storage_keys; S390CPU *s390_cpu_addr2state(uint16_t cpu_addr) { @@ -176,7 +177,7 @@ void s390_init_ipl_dev(const char *kernel_filename, qdev_init_nofail(dev); } -void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys) +void s390_init_cpus(const char *cpu_model) { int i; @@ -231,7 +232,6 @@ static void s390_init(QEMUMachineInitArgs *args) MemoryRegion *sysmem = get_system_memory(); MemoryRegion *ram = g_new(MemoryRegion, 1); int shift = 0; - uint8_t *storage_keys; void *virtio_region; hwaddr virtio_region_len; hwaddr virtio_region_start; @@ -273,7 +273,7 @@ static void s390_init(QEMUMachineInitArgs *args) storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE); /* init CPUs */ - s390_init_cpus(args->cpu_model, storage_keys); + s390_init_cpus(args->cpu_model); /* Create VirtIO network adapters */ s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390"); diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h index 5c405e7..c1cb042 100644 --- a/hw/s390x/s390-virtio.h +++ b/hw/s390x/s390-virtio.h @@ -20,7 +20,7 @@ typedef int (*s390_virtio_fn)(const uint64_t *args); void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn); -void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys); +void s390_init_cpus(const char *cpu_model); void s390_init_ipl_dev(const char *kernel_filename, const char *kernel_cmdline, const char *initrd_filename, diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h index 68b5ab7..b1432c7 100644 --- a/target-s390x/cpu.h +++ b/target-s390x/cpu.h @@ -381,6 +381,9 @@ static inline void kvm_s390_interrupt_internal(S390CPU *cpu, int type, { } #endif + +extern uint8_t *storage_keys; + S390CPU *s390_cpu_addr2state(uint16_t cpu_addr); void s390_add_running_cpu(S390CPU *cpu); unsigned s390_del_running_cpu(S390CPU *cpu);