On 12/16/2013 04:42 PM, Alexander Graf wrote:
>
> On 16.12.2013, at 21:51, Matthew Rosato <mjros...@linux.vnet.ibm.com> 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 <mjros...@linux.vnet.ibm.com>
>> ---
>> 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
>
>
>
>
