Re: [Qemu-devel] [PATCH] s390: Storage key global access

[Jason J. Herne]

On 02/06/2014 10:19 AM, Christian Borntraeger wrote:
> On 22/01/14 16:48, Jason J. Herne wrote:
> > From: "Jason J. Herne" <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> >   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 :).

--
-- Jason J. Herne (address@hidden)