[PATCH RFCv3 0/9] s390x: initial support for virtio-mem

[PATCH RFCv3 0/9] s390x: initial support for virtio-mem

[David Hildenbrand]

This wires up the initial, basic version of virito-mem for s390x. General
information about virtio-mem can be found at [1] and in QEMU commit [2].
Patch #5 contains a short example for s390x.

virtio-mem for x86-64 Linux is part of v5.8-rc1. A branch with a s390x
prototype can be found at:
    git@github.com:davidhildenbrand/linux.git virtio-mem-s390x

Note that the kernel should either be compiled via
 CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE, or "memhp_default_state=online"
 should be passed on the kernel cmdline.

This series can be found at:
    git@github.com:davidhildenbrand/qemu.git virtio-mem-s390x-rfcv3

Related to s390x, we'll have to tackle migration of storage keys and
storage attributes (especially, maybe skipping unplugged parts). Not sure
if I am missing something else (any ideas?). For virtio-mem in general,
there are a couple of TODOs, e.g., documented in [1] and [2], both in QEMU
and Linux. However, the basics are around.

I only tested this with fairly small amount of RAM in a z/VM environemnt
and under TCG ...

[1] https://virtio-mem.gitlab.io/
[2] 910b25766b33 ("virtio-mem: Paravirtualized memory hot(un)plug")

RFCv2 -> RFCv3:
- Use a new diag500 subcode to communicate the memory device region. Don't
  use diag260. Clarify why device memory is never exposed via other
  HW/firmware interface. (thanks Heiko for the discussion and looking into
  this!). This needed kernel changes (updated the Linux kernel branch)
- Added
-- "s390x: remove hypercall registration mechanism"
-- "s390x: prepare for more diag500 hypercalls"
-- "s390x: rename s390-virtio-hcall* to s390-hypercall*"
-- "s390x/diag500: subcode to query device memory region"
- "s390x: initial support for virtio-mem"
-- Also wire-up virtio-mem-pci, although it does not seem to be compatible
   (yet?) due to MSI-X

RFCv1 -> RFCv2:
- "s390x/diag: no need to check for PGM_PRIVILEGED in diag308"
-- Added
- "s390x/diag: implement diag260"
-- Implement according to doc (fix error cases)
-- Implement subcode 0xc.
-- Enable the new diag unconditionally
- "s390x: prepare device memory address space"
-- Expose maxram size now via diag260 (0xc), not via SCLP. Unfmodified
   guests can now boot without any issues. This needed kernel changes
   (updated the Linux kernel branch)
- "s390x: implement virtio-mem-ccw"
-- Force virtio revision 1

David Hildenbrand (9):
  s390x: move setting of maximum ram size to machine init
  s390x/diag: no need to check for PGM_PRIVILEGED in diag308
  s390x: remove hypercall registration mechanism
  s390x: prepare for more diag500 hypercalls
  s390x: rename s390-virtio-hcall* to s390-hypercall*
  s390x/diag: subcode to query device memory region
  s390x: prepare device memory address space
  s390x: implement virtio-mem-ccw
  s390x: initial support for virtio-mem

 hw/s390x/Kconfig                   |   1 +
 hw/s390x/Makefile.objs             |   3 +-
 hw/s390x/s390-hypercall.c          |  79 +++++++++++
 hw/s390x/s390-hypercall.h          |  22 +++
 hw/s390x/s390-virtio-ccw.c         | 207 +++++++++++++++++++++++------
 hw/s390x/s390-virtio-hcall.c       |  41 ------
 hw/s390x/s390-virtio-hcall.h       |  23 ----
 hw/s390x/sclp.c                    |  21 +--
 hw/s390x/virtio-ccw-mem.c          | 167 +++++++++++++++++++++++
 hw/s390x/virtio-ccw.h              |  13 ++
 hw/virtio/virtio-mem.c             |   2 +
 include/hw/s390x/s390-virtio-ccw.h |   3 +
 target/s390x/diag.c                |   5 -
 target/s390x/kvm.c                 |  17 +--
 target/s390x/misc_helper.c         |   5 +-
 15 files changed, 466 insertions(+), 143 deletions(-)
 create mode 100644 hw/s390x/s390-hypercall.c
 create mode 100644 hw/s390x/s390-hypercall.h
 delete mode 100644 hw/s390x/s390-virtio-hcall.c
 delete mode 100644 hw/s390x/s390-virtio-hcall.h
 create mode 100644 hw/s390x/virtio-ccw-mem.c

-- 
2.26.2

[PATCH RFCv3 1/9] s390x: move setting of maximum ram size to machine init

[David Hildenbrand]

As we no longer fixup the maximum ram size in sclp code, let's move
setting the maximum ram size to ccw_init()->s390_memory_init(), which
now looks like a better fit.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 19 ++++++++++++++++---
 hw/s390x/sclp.c            | 15 +--------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8cc2f25d8a..dca8e43001 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -158,13 +158,26 @@ static void virtio_ccw_register_hcalls(void)
                                    virtio_ccw_hcall_early_printk);
 }
 
-static void s390_memory_init(MemoryRegion *ram)
+static void s390_memory_init(MachineState *machine)
 {
     MemoryRegion *sysmem = get_system_memory();
     Error *local_err = NULL;
+    uint64_t hw_limit;
+    int ret;
+
+    /* We have to set the memory limit before adding any regions to sysmem. */
+    ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
+    if (ret == -E2BIG) {
+        error_report("host supports a maximum of %" PRIu64 " GB",
+                     hw_limit / GiB);
+        exit(EXIT_FAILURE);
+    } else if (ret) {
+        error_report("setting the guest size failed");
+        exit(EXIT_FAILURE);
+    }
 
     /* allocate RAM for core */
-    memory_region_add_subregion(sysmem, 0, ram);
+    memory_region_add_subregion(sysmem, 0, machine->ram);
 
     /*
      * Configure the maximum page size. As no memory devices were created
@@ -247,7 +260,7 @@ static void ccw_init(MachineState *machine)
 
     s390_sclp_init();
     /* init memory + setup max page size. Required for the CPU model */
-    s390_memory_init(machine->ram);
+    s390_memory_init(machine);
 
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index a0ce444b4b..f59195e15a 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -327,27 +327,14 @@ void s390_sclp_init(void)
 
 static void sclp_realize(DeviceState *dev, Error **errp)
 {
-    MachineState *machine = MACHINE(qdev_get_machine());
     SCLPDevice *sclp = SCLP(dev);
-    uint64_t hw_limit;
-    int ret;
 
     /*
      * qdev_device_add searches the sysbus for TYPE_SCLP_EVENTS_BUS. As long
      * as we can't find a fitting bus via the qom tree, we have to add the
      * event facility to the sysbus, so e.g. a sclp console can be created.
      */
-    if (!sysbus_realize(SYS_BUS_DEVICE(sclp->event_facility), errp)) {
-        return;
-    }
-
-    ret = s390_set_memory_limit(machine->maxram_size, &hw_limit);
-    if (ret == -E2BIG) {
-        error_setg(errp, "host supports a maximum of %" PRIu64 " GB",
-                   hw_limit / GiB);
-    } else if (ret) {
-        error_setg(errp, "setting the guest size failed");
-    }
+    sysbus_realize(SYS_BUS_DEVICE(sclp->event_facility), errp);
 }
 
 static void sclp_memory_init(SCLPDevice *sclp)
-- 
2.26.2

Re: [PATCH RFCv3 1/9] s390x: move setting of maximum ram size to machine init

[Cornelia Huck]

On Fri, 24 Jul 2020 16:37:42 +0200
David Hildenbrand <david@redhat.com> wrote:

> As we no longer fixup the maximum ram size in sclp code, let's move
> setting the maximum ram size to ccw_init()->s390_memory_init(), which
> now looks like a better fit.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 19 ++++++++++++++++---
>  hw/s390x/sclp.c            | 15 +--------------
>  2 files changed, 17 insertions(+), 17 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH RFCv3 2/9] s390x/diag: no need to check for PGM_PRIVILEGED in diag308

[David Hildenbrand]

Whenever we reach this point via KVM or TCG, we already verified that we
are running in the supervisor state.

TCG checks this via IF_PRIV, KVM checks this directly in the diag
instruction handler, before exiting to userspace.

Acked-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/diag.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 1a48429564..be70aecd72 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -80,11 +80,6 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
     uint64_t subcode = env->regs[r3];
     IplParameterBlock *iplb;
 
-    if (env->psw.mask & PSW_MASK_PSTATE) {
-        s390_program_interrupt(env, PGM_PRIVILEGED, ra);
-        return;
-    }
-
     if (subcode & ~0x0ffffULL) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
-- 
2.26.2

Re: [PATCH RFCv3 2/9] s390x/diag: no need to check for PGM_PRIVILEGED in diag308

[Cornelia Huck]

On Fri, 24 Jul 2020 16:37:43 +0200
David Hildenbrand <david@redhat.com> wrote:

> Whenever we reach this point via KVM or TCG, we already verified that we
> are running in the supervisor state.
> 
> TCG checks this via IF_PRIV, KVM checks this directly in the diag
> instruction handler, before exiting to userspace.
> 
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/diag.c | 5 -----
>  1 file changed, 5 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH RFCv3 3/9] s390x: remove hypercall registration mechanism

[David Hildenbrand]

Nowadays, we only have a single machine type in QEMU, everything is based
on virtio-ccw and the traditional virtio machine does no longer exist. No
need to dynamically register diag500 handlers. Move the two existing
handlers into diag500.c.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c   | 46 -----------------------------
 hw/s390x/s390-virtio-hcall.c | 56 ++++++++++++++++++++++++------------
 hw/s390x/s390-virtio-hcall.h |  2 --
 3 files changed, 38 insertions(+), 66 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index dca8e43001..b481e5e5bd 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -17,7 +17,6 @@
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
 #include "exec/ram_addr.h"
-#include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/ioinst.h"
@@ -116,48 +115,6 @@ static void subsystem_reset(void)
     }
 }
 
-static int virtio_ccw_hcall_notify(const uint64_t *args)
-{
-    uint64_t subch_id = args[0];
-    uint64_t queue = args[1];
-    SubchDev *sch;
-    int cssid, ssid, schid, m;
-
-    if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) {
-        return -EINVAL;
-    }
-    sch = css_find_subch(m, cssid, ssid, schid);
-    if (!sch || !css_subch_visible(sch)) {
-        return -EINVAL;
-    }
-    if (queue >= VIRTIO_QUEUE_MAX) {
-        return -EINVAL;
-    }
-    virtio_queue_notify(virtio_ccw_get_vdev(sch), queue);
-    return 0;
-
-}
-
-static int virtio_ccw_hcall_early_printk(const uint64_t *args)
-{
-    uint64_t mem = args[0];
-
-    if (mem < ram_size) {
-        /* Early printk */
-        return 0;
-    }
-    return -EINVAL;
-}
-
-static void virtio_ccw_register_hcalls(void)
-{
-    s390_register_virtio_hypercall(KVM_S390_VIRTIO_CCW_NOTIFY,
-                                   virtio_ccw_hcall_notify);
-    /* Tolerate early printk. */
-    s390_register_virtio_hypercall(KVM_S390_VIRTIO_NOTIFY,
-                                   virtio_ccw_hcall_early_printk);
-}
-
 static void s390_memory_init(MachineState *machine)
 {
     MemoryRegion *sysmem = get_system_memory();
@@ -284,9 +241,6 @@ static void ccw_init(MachineState *machine)
                               OBJECT(dev));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
-    /* register hypercalls */
-    virtio_ccw_register_hcalls();
-
     s390_enable_css_support(s390_cpu_addr2state(0));
 
     ret = css_create_css_image(VIRTUAL_CSSID, true);
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
index ec7cf8beb3..5e14bd49b7 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -12,30 +12,50 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/ioinst.h"
+#include "hw/s390x/css.h"
+#include "virtio-ccw.h"
 
-#define MAX_DIAG_SUBCODES 255
-
-static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
-
-void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
+static int handle_virtio_notify(uint64_t mem)
 {
-    assert(code < MAX_DIAG_SUBCODES);
-    assert(!s390_diag500_table[code]);
-
-    s390_diag500_table[code] = fn;
+    if (mem < ram_size) {
+        /* Tolerate early printk. */
+        return 0;
+    }
+    return -EINVAL;
 }
 
-int s390_virtio_hypercall(CPUS390XState *env)
+static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t queue)
 {
-    s390_virtio_fn fn;
+    SubchDev *sch;
+    int cssid, ssid, schid, m;
 
-    if (env->regs[1] < MAX_DIAG_SUBCODES) {
-        fn = s390_diag500_table[env->regs[1]];
-        if (fn) {
-            env->regs[2] = fn(&env->regs[2]);
-            return 0;
-        }
+    if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) {
+        return -EINVAL;
     }
+    sch = css_find_subch(m, cssid, ssid, schid);
+    if (!sch || !css_subch_visible(sch)) {
+        return -EINVAL;
+    }
+    if (queue >= VIRTIO_QUEUE_MAX) {
+        return -EINVAL;
+    }
+    virtio_queue_notify(virtio_ccw_get_vdev(sch), queue);
+    return 0;
+}
 
-    return -EINVAL;
+int s390_virtio_hypercall(CPUS390XState *env)
+{
+     const uint64_t subcode = env->regs[1];
+
+     switch (subcode) {
+     case KVM_S390_VIRTIO_NOTIFY:
+         env->regs[2] = handle_virtio_notify(env->regs[2]);
+         return 0;
+     case KVM_S390_VIRTIO_CCW_NOTIFY:
+         env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
+         return 0;
+     default:
+         return -EINVAL;
+     }
 }
diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
index 9800c4b351..67e11ea39a 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -17,7 +17,5 @@
 /* The only thing that we need from the old kvm_virtio.h file */
 #define KVM_S390_VIRTIO_NOTIFY 0
 
-typedef int (*s390_virtio_fn)(const uint64_t *args);
-void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
 int s390_virtio_hypercall(CPUS390XState *env);
 #endif /* HW_S390_VIRTIO_HCALL_H */
-- 
2.26.2

Re: [PATCH RFCv3 3/9] s390x: remove hypercall registration mechanism

[Cornelia Huck]

On Fri, 24 Jul 2020 16:37:44 +0200
David Hildenbrand <david@redhat.com> wrote:

> Nowadays, we only have a single machine type in QEMU, everything is based
> on virtio-ccw and the traditional virtio machine does no longer exist. No
> need to dynamically register diag500 handlers. Move the two existing

Hm, do we want to make certain subcodes available only if certain code
has been configured? If yes, it might make sense to keep the mechanism.

> handlers into diag500.c.

In any case, that file does not exist :)

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c   | 46 -----------------------------
>  hw/s390x/s390-virtio-hcall.c | 56 ++++++++++++++++++++++++------------
>  hw/s390x/s390-virtio-hcall.h |  2 --
>  3 files changed, 38 insertions(+), 66 deletions(-)
> 

(...)

> diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
> index ec7cf8beb3..5e14bd49b7 100644
> --- a/hw/s390x/s390-virtio-hcall.c
> +++ b/hw/s390x/s390-virtio-hcall.c
> @@ -12,30 +12,50 @@
>  #include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "hw/s390x/s390-virtio-hcall.h"
> +#include "hw/s390x/ioinst.h"

(Maybe you could remove the ioinst.h include from s390-virtio-ccw.c
with this change?)

> +#include "hw/s390x/css.h"
> +#include "virtio-ccw.h"
>  
> -#define MAX_DIAG_SUBCODES 255
> -
> -static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
> -
> -void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
> +static int handle_virtio_notify(uint64_t mem)
>  {
> -    assert(code < MAX_DIAG_SUBCODES);
> -    assert(!s390_diag500_table[code]);
> -
> -    s390_diag500_table[code] = fn;
> +    if (mem < ram_size) {
> +        /* Tolerate early printk. */

I'm wondering if we still need this. Probably doesn't hurt too much to
keep it around, though.

> +        return 0;
> +    }
> +    return -EINVAL;
>  }
>  

(...)

Re: [PATCH RFCv3 3/9] s390x: remove hypercall registration mechanism

[David Hildenbrand]

On 27.07.20 11:24, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 16:37:44 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Nowadays, we only have a single machine type in QEMU, everything is based
>> on virtio-ccw and the traditional virtio machine does no longer exist. No
>> need to dynamically register diag500 handlers. Move the two existing
> 
> Hm, do we want to make certain subcodes available only if certain code
> has been configured? If yes, it might make sense to keep the mechanism.

I don't see a need to do that. And if it's a compile-time configuration
it can be handled within this file just fine (switch-case).

(the registration/call mechanism, including parameter passing and return
value handling is suboptimal for new subcodes.)

> 
>> handlers into diag500.c.
> 
> In any case, that file does not exist :)

Yeah, after reshuffling code 10 times ... thanks :)

> 
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c   | 46 -----------------------------
>>  hw/s390x/s390-virtio-hcall.c | 56 ++++++++++++++++++++++++------------
>>  hw/s390x/s390-virtio-hcall.h |  2 --
>>  3 files changed, 38 insertions(+), 66 deletions(-)
>>
> 
> (...)
> 
>> diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
>> index ec7cf8beb3..5e14bd49b7 100644
>> --- a/hw/s390x/s390-virtio-hcall.c
>> +++ b/hw/s390x/s390-virtio-hcall.c
>> @@ -12,30 +12,50 @@
>>  #include "qemu/osdep.h"
>>  #include "cpu.h"
>>  #include "hw/s390x/s390-virtio-hcall.h"
>> +#include "hw/s390x/ioinst.h"
> 
> (Maybe you could remove the ioinst.h include from s390-virtio-ccw.c
> with this change?)

I think I tried and it wouldn't compile. But my memory might be wrong.
Will retry.

> 
>> +#include "hw/s390x/css.h"
>> +#include "virtio-ccw.h"
>>  
>> -#define MAX_DIAG_SUBCODES 255
>> -
>> -static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
>> -
>> -void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
>> +static int handle_virtio_notify(uint64_t mem)
>>  {
>> -    assert(code < MAX_DIAG_SUBCODES);
>> -    assert(!s390_diag500_table[code]);
>> -
>> -    s390_diag500_table[code] = fn;
>> +    if (mem < ram_size) {
>> +        /* Tolerate early printk. */
> 
> I'm wondering if we still need this. Probably doesn't hurt too much to
> keep it around, though.

Yeah, I just sticked to current code. Thanks!

-- 
Thanks,

David / dhildenb

Re: [PATCH RFCv3 3/9] s390x: remove hypercall registration mechanism

[Christian Borntraeger]

On 27.07.20 11:24, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 16:37:44 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Nowadays, we only have a single machine type in QEMU, everything is based
>> on virtio-ccw and the traditional virtio machine does no longer exist. No
>> need to dynamically register diag500 handlers. Move the two existing
> 
> Hm, do we want to make certain subcodes available only if certain code
> has been configured? If yes, it might make sense to keep the mechanism.
> 

I think the registration really makes the code hard to read. So Removal
of that is
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>


>> handlers into diag500.c.
> 
> In any case, that file does not exist :)
> 
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c   | 46 -----------------------------
>>  hw/s390x/s390-virtio-hcall.c | 56 ++++++++++++++++++++++++------------
>>  hw/s390x/s390-virtio-hcall.h |  2 --
>>  3 files changed, 38 insertions(+), 66 deletions(-)
>>
> 
> (...)
> 
>> diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
>> index ec7cf8beb3..5e14bd49b7 100644
>> --- a/hw/s390x/s390-virtio-hcall.c
>> +++ b/hw/s390x/s390-virtio-hcall.c
>> @@ -12,30 +12,50 @@
>>  #include "qemu/osdep.h"
>>  #include "cpu.h"
>>  #include "hw/s390x/s390-virtio-hcall.h"
>> +#include "hw/s390x/ioinst.h"
> 
> (Maybe you could remove the ioinst.h include from s390-virtio-ccw.c
> with this change?)
> 
>> +#include "hw/s390x/css.h"
>> +#include "virtio-ccw.h"
>>  
>> -#define MAX_DIAG_SUBCODES 255
>> -
>> -static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
>> -
>> -void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
>> +static int handle_virtio_notify(uint64_t mem)
>>  {
>> -    assert(code < MAX_DIAG_SUBCODES);
>> -    assert(!s390_diag500_table[code]);
>> -
>> -    s390_diag500_table[code] = fn;
>> +    if (mem < ram_size) {
>> +        /* Tolerate early printk. */
> 
> I'm wondering if we still need this. Probably doesn't hurt too much to
> keep it around, though.
> 
>> +        return 0;
>> +    }
>> +    return -EINVAL;
>>  }
>>  
> 
> (...)
> 
> 

[PATCH RFCv3 4/9] s390x: prepare for more diag500 hypercalls

[David Hildenbrand]

Let's generalize, abstacting the virtio bits. diag500 is now a generic
hypercall to handle QEMU/KVM specific things. Explicitly specify all
already defined subcodes, including legacy ones (so we know what we can
use for new hypercalls). While at it, move exception handling into the
handler.

We'll rename the files separately, so git properly detects the rename.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-hcall.c | 14 +++++++-------
 hw/s390x/s390-virtio-hcall.h | 12 ++++++------
 target/s390x/kvm.c           | 15 +++------------
 target/s390x/misc_helper.c   |  3 ++-
 4 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
index 5e14bd49b7..7c4ca5d3b2 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -1,5 +1,5 @@
 /*
- * Support for virtio hypercalls on s390
+ * Support for QEMU/KVM-specific hypercalls on s390
  *
  * Copyright 2012 IBM Corp.
  * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
@@ -44,18 +44,18 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t queue)
     return 0;
 }
 
-int s390_virtio_hypercall(CPUS390XState *env)
+void handle_diag_500(CPUS390XState *env, uintptr_t ra)
 {
      const uint64_t subcode = env->regs[1];
 
      switch (subcode) {
-     case KVM_S390_VIRTIO_NOTIFY:
+     case DIAG500_VIRTIO_NOTIFY:
          env->regs[2] = handle_virtio_notify(env->regs[2]);
-         return 0;
-     case KVM_S390_VIRTIO_CCW_NOTIFY:
+         break;
+     case DIAG500_VIRTIO_CCW_NOTIFY:
          env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
-         return 0;
+         break;
      default:
-         return -EINVAL;
+         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
      }
 }
diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
index 67e11ea39a..2214216ce8 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -1,5 +1,5 @@
 /*
- * Support for virtio hypercalls on s390x
+ * Support for QEMU/KVM-specific hypercalls on s390
  *
  * Copyright IBM Corp. 2012, 2017
  * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
@@ -12,10 +12,10 @@
 #ifndef HW_S390_VIRTIO_HCALL_H
 #define HW_S390_VIRTIO_HCALL_H
 
-#include "standard-headers/asm-s390/virtio-ccw.h"
+#define DIAG500_VIRTIO_NOTIFY          0 /* legacy, implemented as a NOP */
+#define DIAG500_VIRTIO_RESET           1 /* legacy */
+#define DIAG500_VIRTIO_SET_STATUS      2 /* legacy */
+#define DIAG500_VIRTIO_CCW_NOTIFY      3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
 
-/* The only thing that we need from the old kvm_virtio.h file */
-#define KVM_S390_VIRTIO_NOTIFY 0
-
-int s390_virtio_hypercall(CPUS390XState *env);
+void handle_diag_500(CPUS390XState *env, uintptr_t ra);
 #endif /* HW_S390_VIRTIO_HCALL_H */
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index f2f75d2a57..dc00750387 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1551,18 +1551,9 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
     return r;
 }
 
-static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
+static void handle_hypercall(S390CPU *cpu, struct kvm_run *run)
 {
-    CPUS390XState *env = &cpu->env;
-    int ret;
-
-    ret = s390_virtio_hypercall(env);
-    if (ret == -EINVAL) {
-        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
-        return 0;
-    }
-
-    return ret;
+    handle_diag_500(&cpu->env, RA_IGNORED);
 }
 
 static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
@@ -1621,7 +1612,7 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
         kvm_handle_diag_308(cpu, run);
         break;
     case DIAG_KVM_HYPERCALL:
-        r = handle_hypercall(cpu, run);
+        handle_hypercall(cpu, run);
         break;
     case DIAG_KVM_BREAKPOINT:
         r = handle_sw_breakpoint(cpu, run);
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 58dbc023eb..cfcbfbe50c 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -119,8 +119,9 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)
     case 0x500:
         /* KVM hypercall */
         qemu_mutex_lock_iothread();
-        r = s390_virtio_hypercall(env);
+        handle_diag_500(env, GETPC());
         qemu_mutex_unlock_iothread();
+        r = 0;
         break;
     case 0x44:
         /* yield */
-- 
2.26.2

Re: [PATCH RFCv3 4/9] s390x: prepare for more diag500 hypercalls

[Cornelia Huck]

On Fri, 24 Jul 2020 16:37:45 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's generalize, abstacting the virtio bits. diag500 is now a generic
> hypercall to handle QEMU/KVM specific things. Explicitly specify all
> already defined subcodes, including legacy ones (so we know what we can
> use for new hypercalls). While at it, move exception handling into the
> handler.

IIRC, diag 500 had been reserved as "KVM stuff" and not just "virtio
stuff", so that should be fine.

The kernel documentation explicitly talks about "KVM virtio functions",
though; you may want to tweak this (and also add a reference to any new
subcodes.)

[Do we have a good resting place for documenting non-virtio-specific
subcodes?]

> 
> We'll rename the files separately, so git properly detects the rename.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-hcall.c | 14 +++++++-------
>  hw/s390x/s390-virtio-hcall.h | 12 ++++++------
>  target/s390x/kvm.c           | 15 +++------------
>  target/s390x/misc_helper.c   |  3 ++-
>  4 files changed, 18 insertions(+), 26 deletions(-)
>

(...)

> diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
> index 67e11ea39a..2214216ce8 100644
> --- a/hw/s390x/s390-virtio-hcall.h
> +++ b/hw/s390x/s390-virtio-hcall.h
> @@ -1,5 +1,5 @@
>  /*
> - * Support for virtio hypercalls on s390x
> + * Support for QEMU/KVM-specific hypercalls on s390
>   *
>   * Copyright IBM Corp. 2012, 2017
>   * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
> @@ -12,10 +12,10 @@
>  #ifndef HW_S390_VIRTIO_HCALL_H
>  #define HW_S390_VIRTIO_HCALL_H
>  
> -#include "standard-headers/asm-s390/virtio-ccw.h"
> +#define DIAG500_VIRTIO_NOTIFY          0 /* legacy, implemented as a NOP */
> +#define DIAG500_VIRTIO_RESET           1 /* legacy */
> +#define DIAG500_VIRTIO_SET_STATUS      2 /* legacy */
> +#define DIAG500_VIRTIO_CCW_NOTIFY      3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
>  
> -/* The only thing that we need from the old kvm_virtio.h file */
> -#define KVM_S390_VIRTIO_NOTIFY 0

It feels a bit odd to define it here; but this is host/guest api and
won't change anyway.

> -
> -int s390_virtio_hypercall(CPUS390XState *env);
> +void handle_diag_500(CPUS390XState *env, uintptr_t ra);
>  #endif /* HW_S390_VIRTIO_HCALL_H */

(...)

Re: [PATCH RFCv3 4/9] s390x: prepare for more diag500 hypercalls

[Christian Borntraeger]

On 27.07.20 11:42, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 16:37:45 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's generalize, abstacting the virtio bits. diag500 is now a generic
>> hypercall to handle QEMU/KVM specific things. Explicitly specify all
>> already defined subcodes, including legacy ones (so we know what we can
>> use for new hypercalls). While at it, move exception handling into the
>> handler.
> 
> IIRC, diag 500 had been reserved as "KVM stuff" and not just "virtio
> stuff", so that should be fine.
> 
> The kernel documentation explicitly talks about "KVM virtio functions",
> though; you may want to tweak this (and also add a reference to any new
> subcodes.)
> 
> [Do we have a good resting place for documenting non-virtio-specific
> subcodes?]
> 


We have reserved 500-507 for KVM and 500 is marked as "KVM hypervisor call".
As 501 was gdb breakpoint, you could use 502,503 or just a new subcode
for 500.

[PATCH RFCv3 5/9] s390x: rename s390-virtio-hcall* to s390-hypercall*

[David Hildenbrand]

Let's make it clearer that we are talking about general
QEMU/KVM-specific hypercalls.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/Makefile.objs                             | 2 +-
 hw/s390x/{s390-virtio-hcall.c => s390-hypercall.c} | 2 +-
 hw/s390x/{s390-virtio-hcall.h => s390-hypercall.h} | 6 +++---
 target/s390x/kvm.c                                 | 2 +-
 target/s390x/misc_helper.c                         | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)
 rename hw/s390x/{s390-virtio-hcall.c => s390-hypercall.c} (97%)
 rename hw/s390x/{s390-virtio-hcall.h => s390-hypercall.h} (86%)

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index a46a1c7894..2a35be0cda 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -1,4 +1,4 @@
-obj-y += s390-virtio-hcall.o
+obj-y += s390-hypercall.o
 obj-y += sclp.o
 obj-y += event-facility.o
 obj-y += sclpquiesce.o
diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-hypercall.c
similarity index 97%
rename from hw/s390x/s390-virtio-hcall.c
rename to hw/s390x/s390-hypercall.c
index 7c4ca5d3b2..20d4f6e159 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-hypercall.c
@@ -11,7 +11,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
-#include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/s390-hypercall.h"
 #include "hw/s390x/ioinst.h"
 #include "hw/s390x/css.h"
 #include "virtio-ccw.h"
diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-hypercall.h
similarity index 86%
rename from hw/s390x/s390-virtio-hcall.h
rename to hw/s390x/s390-hypercall.h
index 2214216ce8..e6b958db41 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-hypercall.h
@@ -9,8 +9,8 @@
  * directory.
  */
 
-#ifndef HW_S390_VIRTIO_HCALL_H
-#define HW_S390_VIRTIO_HCALL_H
+#ifndef HW_S390_HYPERCALL_H
+#define HW_S390_HYPERCALL_H
 
 #define DIAG500_VIRTIO_NOTIFY          0 /* legacy, implemented as a NOP */
 #define DIAG500_VIRTIO_RESET           1 /* legacy */
@@ -18,4 +18,4 @@
 #define DIAG500_VIRTIO_CCW_NOTIFY      3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
 
 void handle_diag_500(CPUS390XState *env, uintptr_t ra);
-#endif /* HW_S390_VIRTIO_HCALL_H */
+#endif /* HW_S390_HYPERCALL_H */
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index dc00750387..380fa6de26 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -49,7 +49,7 @@
 #include "hw/s390x/ebcdic.h"
 #include "exec/memattrs.h"
 #include "hw/s390x/s390-virtio-ccw.h"
-#include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/s390-hypercall.h"
 #include "hw/s390x/pv.h"
 
 #ifndef DEBUG_KVM
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index cfcbfbe50c..7d173c081c 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -36,7 +36,7 @@
 #include "sysemu/cpus.h"
 #include "sysemu/sysemu.h"
 #include "hw/s390x/ebcdic.h"
-#include "hw/s390x/s390-virtio-hcall.h"
+#include "hw/s390x/s390-hypercall.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/ioinst.h"
-- 
2.26.2

[PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[David Hildenbrand]

A guest OS that is aware of memory devices (placed into the device
memory region located in guest physical address space) has to know at least
the end address of the device memory region during boot, for example, to
prepare the kernel virtual address space accordingly (e.g., select page
table hierarchy). The device memory region is located above the SCLP
maximum storage increment.

Let's provide a new diag500 subcode to query the location of the device
memory region under QEMU/KVM. This way, esp. Linux who's wants to support
virtio-based memory devices can query the location of this region and
derive the maximum possible PFN.

Let's use a specification exception in case no such memory region
exists (e.g., maxmem wasn't specified, or on old QEMU machines). We'll
unlock this with future patches that prepare and instanciate the device
memory region.

Memory managed by memory devices should never be detected and used
without having proper support for them in the guest (IOW, a driver that
detects and handles the devices). It's not exposed via other HW/firmware
interfaces (e.g., SCLP, diag260). In the near future, the focus is on
supporting virtio-based memory devices like vitio-mem. Other memory devices
are imaginable in the future (e.g., expose DIMMs via a KVM-specific
interface to s390x guests).

Note: We don't want to include the device memory region within the
SCLP-defined maximum storage increment, because especially older
guests will will sense (via tprot) accessible memory within this range.
If an unmodified guest would detect and use device memory, it could end
badly. The memory might have different semantics (e.g., a disk provided
via virtio-pmem a.k.a. DAX) and might require a handshake first (e.g.,
unplugged memory part of virtio-mem in some cases), before memory that
might look accessible can actually be used without surprises.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-hypercall.c | 18 ++++++++++++++++++
 hw/s390x/s390-hypercall.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/hw/s390x/s390-hypercall.c b/hw/s390x/s390-hypercall.c
index 20d4f6e159..ac21f4576e 100644
--- a/hw/s390x/s390-hypercall.c
+++ b/hw/s390x/s390-hypercall.c
@@ -11,6 +11,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "hw/boards.h"
 #include "hw/s390x/s390-hypercall.h"
 #include "hw/s390x/ioinst.h"
 #include "hw/s390x/css.h"
@@ -44,6 +45,20 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t queue)
     return 0;
 }
 
+static void handle_device_memory_region(CPUS390XState *env, uintptr_t ra)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+
+    if (!machine->device_memory ||
+        !memory_region_size(&machine->device_memory->mr)) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        return;
+    }
+    env->regs[2] = machine->device_memory->base;
+    env->regs[3] = machine->device_memory->base +
+                   memory_region_size(&machine->device_memory->mr) - 1;
+}
+
 void handle_diag_500(CPUS390XState *env, uintptr_t ra)
 {
      const uint64_t subcode = env->regs[1];
@@ -55,6 +70,9 @@ void handle_diag_500(CPUS390XState *env, uintptr_t ra)
      case DIAG500_VIRTIO_CCW_NOTIFY:
          env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
          break;
+     case DIAG500_DEVICE_MEMORY_REGION:
+        handle_device_memory_region(env, ra);
+        break;
      default:
          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
      }
diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
index e6b958db41..1b179d7d99 100644
--- a/hw/s390x/s390-hypercall.h
+++ b/hw/s390x/s390-hypercall.h
@@ -16,6 +16,7 @@
 #define DIAG500_VIRTIO_RESET           1 /* legacy */
 #define DIAG500_VIRTIO_SET_STATUS      2 /* legacy */
 #define DIAG500_VIRTIO_CCW_NOTIFY      3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
+#define DIAG500_DEVICE_MEMORY_REGION   4
 
 void handle_diag_500(CPUS390XState *env, uintptr_t ra);
 #endif /* HW_S390_HYPERCALL_H */
-- 
2.26.2

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[Cornelia Huck]

On Fri, 24 Jul 2020 16:37:47 +0200
David Hildenbrand <david@redhat.com> wrote:

> A guest OS that is aware of memory devices (placed into the device
> memory region located in guest physical address space) has to know at least
> the end address of the device memory region during boot, for example, to
> prepare the kernel virtual address space accordingly (e.g., select page
> table hierarchy). The device memory region is located above the SCLP
> maximum storage increment.
> 
> Let's provide a new diag500 subcode to query the location of the device
> memory region under QEMU/KVM. This way, esp. Linux who's wants to support
> virtio-based memory devices can query the location of this region and
> derive the maximum possible PFN.
> 
> Let's use a specification exception in case no such memory region
> exists (e.g., maxmem wasn't specified, or on old QEMU machines). We'll
> unlock this with future patches that prepare and instanciate the device
> memory region.

Specification exception on old machines seems reasonable. But maybe
newer machines can use a different return value for "no memory regions"?

> 
> Memory managed by memory devices should never be detected and used
> without having proper support for them in the guest (IOW, a driver that
> detects and handles the devices). It's not exposed via other HW/firmware
> interfaces (e.g., SCLP, diag260). In the near future, the focus is on
> supporting virtio-based memory devices like vitio-mem. Other memory devices
> are imaginable in the future (e.g., expose DIMMs via a KVM-specific
> interface to s390x guests).
> 
> Note: We don't want to include the device memory region within the
> SCLP-defined maximum storage increment, because especially older
> guests will will sense (via tprot) accessible memory within this range.
> If an unmodified guest would detect and use device memory, it could end
> badly. The memory might have different semantics (e.g., a disk provided
> via virtio-pmem a.k.a. DAX) and might require a handshake first (e.g.,
> unplugged memory part of virtio-mem in some cases), before memory that
> might look accessible can actually be used without surprises.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-hypercall.c | 18 ++++++++++++++++++
>  hw/s390x/s390-hypercall.h |  1 +
>  2 files changed, 19 insertions(+)

(...)

> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
> index e6b958db41..1b179d7d99 100644
> --- a/hw/s390x/s390-hypercall.h
> +++ b/hw/s390x/s390-hypercall.h
> @@ -16,6 +16,7 @@
>  #define DIAG500_VIRTIO_RESET           1 /* legacy */
>  #define DIAG500_VIRTIO_SET_STATUS      2 /* legacy */
>  #define DIAG500_VIRTIO_CCW_NOTIFY      3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
> +#define DIAG500_DEVICE_MEMORY_REGION   4

Regardless what we end up with, this needs to be specified
somewhere(tm).

>  
>  void handle_diag_500(CPUS390XState *env, uintptr_t ra);
>  #endif /* HW_S390_HYPERCALL_H */

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[David Hildenbrand]

On 27.07.20 11:48, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 16:37:47 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> A guest OS that is aware of memory devices (placed into the device
>> memory region located in guest physical address space) has to know at least
>> the end address of the device memory region during boot, for example, to
>> prepare the kernel virtual address space accordingly (e.g., select page
>> table hierarchy). The device memory region is located above the SCLP
>> maximum storage increment.
>>
>> Let's provide a new diag500 subcode to query the location of the device
>> memory region under QEMU/KVM. This way, esp. Linux who's wants to support
>> virtio-based memory devices can query the location of this region and
>> derive the maximum possible PFN.
>>
>> Let's use a specification exception in case no such memory region
>> exists (e.g., maxmem wasn't specified, or on old QEMU machines). We'll
>> unlock this with future patches that prepare and instanciate the device
>> memory region.
> 
> Specification exception on old machines seems reasonable. But maybe
> newer machines can use a different return value for "no memory regions"?

Hm, I don't see any benefit to distinguish the two cases of "no device
memory region". Should the guest really care?

[...]
> 
> (...)
> 
>> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
>> index e6b958db41..1b179d7d99 100644
>> --- a/hw/s390x/s390-hypercall.h
>> +++ b/hw/s390x/s390-hypercall.h
>> @@ -16,6 +16,7 @@
>>  #define DIAG500_VIRTIO_RESET           1 /* legacy */
>>  #define DIAG500_VIRTIO_SET_STATUS      2 /* legacy */
>>  #define DIAG500_VIRTIO_CCW_NOTIFY      3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
>> +#define DIAG500_DEVICE_MEMORY_REGION   4
> 
> Regardless what we end up with, this needs to be specified
> somewhere(tm).
> 

Yeah, there, we should also document the existing subcodes. What would
be the right place for this? The kernel feels somewhat wrong to me.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[Cornelia Huck]

On Mon, 27 Jul 2020 11:52:30 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 27.07.20 11:48, Cornelia Huck wrote:
> > On Fri, 24 Jul 2020 16:37:47 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> A guest OS that is aware of memory devices (placed into the device
> >> memory region located in guest physical address space) has to know at least
> >> the end address of the device memory region during boot, for example, to
> >> prepare the kernel virtual address space accordingly (e.g., select page
> >> table hierarchy). The device memory region is located above the SCLP
> >> maximum storage increment.
> >>
> >> Let's provide a new diag500 subcode to query the location of the device
> >> memory region under QEMU/KVM. This way, esp. Linux who's wants to support
> >> virtio-based memory devices can query the location of this region and
> >> derive the maximum possible PFN.
> >>
> >> Let's use a specification exception in case no such memory region
> >> exists (e.g., maxmem wasn't specified, or on old QEMU machines). We'll
> >> unlock this with future patches that prepare and instanciate the device
> >> memory region.  
> > 
> > Specification exception on old machines seems reasonable. But maybe
> > newer machines can use a different return value for "no memory regions"?  
> 
> Hm, I don't see any benefit to distinguish the two cases of "no device
> memory region". Should the guest really care?

No idea, was just a random thought.

> 
> [...]
> > 
> > (...)
> >   
> >> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
> >> index e6b958db41..1b179d7d99 100644
> >> --- a/hw/s390x/s390-hypercall.h
> >> +++ b/hw/s390x/s390-hypercall.h
> >> @@ -16,6 +16,7 @@
> >>  #define DIAG500_VIRTIO_RESET           1 /* legacy */
> >>  #define DIAG500_VIRTIO_SET_STATUS      2 /* legacy */
> >>  #define DIAG500_VIRTIO_CCW_NOTIFY      3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
> >> +#define DIAG500_DEVICE_MEMORY_REGION   4  
> > 
> > Regardless what we end up with, this needs to be specified
> > somewhere(tm).
> >   
> 
> Yeah, there, we should also document the existing subcodes. What would
> be the right place for this? The kernel feels somewhat wrong to me.

The still supported subcode 3 is properly specified in the virtio spec.
That's not a good place for that new one, though.

QEMU is probably a better place than the kernel to specify stuff,
although it's not really ideal, either. OTOH, do we ever expect other
hypervisors to implement this new subcode?

So maybe under doc/specs/?

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[David Hildenbrand]

On 27.07.20 12:09, Cornelia Huck wrote:
> On Mon, 27 Jul 2020 11:52:30 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 27.07.20 11:48, Cornelia Huck wrote:
>>> On Fri, 24 Jul 2020 16:37:47 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> A guest OS that is aware of memory devices (placed into the device
>>>> memory region located in guest physical address space) has to know at least
>>>> the end address of the device memory region during boot, for example, to
>>>> prepare the kernel virtual address space accordingly (e.g., select page
>>>> table hierarchy). The device memory region is located above the SCLP
>>>> maximum storage increment.
>>>>
>>>> Let's provide a new diag500 subcode to query the location of the device
>>>> memory region under QEMU/KVM. This way, esp. Linux who's wants to support
>>>> virtio-based memory devices can query the location of this region and
>>>> derive the maximum possible PFN.
>>>>
>>>> Let's use a specification exception in case no such memory region
>>>> exists (e.g., maxmem wasn't specified, or on old QEMU machines). We'll
>>>> unlock this with future patches that prepare and instanciate the device
>>>> memory region.  
>>>
>>> Specification exception on old machines seems reasonable. But maybe
>>> newer machines can use a different return value for "no memory regions"?  
>>
>> Hm, I don't see any benefit to distinguish the two cases of "no device
>> memory region". Should the guest really care?
> 
> No idea, was just a random thought.
> 
>>
>> [...]
>>>
>>> (...)
>>>   
>>>> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
>>>> index e6b958db41..1b179d7d99 100644
>>>> --- a/hw/s390x/s390-hypercall.h
>>>> +++ b/hw/s390x/s390-hypercall.h
>>>> @@ -16,6 +16,7 @@
>>>>  #define DIAG500_VIRTIO_RESET           1 /* legacy */
>>>>  #define DIAG500_VIRTIO_SET_STATUS      2 /* legacy */
>>>>  #define DIAG500_VIRTIO_CCW_NOTIFY      3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
>>>> +#define DIAG500_DEVICE_MEMORY_REGION   4  
>>>
>>> Regardless what we end up with, this needs to be specified
>>> somewhere(tm).
>>>   
>>
>> Yeah, there, we should also document the existing subcodes. What would
>> be the right place for this? The kernel feels somewhat wrong to me.
> 
> The still supported subcode 3 is properly specified in the virtio spec.
> That's not a good place for that new one, though.
> 
> QEMU is probably a better place than the kernel to specify stuff,
> although it's not really ideal, either. OTOH, do we ever expect other
> hypervisors to implement this new subcode?

cloud-hypervisor implements virtio-mem. If it were ever to support s390x
(guess it does not yet), it would also want to implement that one. But
then, it can just look at QEMU doc I guess :)


-- 
Thanks,

David / dhildenb

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[Heiko Carstens]

On Mon, Jul 27, 2020 at 12:12:02PM +0200, David Hildenbrand wrote:
> >>>> +#define DIAG500_DEVICE_MEMORY_REGION   4  
> >>>
> >>> Regardless what we end up with, this needs to be specified
> >>> somewhere(tm).
> >>
> >> Yeah, there, we should also document the existing subcodes. What would
> >> be the right place for this? The kernel feels somewhat wrong to me.
> > 
> > The still supported subcode 3 is properly specified in the virtio spec.
> > That's not a good place for that new one, though.
> > 
> > QEMU is probably a better place than the kernel to specify stuff,
> > although it's not really ideal, either. OTOH, do we ever expect other
> > hypervisors to implement this new subcode?
> 
> cloud-hypervisor implements virtio-mem. If it were ever to support s390x
> (guess it does not yet), it would also want to implement that one. But
> then, it can just look at QEMU doc I guess :)

It must be well defined and easy to find also for kernel developers
who actually have to care about memory detection code :)

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[David Hildenbrand]

On 27.07.20 13:15, Heiko Carstens wrote:
> On Mon, Jul 27, 2020 at 12:12:02PM +0200, David Hildenbrand wrote:
>>>>>> +#define DIAG500_DEVICE_MEMORY_REGION   4  
>>>>>
>>>>> Regardless what we end up with, this needs to be specified
>>>>> somewhere(tm).
>>>>
>>>> Yeah, there, we should also document the existing subcodes. What would
>>>> be the right place for this? The kernel feels somewhat wrong to me.
>>>
>>> The still supported subcode 3 is properly specified in the virtio spec.
>>> That's not a good place for that new one, though.
>>>
>>> QEMU is probably a better place than the kernel to specify stuff,
>>> although it's not really ideal, either. OTOH, do we ever expect other
>>> hypervisors to implement this new subcode?
>>
>> cloud-hypervisor implements virtio-mem. If it were ever to support s390x
>> (guess it does not yet), it would also want to implement that one. But
>> then, it can just look at QEMU doc I guess :)
> 
> It must be well defined and easy to find also for kernel developers
> who actually have to care about memory detection code :)

So I'd suggest documenting it in QEMU (docs/specs ...) for now, and
referencing it from the relevant Linux patch - other suggestions?

-- 
Thanks,

David / dhildenb

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[Cornelia Huck]

On Mon, 27 Jul 2020 14:02:47 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 27.07.20 13:15, Heiko Carstens wrote:
> > On Mon, Jul 27, 2020 at 12:12:02PM +0200, David Hildenbrand wrote:  
> >>>>>> +#define DIAG500_DEVICE_MEMORY_REGION   4    
> >>>>>
> >>>>> Regardless what we end up with, this needs to be specified
> >>>>> somewhere(tm).  
> >>>>
> >>>> Yeah, there, we should also document the existing subcodes. What would
> >>>> be the right place for this? The kernel feels somewhat wrong to me.  
> >>>
> >>> The still supported subcode 3 is properly specified in the virtio spec.
> >>> That's not a good place for that new one, though.
> >>>
> >>> QEMU is probably a better place than the kernel to specify stuff,
> >>> although it's not really ideal, either. OTOH, do we ever expect other
> >>> hypervisors to implement this new subcode?  
> >>
> >> cloud-hypervisor implements virtio-mem. If it were ever to support s390x
> >> (guess it does not yet), it would also want to implement that one. But
> >> then, it can just look at QEMU doc I guess :)  
> > 
> > It must be well defined and easy to find also for kernel developers
> > who actually have to care about memory detection code :)  
> 
> So I'd suggest documenting it in QEMU (docs/specs ...) for now, and
> referencing it from the relevant Linux patch - other suggestions?

That's probably the easiest way for now... the kernel's s390-diag.rst
should also point to it.

However, I think we really need a central place for definitions that
are not just a Linux/QEMU interface, but can potentially also be used
by other hypervisors/guests. Nothing as complicated as an OASIS spec,
but maybe a git??b project?

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[David Hildenbrand]

On 28.07.20 09:10, Cornelia Huck wrote:
> On Mon, 27 Jul 2020 14:02:47 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 27.07.20 13:15, Heiko Carstens wrote:
>>> On Mon, Jul 27, 2020 at 12:12:02PM +0200, David Hildenbrand wrote:  
>>>>>>>> +#define DIAG500_DEVICE_MEMORY_REGION   4    
>>>>>>>
>>>>>>> Regardless what we end up with, this needs to be specified
>>>>>>> somewhere(tm).  
>>>>>>
>>>>>> Yeah, there, we should also document the existing subcodes. What would
>>>>>> be the right place for this? The kernel feels somewhat wrong to me.  
>>>>>
>>>>> The still supported subcode 3 is properly specified in the virtio spec.
>>>>> That's not a good place for that new one, though.
>>>>>
>>>>> QEMU is probably a better place than the kernel to specify stuff,
>>>>> although it's not really ideal, either. OTOH, do we ever expect other
>>>>> hypervisors to implement this new subcode?  
>>>>
>>>> cloud-hypervisor implements virtio-mem. If it were ever to support s390x
>>>> (guess it does not yet), it would also want to implement that one. But
>>>> then, it can just look at QEMU doc I guess :)  
>>>
>>> It must be well defined and easy to find also for kernel developers
>>> who actually have to care about memory detection code :)  
>>
>> So I'd suggest documenting it in QEMU (docs/specs ...) for now, and
>> referencing it from the relevant Linux patch - other suggestions?
> 
> That's probably the easiest way for now... the kernel's s390-diag.rst
> should also point to it.
> 
> However, I think we really need a central place for definitions that
> are not just a Linux/QEMU interface, but can potentially also be used
> by other hypervisors/guests. Nothing as complicated as an OASIS spec,
> but maybe a git??b project?

Sounds good. Maintainers? I can volunteer (+setup/create initial
version), but would be good to have other QEMU/KVM maintainers there as
well.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[Cornelia Huck]

On Wed, 29 Jul 2020 10:57:58 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 28.07.20 09:10, Cornelia Huck wrote:

> > However, I think we really need a central place for definitions that
> > are not just a Linux/QEMU interface, but can potentially also be used
> > by other hypervisors/guests. Nothing as complicated as an OASIS spec,
> > but maybe a git??b project?  
> 
> Sounds good. Maintainers? I can volunteer (+setup/create initial
> version), but would be good to have other QEMU/KVM maintainers there as
> well.

Count me in. Best as a collection of rst or markdown documents, I guess?

gitlab or github? Either would be fine with me.

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[David Hildenbrand]

On 29.07.20 11:37, Cornelia Huck wrote:
> On Wed, 29 Jul 2020 10:57:58 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 28.07.20 09:10, Cornelia Huck wrote:
> 
>>> However, I think we really need a central place for definitions that
>>> are not just a Linux/QEMU interface, but can potentially also be used
>>> by other hypervisors/guests. Nothing as complicated as an OASIS spec,
>>> but maybe a git??b project?  
>>
>> Sounds good. Maintainers? I can volunteer (+setup/create initial
>> version), but would be good to have other QEMU/KVM maintainers there as
>> well.
> 
> Count me in. Best as a collection of rst or markdown documents, I guess?
> 

Yes, alternatively, gitlab/github pages? (which essentially convert
markdown in the repository to html - both is accessible)

> gitlab or github? Either would be fine with me.

Same on my side.

-- 
Thanks,

David / dhildenb

Re: [PATCH RFCv3 6/9] s390x/diag: subcode to query device memory region

[Cornelia Huck]

On Wed, 29 Jul 2020 11:57:04 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 29.07.20 11:37, Cornelia Huck wrote:
> > On Wed, 29 Jul 2020 10:57:58 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> On 28.07.20 09:10, Cornelia Huck wrote:  
> >   
> >>> However, I think we really need a central place for definitions that
> >>> are not just a Linux/QEMU interface, but can potentially also be used
> >>> by other hypervisors/guests. Nothing as complicated as an OASIS spec,
> >>> but maybe a git??b project?    
> >>
> >> Sounds good. Maintainers? I can volunteer (+setup/create initial
> >> version), but would be good to have other QEMU/KVM maintainers there as
> >> well.  
> > 
> > Count me in. Best as a collection of rst or markdown documents, I guess?
> >   
> 
> Yes, alternatively, gitlab/github pages? (which essentially convert
> markdown in the repository to html - both is accessible)

I guess anything that is (a) easily editable, (b) easily accessible on
the web, and (c) easily downloadable is fine with me.

> 
> > gitlab or github? Either would be fine with me.  
> 
> Same on my side.
> 

[PATCH RFCv3 7/9] s390x: prepare device memory address space

[David Hildenbrand]

Let's allocate the device memory information and setup the device
memory address space. The RAM size returned via SCLP is not modified. Guest
OSs which support memory devices (like virtio-mem) are expected to
consult diag500(4).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c         | 37 ++++++++++++++++++++++++++++++
 hw/s390x/sclp.c                    |  6 ++++-
 include/hw/s390x/s390-virtio-ccw.h |  3 +++
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b481e5e5bd..23b94256e0 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -115,6 +115,29 @@ static void subsystem_reset(void)
     }
 }
 
+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_IS_ALIGNED(machine->maxram_size, MiB)) {
+            error_report("maximum memory size must be aligned to 1 MB");
+            exit(EXIT_FAILURE);
+        }
+
+        machine->device_memory->base = machine->ram_size;
+        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();
@@ -149,6 +172,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,
@@ -569,6 +597,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;
@@ -665,6 +694,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);
@@ -783,8 +817,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..316ff9c218 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,7 +111,10 @@ 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 */
+    /*
+     * We don't support standby memory. Currently, maxram_size is only
+     * implicitly exposed via diag500(4) to indicate the device memory region.
+     */
     rnmax = machine->ram_size >> sclp->increment_size;
     if (rnmax < 0x10000) {
         read_info->rnmax = cpu_to_be16(rnmax);
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index cd1dccc6e3..960eb9344f 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);
+/* memory devices are allowed by the machine (device memory region created). */
+bool memory_devices_allowed(void);
 
 /**
  * Returns true if (vmstate based) migration of the channel subsystem
-- 
2.26.2

Re: [PATCH RFCv3 7/9] s390x: prepare device memory address space

[Cornelia Huck]

On Fri, 24 Jul 2020 16:37:48 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's allocate the device memory information and setup the device
> memory address space. The RAM size returned via SCLP is not modified. Guest
> OSs which support memory devices (like virtio-mem) are expected to
> consult diag500(4).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c         | 37 ++++++++++++++++++++++++++++++
>  hw/s390x/sclp.c                    |  6 ++++-
>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>  3 files changed, 45 insertions(+), 1 deletion(-)
> 

(...)

> @@ -783,8 +817,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;

I guess that needs to go one version up. (I plan to send the 5.2
machines patch in the next days.)

>  }
>  DEFINE_CCW_MACHINE(5_0, "5.0", false);

(...)

Re: [PATCH RFCv3 7/9] s390x: prepare device memory address space

[David Hildenbrand]

On 27.07.20 11:56, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 16:37:48 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's allocate the device memory information and setup the device
>> memory address space. The RAM size returned via SCLP is not modified. Guest
>> OSs which support memory devices (like virtio-mem) are expected to
>> consult diag500(4).
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c         | 37 ++++++++++++++++++++++++++++++
>>  hw/s390x/sclp.c                    |  6 ++++-
>>  include/hw/s390x/s390-virtio-ccw.h |  3 +++
>>  3 files changed, 45 insertions(+), 1 deletion(-)
>>
> 
> (...)
> 
>> @@ -783,8 +817,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;
> 
> I guess that needs to go one version up. (I plan to send the 5.2
> machines patch in the next days.)

Indeed!

-- 
Thanks,

David / dhildenb

[PATCH RFCv3 8/9] s390x: implement virtio-mem-ccw

[David Hildenbrand]

Add a proper CCW proxy device, similar to the PCI variant.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/virtio-ccw-mem.c | 167 ++++++++++++++++++++++++++++++++++++++
 hw/s390x/virtio-ccw.h     |  13 +++
 2 files changed, 180 insertions(+)
 create mode 100644 hw/s390x/virtio-ccw-mem.c

diff --git a/hw/s390x/virtio-ccw-mem.c b/hw/s390x/virtio-ccw-mem.c
new file mode 100644
index 0000000000..c38dafd7f0
--- /dev/null
+++ b/hw/s390x/virtio-ccw-mem.c
@@ -0,0 +1,167 @@
+/*
+ * Virtio MEM CCW device
+ *
+ * Copyright (C) 2020 Red Hat, Inc.
+ *
+ * Authors:
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/virtio.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "virtio-ccw.h"
+#include "hw/mem/memory-device.h"
+#include "qapi/qapi-events-misc.h"
+
+static void virtio_ccw_mem_realize(VirtioCcwDevice *ccw_dev, Error **errp)
+{
+    VirtIOMEMCcw *ccw_mem = VIRTIO_MEM_CCW(ccw_dev);
+    DeviceState *vdev = DEVICE(&ccw_mem->vdev);
+
+    qdev_realize(vdev, BUS(&ccw_dev->bus), errp);
+}
+
+static void virtio_ccw_mem_set_addr(MemoryDeviceState *md, uint64_t addr,
+                                    Error **errp)
+{
+    object_property_set_uint(OBJECT(md), VIRTIO_MEM_ADDR_PROP, addr, errp);
+}
+
+static uint64_t virtio_ccw_mem_get_addr(const MemoryDeviceState *md)
+{
+    return object_property_get_uint(OBJECT(md), VIRTIO_MEM_ADDR_PROP,
+                                    &error_abort);
+}
+
+static MemoryRegion *virtio_ccw_mem_get_memory_region(MemoryDeviceState *md,
+                                                      Error **errp)
+{
+    VirtIOMEMCcw *ccw_mem = VIRTIO_MEM_CCW(md);
+    VirtIOMEM *vmem = VIRTIO_MEM(&ccw_mem->vdev);
+    VirtIOMEMClass *vmc = VIRTIO_MEM_GET_CLASS(vmem);
+
+    return vmc->get_memory_region(vmem, errp);
+}
+
+static uint64_t virtio_ccw_mem_get_plugged_size(const MemoryDeviceState *md,
+                                                Error **errp)
+{
+    return object_property_get_uint(OBJECT(md), VIRTIO_MEM_SIZE_PROP,
+                                    errp);
+}
+
+static void virtio_ccw_mem_fill_device_info(const MemoryDeviceState *md,
+                                            MemoryDeviceInfo *info)
+{
+    VirtioMEMDeviceInfo *vi = g_new0(VirtioMEMDeviceInfo, 1);
+    VirtIOMEMCcw *ccw_mem = VIRTIO_MEM_CCW(md);
+    VirtIOMEM *vmem = VIRTIO_MEM(&ccw_mem->vdev);
+    VirtIOMEMClass *vpc = VIRTIO_MEM_GET_CLASS(vmem);
+    DeviceState *dev = DEVICE(md);
+
+    if (dev->id) {
+        vi->has_id = true;
+        vi->id = g_strdup(dev->id);
+    }
+
+    /* let the real device handle everything else */
+    vpc->fill_device_info(vmem, vi);
+
+    info->u.virtio_mem.data = vi;
+    info->type = MEMORY_DEVICE_INFO_KIND_VIRTIO_MEM;
+}
+
+static void virtio_ccw_mem_size_change_notify(Notifier *notifier, void *data)
+{
+    VirtIOMEMCcw *ccw_mem = container_of(notifier, VirtIOMEMCcw,
+                                         size_change_notifier);
+    DeviceState *dev = DEVICE(ccw_mem);
+    const uint64_t * const size_p = data;
+    const char *id = NULL;
+
+    if (dev->id) {
+        id = g_strdup(dev->id);
+    }
+
+    qapi_event_send_memory_device_size_change(!!id, id, *size_p);
+}
+
+static void virtio_ccw_mem_instance_init(Object *obj)
+{
+    VirtioCcwDevice *ccw_dev = VIRTIO_CCW_DEVICE(obj);
+    VirtIOMEMCcw *ccw_mem = VIRTIO_MEM_CCW(obj);
+    VirtIOMEMClass *vmc;
+    VirtIOMEM *vmem;
+
+    ccw_dev->force_revision_1 = true;
+    virtio_instance_init_common(obj, &ccw_mem->vdev, sizeof(ccw_mem->vdev),
+                                TYPE_VIRTIO_MEM);
+
+    ccw_mem->size_change_notifier.notify = virtio_ccw_mem_size_change_notify;
+    vmem = VIRTIO_MEM(&ccw_mem->vdev);
+    vmc = VIRTIO_MEM_GET_CLASS(vmem);
+    /*
+     * We never remove the notifier again, as we expect both devices to
+     * disappear at the same time.
+     */
+    vmc->add_size_change_notifier(vmem, &ccw_mem->size_change_notifier);
+
+    object_property_add_alias(obj, VIRTIO_MEM_BLOCK_SIZE_PROP,
+                              OBJECT(&ccw_mem->vdev),
+                              VIRTIO_MEM_BLOCK_SIZE_PROP);
+    object_property_add_alias(obj, VIRTIO_MEM_SIZE_PROP, OBJECT(&ccw_mem->vdev),
+                              VIRTIO_MEM_SIZE_PROP);
+    object_property_add_alias(obj, VIRTIO_MEM_REQUESTED_SIZE_PROP,
+                              OBJECT(&ccw_mem->vdev),
+                              VIRTIO_MEM_REQUESTED_SIZE_PROP);
+}
+
+static Property virtio_ccw_mem_properties[] = {
+    DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
+                    VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_ccw_mem_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtIOCCWDeviceClass *k = VIRTIO_CCW_DEVICE_CLASS(klass);
+    MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(klass);
+
+    k->realize = virtio_ccw_mem_realize;
+    device_class_set_props(dc, virtio_ccw_mem_properties);
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+
+    mdc->get_addr = virtio_ccw_mem_get_addr;
+    mdc->set_addr = virtio_ccw_mem_set_addr;
+    mdc->get_plugged_size = virtio_ccw_mem_get_plugged_size;
+    mdc->get_memory_region = virtio_ccw_mem_get_memory_region;
+    mdc->fill_device_info = virtio_ccw_mem_fill_device_info;
+}
+
+static const TypeInfo virtio_ccw_mem = {
+    .name = TYPE_VIRTIO_MEM_CCW,
+    .parent = TYPE_VIRTIO_CCW_DEVICE,
+    .instance_size = sizeof(VirtIOMEMCcw),
+    .instance_init = virtio_ccw_mem_instance_init,
+    .class_init = virtio_ccw_mem_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_MEMORY_DEVICE },
+        { }
+    },
+};
+
+static void virtio_ccw_mem_register(void)
+{
+    type_register_static(&virtio_ccw_mem);
+}
+
+type_init(virtio_ccw_mem_register)
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index c0e3355248..77aa87c41f 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -29,6 +29,7 @@
 #endif /* CONFIG_VHOST_VSOCK */
 #include "hw/virtio/virtio-gpu.h"
 #include "hw/virtio/virtio-input.h"
+#include "hw/virtio/virtio-mem.h"
 
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/css.h"
@@ -256,4 +257,16 @@ typedef struct VirtIOInputHIDCcw {
     VirtIOInputHID vdev;
 } VirtIOInputHIDCcw;
 
+/* virtio-mem-ccw */
+
+#define TYPE_VIRTIO_MEM_CCW "virtio-mem-ccw"
+#define VIRTIO_MEM_CCW(obj) \
+        OBJECT_CHECK(VirtIOMEMCcw, (obj), TYPE_VIRTIO_MEM_CCW)
+
+typedef struct VirtIOMEMCcw {
+    VirtioCcwDevice parent_obj;
+    VirtIOMEM vdev;
+    Notifier size_change_notifier;
+} VirtIOMEMCcw;
+
 #endif
-- 
2.26.2

Re: [PATCH RFCv3 8/9] s390x: implement virtio-mem-ccw

[Cornelia Huck]

On Fri, 24 Jul 2020 16:37:49 +0200
David Hildenbrand <david@redhat.com> wrote:

> Add a proper CCW proxy device, similar to the PCI variant.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/virtio-ccw-mem.c | 167 ++++++++++++++++++++++++++++++++++++++
>  hw/s390x/virtio-ccw.h     |  13 +++
>  2 files changed, 180 insertions(+)
>  create mode 100644 hw/s390x/virtio-ccw-mem.c

What happens on an old machine type -- I guess the device is just
useless?

Re: [PATCH RFCv3 8/9] s390x: implement virtio-mem-ccw

[David Hildenbrand]

On 27.07.20 11:58, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 16:37:49 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Add a proper CCW proxy device, similar to the PCI variant.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/virtio-ccw-mem.c | 167 ++++++++++++++++++++++++++++++++++++++
>>  hw/s390x/virtio-ccw.h     |  13 +++
>>  2 files changed, 180 insertions(+)
>>  create mode 100644 hw/s390x/virtio-ccw-mem.c
> 
> What happens on an old machine type -- I guess the device is just
> useless?

Trying to plug will fail in the pre_plug stage via

hw/mem/memory-device.c:memory_device_pre_plug()->memory_device_get_free_addr()

with "memory devices (e.g. for memory hotplug) are not supported by the
machine"

whereby never machines where "maxmem" was not properly specified will
result in

"memory devices (e.g. for memory hotplug) are not enabled, please
specify the maxmem option"

-- 
Thanks,

David / dhildenb

Re: [PATCH RFCv3 8/9] s390x: implement virtio-mem-ccw

[Cornelia Huck]

On Mon, 27 Jul 2020 12:02:07 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 27.07.20 11:58, Cornelia Huck wrote:
> > On Fri, 24 Jul 2020 16:37:49 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Add a proper CCW proxy device, similar to the PCI variant.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  hw/s390x/virtio-ccw-mem.c | 167 ++++++++++++++++++++++++++++++++++++++
> >>  hw/s390x/virtio-ccw.h     |  13 +++
> >>  2 files changed, 180 insertions(+)
> >>  create mode 100644 hw/s390x/virtio-ccw-mem.c  
> > 
> > What happens on an old machine type -- I guess the device is just
> > useless?  
> 
> Trying to plug will fail in the pre_plug stage via
> 
> hw/mem/memory-device.c:memory_device_pre_plug()->memory_device_get_free_addr()
> 
> with "memory devices (e.g. for memory hotplug) are not supported by the
> machine"
> 
> whereby never machines where "maxmem" was not properly specified will
> result in
> 
> "memory devices (e.g. for memory hotplug) are not enabled, please
> specify the maxmem option"
> 

Ah, sounds good :)

[PATCH RFCv3 9/9] s390x: initial support for virtio-mem

[David Hildenbrand]

Let's wire up the initial, basic virtio-mem implementation in QEMU. It will
have to see some important extensions (esp., resizeable allocations)
before it can be considered production ready. Also, the focus on the Linux
driver side is on memory hotplug, there are a lot of things optimize in
the future to improve memory unplug capabilities. However, the basics
are in place.

Block migration for now, as we'll have to take proper care of storage
keys and storage attributes. Also, make sure to not hotplug huge pages
to a setup without huge pages.

With a Linux guest that supports virtio-mem (and has
CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE set for now), a basic example.

1. Start a VM with 2G initial memory and a virtio-mem device with a maximum
   capacity of 18GB (and an initial size of 300M):
    sudo qemu-system-s390x \
        --enable-kvm \
        -m 2G,maxmem=20G \
        -smp 4 \
        -nographic \
        -chardev socket,id=monitor,path=/var/tmp/monitor,server,nowait \
        -mon chardev=monitor,mode=readline \
        -net nic -net user \
        -hda s390x.cow2 \
        -object memory-backend-ram,id=mem0,size=18G \
        -device virtio-mem-ccw,id=vm0,memdev=mem0,requested-size=300M

2. Query the current size of virtio-mem device:
    (qemu) info memory-devices
    Memory device [virtio-mem]: "vm0"
      memaddr: 0x80000000
      node: 0
      requested-size: 314572800
      size: 314572800
      max-size: 19327352832
      block-size: 1048576
      memdev: /objects/mem0

3. Request to grow it to 8GB:
    (qemu) qom-set vm0 requested-size 8G
    (qemu) info memory-devices
    Memory device [virtio-mem]: "vm0"
      memaddr: 0x80000000
      node: 0
      requested-size: 8589934592
      size: 8589934592
      max-size: 19327352832
      block-size: 1048576
      memdev: /objects/mem0

4. Request to shrink it to 800M (might take a while, might not fully
   succeed, and might not be able to remove memory blocks in Linux):
  (qemu) qom-set vm0 requested-size 800M
  (qemu) info memory-devices
  Memory device [virtio-mem]: "vm0"
    memaddr: 0x80000000
    node: 0
    requested-size: 838860800
    size: 838860800
    max-size: 19327352832
    block-size: 1048576
    memdev: /objects/mem0

Note 1: Due to lack of resizeable allocations, we will go ahead and
reserve a 18GB vmalloc area + size the QEMU RAM slot + KVM mamory slot
18GB. echo 1 > /proc/sys/vm/overcommit_memory might be required for
now. In the future, this area will instead grow on actual demand and shrink
when possible.

Note 2: Although virtio-mem-pci is wired up as well, it does not seem to
work currently on s390x due to lack of MSI-X.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/Kconfig           |   1 +
 hw/s390x/Makefile.objs     |   1 +
 hw/s390x/s390-virtio-ccw.c | 121 ++++++++++++++++++++++++++++++++++++-
 hw/virtio/virtio-mem.c     |   2 +
 4 files changed, 123 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig
index 5e7d8a2bae..b8619c1adc 100644
--- a/hw/s390x/Kconfig
+++ b/hw/s390x/Kconfig
@@ -10,3 +10,4 @@ config S390_CCW_VIRTIO
     select SCLPCONSOLE
     select VIRTIO_CCW
     select MSI_NONBROKEN
+    select VIRTIO_MEM_SUPPORTED
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 2a35be0cda..279080afb8 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -20,6 +20,7 @@ obj-$(CONFIG_VIRTIO_NET) += virtio-ccw-net.o
 obj-$(CONFIG_VIRTIO_BLK) += virtio-ccw-blk.o
 obj-$(call land,$(CONFIG_VIRTIO_9P),$(CONFIG_VIRTFS)) += virtio-ccw-9p.o
 obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock-ccw.o
+obj-$(CONFIG_VIRTIO_MEM) += virtio-ccw-mem.o
 endif
 obj-y += css-bridge.o
 obj-y += ccw-device.o
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 23b94256e0..7239f5a212 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -44,6 +44,8 @@
 #include "sysemu/sysemu.h"
 #include "hw/s390x/pv.h"
 #include "migration/blocker.h"
+#include "hw/mem/memory-device.h"
+#include "hw/virtio/virtio-mem-pci.h"
 
 static Error *pv_mig_blocker;
 
@@ -488,11 +490,121 @@ static void s390_machine_reset(MachineState *machine)
     s390_ipl_clear_reset_request();
 }
 
+static void s390_virtio_md_pre_plug(HotplugHandler *hotplug_dev,
+                                    DeviceState *dev, Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+    MemoryDeviceState *md = MEMORY_DEVICE(dev);
+    MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
+    Error *local_err = NULL;
+
+    if (!hotplug_dev2 && dev->hotplugged) {
+        /*
+         * Without a bus hotplug handler, we cannot control the plug/unplug
+         * order. We should never reach this point when hotplugging, however,
+         * better add a safety net.
+         */
+        error_setg(errp, "hotplug of virtio based memory devices not supported"
+                         " on this bus.");
+        return;
+    }
+
+    /*
+     * KVM does not support device memory with a bigger page size than initial
+     * memory. The new memory backend is not mapped yet, so
+     * qemu_maxrampagesize() won't consider it.
+     */
+    if (kvm_enabled()) {
+        MemoryRegion *mr = mdc->get_memory_region(md, &local_err);
+
+        if (local_err) {
+            goto out;
+        }
+        if (qemu_ram_pagesize(mr->ram_block) > qemu_maxrampagesize()) {
+            error_setg(&local_err, "Device memory has a bigger page size than"
+                       " initial memory");
+            goto out;
+        }
+    }
+
+    /*
+     * First, see if we can plug this memory device at all. If that
+     * succeeds, branch of to the actual hotplug handler.
+     */
+    memory_device_pre_plug(md, MACHINE(hotplug_dev), NULL, &local_err);
+    if (!local_err && hotplug_dev2) {
+        hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err);
+    }
+out:
+    error_propagate(errp, local_err);
+}
+
+static void s390_virtio_md_plug(HotplugHandler *hotplug_dev,
+                                DeviceState *dev, Error **errp)
+{
+    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
+    static Error *migration_blocker;
+    bool add_blocker = !migration_blocker;
+    Error *local_err = NULL;
+
+    /*
+     * Until we support migration of storage keys and storage attributes
+     * for anything that's not initial memory, let's block migration.
+     */
+    if (add_blocker) {
+        error_setg(&migration_blocker, "storage keys/attributes not yet"
+                   " migrated for memory devices");
+        migrate_add_blocker(migration_blocker, &local_err);
+        if (local_err) {
+            error_free_or_abort(&migration_blocker);
+            goto out;
+        }
+    }
+
+    /*
+     * Plug the memory device first and then branch off to the actual
+     * hotplug handler. If that one fails, we can easily undo the memory
+     * device bits.
+     */
+    memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
+    if (hotplug_dev2) {
+        hotplug_handler_plug(hotplug_dev2, dev, &local_err);
+        if (local_err) {
+            memory_device_unplug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
+            if (add_blocker) {
+                migrate_del_blocker(migration_blocker);
+                error_free_or_abort(&migration_blocker);
+            }
+        }
+    }
+out:
+    error_propagate(errp, local_err);
+}
+
+static void s390_virtio_md_unplug_request(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
+{
+    /* We don't support hot unplug of virtio based memory devices */
+    error_setg(errp, "virtio based memory devices cannot be unplugged.");
+}
+
+static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
+                                         DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
+        s390_virtio_md_pre_plug(hotplug_dev, dev, errp);
+    }
+}
+
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         s390_cpu_plug(hotplug_dev, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW) ||
+               object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
+        s390_virtio_md_plug(hotplug_dev, dev, errp);
     }
 }
 
@@ -501,7 +613,9 @@ static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         error_setg(errp, "CPU hot unplug not supported on this machine");
-        return;
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW) ||
+               object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
+        s390_virtio_md_unplug_request(hotplug_dev, dev, errp);
     }
 }
 
@@ -542,7 +656,9 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
 static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
                                                 DeviceState *dev)
 {
-    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_CCW) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MEM_PCI)) {
         return HOTPLUG_HANDLER(machine);
     }
     return NULL;
@@ -614,6 +730,7 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->possible_cpu_arch_ids = s390_possible_cpu_arch_ids;
     /* it is overridden with 'host' cpu *in kvm_arch_init* */
     mc->default_cpu_type = S390_CPU_TYPE_NAME("qemu");
+    hc->pre_plug = s390_machine_device_pre_plug;
     hc->plug = s390_machine_device_plug;
     hc->unplug_request = s390_machine_device_unplug_request;
     nc->nmi_monitor_handler = s390_nmi;
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index c12e9f79b0..fce86e4411 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -53,6 +53,8 @@
  */
 #if defined(TARGET_X86_64) || defined(TARGET_I386)
 #define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB))
+#elif defined(TARGET_S390X)
+#define VIRTIO_MEM_USABLE_EXTENT (2 * (256 * MiB))
 #else
 #error VIRTIO_MEM_USABLE_EXTENT not defined
 #endif
-- 
2.26.2

Re: [PATCH RFCv3 9/9] s390x: initial support for virtio-mem

[Cornelia Huck]

On Fri, 24 Jul 2020 16:37:50 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's wire up the initial, basic virtio-mem implementation in QEMU. It will
> have to see some important extensions (esp., resizeable allocations)
> before it can be considered production ready. Also, the focus on the Linux
> driver side is on memory hotplug, there are a lot of things optimize in
> the future to improve memory unplug capabilities. However, the basics
> are in place.
> 
> Block migration for now, as we'll have to take proper care of storage
> keys and storage attributes. Also, make sure to not hotplug huge pages
> to a setup without huge pages.
> 
> With a Linux guest that supports virtio-mem (and has
> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE set for now), a basic example.
> 
> 1. Start a VM with 2G initial memory and a virtio-mem device with a maximum
>    capacity of 18GB (and an initial size of 300M):
>     sudo qemu-system-s390x \
>         --enable-kvm \
>         -m 2G,maxmem=20G \
>         -smp 4 \
>         -nographic \
>         -chardev socket,id=monitor,path=/var/tmp/monitor,server,nowait \
>         -mon chardev=monitor,mode=readline \
>         -net nic -net user \
>         -hda s390x.cow2 \
>         -object memory-backend-ram,id=mem0,size=18G \
>         -device virtio-mem-ccw,id=vm0,memdev=mem0,requested-size=300M
> 
> 2. Query the current size of virtio-mem device:
>     (qemu) info memory-devices
>     Memory device [virtio-mem]: "vm0"
>       memaddr: 0x80000000
>       node: 0
>       requested-size: 314572800
>       size: 314572800
>       max-size: 19327352832
>       block-size: 1048576
>       memdev: /objects/mem0
> 
> 3. Request to grow it to 8GB:
>     (qemu) qom-set vm0 requested-size 8G
>     (qemu) info memory-devices
>     Memory device [virtio-mem]: "vm0"
>       memaddr: 0x80000000
>       node: 0
>       requested-size: 8589934592
>       size: 8589934592
>       max-size: 19327352832
>       block-size: 1048576
>       memdev: /objects/mem0
> 
> 4. Request to shrink it to 800M (might take a while, might not fully
>    succeed, and might not be able to remove memory blocks in Linux):
>   (qemu) qom-set vm0 requested-size 800M
>   (qemu) info memory-devices
>   Memory device [virtio-mem]: "vm0"
>     memaddr: 0x80000000
>     node: 0
>     requested-size: 838860800
>     size: 838860800
>     max-size: 19327352832
>     block-size: 1048576
>     memdev: /objects/mem0
> 
> Note 1: Due to lack of resizeable allocations, we will go ahead and
> reserve a 18GB vmalloc area + size the QEMU RAM slot + KVM mamory slot
> 18GB. echo 1 > /proc/sys/vm/overcommit_memory might be required for
> now. In the future, this area will instead grow on actual demand and shrink
> when possible.
> 
> Note 2: Although virtio-mem-pci is wired up as well, it does not seem to
> work currently on s390x due to lack of MSI-X.

IIRC, you can trick virtio-pci into using msi-x via nvectors. Might be
interesting to try.

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/Kconfig           |   1 +
>  hw/s390x/Makefile.objs     |   1 +
>  hw/s390x/s390-virtio-ccw.c | 121 ++++++++++++++++++++++++++++++++++++-
>  hw/virtio/virtio-mem.c     |   2 +
>  4 files changed, 123 insertions(+), 2 deletions(-)

Re: [PATCH RFCv3 9/9] s390x: initial support for virtio-mem

[David Hildenbrand]

On 27.07.20 12:03, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 16:37:50 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's wire up the initial, basic virtio-mem implementation in QEMU. It will
>> have to see some important extensions (esp., resizeable allocations)
>> before it can be considered production ready. Also, the focus on the Linux
>> driver side is on memory hotplug, there are a lot of things optimize in
>> the future to improve memory unplug capabilities. However, the basics
>> are in place.
>>
>> Block migration for now, as we'll have to take proper care of storage
>> keys and storage attributes. Also, make sure to not hotplug huge pages
>> to a setup without huge pages.
>>
>> With a Linux guest that supports virtio-mem (and has
>> CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE set for now), a basic example.
>>
>> 1. Start a VM with 2G initial memory and a virtio-mem device with a maximum
>>    capacity of 18GB (and an initial size of 300M):
>>     sudo qemu-system-s390x \
>>         --enable-kvm \
>>         -m 2G,maxmem=20G \
>>         -smp 4 \
>>         -nographic \
>>         -chardev socket,id=monitor,path=/var/tmp/monitor,server,nowait \
>>         -mon chardev=monitor,mode=readline \
>>         -net nic -net user \
>>         -hda s390x.cow2 \
>>         -object memory-backend-ram,id=mem0,size=18G \
>>         -device virtio-mem-ccw,id=vm0,memdev=mem0,requested-size=300M
>>
>> 2. Query the current size of virtio-mem device:
>>     (qemu) info memory-devices
>>     Memory device [virtio-mem]: "vm0"
>>       memaddr: 0x80000000
>>       node: 0
>>       requested-size: 314572800
>>       size: 314572800
>>       max-size: 19327352832
>>       block-size: 1048576
>>       memdev: /objects/mem0
>>
>> 3. Request to grow it to 8GB:
>>     (qemu) qom-set vm0 requested-size 8G
>>     (qemu) info memory-devices
>>     Memory device [virtio-mem]: "vm0"
>>       memaddr: 0x80000000
>>       node: 0
>>       requested-size: 8589934592
>>       size: 8589934592
>>       max-size: 19327352832
>>       block-size: 1048576
>>       memdev: /objects/mem0
>>
>> 4. Request to shrink it to 800M (might take a while, might not fully
>>    succeed, and might not be able to remove memory blocks in Linux):
>>   (qemu) qom-set vm0 requested-size 800M
>>   (qemu) info memory-devices
>>   Memory device [virtio-mem]: "vm0"
>>     memaddr: 0x80000000
>>     node: 0
>>     requested-size: 838860800
>>     size: 838860800
>>     max-size: 19327352832
>>     block-size: 1048576
>>     memdev: /objects/mem0
>>
>> Note 1: Due to lack of resizeable allocations, we will go ahead and
>> reserve a 18GB vmalloc area + size the QEMU RAM slot + KVM mamory slot
>> 18GB. echo 1 > /proc/sys/vm/overcommit_memory might be required for
>> now. In the future, this area will instead grow on actual demand and shrink
>> when possible.
>>
>> Note 2: Although virtio-mem-pci is wired up as well, it does not seem to
>> work currently on s390x due to lack of MSI-X.
> 
> IIRC, you can trick virtio-pci into using msi-x via nvectors. Might be
> interesting to try.

Ah, okay, I tried to find an option to enable it "msi-x=on" but failed.
Will give it a try. Thanks!


-- 
Thanks,

David / dhildenb