Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs

[Matthew Rosato]

On 12/16/2013 04:42 PM, Alexander Graf wrote:
> 
> On 16.12.2013, at 21:51, Matthew Rosato <address@hidden> wrote:
> 
>> Add memory information to read SCP info and add handlers for
>> Read Storage Element Information, Attach Storage Element,
>> Assign Storage and Unassign Storage.
>>
>> Signed-off-by: Matthew Rosato <address@hidden>
>> ---
>> hw/s390x/sclp.c         |  233 
>> +++++++++++++++++++++++++++++++++++++++++++++--
>> include/hw/s390x/sclp.h |    2 +
>> 2 files changed, 229 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index cb53d7e..5d27fc3 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -15,9 +15,15 @@
>> #include "cpu.h"
>> #include "sysemu/kvm.h"
>> #include "exec/memory.h"
>> -
>> +#include "exec/address-spaces.h"
>> #include "hw/s390x/sclp.h"
>>
>> +int shift;
>> +ram_addr_t standby_subregion_size;
>> +ram_addr_t padded_ram_size;
>> +ram_addr_t rzm;
>> +char *standby_state_map;
> 
> Do you really need all these globals?
> 

I think this ties into your device comment below - I agree that these
should be encapsulated.

>> +
>> static inline S390SCLPDevice *get_event_facility(void)
>> {
>>     ObjectProperty *op = object_property_find(qdev_get_machine(),
>> @@ -31,16 +37,215 @@ static inline S390SCLPDevice *get_event_facility(void)
>> static void read_SCP_info(SCCB *sccb)
>> {
>>     ReadInfo *read_info = (ReadInfo *) sccb;
>> -    int shift = 0;
>> +    int rnsize, rnmax;
>> +    int max_avail_slots = MAX_AVAIL_SLOTS;
>> +#ifdef CONFIG_KVM
>> +    max_avail_slots = kvm_check_extension(kvm_state, KVM_CAP_NR_MEMSLOTS);
> 
> Please don't call kvm specific code from non-kvm specific code. Check out 
> kvm_s390_io_interrupt() as an example how to plumb it in. Please also check 
> kvm_enabled(), as CONFIG_KVM doesn't mean that you actually end up using KVM. 
> It's probably best to completely abstract this away in a helper.
> 

OK, will do.

>> +#endif
>> +
>> +    read_info->facilities = 0;
>> +
>> +    if (standby_mem_size) {
> 
> I don't understand why we have 2 code paths - one for standby and one 
> without. Can't we just handle all cases as standby?
> 

Yes I think so -  everything should fall-through naturally and we can
just skip the malloc of standby_state_map.

>> +        /*
>> +         * The storage increment size is a multiple of 1M and is a power of 
>> 2.
>> +         * The number of storage increments must be 1020 or fewer.
>> +         */
>> +        while ((ram_size >> (20 + shift)) > 1020) {
>> +            shift++;
>> +        }
>> +        while ((standby_mem_size >> (20 + shift)) > 1020) {
>> +            shift++;
>> +        }
>> +
>> +        standby_subregion_size = MEM_SECTION_SIZE;
>> +        /* Deduct the memory slot already used for core */
>> +        while ((standby_subregion_size * (max_avail_slots - 1)
>> +               < standby_mem_size)) {
>> +            standby_subregion_size = standby_subregion_size << 1;
>> +        }
>> +        /*
>> +         * Initialize mapping of guest standby memory sections indicating 
>> which
>> +         * are and are not online. Assume all standby memory begins offline.
>> +         */
>> +        if (standby_mem_size % standby_subregion_size) {
>> +            standby_state_map = g_malloc0((standby_mem_size /
>> +                                           standby_subregion_size + 1) *
>> +                                          (standby_subregion_size /
>> +                                           MEM_SECTION_SIZE));
>> +        } else {
>> +            standby_state_map = g_malloc0(standby_mem_size / 
>> MEM_SECTION_SIZE);
>> +        }
> 
> You don't free this thing when the guest calls read_SCP_info twice.
> 

Oops.  Acknowledged.

>> +
>> +        padded_ram_size = ram_size + pad_size;
>> +        rzm = 1 << (20 + shift);
>> +
>> +    } else {
>> +        while ((ram_size >> (20 + shift)) > 65535) {
>> +            shift++;
>> +        }
>> +    }
>>
>> -    while ((ram_size >> (20 + shift)) > 65535) {
>> -        shift++;
>> +    rnsize = 1 << shift;
>> +    if (rnsize <= 128) {
>> +        read_info->rnsize = rnsize;
>> +    } else {
>> +        read_info->rnsize = 0;
>> +        read_info->rnsize2 = cpu_to_be32(rnsize);
>> +    }
>> +
>> +    rnmax = (ram_size + standby_mem_size + pad_size) >> (20 + shift);
>> +    if (rnmax < 0x10000) {
>> +        read_info->rnmax = cpu_to_be16(rnmax);
>> +    } else {
>> +        read_info->rnmax = cpu_to_be16(0);
>> +        read_info->rnmax2 = cpu_to_be64(rnmax);
>> +    }
>> +
>> +    if (standby_mem_size) {
>> +        read_info->facilities |= 
>> cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
> 
> Any reason to make this conditional?
> 

I suppose not -- I'll make this unconditional.

>>     }
>> -    read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift));
>> -    read_info->rnsize = 1 << shift;
>>     sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
>> }

[...]

>> +static void unassign_storage(SCCB *sccb)
>> +{
>> +    MemoryRegion *mr = NULL;
>> +    AssignStorage *assign_info = (AssignStorage *) sccb;
>> +    ram_addr_t unassign_addr = (assign_info->rn - 1) * rzm;
>> +    MemoryRegion *sysmem = get_system_memory();
>> +
>> +    /* if the addr is a multiple of 256 MB */
>> +    if ((unassign_addr % MEM_SECTION_SIZE == 0) &&
>> +        (unassign_addr >= padded_ram_size)) {
>> +        standby_state_map[(unassign_addr -
>> +                           padded_ram_size) / MEM_SECTION_SIZE] = 0;
>> +        mr = memory_region_find(sysmem, unassign_addr, 1).mr;
>> +        if (mr) {
>> +            int i;
>> +            int is_removable = 1;
>> +            ram_addr_t map_offset = (unassign_addr - padded_ram_size -
>> +                                     (unassign_addr - padded_ram_size)
>> +                                     % standby_subregion_size);
>> +            for (i = 0;
>> +                 i < (standby_subregion_size / MEM_SECTION_SIZE);
>> +                 i++) {
>> +
>> +                if (standby_state_map[i + map_offset / MEM_SECTION_SIZE]) {
>> +                    is_removable = 0;
>> +                    break;
>> +                }
>> +            }
>> +            if (is_removable) {
>> +                memory_region_del_subregion(sysmem, mr);
>> +                memory_region_destroy(mr);
> 
> no g_free()?

Oops.  Acknowledged.

> 
>> +            }
>> +        }
>> +    }
>> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
>> +}
>> +
>> static void sclp_execute(SCCB *sccb, uint64_t code)
>> {
>>     S390SCLPDevice *sdev = get_event_facility();
>> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code)
>>     case SCLP_CMDW_READ_SCP_INFO_FORCED:
>>         read_SCP_info(sccb);
>>         break;
>> +    case SCLP_READ_STORAGE_ELEMENT_INFO:
>> +        if (code & 0xff00) {
>> +            read_storage_element1_info(sccb);
>> +        } else {
>> +            read_storage_element0_info(sccb);
>> +        }
>> +        break;
>> +    case SCLP_ATTACH_STORAGE_ELEMENT:
>> +        attach_storage_element(sccb, (code & 0xff00) >> 8);
>> +        break;
>> +    case SCLP_ASSIGN_STORAGE:
>> +        assign_storage(sccb);
>> +        break;
>> +    case SCLP_UNASSIGN_STORAGE:
>> +        unassign_storage(sccb);
>> +        break;
> 
> Do you think it'd be possible to model this whole thing as a device that has 
> its own state? That's where you could keep the bitmap for example. You'd only 
> need some callback mechanism to hook into the SCLP calls, but the PPC guys 
> already have something similar with their hypercalls.
> 

OK, let me look into this for the next version.

> Overall, the code could use _a lot_ more comments.

Will do.  Thanks for your comments.

> 
> 
> Alex
> 
> 
> 
>