>> As migration support is not working, this change cannot really break
>> migration. As the memory hotplug device was always created, we can
>> simply continue faking support for SCLP_FC_ASSIGN_ATTACH_READ_STOR and
>> expose the same information just as if no maxmem and slots parameter was
>> given on the command line (to not break migration of ordinary guests).
>
> I'm a bit worried here, though. Doesn't that imply a guest-visible
> change?
See below.
>
>>
>> Signed-off-by: David Hildenbrand <da...@redhat.com>
>> ---
>> hw/s390x/sclp.c | 261
>> ++++--------------------------------------------
>> include/hw/s390x/sclp.h | 19 ----
>> target/s390x/cpu.h | 2 -
>> 3 files changed, 18 insertions(+), 264 deletions(-)
>
> Nice diffstat :)
>
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 276972b59f..0a2114e592 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -15,9 +15,7 @@
>> #include "qemu/osdep.h"
>> #include "qapi/error.h"
>> #include "cpu.h"
>> -#include "exec/memory.h"
>> #include "sysemu/sysemu.h"
>> -#include "exec/address-spaces.h"
>> #include "hw/boards.h"
>> #include "hw/s390x/sclp.h"
>> #include "hw/s390x/event-facility.h"
>> @@ -57,10 +55,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>> {
>> ReadInfo *read_info = (ReadInfo *) sccb;
>> MachineState *machine = MACHINE(qdev_get_machine());
>> - sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev();
>> int cpu_count;
>> int rnsize, rnmax;
>> - int slots = MIN(machine->ram_slots, s390_get_memslot_count());
>> IplParameterBlock *ipib = s390_ipl_get_iplb();
>>
>> /* CPU information */
>> @@ -78,38 +74,9 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>> read_info->conf_char_ext);
>>
>> read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>> - SCLP_HAS_IOA_RECONFIG);
>> -
>> - /* Memory Hotplug is only supported for the ccw machine type */
>> - if (mhd) {
>> - mhd->standby_subregion_size = MEM_SECTION_SIZE;
>> - /* Deduct the memory slot already used for core */
>> - if (slots > 0) {
>> - while ((mhd->standby_subregion_size * (slots - 1)
>> - < mhd->standby_mem_size)) {
>> - mhd->standby_subregion_size = mhd->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 (mhd->standby_state_map == 0) {
>> - if (mhd->standby_mem_size % mhd->standby_subregion_size) {
>> - mhd->standby_state_map = g_malloc0((mhd->standby_mem_size /
>> - mhd->standby_subregion_size +
>> 1) *
>> - (mhd->standby_subregion_size /
>> - MEM_SECTION_SIZE));
>> - } else {
>> - mhd->standby_state_map = g_malloc0(mhd->standby_mem_size /
>> - MEM_SECTION_SIZE);
>> - }
>> - }
>> - mhd->padded_ram_size = ram_size + mhd->pad_size;
>> - mhd->rzm = 1 << mhd->increment_size;
>> + SCLP_HAS_IOA_RECONFIG |
>> + SCLP_FC_ASSIGN_ATTACH_READ_STOR);
>>
>> - read_info->facilities |=
>> cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR);
>> - }
>
> Previously we only indicated this facility if we actually had standby
> mem, didn't we?
Indeed. I missed that init_sclp_memory_hotplug_dev() was called
conditionally (blindly deleting stuff). This way we can remove even more.
>
> (...)
>
>> static void assign_storage(SCLPDevice *sclp, SCCB *sccb)
>> {
>> - MemoryRegion *mr = NULL;
>> - uint64_t this_subregion_size;
>> - AssignStorage *assign_info = (AssignStorage *) sccb;
>> - sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev();
>> - ram_addr_t assign_addr;
>> - MemoryRegion *sysmem = get_system_memory();
>> -
>> - if (!mhd) {
>> - sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> - return;
>> - }
>> - assign_addr = (be16_to_cpu(assign_info->rn) - 1) * mhd->rzm;
>> -
>> - if ((assign_addr % MEM_SECTION_SIZE == 0) &&
>> - (assign_addr >= mhd->padded_ram_size)) {
>> - /* Re-use existing memory region if found */
>> - mr = memory_region_find(sysmem, assign_addr, 1).mr;
>> - memory_region_unref(mr);
>> - if (!mr) {
>> -
>> - MemoryRegion *standby_ram = g_new(MemoryRegion, 1);
>> -
>> - /* offset to align to standby_subregion_size for allocation */
>> - ram_addr_t offset = assign_addr -
>> - (assign_addr - mhd->padded_ram_size)
>> - % mhd->standby_subregion_size;
>> -
>> - /* strlen("standby.ram") + 4 (Max of KVM_MEMORY_SLOTS) + NULL
>> */
>> - char id[16];
>> - snprintf(id, 16, "standby.ram%d",
>> - (int)((offset - mhd->padded_ram_size) /
>> - mhd->standby_subregion_size) + 1);
>> -
>> - /* Allocate a subregion of the calculated
>> standby_subregion_size */
>> - if (offset + mhd->standby_subregion_size >
>> - mhd->padded_ram_size + mhd->standby_mem_size) {
>> - this_subregion_size = mhd->padded_ram_size +
>> - mhd->standby_mem_size - offset;
>> - } else {
>> - this_subregion_size = mhd->standby_subregion_size;
>> - }
>> -
>> - memory_region_init_ram(standby_ram, NULL, id,
>> this_subregion_size,
>> - &error_fatal);
>> - /* This is a hack to make memory hotunplug work again. Once we
>> have
>> - * subdevices, we have to unparent them when unassigning memory,
>> - * instead of doing it via the ref count of the MemoryRegion. */
>> - object_ref(OBJECT(standby_ram));
>> - object_unparent(OBJECT(standby_ram));
>> - memory_region_add_subregion(sysmem, offset, standby_ram);
>> - }
>> - /* The specified subregion is no longer in standby */
>> - mhd->standby_state_map[(assign_addr - mhd->padded_ram_size)
>> - / MEM_SECTION_SIZE] = 1;
>> - }
>> + /* FIXME, is there really no error code to indicate? */
>> sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
>
> Again, I think we did not actually have a mhd if we did not have
> standby mem, so this already returned invalid command before? What am I
> missing?
Nothing, we actually have to get rid of it.
Thanks
--
Thanks,
David / dhildenb
