diff mbox series

[RFC,3/5] s390x: prepare device memory address space

Message ID 20200708185135.46694-4-david@redhat.com (mailing list archive)
State New, archived
Headers show
Series s390x: initial support for virtio-mem | expand

Commit Message

David Hildenbrand July 8, 2020, 6:51 p.m. UTC
Let's allocate the device memory information and setup the device
memory address space. Expose the maximum ramsize via SCLP and the actual
initial ramsize via diag260.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c         | 43 ++++++++++++++++++++++++++++++
 hw/s390x/sclp.c                    | 12 +++++++--
 include/hw/s390x/s390-virtio-ccw.h |  3 +++
 target/s390x/diag.c                |  4 +--
 4 files changed, 58 insertions(+), 4 deletions(-)

Comments

Cornelia Huck July 9, 2020, 10:59 a.m. UTC | #1
On Wed,  8 Jul 2020 20:51:33 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's allocate the device memory information and setup the device
> memory address space. Expose the maximum ramsize via SCLP and the actual
> initial ramsize via diag260.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c         | 43 ++++++++++++++++++++++++++++++
>  hw/s390x/sclp.c                    | 12 +++++++--
>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>  target/s390x/diag.c                |  4 +--
>  4 files changed, 58 insertions(+), 4 deletions(-)

(...)

> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index c3b1e24b2c..6b33eb0efc 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -32,8 +32,8 @@ void handle_diag_260(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>      ram_addr_t addr, length;
>      uint64_t tmp;
>  
> -    /* TODO: Unlock with new QEMU machine. */
> -    if (false) {
> +    /* Support for diag260 is glued to support for memory devices. */

I'm wondering why you need to do this... sure, the availability of a
new diagnose could be perceived as a guest-visible change, but does the
information presented change anything? Without memory devices, it will
just duplicate the information already reported via SCLP, IIUC?

> +    if (!memory_devices_allowed()) {
>          s390_program_interrupt(env, PGM_OPERATION, ra);
>          return;
>      }
David Hildenbrand July 10, 2020, 7:46 a.m. UTC | #2
On 09.07.20 12:59, Cornelia Huck wrote:
> On Wed,  8 Jul 2020 20:51:33 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's allocate the device memory information and setup the device
>> memory address space. Expose the maximum ramsize via SCLP and the actual
>> initial ramsize via diag260.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c         | 43 ++++++++++++++++++++++++++++++
>>  hw/s390x/sclp.c                    | 12 +++++++--
>>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>>  target/s390x/diag.c                |  4 +--
>>  4 files changed, 58 insertions(+), 4 deletions(-)
> 
> (...)
> 
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index c3b1e24b2c..6b33eb0efc 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -32,8 +32,8 @@ void handle_diag_260(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>>      ram_addr_t addr, length;
>>      uint64_t tmp;
>>  
>> -    /* TODO: Unlock with new QEMU machine. */
>> -    if (false) {
>> +    /* Support for diag260 is glued to support for memory devices. */
> 
> I'm wondering why you need to do this... sure, the availability of a
> new diagnose could be perceived as a guest-visible change, but does the
> information presented change anything? Without memory devices, it will
> just duplicate the information already reported via SCLP, IIUC?

Yes, it's essentially providing redundant information without memory
devices.

One could sense diag260 in the guest and assume it will work on
successive invocations. E.g., issue subcode 0xc while checking for
exceptions, then issue subcode 0x10 without checking for exceptions. If
we migrate in between, we could be in trouble.

Yes, it's somewhat unlikely, I don't have a strong opinion here. Gluing
it to some migration-safe mechanism (here, the machine) felt like the
right thing to do.
diff mbox series

Patch

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 2e6d292c23..577590e623 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -160,6 +160,35 @@  static void virtio_ccw_register_hcalls(void)
                                    virtio_ccw_hcall_early_printk);
 }
 
+static void s390_device_memory_init(MachineState *machine)
+{
+    MemoryRegion *sysmem = get_system_memory();
+
+    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
+
+    /* initialize device memory address space */
+    if (machine->ram_size < machine->maxram_size) {
+        ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size;
+
+        if (QEMU_ALIGN_UP(machine->maxram_size, MiB) != machine->maxram_size) {
+            error_report("maximum memory size must by aligned to 1 MB");
+            exit(EXIT_FAILURE);
+        }
+
+        machine->device_memory->base = machine->ram_size;
+        if (machine->device_memory->base + device_mem_size < device_mem_size) {
+            error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
+                         machine->maxram_size);
+            exit(EXIT_FAILURE);
+        }
+
+        memory_region_init(&machine->device_memory->mr, OBJECT(machine),
+                           "device-memory", device_mem_size);
+        memory_region_add_subregion(sysmem, machine->device_memory->base,
+                                    &machine->device_memory->mr);
+    }
+}
+
 static void s390_memory_init(MachineState *machine)
 {
     MemoryRegion *sysmem = get_system_memory();
@@ -194,6 +223,11 @@  static void s390_memory_init(MachineState *machine)
     s390_skeys_init();
     /* Initialize storage attributes device */
     s390_stattrib_init();
+
+    /* Support for memory devices is glued to compat machines. */
+    if (memory_devices_allowed()) {
+        s390_device_memory_init(machine);
+    }
 }
 
 static void s390_init_ipl_dev(const char *kernel_filename,
@@ -617,6 +651,7 @@  static void ccw_machine_class_init(ObjectClass *oc, void *data)
     s390mc->cpu_model_allowed = true;
     s390mc->css_migration_enabled = true;
     s390mc->hpage_1m_allowed = true;
+    s390mc->memory_devices_allowed = true;
     mc->init = ccw_init;
     mc->reset = s390_machine_reset;
     mc->hot_add_cpu = s390_hot_add_cpu;
@@ -713,6 +748,11 @@  bool hpage_1m_allowed(void)
     return get_machine_class()->hpage_1m_allowed;
 }
 
+bool memory_devices_allowed(void)
+{
+    return get_machine_class()->memory_devices_allowed;
+}
+
 static char *machine_get_loadparm(Object *obj, Error **errp)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
@@ -831,8 +871,11 @@  static void ccw_machine_5_0_instance_options(MachineState *machine)
 
 static void ccw_machine_5_0_class_options(MachineClass *mc)
 {
+    S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
+
     ccw_machine_5_1_class_options(mc);
     compat_props_add(mc->compat_props, hw_compat_5_0, hw_compat_5_0_len);
+    s390mc->memory_devices_allowed = false;
 }
 DEFINE_CCW_MACHINE(5_0, "5.0", false);
 
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index f59195e15a..85d3505597 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -22,6 +22,7 @@ 
 #include "hw/s390x/event-facility.h"
 #include "hw/s390x/s390-pci-bus.h"
 #include "hw/s390x/ipl.h"
+#include "hw/s390x/s390-virtio-ccw.h"
 
 static inline SCLPDevice *get_sclp_device(void)
 {
@@ -110,8 +111,15 @@  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
         read_info->rnsize2 = cpu_to_be32(rnsize);
     }
 
-    /* we don't support standby memory, maxram_size is never exposed */
-    rnmax = machine->ram_size >> sclp->increment_size;
+    /*
+     * Support for maxram was added with support for memory devices. The
+     * size of the initial memory is exposed via diag260.
+     */
+    if (memory_devices_allowed()) {
+        rnmax = machine->maxram_size >> sclp->increment_size;
+    } else {
+        rnmax = machine->ram_size >> sclp->increment_size;
+    }
     if (rnmax < 0x10000) {
         read_info->rnmax = cpu_to_be16(rnmax);
     } else {
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index cd1dccc6e3..3a1e7e2a6d 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -41,6 +41,7 @@  typedef struct S390CcwMachineClass {
     bool cpu_model_allowed;
     bool css_migration_enabled;
     bool hpage_1m_allowed;
+    bool memory_devices_allowed;
 } S390CcwMachineClass;
 
 /* runtime-instrumentation allowed by the machine */
@@ -49,6 +50,8 @@  bool ri_allowed(void);
 bool cpu_model_allowed(void);
 /* 1M huge page mappings allowed by the machine */
 bool hpage_1m_allowed(void);
+/* Allow memory devices and diag260. */
+bool memory_devices_allowed(void);
 
 /**
  * Returns true if (vmstate based) migration of the channel subsystem
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index c3b1e24b2c..6b33eb0efc 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -32,8 +32,8 @@  void handle_diag_260(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
     ram_addr_t addr, length;
     uint64_t tmp;
 
-    /* TODO: Unlock with new QEMU machine. */
-    if (false) {
+    /* Support for diag260 is glued to support for memory devices. */
+    if (!memory_devices_allowed()) {
         s390_program_interrupt(env, PGM_OPERATION, ra);
         return;
     }