[PATCH v1 00/14] s390x: virtio-mem support

[PATCH v1 00/14] s390x: virtio-mem support

[David Hildenbrand]

This series is based on:
    [PATCH v2] virtio: kconfig: memory devices are PCI only [1]

I finally found the time (IOW forced myself) to finish virtio-mem
support on s390x. The last RFC was from 2020, so I won't talk about
what changed -- a lot changed in the meantime :)

There is really not much left to do on s390x, because virtio-mem already
implements most things we need today (e.g., early-migration,
unplugged-inaccessible). The biggest part of this series is just doing what
we do with virtio-pci, wiring it up in the machine hotplug handler and ...
well, messing with the physical memory layout where we can now exceed
initial RAM size and have sparsity (memory holes).

I tested a lot of things, including:
 * Memory hotplug/unplug
 * Device hotplug/unplug
 * System resets / reboots
 * Migrate to/from file (including storage attributes under KVM)
 * Basic live migration
 * Basic postcopy live migration

More details on how to use it on s390x -- which is pretty much how
we use it on other architectures, except
s/virtio-mem-pci/virtio-mem-ccw/ --- is in the last patch.

This series introduces a new diag(500) "STORAGE LIMIT" subcode that will
be documented at [2] once this+kernel part go upstream.

There are not many s390x-specific virtio-mem future work items, except:
* Storage attribute migration might be improved
* We might want to reset storage attributes of unplugged memory
  (might or might not be required for upcoming page table reclaim in
   Linux; TBD)

I'll send out the kernel driver bits soon.

[1] https://lkml.kernel.org/r/20240906101658.514470-1-pbonzini@redhat.com
[2] https://gitlab.com/davidhildenbrand/s390x-os-virt-spec

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Eric Farman <farman@linux.ibm.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: Janosch Frank <frankja@linux.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>

David Hildenbrand (14):
  s390x/s390-virtio-ccw: don't crash on weird RAM sizes
  s390x/s390-virtio-hcall: remove hypercall registration mechanism
  s390x/s390-virtio-hcall: prepare for more diag500 hypercalls
  s390x: rename s390-virtio-hcall* to s390-hypercall*
  s390x/s390-virtio-ccw: move setting the maximum guest size from sclp
    to machine code
  s390x: introduce s390_get_memory_limit()
  s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT
  s390x/s390-stattrib-kvm: prepare memory devices and sparse memory
    layouts
  s390x/s390-skeys: prepare for memory devices
  s390x/pv: check initial, not maximum RAM size
  s390x/s390-virtio-ccw: prepare for memory devices
  s390x: introduce s390_get_max_pagesize()
  s390x/virtio-ccw: add support for virtio based memory devices
  s390x: virtio-mem support

 MAINTAINERS                    |   4 +
 hw/s390x/Kconfig               |   1 +
 hw/s390x/meson.build           |   4 +-
 hw/s390x/s390-hypercall.c      |  77 +++++++++++
 hw/s390x/s390-hypercall.h      |  25 ++++
 hw/s390x/s390-skeys.c          |   4 +-
 hw/s390x/s390-stattrib-kvm.c   |  63 +++++----
 hw/s390x/s390-virtio-ccw.c     | 143 +++++++++++++--------
 hw/s390x/s390-virtio-hcall.c   |  41 ------
 hw/s390x/s390-virtio-hcall.h   |  25 ----
 hw/s390x/sclp.c                |  17 +--
 hw/s390x/virtio-ccw-md.c       | 153 ++++++++++++++++++++++
 hw/s390x/virtio-ccw-md.h       |  44 +++++++
 hw/s390x/virtio-ccw-mem.c      | 226 +++++++++++++++++++++++++++++++++
 hw/s390x/virtio-ccw-mem.h      |  34 +++++
 hw/virtio/Kconfig              |   1 +
 hw/virtio/virtio-mem.c         |   4 +-
 target/s390x/cpu-sysemu.c      |  35 ++++-
 target/s390x/cpu.h             |   2 +
 target/s390x/kvm/kvm.c         |  12 +-
 target/s390x/kvm/pv.c          |   2 +-
 target/s390x/tcg/misc_helper.c |   6 +-
 22 files changed, 746 insertions(+), 177 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-md.c
 create mode 100644 hw/s390x/virtio-ccw-md.h
 create mode 100644 hw/s390x/virtio-ccw-mem.c
 create mode 100644 hw/s390x/virtio-ccw-mem.h

-- 
2.46.0

[PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[David Hildenbrand]

KVM is not happy when starting a VM with weird RAM sizes:

  # qemu-system-s390x --enable-kvm --nographic -m 1234K
  qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
    failed, slot=0, start=0x0, size=0x244000: Invalid argument
  kvm_set_phys_mem: error registering slot: Invalid argument
  Aborted (core dumped)

Let's handle that in a better way by rejecting such weird RAM sizes
right from the start:

  # qemu-system-s390x --enable-kvm --nographic -m 1234K
  qemu-system-s390x: ram size must be multiples of 1 MiB

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 18240a0fd8..e30cf0a2a1 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -180,6 +180,17 @@ static void s390_memory_init(MemoryRegion *ram)
 {
     MemoryRegion *sysmem = get_system_memory();
 
+    if (!QEMU_IS_ALIGNED(memory_region_size(ram), 1 * MiB)) {
+        /*
+         * The SCLP cannot possibly expose smaller granularity right now and KVM
+         * cannot handle smaller granularity. As we don't support NUMA, the
+         * region size directly corresponds to machine->ram_size, and the region
+         * is a single RAM memory region.
+         */
+        error_report("ram size must be multiples of 1 MiB");
+        exit(EXIT_FAILURE);
+    }
+
     /* allocate RAM for core */
     memory_region_add_subregion(sysmem, 0, ram);
 
-- 
2.46.0

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[Janosch Frank]

On 9/10/24 7:57 PM, David Hildenbrand wrote:
> KVM is not happy when starting a VM with weird RAM sizes:
> 
>    # qemu-system-s390x --enable-kvm --nographic -m 1234K
>    qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
>      failed, slot=0, start=0x0, size=0x244000: Invalid argument
>    kvm_set_phys_mem: error registering slot: Invalid argument
>    Aborted (core dumped)
> 
> Let's handle that in a better way by rejecting such weird RAM sizes
> right from the start:
>

Huh, I always assumed that ram is handled in multiples of 1MB in QEMU.

Acked-by: Janosch Frank <frankja@linux.ibm.com>

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[David Hildenbrand]

On 11.09.24 13:28, Janosch Frank wrote:
> On 9/10/24 7:57 PM, David Hildenbrand wrote:
>> KVM is not happy when starting a VM with weird RAM sizes:
>>
>>     # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>     qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
>>       failed, slot=0, start=0x0, size=0x244000: Invalid argument
>>     kvm_set_phys_mem: error registering slot: Invalid argument
>>     Aborted (core dumped)
>>
>> Let's handle that in a better way by rejecting such weird RAM sizes
>> right from the start:
>>
> 
> Huh, I always assumed that ram is handled in multiples of 1MB in QEMU.

Me as well, I did not dig if that changed at some point ... or why such 
odd sizes would even be required :)

Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[Thomas Huth]

On 11/09/2024 14.38, David Hildenbrand wrote:
> On 11.09.24 13:28, Janosch Frank wrote:
>> On 9/10/24 7:57 PM, David Hildenbrand wrote:
>>> KVM is not happy when starting a VM with weird RAM sizes:
>>>
>>>     # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>>     qemu-system-s390x: kvm_set_user_memory_region: 
>>> KVM_SET_USER_MEMORY_REGION
>>>       failed, slot=0, start=0x0, size=0x244000: Invalid argument
>>>     kvm_set_phys_mem: error registering slot: Invalid argument
>>>     Aborted (core dumped)
>>>
>>> Let's handle that in a better way by rejecting such weird RAM sizes
>>> right from the start:
>>>
>>
>> Huh, I always assumed that ram is handled in multiples of 1MB in QEMU.
> 
> Me as well, I did not dig if that changed at some point ... or why such odd 
> sizes would even be required :)

I guess it's there for some old PC hardware ... Remember, 640K ought to be 
enough for anybody.

  Thomas

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[David Hildenbrand]

On 11.09.24 14:46, Thomas Huth wrote:
> On 11/09/2024 14.38, David Hildenbrand wrote:
>> On 11.09.24 13:28, Janosch Frank wrote:
>>> On 9/10/24 7:57 PM, David Hildenbrand wrote:
>>>> KVM is not happy when starting a VM with weird RAM sizes:
>>>>
>>>>      # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>>>      qemu-system-s390x: kvm_set_user_memory_region:
>>>> KVM_SET_USER_MEMORY_REGION
>>>>        failed, slot=0, start=0x0, size=0x244000: Invalid argument
>>>>      kvm_set_phys_mem: error registering slot: Invalid argument
>>>>      Aborted (core dumped)
>>>>
>>>> Let's handle that in a better way by rejecting such weird RAM sizes
>>>> right from the start:
>>>>
>>>
>>> Huh, I always assumed that ram is handled in multiples of 1MB in QEMU.
>>
>> Me as well, I did not dig if that changed at some point ... or why such odd
>> sizes would even be required :)
> 
> I guess it's there for some old PC hardware ... Remember, 640K ought to be
> enough for anybody.

True, maybe the "default to MiB" made some of us believe that something
smaller is impossible :)

And in fact, I think suffix support was added in

commit 6e1d3c1c855818a6d1399698572afae0d11b872b
Author: Igor Mammedov <imammedo@redhat.com>
Date:   Wed Nov 27 01:27:35 2013 +0100

     vl: convert -m to QemuOpts
     
     Adds option to -m
      "size" - startup memory amount
     
     For compatibility with legacy CLI if suffix-less number is passed,
     it assumes amount in Mb.
     
     Otherwise user is free to use suffixed number using suffixes b,k/K,M,G


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[Thomas Huth]

On 10/09/2024 19.57, David Hildenbrand wrote:
> KVM is not happy when starting a VM with weird RAM sizes:
> 
>    # qemu-system-s390x --enable-kvm --nographic -m 1234K
>    qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
>      failed, slot=0, start=0x0, size=0x244000: Invalid argument
>    kvm_set_phys_mem: error registering slot: Invalid argument
>    Aborted (core dumped)
> 
> Let's handle that in a better way by rejecting such weird RAM sizes
> right from the start:
> 
>    # qemu-system-s390x --enable-kvm --nographic -m 1234K
>    qemu-system-s390x: ram size must be multiples of 1 MiB
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[Eric Farman]

On Tue, 2024-09-10 at 19:57 +0200, David Hildenbrand wrote:
> KVM is not happy when starting a VM with weird RAM sizes:
> 
>   # qemu-system-s390x --enable-kvm --nographic -m 1234K
>   qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
>     failed, slot=0, start=0x0, size=0x244000: Invalid argument
>   kvm_set_phys_mem: error registering slot: Invalid argument
>   Aborted (core dumped)
> 
> Let's handle that in a better way by rejecting such weird RAM sizes
> right from the start:
> 
>   # qemu-system-s390x --enable-kvm --nographic -m 1234K
>   qemu-system-s390x: ram size must be multiples of 1 MiB
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

TIL. Thanks David!

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[David Hildenbrand]

On 10.09.24 19:57, David Hildenbrand wrote:
> KVM is not happy when starting a VM with weird RAM sizes:
> 
>    # qemu-system-s390x --enable-kvm --nographic -m 1234K
>    qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
>      failed, slot=0, start=0x0, size=0x244000: Invalid argument
>    kvm_set_phys_mem: error registering slot: Invalid argument
>    Aborted (core dumped)
> 
> Let's handle that in a better way by rejecting such weird RAM sizes
> right from the start:
> 
>    # qemu-system-s390x --enable-kvm --nographic -m 1234K
>    qemu-system-s390x: ram size must be multiples of 1 MiB
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 18240a0fd8..e30cf0a2a1 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -180,6 +180,17 @@ static void s390_memory_init(MemoryRegion *ram)
>   {
>       MemoryRegion *sysmem = get_system_memory();
>   
> +    if (!QEMU_IS_ALIGNED(memory_region_size(ram), 1 * MiB)) {
> +        /*
> +         * The SCLP cannot possibly expose smaller granularity right now and KVM
> +         * cannot handle smaller granularity. As we don't support NUMA, the
> +         * region size directly corresponds to machine->ram_size, and the region
> +         * is a single RAM memory region.
> +         */
> +        error_report("ram size must be multiples of 1 MiB");
> +        exit(EXIT_FAILURE);
> +    }

I'll switch to

	error_setg(&error_fatal, "ram size must be multiples of 1 MiB");

here, to avoid the manual exit().

Please someone shout if I should keep it as is.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[Thomas Huth]

On 23/09/2024 11.19, David Hildenbrand wrote:
> On 10.09.24 19:57, David Hildenbrand wrote:
>> KVM is not happy when starting a VM with weird RAM sizes:
>>
>>    # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>    qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
>>      failed, slot=0, start=0x0, size=0x244000: Invalid argument
>>    kvm_set_phys_mem: error registering slot: Invalid argument
>>    Aborted (core dumped)
>>
>> Let's handle that in a better way by rejecting such weird RAM sizes
>> right from the start:
>>
>>    # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>    qemu-system-s390x: ram size must be multiples of 1 MiB
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 18240a0fd8..e30cf0a2a1 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -180,6 +180,17 @@ static void s390_memory_init(MemoryRegion *ram)
>>   {
>>       MemoryRegion *sysmem = get_system_memory();
>> +    if (!QEMU_IS_ALIGNED(memory_region_size(ram), 1 * MiB)) {
>> +        /*
>> +         * The SCLP cannot possibly expose smaller granularity right now 
>> and KVM
>> +         * cannot handle smaller granularity. As we don't support NUMA, the
>> +         * region size directly corresponds to machine->ram_size, and the 
>> region
>> +         * is a single RAM memory region.
>> +         */
>> +        error_report("ram size must be multiples of 1 MiB");
>> +        exit(EXIT_FAILURE);
>> +    }
> 
> I'll switch to
> 
>      error_setg(&error_fatal, "ram size must be multiples of 1 MiB");
> 
> here, to avoid the manual exit().
> 
> Please someone shout if I should keep it as is.

Please keep it, according to include/qapi/error.h:

  * Please don't error_setg(&error_fatal, ...), use error_report() and
  * exit(), because that's more obvious.

  Thanks,
   Thomas

Re: [PATCH v1 01/14] s390x/s390-virtio-ccw: don't crash on weird RAM sizes

[David Hildenbrand]

On 23.09.24 17:36, Thomas Huth wrote:
> On 23/09/2024 11.19, David Hildenbrand wrote:
>> On 10.09.24 19:57, David Hildenbrand wrote:
>>> KVM is not happy when starting a VM with weird RAM sizes:
>>>
>>>     # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>>     qemu-system-s390x: kvm_set_user_memory_region: KVM_SET_USER_MEMORY_REGION
>>>       failed, slot=0, start=0x0, size=0x244000: Invalid argument
>>>     kvm_set_phys_mem: error registering slot: Invalid argument
>>>     Aborted (core dumped)
>>>
>>> Let's handle that in a better way by rejecting such weird RAM sizes
>>> right from the start:
>>>
>>>     # qemu-system-s390x --enable-kvm --nographic -m 1234K
>>>     qemu-system-s390x: ram size must be multiples of 1 MiB
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    hw/s390x/s390-virtio-ccw.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 18240a0fd8..e30cf0a2a1 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -180,6 +180,17 @@ static void s390_memory_init(MemoryRegion *ram)
>>>    {
>>>        MemoryRegion *sysmem = get_system_memory();
>>> +    if (!QEMU_IS_ALIGNED(memory_region_size(ram), 1 * MiB)) {
>>> +        /*
>>> +         * The SCLP cannot possibly expose smaller granularity right now
>>> and KVM
>>> +         * cannot handle smaller granularity. As we don't support NUMA, the
>>> +         * region size directly corresponds to machine->ram_size, and the
>>> region
>>> +         * is a single RAM memory region.
>>> +         */
>>> +        error_report("ram size must be multiples of 1 MiB");
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>
>> I'll switch to
>>
>>       error_setg(&error_fatal, "ram size must be multiples of 1 MiB");
>>
>> here, to avoid the manual exit().
>>
>> Please someone shout if I should keep it as is.
> 
> Please keep it, according to include/qapi/error.h:
> 
>    * Please don't error_setg(&error_fatal, ...), use error_report() and
>    * exit(), because that's more obvious.
> 

Controversial, but will do. Thanks!

-- 
Cheers,

David / dhildenb

[PATCH v1 02/14] s390x/s390-virtio-hcall: 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 s390-virtio-hcall.c.

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e30cf0a2a1..57c95786f6 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -16,11 +16,8 @@
 #include "exec/ram_addr.h"
 #include "exec/confidential-guest-support.h"
 #include "hw/boards.h"
-#include "hw/s390x/s390-virtio-hcall.h"
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
-#include "hw/s390x/ioinst.h"
-#include "hw/s390x/css.h"
 #include "virtio-ccw.h"
 #include "qemu/config-file.h"
 #include "qemu/ctype.h"
@@ -124,58 +121,6 @@ static void subsystem_reset(void)
     }
 }
 
-static int virtio_ccw_hcall_notify(const uint64_t *args)
-{
-    uint64_t subch_id = args[0];
-    uint64_t data = args[1];
-    SubchDev *sch;
-    VirtIODevice *vdev;
-    int cssid, ssid, schid, m;
-    uint16_t vq_idx = data;
-
-    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;
-    }
-
-    vdev = virtio_ccw_get_vdev(sch);
-    if (vq_idx >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, vq_idx)) {
-        return -EINVAL;
-    }
-
-    if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
-        virtio_queue_set_shadow_avail_idx(virtio_get_queue(vdev, vq_idx),
-                                          (data >> 16) & 0xFFFF);
-    }
-
-    virtio_queue_notify(vdev, vq_idx);
-    return 0;
-}
-
-static int virtio_ccw_hcall_early_printk(const uint64_t *args)
-{
-    uint64_t mem = args[0];
-    MachineState *ms = MACHINE(qdev_get_machine());
-
-    if (mem < ms->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(MemoryRegion *ram)
 {
     MemoryRegion *sysmem = get_system_memory();
@@ -302,9 +247,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..ca49e3cd22 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -11,31 +11,64 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "hw/boards.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 int handle_virtio_notify(uint64_t mem)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
 
-static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
+    if (mem < ms->ram_size) {
+        /* Early printk */
+        return 0;
+    }
+    return -EINVAL;
+}
 
-void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
+static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data)
 {
-    assert(code < MAX_DIAG_SUBCODES);
-    assert(!s390_diag500_table[code]);
+    SubchDev *sch;
+    VirtIODevice *vdev;
+    int cssid, ssid, schid, m;
+    uint16_t vq_idx = data;
+
+    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;
+    }
 
-    s390_diag500_table[code] = fn;
+    vdev = virtio_ccw_get_vdev(sch);
+    if (vq_idx >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, vq_idx)) {
+        return -EINVAL;
+    }
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+        virtio_queue_set_shadow_avail_idx(virtio_get_queue(vdev, vq_idx),
+                                          (data >> 16) & 0xFFFF);
+    }
+
+    virtio_queue_notify(vdev, vq_idx);
+    return 0;
 }
 
 int s390_virtio_hypercall(CPUS390XState *env)
 {
-    s390_virtio_fn fn;
-
-    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;
-        }
-    }
+    const uint64_t subcode = env->regs[1];
 
-    return -EINVAL;
+    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 3ae6d6ae3a..3d9fe147d2 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -18,8 +18,6 @@
 /* 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.46.0

Re: [PATCH v1 02/14] s390x/s390-virtio-hcall: remove hypercall registration mechanism

[Thomas Huth]

On 10/09/2024 19.57, David Hildenbrand 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
> handlers into s390-virtio-hcall.c.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-virtio-ccw.c   | 58 --------------------------------
>   hw/s390x/s390-virtio-hcall.c | 65 +++++++++++++++++++++++++++---------
>   hw/s390x/s390-virtio-hcall.h |  2 --
>   3 files changed, 49 insertions(+), 76 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v1 03/14] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls

[David Hildenbrand]

Let's generalize, abstracting 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).

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   |  8 ++++----
 hw/s390x/s390-virtio-hcall.h   | 11 ++++++-----
 target/s390x/kvm/kvm.c         | 10 ++--------
 target/s390x/tcg/misc_helper.c |  4 ++--
 4 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
index ca49e3cd22..4cddf69fbb 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 hypercalls on s390
  *
  * Copyright 2012 IBM Corp.
  * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
@@ -57,15 +57,15 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data)
     return 0;
 }
 
-int s390_virtio_hypercall(CPUS390XState *env)
+int handle_diag_500(CPUS390XState *env)
 {
     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:
+    case DIAG500_VIRTIO_CCW_NOTIFY:
         env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
         return 0;
     default:
diff --git a/hw/s390x/s390-virtio-hcall.h b/hw/s390x/s390-virtio-hcall.h
index 3d9fe147d2..e4f76aca82 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 hypercalls on s390x
  *
  * Copyright IBM Corp. 2012, 2017
  * Author(s): Cornelia Huck <cornelia.huck@de.ibm.com>
@@ -12,12 +12,13 @@
 #ifndef HW_S390_VIRTIO_HCALL_H
 #define HW_S390_VIRTIO_HCALL_H
 
-#include "standard-headers/asm-s390/virtio-ccw.h"
 #include "cpu.h"
 
-/* The only thing that we need from the old kvm_virtio.h file */
-#define KVM_S390_VIRTIO_NOTIFY 0
+#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 */
 
-int s390_virtio_hypercall(CPUS390XState *env);
+int handle_diag_500(CPUS390XState *env);
 
 #endif /* HW_S390_VIRTIO_HCALL_H */
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 94181d9281..ac292b184a 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -1493,16 +1493,10 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
 
 static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
 {
-    CPUS390XState *env = &cpu->env;
-    int ret;
-
-    ret = s390_virtio_hypercall(env);
-    if (ret == -EINVAL) {
+    if (handle_diag_500(&cpu->env)) {
         kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
-        return 0;
     }
-
-    return ret;
+    return 0;
 }
 
 static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 303f86d363..58757585a2 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -117,9 +117,9 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)
 
     switch (num) {
     case 0x500:
-        /* KVM hypercall */
+        /* QEMU/KVM hypercall */
         bql_lock();
-        r = s390_virtio_hypercall(env);
+        r = handle_diag_500(env);
         bql_unlock();
         break;
     case 0x44:
-- 
2.46.0

Re: [PATCH v1 03/14] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls

[Thomas Huth]

On 10/09/2024 19.57, David Hildenbrand wrote:
> Let's generalize, abstracting 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).
> 
> 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   |  8 ++++----
>   hw/s390x/s390-virtio-hcall.h   | 11 ++++++-----
>   target/s390x/kvm/kvm.c         | 10 ++--------
>   target/s390x/tcg/misc_helper.c |  4 ++--
>   4 files changed, 14 insertions(+), 19 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v1 03/14] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls

[Nina Schoetterl-Glausch]

On Tue, 2024-09-10 at 19:57 +0200, David Hildenbrand wrote:
> Let's generalize, abstracting 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).
> 
> 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   |  8 ++++----
>  hw/s390x/s390-virtio-hcall.h   | 11 ++++++-----
>  target/s390x/kvm/kvm.c         | 10 ++--------
>  target/s390x/tcg/misc_helper.c |  4 ++--
>  4 files changed, 14 insertions(+), 19 deletions(-)
> 
[...]
> 
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 94181d9281..ac292b184a 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -1493,16 +1493,10 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
>  
>  static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)

Might as well make the return type void then.
More importantly, are you changing the behavior?
If I'm reading the code right, previously handle_instruction would inject an
additional PGM_OPERATION due to the negative return value, which does seem off to me.
If so, IMO this change should go into a separate patch.
I'm also wondering if the injection of PGM_SPECIFICATION should just go into
handle_diag_500 and handle_hypercall should just be inlined.

>  {
> -    CPUS390XState *env = &cpu->env;
> -    int ret;
> -
> -    ret = s390_virtio_hypercall(env);
> -    if (ret == -EINVAL) {
> +    if (handle_diag_500(&cpu->env)) {
>          kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> -        return 0;
>      }
> -
> -    return ret;
> +    return 0;
>  }

Re: [PATCH v1 03/14] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls

[David Hildenbrand]

On 12.09.24 15:22, Nina Schoetterl-Glausch wrote:
> On Tue, 2024-09-10 at 19:57 +0200, David Hildenbrand wrote:
>> Let's generalize, abstracting 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).
>>
>> 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   |  8 ++++----
>>   hw/s390x/s390-virtio-hcall.h   | 11 ++++++-----
>>   target/s390x/kvm/kvm.c         | 10 ++--------
>>   target/s390x/tcg/misc_helper.c |  4 ++--
>>   4 files changed, 14 insertions(+), 19 deletions(-)
>>
> [...]
>>
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 94181d9281..ac292b184a 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -1493,16 +1493,10 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
>>   
>>   static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
> 
> Might as well make the return type void then.

Agreed.

> More importantly, are you changing the behavior?
> If I'm reading the code right, previously handle_instruction would inject an
> additional PGM_OPERATION due to the negative return value, which does seem off to me.
> If so, IMO this change should go into a separate patch.

You are right, agreed.

> I'm also wondering if the injection of PGM_SPECIFICATION should just go into
> handle_diag_500 and handle_hypercall should just be inlined.
> 

I had exactly that, and used s390_program_interrupt(env, 
PGM_SPECIFICATION, ra); let me revive that.

Thanks!
-- 
Cheers,

David / dhildenb

Re: [PATCH v1 03/14] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls

[David Hildenbrand]

On 17.09.24 12:45, David Hildenbrand wrote:
> On 12.09.24 15:22, Nina Schoetterl-Glausch wrote:
>> On Tue, 2024-09-10 at 19:57 +0200, David Hildenbrand wrote:
>>> Let's generalize, abstracting 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).
>>>
>>> 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   |  8 ++++----
>>>    hw/s390x/s390-virtio-hcall.h   | 11 ++++++-----
>>>    target/s390x/kvm/kvm.c         | 10 ++--------
>>>    target/s390x/tcg/misc_helper.c |  4 ++--
>>>    4 files changed, 14 insertions(+), 19 deletions(-)
>>>
>> [...]
>>>
>>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>>> index 94181d9281..ac292b184a 100644
>>> --- a/target/s390x/kvm/kvm.c
>>> +++ b/target/s390x/kvm/kvm.c
>>> @@ -1493,16 +1493,10 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
>>>    
>>>    static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
>>
>> Might as well make the return type void then.
> 
> Agreed.
> 
>> More importantly, are you changing the behavior?
>> If I'm reading the code right, previously handle_instruction would inject an
>> additional PGM_OPERATION due to the negative return value, which does seem off to me.
>> If so, IMO this change should go into a separate patch.
> 
> You are right, agreed.

Ah, reading again, we have a "return 0;" after the 
"kvm_s390_program_interrupt", so it should work as expected.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 03/14] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls

[David Hildenbrand]

On 17.09.24 12:50, David Hildenbrand wrote:
> On 17.09.24 12:45, David Hildenbrand wrote:
>> On 12.09.24 15:22, Nina Schoetterl-Glausch wrote:
>>> On Tue, 2024-09-10 at 19:57 +0200, David Hildenbrand wrote:
>>>> Let's generalize, abstracting 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).
>>>>
>>>> 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   |  8 ++++----
>>>>     hw/s390x/s390-virtio-hcall.h   | 11 ++++++-----
>>>>     target/s390x/kvm/kvm.c         | 10 ++--------
>>>>     target/s390x/tcg/misc_helper.c |  4 ++--
>>>>     4 files changed, 14 insertions(+), 19 deletions(-)
>>>>
>>> [...]
>>>>
>>>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>>>> index 94181d9281..ac292b184a 100644
>>>> --- a/target/s390x/kvm/kvm.c
>>>> +++ b/target/s390x/kvm/kvm.c
>>>> @@ -1493,16 +1493,10 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
>>>>     
>>>>     static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
>>>
>>> Might as well make the return type void then.
>>
>> Agreed.
>>
>>> More importantly, are you changing the behavior?
>>> If I'm reading the code right, previously handle_instruction would inject an
>>> additional PGM_OPERATION due to the negative return value, which does seem off to me.
>>> If so, IMO this change should go into a separate patch.
>>
>> You are right, agreed.
> 
> Ah, reading again, we have a "return 0;" after the
> "kvm_s390_program_interrupt", so it should work as expected.
> 

The following in should be what you suggest:

diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
index 4cddf69fbb..5fb78a719e 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-virtio-hcall.c
@@ -57,18 +57,19 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data)
      return 0;
  }
  
-int handle_diag_500(CPUS390XState *env)
+void handle_diag_500(S390CPU *cpu, uintptr_t ra)
  {
+    CPUS390XState *env = &cpu->env;
      const uint64_t subcode = env->regs[1];
  
      switch (subcode) {
      case DIAG500_VIRTIO_NOTIFY:
          env->regs[2] = handle_virtio_notify(env->regs[2]);
-        return 0;
+        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 e4f76aca82..dca456b926 100644
--- a/hw/s390x/s390-virtio-hcall.h
+++ b/hw/s390x/s390-virtio-hcall.h
@@ -19,6 +19,6 @@
  #define DIAG500_VIRTIO_SET_STATUS       2 /* legacy */
  #define DIAG500_VIRTIO_CCW_NOTIFY       3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
  
-int handle_diag_500(CPUS390XState *env);
+void handle_diag_500(S390CPU *cpu, uintptr_t ra);
  
  #endif /* HW_S390_VIRTIO_HCALL_H */
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index ac292b184a..2f9a54fe04 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -1491,14 +1491,6 @@ 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)
-{
-    if (handle_diag_500(&cpu->env)) {
-        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
-    }
-    return 0;
-}
-
  static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
  {
      uint64_t r1, r3;
@@ -1595,7 +1587,7 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
          handle_diag_318(cpu, run);
          break;
      case DIAG_KVM_HYPERCALL:
-        r = handle_hypercall(cpu, run);
+        handle_diag_500(cpu, RA_IGNORED)
          break;
      case DIAG_KVM_BREAKPOINT:
          r = handle_sw_breakpoint(cpu, run);
diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 58757585a2..170fe00629 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -119,7 +119,7 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)
      case 0x500:
          /* QEMU/KVM hypercall */
          bql_lock();
-        r = handle_diag_500(env);
+        handle_diag_500(env_archcpu(env), GETPC());
          bql_unlock();
          break;
      case 0x44:


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 03/14] s390x/s390-virtio-hcall: prepare for more diag500 hypercalls

[Nina Schoetterl-Glausch]

On Tue, 2024-09-17 at 13:02 +0200, David Hildenbrand wrote:
> On 17.09.24 12:50, David Hildenbrand wrote:
> > On 17.09.24 12:45, David Hildenbrand wrote:
> > > On 12.09.24 15:22, Nina Schoetterl-Glausch wrote:
> > > > On Tue, 2024-09-10 at 19:57 +0200, David Hildenbrand wrote:
> > > > > Let's generalize, abstracting 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).
> > > > > 
> > > > > 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   |  8 ++++----
> > > > >     hw/s390x/s390-virtio-hcall.h   | 11 ++++++-----
> > > > >     target/s390x/kvm/kvm.c         | 10 ++--------
> > > > >     target/s390x/tcg/misc_helper.c |  4 ++--
> > > > >     4 files changed, 14 insertions(+), 19 deletions(-)
> > > > > 
> > > > [...]
> > > > > 
> > > > > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> > > > > index 94181d9281..ac292b184a 100644
> > > > > --- a/target/s390x/kvm/kvm.c
> > > > > +++ b/target/s390x/kvm/kvm.c
> > > > > @@ -1493,16 +1493,10 @@ static int handle_e3(S390CPU *cpu, struct kvm_run *run, uint8_t ipbl)
> > > > >     
> > > > >     static int handle_hypercall(S390CPU *cpu, struct kvm_run *run)
> > > > 
> > > > Might as well make the return type void then.
> > > 
> > > Agreed.
> > > 
> > > > More importantly, are you changing the behavior?
> > > > If I'm reading the code right, previously handle_instruction would inject an
> > > > additional PGM_OPERATION due to the negative return value, which does seem off to me.
> > > > If so, IMO this change should go into a separate patch.
> > > 
> > > You are right, agreed.
> > 
> > Ah, reading again, we have a "return 0;" after the
> > "kvm_s390_program_interrupt", so it should work as expected.
> > 
> 
> The following in should be what you suggest:

Yup, looks good.
> 
> diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
> index 4cddf69fbb..5fb78a719e 100644
> --- a/hw/s390x/s390-virtio-hcall.c
> +++ b/hw/s390x/s390-virtio-hcall.c
> @@ -57,18 +57,19 @@ static int handle_virtio_ccw_notify(uint64_t subch_id, uint64_t data)
>       return 0;
>   }
>   
> -int handle_diag_500(CPUS390XState *env)
> +void handle_diag_500(S390CPU *cpu, uintptr_t ra)

I don't see a point to passing a cpu when we only need the env, but it doesn't matter.

>   {
> +    CPUS390XState *env = &cpu->env;
>       const uint64_t subcode = env->regs[1];
>   
>       switch (subcode) {
>       case DIAG500_VIRTIO_NOTIFY:
>           env->regs[2] = handle_virtio_notify(env->regs[2]);
> -        return 0;
> +        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 e4f76aca82..dca456b926 100644
> --- a/hw/s390x/s390-virtio-hcall.h
> +++ b/hw/s390x/s390-virtio-hcall.h
> @@ -19,6 +19,6 @@
>   #define DIAG500_VIRTIO_SET_STATUS       2 /* legacy */
>   #define DIAG500_VIRTIO_CCW_NOTIFY       3 /* KVM_S390_VIRTIO_CCW_NOTIFY */
>   
> -int handle_diag_500(CPUS390XState *env);
> +void handle_diag_500(S390CPU *cpu, uintptr_t ra);
>   
>   #endif /* HW_S390_VIRTIO_HCALL_H */
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index ac292b184a..2f9a54fe04 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -1491,14 +1491,6 @@ 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)
> -{
> -    if (handle_diag_500(&cpu->env)) {
> -        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> -    }
> -    return 0;
> -}
> -
>   static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
>   {
>       uint64_t r1, r3;
> @@ -1595,7 +1587,7 @@ static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
>           handle_diag_318(cpu, run);
>           break;
>       case DIAG_KVM_HYPERCALL:
> -        r = handle_hypercall(cpu, run);
> +        handle_diag_500(cpu, RA_IGNORED)
>           break;
>       case DIAG_KVM_BREAKPOINT:
>           r = handle_sw_breakpoint(cpu, run);
> diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
> index 58757585a2..170fe00629 100644
> --- a/target/s390x/tcg/misc_helper.c
> +++ b/target/s390x/tcg/misc_helper.c
> @@ -119,7 +119,7 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)
>       case 0x500:
>           /* QEMU/KVM hypercall */
>           bql_lock();
> -        r = handle_diag_500(env);
> +        handle_diag_500(env_archcpu(env), GETPC());
>           bql_unlock();
>           break;
>       case 0x44:
> 
> 

[PATCH v1 04/14] 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/meson.build                               | 2 +-
 hw/s390x/{s390-virtio-hcall.c => s390-hypercall.c} | 2 +-
 hw/s390x/{s390-virtio-hcall.h => s390-hypercall.h} | 6 +++---
 target/s390x/kvm/kvm.c                             | 2 +-
 target/s390x/tcg/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/meson.build b/hw/s390x/meson.build
index 482fd13420..71ec747f4c 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -12,7 +12,7 @@ s390x_ss.add(files(
   's390-pci-inst.c',
   's390-skeys.c',
   's390-stattrib.c',
-  's390-virtio-hcall.c',
+  's390-hypercall.c',
   'sclp.c',
   'sclpcpu.c',
   'sclpquiesce.c',
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 4cddf69fbb..f09e8a1d81 100644
--- a/hw/s390x/s390-virtio-hcall.c
+++ b/hw/s390x/s390-hypercall.c
@@ -12,7 +12,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "hw/boards.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 e4f76aca82..b7ac29f444 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
 
 #include "cpu.h"
 
@@ -21,4 +21,4 @@
 
 int handle_diag_500(CPUS390XState *env);
 
-#endif /* HW_S390_VIRTIO_HCALL_H */
+#endif /* HW_S390_HYPERCALL_H */
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index ac292b184a..062296206a 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/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 "target/s390x/kvm/pv.h"
 
 #define kvm_vm_check_mem_attr(s, attr) \
diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index 58757585a2..718985f3a3 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/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.46.0

Re: [PATCH v1 04/14] s390x: rename s390-virtio-hcall* to s390-hypercall*

[Thomas Huth]

On 10/09/2024 19.57, David Hildenbrand wrote:
> 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/meson.build                               | 2 +-
>   hw/s390x/{s390-virtio-hcall.c => s390-hypercall.c} | 2 +-
>   hw/s390x/{s390-virtio-hcall.h => s390-hypercall.h} | 6 +++---
>   target/s390x/kvm/kvm.c                             | 2 +-
>   target/s390x/tcg/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%)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v1 05/14] s390x/s390-virtio-ccw: move setting the maximum guest size from sclp to machine code

[David Hildenbrand]

Nowadays, it feels more natural to have that code located in
s390_memory_init(), where we also have direct access to the machine
object.

While at it, use the actual RAM size, not the maximum RAM size which
cannot currently be reached without support for any memory devices.

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 57c95786f6..08156f0682 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -121,11 +121,15 @@ static void subsystem_reset(void)
     }
 }
 
-static void s390_memory_init(MemoryRegion *ram)
+static void s390_memory_init(MachineState *machine)
 {
     MemoryRegion *sysmem = get_system_memory();
+    MemoryRegion *ram = machine->ram;
+    uint64_t ram_size = memory_region_size(ram);
+    uint64_t hw_limit;
+    int ret;
 
-    if (!QEMU_IS_ALIGNED(memory_region_size(ram), 1 * MiB)) {
+    if (!QEMU_IS_ALIGNED(ram_size, 1 * MiB)) {
         /*
          * The SCLP cannot possibly expose smaller granularity right now and KVM
          * cannot handle smaller granularity. As we don't support NUMA, the
@@ -136,7 +140,17 @@ static void s390_memory_init(MemoryRegion *ram)
         exit(EXIT_FAILURE);
     }
 
-    /* allocate RAM for core */
+    ret = s390_set_memory_limit(ram_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);
+    }
+
+    /* Map the initial memory. Must happen after setting the memory limit. */
     memory_region_add_subregion(sysmem, 0, ram);
 
     /*
@@ -217,7 +231,7 @@ static void ccw_init(MachineState *machine)
     qdev_realize_and_unref(DEVICE(ms->sclp), NULL, &error_fatal);
 
     /* 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 e725dcd5fd..fac09816bf 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -381,10 +381,7 @@ void sclp_service_interrupt(uint32_t sccb)
 /* qemu object creation and initialization functions */
 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
@@ -394,14 +391,6 @@ static void sclp_realize(DeviceState *dev, Error **errp)
     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");
-    }
 }
 
 static void sclp_memory_init(SCLPDevice *sclp)
-- 
2.46.0

Re: [PATCH v1 05/14] s390x/s390-virtio-ccw: move setting the maximum guest size from sclp to machine code

[Thomas Huth]

On 10/09/2024 19.58, David Hildenbrand wrote:
> Nowadays, it feels more natural to have that code located in
> s390_memory_init(), where we also have direct access to the machine
> object.
> 
> While at it, use the actual RAM size, not the maximum RAM size which
> cannot currently be reached without support for any memory devices.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 22 ++++++++++++++++++----
>   hw/s390x/sclp.c            | 11 -----------
>   2 files changed, 18 insertions(+), 15 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v1 06/14] s390x: introduce s390_get_memory_limit()

[David Hildenbrand]

Let's add s390_get_memory_limit(), to query what has been successfully
set via s390_set_memory_limit(). Allow setting the limit only once.

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

diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 1cd30c1d84..1915567b3a 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -255,12 +255,27 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
     return s390_count_running_cpus();
 }
 
+static uint64_t memory_limit;
+
 int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
 {
+    int ret = 0;
+
+    if (memory_limit) {
+        return -EBUSY;
+    }
     if (kvm_enabled()) {
-        return kvm_s390_set_mem_limit(new_limit, hw_limit);
+        ret = kvm_s390_set_mem_limit(new_limit, hw_limit);
+    }
+    if (!ret) {
+        memory_limit = new_limit;
     }
-    return 0;
+    return ret;
+}
+
+uint64_t s390_get_memory_limit(void)
+{
+    return memory_limit;
 }
 
 void s390_set_max_pagesize(uint64_t pagesize, Error **errp)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index d6b75ad0e0..7a51b606ed 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -895,6 +895,7 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg)
 /* cpu.c */
 void s390_crypto_reset(void);
 int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit);
+uint64_t s390_get_memory_limit(void);
 void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
 void s390_cmma_reset(void);
 void s390_enable_css_support(S390CPU *cpu);
-- 
2.46.0

Re: [PATCH v1 06/14] s390x: introduce s390_get_memory_limit()

[Thomas Huth]

On 10/09/2024 19.58, David Hildenbrand wrote:
> Let's add s390_get_memory_limit(), to query what has been successfully
> set via s390_set_memory_limit(). Allow setting the limit only once.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   target/s390x/cpu-sysemu.c | 19 +++++++++++++++++--
>   target/s390x/cpu.h        |  1 +
>   2 files changed, 18 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v1 06/14] s390x: introduce s390_get_memory_limit()

[Nina Schoetterl-Glausch]

On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
> Let's add s390_get_memory_limit(), to query what has been successfully
> set via s390_set_memory_limit(). Allow setting the limit only once.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Comment below.
> ---
>  target/s390x/cpu-sysemu.c | 19 +++++++++++++++++--
>  target/s390x/cpu.h        |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
> index 1cd30c1d84..1915567b3a 100644
> --- a/target/s390x/cpu-sysemu.c
> +++ b/target/s390x/cpu-sysemu.c
> @@ -255,12 +255,27 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
>      return s390_count_running_cpus();
>  }
>  
> +static uint64_t memory_limit;
> +
>  int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
>  {
> +    int ret = 0;
> +
> +    if (memory_limit) {
> +        return -EBUSY;
> +    }
>      if (kvm_enabled()) {
> -        return kvm_s390_set_mem_limit(new_limit, hw_limit);
> +        ret = kvm_s390_set_mem_limit(new_limit, hw_limit);
> +    }
> +    if (!ret) {
> +        memory_limit = new_limit;
>      }
> -    return 0;
> +    return ret;
> +}
> +
> +uint64_t s390_get_memory_limit(void)
> +{

Might be nice to guard/warn against s390_set_memory_limit not having been called before.

> +    return memory_limit;
>  }
>  
>  void s390_set_max_pagesize(uint64_t pagesize, Error **errp)
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d6b75ad0e0..7a51b606ed 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -895,6 +895,7 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg)
>  /* cpu.c */
>  void s390_crypto_reset(void);
>  int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit);
> +uint64_t s390_get_memory_limit(void);
>  void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
>  void s390_cmma_reset(void);
>  void s390_enable_css_support(S390CPU *cpu);

Re: [PATCH v1 06/14] s390x: introduce s390_get_memory_limit()

[David Hildenbrand]

On 16.09.24 15:20, Nina Schoetterl-Glausch wrote:
> On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
>> Let's add s390_get_memory_limit(), to query what has been successfully
>> set via s390_set_memory_limit(). Allow setting the limit only once.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> 
> Comment below.
>> ---
>>   target/s390x/cpu-sysemu.c | 19 +++++++++++++++++--
>>   target/s390x/cpu.h        |  1 +
>>   2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
>> index 1cd30c1d84..1915567b3a 100644
>> --- a/target/s390x/cpu-sysemu.c
>> +++ b/target/s390x/cpu-sysemu.c
>> @@ -255,12 +255,27 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
>>       return s390_count_running_cpus();
>>   }
>>   
>> +static uint64_t memory_limit;
>> +
>>   int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
>>   {
>> +    int ret = 0;
>> +
>> +    if (memory_limit) {
>> +        return -EBUSY;
>> +    }
>>       if (kvm_enabled()) {
>> -        return kvm_s390_set_mem_limit(new_limit, hw_limit);
>> +        ret = kvm_s390_set_mem_limit(new_limit, hw_limit);
>> +    }
>> +    if (!ret) {
>> +        memory_limit = new_limit;
>>       }
>> -    return 0;
>> +    return ret;
>> +}
>> +
>> +uint64_t s390_get_memory_limit(void)
>> +{
> 
> Might be nice to guard/warn against s390_set_memory_limit not having been called before.
> 

What about the following on top:

diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 1915567b3a..07cb85103a 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -263,6 +263,8 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
  
      if (memory_limit) {
          return -EBUSY;
+    } else if (!new_limit) {
+        return -EINVAL;
      }
      if (kvm_enabled()) {
          ret = kvm_s390_set_mem_limit(new_limit, hw_limit);
@@ -275,6 +277,8 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
  
  uint64_t s390_get_memory_limit(void)
  {
+    /* We expect to be called after s390_set_memory_limit(). */
+    assert(memory_limit);
      return memory_limit;
  }


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 06/14] s390x: introduce s390_get_memory_limit()

[Nina Schoetterl-Glausch]

On Tue, 2024-09-17 at 13:23 +0200, David Hildenbrand wrote:
> On 16.09.24 15:20, Nina Schoetterl-Glausch wrote:
> > On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
> > > Let's add s390_get_memory_limit(), to query what has been successfully
> > > set via s390_set_memory_limit(). Allow setting the limit only once.
> > > 
> > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > 
> > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > 
> > Comment below.
> > > ---
> > >   target/s390x/cpu-sysemu.c | 19 +++++++++++++++++--
> > >   target/s390x/cpu.h        |  1 +
> > >   2 files changed, 18 insertions(+), 2 deletions(-)

[...]

> > > +
> > > +uint64_t s390_get_memory_limit(void)
> > > +{
> > 
> > Might be nice to guard/warn against s390_set_memory_limit not having been called before.
> > 
> 
> What about the following on top:
> 
> diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
> index 1915567b3a..07cb85103a 100644
> --- a/target/s390x/cpu-sysemu.c
> +++ b/target/s390x/cpu-sysemu.c
> @@ -263,6 +263,8 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
>   
>       if (memory_limit) {
>           return -EBUSY;
> +    } else if (!new_limit) {
> +        return -EINVAL;
>       }
>       if (kvm_enabled()) {
>           ret = kvm_s390_set_mem_limit(new_limit, hw_limit);
> @@ -275,6 +277,8 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
>   
>   uint64_t s390_get_memory_limit(void)
>   {
> +    /* We expect to be called after s390_set_memory_limit(). */
> +    assert(memory_limit);
>       return memory_limit;
>   }

Looks good.

Looking at the patch again I'm wondering if using globals in qemu is still encouraged.
I know it's a common pattern today, but seeing efforts like the multiarch binary or Unicorn
I'm wondering if there is aspirations to do things more "cleanly", in general, for far out benefits?
I.e. memory_limit could be a machine property instead.

Re: [PATCH v1 06/14] s390x: introduce s390_get_memory_limit()

[David Hildenbrand]

> 
> Looks good.
> 
> Looking at the patch again I'm wondering if using globals in qemu is still encouraged.
> I know it's a common pattern today, but seeing efforts like the multiarch binary or Unicorn
> I'm wondering if there is aspirations to do things more "cleanly", in general, for far out benefits?
> I.e. memory_limit could be a machine property instead.

Yes, I'll rework that code to simply store it in the machine, moving 
that code out of cpu-sysemu.c int s390-virtio-ccw.c.

Same for patch #12, thanks!

-- 
Cheers,

David / dhildenb

[PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[David Hildenbrand]

A guest OS that supports memory hotplug / memory devices must during
boot be aware of the maximum possible physical memory address that it might
have to handle at a later stage during its runtime

For example, the maximum possible memory address might be required to
prepare the kernel virtual address space accordingly (e.g., select page
table hierarchy depth).

On s390x there is currently no such mechanism that is compatible with
paravirtualized memory devices, because the whole SCLP interface was
designed around the idea of "storage increments" and "standby memory".
Paravirtualized memory devices we want to support, such as virtio-mem, have
no intersection with any of that, but could co-exist with them in the
future if ever needed.

In particular, a guest OS must never detect and use device memory
without the help of a proper device driver. Device memory must not be
exposed in any firmware-provided memory map (SCLP or diag260 on s390x).
For this reason, these memory devices will be places in memory *above*
the "maximum storage increment" exposed via SCLP.

Let's provide a new diag500 subcode to query the memory limit determined in
s390_memory_init().

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

diff --git a/hw/s390x/s390-hypercall.c b/hw/s390x/s390-hypercall.c
index f09e8a1d81..ac48fc0961 100644
--- a/hw/s390x/s390-hypercall.c
+++ b/hw/s390x/s390-hypercall.c
@@ -68,6 +68,9 @@ int handle_diag_500(CPUS390XState *env)
     case DIAG500_VIRTIO_CCW_NOTIFY:
         env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
         return 0;
+    case DIAG500_STORAGE_LIMIT:
+        env->regs[2] = s390_get_memory_limit() - 1;
+        return 0;
     default:
         return -EINVAL;
     }
diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
index b7ac29f444..f0ca62bcbb 100644
--- a/hw/s390x/s390-hypercall.h
+++ b/hw/s390x/s390-hypercall.h
@@ -18,6 +18,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_STORAGE_LIMIT           4
 
 int handle_diag_500(CPUS390XState *env);
 
-- 
2.46.0

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Thomas Huth]

On 10/09/2024 19.58, David Hildenbrand wrote:
> A guest OS that supports memory hotplug / memory devices must during
> boot be aware of the maximum possible physical memory address that it might
> have to handle at a later stage during its runtime
> 
> For example, the maximum possible memory address might be required to
> prepare the kernel virtual address space accordingly (e.g., select page
> table hierarchy depth).
> 
> On s390x there is currently no such mechanism that is compatible with
> paravirtualized memory devices, because the whole SCLP interface was
> designed around the idea of "storage increments" and "standby memory".
> Paravirtualized memory devices we want to support, such as virtio-mem, have
> no intersection with any of that, but could co-exist with them in the
> future if ever needed.
> 
> In particular, a guest OS must never detect and use device memory
> without the help of a proper device driver. Device memory must not be
> exposed in any firmware-provided memory map (SCLP or diag260 on s390x).
> For this reason, these memory devices will be places in memory *above*
> the "maximum storage increment" exposed via SCLP.
> 
> Let's provide a new diag500 subcode to query the memory limit determined in
> s390_memory_init().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-hypercall.c | 3 +++
>   hw/s390x/s390-hypercall.h | 1 +
>   2 files changed, 4 insertions(+)
> 
> diff --git a/hw/s390x/s390-hypercall.c b/hw/s390x/s390-hypercall.c
> index f09e8a1d81..ac48fc0961 100644
> --- a/hw/s390x/s390-hypercall.c
> +++ b/hw/s390x/s390-hypercall.c
> @@ -68,6 +68,9 @@ int handle_diag_500(CPUS390XState *env)
>       case DIAG500_VIRTIO_CCW_NOTIFY:
>           env->regs[2] = handle_virtio_ccw_notify(env->regs[2], env->regs[3]);
>           return 0;
> +    case DIAG500_STORAGE_LIMIT:
> +        env->regs[2] = s390_get_memory_limit() - 1;
> +        return 0;
>       default:
>           return -EINVAL;
>       }
> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
> index b7ac29f444..f0ca62bcbb 100644
> --- a/hw/s390x/s390-hypercall.h
> +++ b/hw/s390x/s390-hypercall.h
> @@ -18,6 +18,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_STORAGE_LIMIT           4
>   
>   int handle_diag_500(CPUS390XState *env);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Sounds very reasonable to me - but it would be good to get an 
Ack/Reviewed-by from IBM folks here (in case they prefer a different 
interface)... hope they'll join the discussion!

  Thomas

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Janosch Frank]

On 9/12/24 10:19 AM, Thomas Huth wrote:
> On 10/09/2024 19.58, David Hildenbrand wrote:

[...]

>> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
>> index b7ac29f444..f0ca62bcbb 100644
>> --- a/hw/s390x/s390-hypercall.h
>> +++ b/hw/s390x/s390-hypercall.h
>> @@ -18,6 +18,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_STORAGE_LIMIT           4
>>    
>>    int handle_diag_500(CPUS390XState *env);
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Sounds very reasonable to me - but it would be good to get an
> Ack/Reviewed-by from IBM folks here (in case they prefer a different
> interface)... hope they'll join the discussion!

I've publicized the series on the internal channels yesterday.
We're aware of the fact that we need to provide review.

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Halil Pasic]

On Thu, 12 Sep 2024 10:19:00 +0200
Thomas Huth <thuth@redhat.com> wrote:

> > diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
> > index b7ac29f444..f0ca62bcbb 100644
> > --- a/hw/s390x/s390-hypercall.h
> > +++ b/hw/s390x/s390-hypercall.h
> > @@ -18,6 +18,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_STORAGE_LIMIT           4
> >   
> >   int handle_diag_500(CPUS390XState *env);  
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> Sounds very reasonable to me - but it would be good to get an 
> Ack/Reviewed-by from IBM folks here (in case they prefer a different 
> interface)... hope they'll join the discussion!
> 
>   Thomas

According to Documentation/virt/kvm/s390/s390-diag.rst in the
linux source tree DIAG 500 is for kvm virtio funcions. 

Based on the commit message this storagelimit DIAG is rather loosely
tied to virtio if at all, so from that perspective DIAG may not be a
perfect fit. OTOH I don't see a better fit either. I would prefer to
have Christian's opinion on this. I have no strong opinion myself.

If we decide to go with a DIAG, I would like to see
Documentation/virt/kvm/s390/s390-diag.rst
updated accordingly.

Also if decide to go with DIAG 500, maybe leaving some space
between the codes more closely tied to virtio and between
the ones less closely tied to virito (for the unlikely case
that we end up wanting another DIAG 500 subcode for virtio) 
might make sense. I.e e could make DIAG500_STORAGE_LIMIT 
8 or 16 instead of 4. Again nothing important, just an idea.

Regards,
Halil

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[David Hildenbrand]

On 27.09.24 20:05, Halil Pasic wrote:
> On Thu, 12 Sep 2024 10:19:00 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>>> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
>>> index b7ac29f444..f0ca62bcbb 100644
>>> --- a/hw/s390x/s390-hypercall.h
>>> +++ b/hw/s390x/s390-hypercall.h
>>> @@ -18,6 +18,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_STORAGE_LIMIT           4
>>>    
>>>    int handle_diag_500(CPUS390XState *env);
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>> Sounds very reasonable to me - but it would be good to get an
>> Ack/Reviewed-by from IBM folks here (in case they prefer a different
>> interface)... hope they'll join the discussion!
>>
>>    Thomas
> 
> According to Documentation/virt/kvm/s390/s390-diag.rst in the
> linux source tree DIAG 500 is for kvm virtio funcions.

I assume you skimmed the QEMU patches, including the one that suggest 
changing (or rather extending) that.

> 
> Based on the commit message this storagelimit DIAG is rather loosely
> tied to virtio if at all, so from that perspective DIAG may not be a
> perfect fit. OTOH I don't see a better fit either. I would prefer to
> have Christian's opinion on this. I have no strong opinion myself.
> 
 > If we decide to go with a DIAG, I would like to see> 
Documentation/virt/kvm/s390/s390-diag.rst
> updated accordingly.

I'll document wherever people fancy. I'll even document in multiple 
locations :)

> 
> Also if decide to go with DIAG 500, maybe leaving some space
> between the codes more closely tied to virtio and between
> the ones less closely tied to virito (for the unlikely case
> that we end up wanting another DIAG 500 subcode for virtio)
> might make sense. I.e e could make DIAG500_STORAGE_LIMIT
> 8 or 16 instead of 4. Again nothing important, just an idea.

I don't see a reason to do that, but in the end I don't care as long as 
people let me know what to do instead.

Thanks for taking a look!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Christian Borntraeger]

Am 27.09.24 um 20:05 schrieb Halil Pasic:
> On Thu, 12 Sep 2024 10:19:00 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>>> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
>>> index b7ac29f444..f0ca62bcbb 100644
>>> --- a/hw/s390x/s390-hypercall.h
>>> +++ b/hw/s390x/s390-hypercall.h
>>> @@ -18,6 +18,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_STORAGE_LIMIT           4
>>>    
>>>    int handle_diag_500(CPUS390XState *env);
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>> Sounds very reasonable to me - but it would be good to get an
>> Ack/Reviewed-by from IBM folks here (in case they prefer a different
>> interface)... hope they'll join the discussion!
>>
>>    Thomas
> 
> According to Documentation/virt/kvm/s390/s390-diag.rst in the
> linux source tree DIAG 500 is for kvm virtio funcions.
> 
> Based on the commit message this storagelimit DIAG is rather loosely
> tied to virtio if at all, so from that perspective DIAG may not be a
> perfect fit. OTOH I don't see a better fit either. I would prefer to
> have Christian's opinion on this. I have no strong opinion myself.

Some remarks:
500 with a new subcode would work, it is marked as KVM hypervisor call
in our docs. 501 was used in the past for software breakpoints.
So we could use 502 as the next free one (This is reserved for KVM).
We do have kvm_stat counters for 500, not sure if people debugging virtio
will care.
The only important question for me is, what code is generated by gcc for
the switch statement and will any variant slow down the virtio doorbell.
diag.c has
         if (!vcpu->kvm->arch.css_support ||
             (vcpu->run->s.regs.gprs[1] != KVM_S390_VIRTIO_CCW_NOTIFY))
                 return -EOPNOTSUPP;

So 500+4 should probably not cause any harm apart from branch prediction
going wrong the first 2 or 3 notifies.

502 will make kvm_s390_handle_diag larger.

So I tend to go with 500+4.

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Halil Pasic]

On Mon, 30 Sep 2024 13:11:31 +0200
Christian Borntraeger <borntraeger@linux.ibm.com> wrote:

> We do have kvm_stat counters for 500, not sure if people debugging virtio
> will care.

Could end up being confusing, as currently we can assume each and every
DIAG 500 is a virtio doorbell. But I don't think the chance of this
causing real headache is big.

> The only important question for me is, what code is generated by gcc for
> the switch statement and will any variant slow down the virtio doorbell.
> diag.c has
>          if (!vcpu->kvm->arch.css_support ||
>              (vcpu->run->s.regs.gprs[1] != KVM_S390_VIRTIO_CCW_NOTIFY))
>                  return -EOPNOTSUPP;
> 
> So 500+4 should probably not cause any harm apart from branch prediction
> going wrong the first 2 or 3 notifies.
> 
> 502 will make kvm_s390_handle_diag larger.

What do you mean by this last paragraph?

I suppose we are talking about
int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)                                 
{                                                                               
        int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;              
                                                                                
        if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)                  
                return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);    
                                                                                
        trace_kvm_s390_handle_diag(vcpu, code);                                 
        switch (code) {                                                         
        case 0x10:                                                              
                return diag_release_pages(vcpu);                                
        case 0x44:                                                              
                return __diag_time_slice_end(vcpu);                             
        case 0x9c:                                                              
                return __diag_time_slice_end_directed(vcpu);                    
        case 0x258:                                                             
                return __diag_page_ref_service(vcpu);                           
        case 0x308:                                                             
                return __diag_ipl_functions(vcpu);                              
        case 0x500:                                                             
                return __diag_virtio_hypercall(vcpu);                           
        default:                                                                
                vcpu->stat.instruction_diagnose_other++;                        
                return -EOPNOTSUPP;                                             
        }                                                                       
}

and my understanding is that the default branch of the switch 
statement would be already suitable for DIAG 502 as it is today
for DIAG 502. So I'm quite confused by your statement that
502 will make kvm_s390_handle_diag larger (as the only meaning
of larger I can think of is more code). Can you please clarify?

Regards,
Halil

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Christian Borntraeger]

Am 30.09.24 um 14:57 schrieb Halil Pasic:
> On Mon, 30 Sep 2024 13:11:31 +0200
> Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> 
>> We do have kvm_stat counters for 500, not sure if people debugging virtio
>> will care.
> 
> Could end up being confusing, as currently we can assume each and every
> DIAG 500 is a virtio doorbell. But I don't think the chance of this
> causing real headache is big.
> 
>> The only important question for me is, what code is generated by gcc for
>> the switch statement and will any variant slow down the virtio doorbell.
>> diag.c has
>>           if (!vcpu->kvm->arch.css_support ||
>>               (vcpu->run->s.regs.gprs[1] != KVM_S390_VIRTIO_CCW_NOTIFY))
>>                   return -EOPNOTSUPP;
>>
>> So 500+4 should probably not cause any harm apart from branch prediction
>> going wrong the first 2 or 3 notifies.
>>
>> 502 will make kvm_s390_handle_diag larger.
> 
> What do you mean by this last paragraph?
> 
> I suppose we are talking about
> int kvm_s390_handle_diag(struct kvm_vcpu *vcpu)
> {
>          int code = kvm_s390_get_base_disp_rs(vcpu, NULL) & 0xffff;
>                                                                                  
>          if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
>                  return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>                                                                                  
>          trace_kvm_s390_handle_diag(vcpu, code);
>          switch (code) {
>          case 0x10:
>                  return diag_release_pages(vcpu);
>          case 0x44:
>                  return __diag_time_slice_end(vcpu);
>          case 0x9c:
>                  return __diag_time_slice_end_directed(vcpu);
>          case 0x258:
>                  return __diag_page_ref_service(vcpu);
>          case 0x308:
>                  return __diag_ipl_functions(vcpu);
>          case 0x500:
>                  return __diag_virtio_hypercall(vcpu);
>          default:
>                  vcpu->stat.instruction_diagnose_other++;
>                  return -EOPNOTSUPP;
>          }
> }
> 
> and my understanding is that the default branch of the switch
> statement would be already suitable for DIAG 502 as it is today
> for DIAG 502. So I'm quite confused by your statement that
> 502 will make kvm_s390_handle_diag larger (as the only meaning
> of larger I can think of is more code). Can you please clarify?

gcc has logic for switch statements that decide about branch table or
a chained compare+jump. I think due to spectre gcc now avoids indirect
branches as much as possible but still a larger switch statement might
kick the decision from inline compare/jump to a branch table.

I am not worried in this particular case this was more or less a
"what could go wrong".

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Halil Pasic]

On Tue, 1 Oct 2024 11:15:02 +0200
Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
[..]
> >> So 500+4 should probably not cause any harm apart from branch prediction
> >> going wrong the first 2 or 3 notifies.
> >>
> >> 502 will make kvm_s390_handle_diag larger.  
> > 
> > What do you mean by this last paragraph?
[..]

> gcc has logic for switch statements that decide about branch table or
> a chained compare+jump. I think due to spectre gcc now avoids indirect
> branches as much as possible but still a larger switch statement might
> kick the decision from inline compare/jump to a branch table.
> 
> I am not worried in this particular case this was more or less a
> "what could go wrong".

Hm, you did state that "502 will make kvm_s390_handle_diag larger". I
suppose now we agree that 502 would not make kvm_s390_handle_diag larger.
Right?

I understood that you prefer 500+4 over 502 because the latter would
make kvm_s390_handle_diag larger. Now that we have, I hope clarified,
that 502 would not make the switch larger, do you still prefer 500+4?

BTW your insights are very appreciated!

Regards,
Halil

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[Christian Borntraeger]

Am 01.10.24 um 15:31 schrieb Halil Pasic:
> On Tue, 1 Oct 2024 11:15:02 +0200
> Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> [..]
>>>> So 500+4 should probably not cause any harm apart from branch prediction
>>>> going wrong the first 2 or 3 notifies.
>>>>
>>>> 502 will make kvm_s390_handle_diag larger.
>>>
>>> What do you mean by this last paragraph?
> [..]
> 
>> gcc has logic for switch statements that decide about branch table or
>> a chained compare+jump. I think due to spectre gcc now avoids indirect
>> branches as much as possible but still a larger switch statement might
>> kick the decision from inline compare/jump to a branch table.
>>
>> I am not worried in this particular case this was more or less a
>> "what could go wrong".
> 
> Hm, you did state that "502 will make kvm_s390_handle_diag larger". I
> suppose now we agree that 502 would not make kvm_s390_handle_diag larger.
> Right?
> 
> I understood that you prefer 500+4 over 502 because the latter would
> make kvm_s390_handle_diag larger. Now that we have, I hope clarified,
> that 502 would not make the switch larger, do you still prefer 500+4?
> 
> BTW your insights are very appreciated!

OK you mean that diag502 is not handled in the kernel but instead via default.
Yes you are right. So it should not matter I guess.

Re: [PATCH v1 07/14] s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT

[David Hildenbrand]

On 30.09.24 13:11, Christian Borntraeger wrote:
> Am 27.09.24 um 20:05 schrieb Halil Pasic:
>> On Thu, 12 Sep 2024 10:19:00 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>>> diff --git a/hw/s390x/s390-hypercall.h b/hw/s390x/s390-hypercall.h
>>>> index b7ac29f444..f0ca62bcbb 100644
>>>> --- a/hw/s390x/s390-hypercall.h
>>>> +++ b/hw/s390x/s390-hypercall.h
>>>> @@ -18,6 +18,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_STORAGE_LIMIT           4
>>>>     
>>>>     int handle_diag_500(CPUS390XState *env);
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>
>>> Sounds very reasonable to me - but it would be good to get an
>>> Ack/Reviewed-by from IBM folks here (in case they prefer a different
>>> interface)... hope they'll join the discussion!
>>>
>>>     Thomas
>>
>> According to Documentation/virt/kvm/s390/s390-diag.rst in the
>> linux source tree DIAG 500 is for kvm virtio funcions.
>>
>> Based on the commit message this storagelimit DIAG is rather loosely
>> tied to virtio if at all, so from that perspective DIAG may not be a
>> perfect fit. OTOH I don't see a better fit either. I would prefer to
>> have Christian's opinion on this. I have no strong opinion myself.
> 
> Some remarks:
> 500 with a new subcode would work, it is marked as KVM hypervisor call
> in our docs. 501 was used in the past for software breakpoints.

Right, we use it in the absence of KVM_CAP_S390_USER_INSTR0.

> So we could use 502 as the next free one (This is reserved for KVM).
> We do have kvm_stat counters for 500, not sure if people debugging virtio
> will care.

It would be one additional trigger during system boot, so likely not 
really an issue. We could always add new stats for selected subcodes 
(i.e, KVM_S390_VIRTIO_CCW_NOTIFY).

> The only important question for me is, what code is generated by gcc for
> the switch statement and will any variant slow down the virtio doorbell.
> diag.c has
>           if (!vcpu->kvm->arch.css_support ||
>               (vcpu->run->s.regs.gprs[1] != KVM_S390_VIRTIO_CCW_NOTIFY))
>                   return -EOPNOTSUPP;
> 
> So 500+4 should probably not cause any harm apart from branch prediction
> going wrong the first 2 or 3 notifies.

Right, it's very unlikely to be noticeable at all.

Thanks for the feedback!

-- 
Cheers,

David / dhildenb

[PATCH v1 08/14] s390x/s390-stattrib-kvm: prepare memory devices and sparse memory layouts

[David Hildenbrand]

With memory devices, we will have storage attributes for memory that
exceeds the initial ram size. Further, we can easily have memory holes,
for which there (currently) are no storage attributes.

In particular, with memory holes, KVM_S390_SET_CMMA_BITS will fail to set
some storage attributes.

So let's do it like we handle storage keys migration, relying on
guest_phys_blocks_append(). However, in contrast to storage key
migration, we will handle it on the migration destination.

This is a preparation for virtio-mem support. Note that ever since the
"early migration" feature was added (x-early-migration), the state
of device blocks (plugged/unplugged) is migrated early such that
guest_phys_blocks_append() will properly consider all currently plugged
memory blocks and skip any unplugged ones.

In the future, we should try getting rid of the large temporary buffer
and also not send any attributes for any memory holes, just so they
get ignored on the destination.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-stattrib-kvm.c | 63 +++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
index eeaa811098..1f1bd507b5 100644
--- a/hw/s390x/s390-stattrib-kvm.c
+++ b/hw/s390x/s390-stattrib-kvm.c
@@ -15,6 +15,7 @@
 #include "hw/s390x/storage-attributes.h"
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
+#include "sysemu/memory_mapping.h"
 #include "exec/ram_addr.h"
 #include "kvm/kvm_s390x.h"
 #include "qapi/error.h"
@@ -84,8 +85,7 @@ static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
                                         uint8_t *values)
 {
     KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
-    MachineState *machine = MACHINE(qdev_get_machine());
-    unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
+    unsigned long max = s390_get_memory_limit() / TARGET_PAGE_SIZE;
 
     if (start_gfn + count > max) {
         error_report("Out of memory bounds when setting storage attributes");
@@ -103,39 +103,56 @@ static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
 static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
 {
     KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
-    MachineState *machine = MACHINE(qdev_get_machine());
-    unsigned long max = machine->ram_size / TARGET_PAGE_SIZE;
-    /* We do not need to reach the maximum buffer size allowed */
-    unsigned long cx, len = KVM_S390_SKEYS_MAX / 2;
+    unsigned long max = s390_get_memory_limit() / TARGET_PAGE_SIZE;
+    unsigned long start_gfn, end_gfn, pages;
+    GuestPhysBlockList guest_phys_blocks;
+    GuestPhysBlock *block;
     int r;
     struct kvm_s390_cmma_log clog = {
         .flags = 0,
         .mask = ~0ULL,
     };
 
-    if (sas->incoming_buffer) {
-        for (cx = 0; cx + len <= max; cx += len) {
-            clog.start_gfn = cx;
-            clog.count = len;
-            clog.values = (uint64_t)(sas->incoming_buffer + cx);
-            r = kvm_vm_ioctl(kvm_state, KVM_S390_SET_CMMA_BITS, &clog);
-            if (r) {
-                error_report("KVM_S390_SET_CMMA_BITS failed: %s", strerror(-r));
-                return;
-            }
-        }
-        if (cx < max) {
-            clog.start_gfn = cx;
-            clog.count = max - cx;
-            clog.values = (uint64_t)(sas->incoming_buffer + cx);
+    if (!sas->incoming_buffer) {
+        return;
+    }
+    guest_phys_blocks_init(&guest_phys_blocks);
+    guest_phys_blocks_append(&guest_phys_blocks);
+
+    QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
+        assert(QEMU_IS_ALIGNED(block->target_start, TARGET_PAGE_SIZE));
+        assert(QEMU_IS_ALIGNED(block->target_end, TARGET_PAGE_SIZE));
+
+        start_gfn = block->target_start / TARGET_PAGE_SIZE;
+        end_gfn = block->target_end / TARGET_PAGE_SIZE;
+
+        while (start_gfn < end_gfn) {
+            /* Don't exceed the maximum buffer size. */
+            pages = MIN(end_gfn - start_gfn, KVM_S390_SKEYS_MAX / 2);
+
+            /*
+             * If we ever get guest physical memory beyond the configured
+             * memory limit, something went very wrong.
+             */
+            assert(start_gfn + pages <= max);
+
+            clog.start_gfn = start_gfn;
+            clog.count = pages;
+            clog.values = (uint64_t)(sas->incoming_buffer + start_gfn);
             r = kvm_vm_ioctl(kvm_state, KVM_S390_SET_CMMA_BITS, &clog);
             if (r) {
                 error_report("KVM_S390_SET_CMMA_BITS failed: %s", strerror(-r));
+                goto out;
             }
+
+            start_gfn += pages;
         }
-        g_free(sas->incoming_buffer);
-        sas->incoming_buffer = NULL;
     }
+
+out:
+    guest_phys_blocks_free(&guest_phys_blocks);
+    g_free(sas->incoming_buffer);
+    sas->incoming_buffer = NULL;
 }
 
 static int kvm_s390_stattrib_set_migrationmode(S390StAttribState *sa, bool val,
-- 
2.46.0

[PATCH v1 09/14] s390x/s390-skeys: prepare for memory devices

[David Hildenbrand]

With memory devices, we will have storage keys for memory that
exceeds the initial ram size.

The TODO already states that current handling is subopimal,
but we won't worry about improving that (TCG-only) thing for now.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-skeys.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index bf22d6863e..8716c2e73e 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -251,9 +251,7 @@ static bool qemu_s390_enable_skeys(S390SKeysState *ss)
      *    g_once_init_enter() is good enough.
      */
     if (g_once_init_enter(&initialized)) {
-        MachineState *machine = MACHINE(qdev_get_machine());
-
-        skeys->key_count = machine->ram_size / TARGET_PAGE_SIZE;
+        skeys->key_count = s390_get_memory_limit() / TARGET_PAGE_SIZE;
         skeys->keydata = g_malloc0(skeys->key_count);
         g_once_init_leave(&initialized, 1);
     }
-- 
2.46.0

[PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[David Hildenbrand]

We actually want to check the available RAM, not the maximum RAM size.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/kvm/pv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index dde836d21a..424cce75ca 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
      * If the feature is not present or if the VM is not larger than 2 GiB,
      * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
      */
-    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
+    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
         !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
         return false;
     }
-- 
2.46.0

Re: [PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[Nina Schoetterl-Glausch]

On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
> We actually want to check the available RAM, not the maximum RAM size.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Nit below.
> ---
>  target/s390x/kvm/pv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
> index dde836d21a..424cce75ca 100644
> --- a/target/s390x/kvm/pv.c
> +++ b/target/s390x/kvm/pv.c
> @@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>       * If the feature is not present or if the VM is not larger than 2 GiB,
>       * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
>       */
> -    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
> +    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
>          !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
>          return false;
>      }

If I understood the kernel code right, the decision is made wrt
the size of the gmap address space, which is the same as the
limit set for the VM. So using s390_get_memory_limit would be
semantically cleaner.

Re: [PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[David Hildenbrand]

On 24.09.24 18:22, Nina Schoetterl-Glausch wrote:
> On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
>> We actually want to check the available RAM, not the maximum RAM size.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Nit below.
>> ---
>>   target/s390x/kvm/pv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
>> index dde836d21a..424cce75ca 100644
>> --- a/target/s390x/kvm/pv.c
>> +++ b/target/s390x/kvm/pv.c
>> @@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>>        * If the feature is not present or if the VM is not larger than 2 GiB,
>>        * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
>>        */
>> -    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
>> +    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
>>           !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
>>           return false;
>>       }
> 
> If I understood the kernel code right, the decision is made wrt
> the size of the gmap address space, which is the same as the
> limit set for the VM. So using s390_get_memory_limit would be
> semantically cleaner.

I wonder if we should just drop the RAM size check. Not convinced the 
slightly faster reboot for such small VMs is really relevant? Makes the 
code more complicated than really necessary.


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[David Hildenbrand]

On 24.09.24 22:17, David Hildenbrand wrote:
> On 24.09.24 18:22, Nina Schoetterl-Glausch wrote:
>> On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
>>> We actually want to check the available RAM, not the maximum RAM size.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>
>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> Nit below.
>>> ---
>>>    target/s390x/kvm/pv.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
>>> index dde836d21a..424cce75ca 100644
>>> --- a/target/s390x/kvm/pv.c
>>> +++ b/target/s390x/kvm/pv.c
>>> @@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>>>         * If the feature is not present or if the VM is not larger than 2 GiB,
>>>         * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
>>>         */
>>> -    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
>>> +    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
>>>            !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
>>>            return false;
>>>        }
>>
>> If I understood the kernel code right, the decision is made wrt
>> the size of the gmap address space, which is the same as the
>> limit set for the VM. So using s390_get_memory_limit would be
>> semantically cleaner.
> 
> I wonder if we should just drop the RAM size check. Not convinced the
> slightly faster reboot for such small VMs is really relevant? Makes the
> code more complicated than really necessary.

Thinking about it,

diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index 424cce75ca..bbb2108546 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
       * If the feature is not present or if the VM is not larger than 2 GiB,
       * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
       */
-    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
+    if (s390_get_memory_limit(ms) < 2 * GiB ||
          !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
          return false;
      }

Is probably the easiest change, thanks!


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[Christian Borntraeger]

Am 24.09.24 um 22:17 schrieb David Hildenbrand:
> On 24.09.24 18:22, Nina Schoetterl-Glausch wrote:
>> On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
>>> We actually want to check the available RAM, not the maximum RAM size.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>
>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> Nit below.
>>> ---
>>>   target/s390x/kvm/pv.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
>>> index dde836d21a..424cce75ca 100644
>>> --- a/target/s390x/kvm/pv.c
>>> +++ b/target/s390x/kvm/pv.c
>>> @@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>>>        * If the feature is not present or if the VM is not larger than 2 GiB,
>>>        * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
>>>        */
>>> -    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
>>> +    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
>>>           !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
>>>           return false;
>>>       }
>>
>> If I understood the kernel code right, the decision is made wrt
>> the size of the gmap address space, which is the same as the
>> limit set for the VM. So using s390_get_memory_limit would be
>> semantically cleaner.
> 
> I wonder if we should just drop the RAM size check. Not convinced the slightly faster reboot for such small VMs is really relevant? Makes the code more complicated than really necessary.

IIRC there have been functional issues with small guests and asnyc. Claudio, do you remember?

Re: [PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[Claudio Imbrenda]

On Mon, 30 Sep 2024 13:15:52 +0200
Christian Borntraeger <borntraeger@linux.ibm.com> wrote:

> Am 24.09.24 um 22:17 schrieb David Hildenbrand:
> > On 24.09.24 18:22, Nina Schoetterl-Glausch wrote:  
> >> On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:  
> >>> We actually want to check the available RAM, not the maximum RAM size.
> >>>
> >>> Signed-off-by: David Hildenbrand <david@redhat.com>  
> >>
> >> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> >> Nit below.  
> >>> ---
> >>>   target/s390x/kvm/pv.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
> >>> index dde836d21a..424cce75ca 100644
> >>> --- a/target/s390x/kvm/pv.c
> >>> +++ b/target/s390x/kvm/pv.c
> >>> @@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
> >>>        * If the feature is not present or if the VM is not larger than 2 GiB,
> >>>        * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
> >>>        */
> >>> -    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
> >>> +    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
> >>>           !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
> >>>           return false;
> >>>       }  
> >>
> >> If I understood the kernel code right, the decision is made wrt
> >> the size of the gmap address space, which is the same as the
> >> limit set for the VM. So using s390_get_memory_limit would be
> >> semantically cleaner.  
> > 
> > I wonder if we should just drop the RAM size check. Not convinced the slightly faster reboot for such small VMs is really relevant? Makes the code more complicated than really necessary.  
> 
> IIRC there have been functional issues with small guests and asnyc. Claudio, do you remember?

if we are <2G, KVM allocates a segment table as the highest level table
for the gmap ASCE. there are pointers lurking around in the reverse
mapping prefix_tree, which point directly into segment tables.

if the ASCE is region3 or higher, that's not an issue. if it's a
segment table, then it's an issue, because those pointers will end up
pointing into freed memory, once the old asce is freed.

in short, we have to guarantee that we will never set aside a gmap ASCE
if it is a segment table.

Re: [PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[David Hildenbrand]

On 30.09.24 13:37, Claudio Imbrenda wrote:
> On Mon, 30 Sep 2024 13:15:52 +0200
> Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> 
>> Am 24.09.24 um 22:17 schrieb David Hildenbrand:
>>> On 24.09.24 18:22, Nina Schoetterl-Glausch wrote:
>>>> On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:
>>>>> We actually want to check the available RAM, not the maximum RAM size.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>
>>>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>>> Nit below.
>>>>> ---
>>>>>    target/s390x/kvm/pv.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
>>>>> index dde836d21a..424cce75ca 100644
>>>>> --- a/target/s390x/kvm/pv.c
>>>>> +++ b/target/s390x/kvm/pv.c
>>>>> @@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>>>>>         * If the feature is not present or if the VM is not larger than 2 GiB,
>>>>>         * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
>>>>>         */
>>>>> -    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
>>>>> +    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
>>>>>            !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
>>>>>            return false;
>>>>>        }
>>>>
>>>> If I understood the kernel code right, the decision is made wrt
>>>> the size of the gmap address space, which is the same as the
>>>> limit set for the VM. So using s390_get_memory_limit would be
>>>> semantically cleaner.
>>>
>>> I wonder if we should just drop the RAM size check. Not convinced the slightly faster reboot for such small VMs is really relevant? Makes the code more complicated than really necessary.
>>
>> IIRC there have been functional issues with small guests and asnyc. Claudio, do you remember?
> 
> if we are <2G, KVM allocates a segment table as the highest level table
> for the gmap ASCE. there are pointers lurking around in the reverse
> mapping prefix_tree, which point directly into segment tables.
> 
> if the ASCE is region3 or higher, that's not an issue. if it's a
> segment table, then it's an issue, because those pointers will end up
> pointing into freed memory, once the old asce is freed.
> 
> in short, we have to guarantee that we will never set aside a gmap ASCE
> if it is a segment table.

Thanks for the details, the kernel seems to properly safeguard against 
that. For now, I'll turn it into a check against the memory limit, which 
directly translates to the gmap ASCE used.

Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 10/14] s390x/pv: check initial, not maximum RAM size

[Claudio Imbrenda]

On Mon, 30 Sep 2024 15:14:52 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 30.09.24 13:37, Claudio Imbrenda wrote:
> > On Mon, 30 Sep 2024 13:15:52 +0200
> > Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> >   
> >> Am 24.09.24 um 22:17 schrieb David Hildenbrand:  
> >>> On 24.09.24 18:22, Nina Schoetterl-Glausch wrote:  
> >>>> On Tue, 2024-09-10 at 19:58 +0200, David Hildenbrand wrote:  
> >>>>> We actually want to check the available RAM, not the maximum RAM size.
> >>>>>
> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com>  
> >>>>
> >>>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> >>>> Nit below.  
> >>>>> ---
> >>>>>    target/s390x/kvm/pv.c | 2 +-
> >>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
> >>>>> index dde836d21a..424cce75ca 100644
> >>>>> --- a/target/s390x/kvm/pv.c
> >>>>> +++ b/target/s390x/kvm/pv.c
> >>>>> @@ -133,7 +133,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
> >>>>>         * If the feature is not present or if the VM is not larger than 2 GiB,
> >>>>>         * KVM_PV_ASYNC_CLEANUP_PREPARE fill fail; no point in attempting it.
> >>>>>         */
> >>>>> -    if ((MACHINE(ms)->maxram_size <= 2 * GiB) ||
> >>>>> +    if ((MACHINE(ms)->ram_size <= 2 * GiB) ||
> >>>>>            !kvm_check_extension(kvm_state, KVM_CAP_S390_PROTECTED_ASYNC_DISABLE)) {
> >>>>>            return false;
> >>>>>        }  
> >>>>
> >>>> If I understood the kernel code right, the decision is made wrt
> >>>> the size of the gmap address space, which is the same as the
> >>>> limit set for the VM. So using s390_get_memory_limit would be
> >>>> semantically cleaner.  
> >>>
> >>> I wonder if we should just drop the RAM size check. Not convinced the slightly faster reboot for such small VMs is really relevant? Makes the code more complicated than really necessary.  
> >>
> >> IIRC there have been functional issues with small guests and asnyc. Claudio, do you remember?  
> > 
> > if we are <2G, KVM allocates a segment table as the highest level table
> > for the gmap ASCE. there are pointers lurking around in the reverse
> > mapping prefix_tree, which point directly into segment tables.
> > 
> > if the ASCE is region3 or higher, that's not an issue. if it's a
> > segment table, then it's an issue, because those pointers will end up
> > pointing into freed memory, once the old asce is freed.
> > 
> > in short, we have to guarantee that we will never set aside a gmap ASCE
> > if it is a segment table.  
> 
> Thanks for the details, the kernel seems to properly safeguard against 

it does, but it returns an error if userspace tries; the check there is
to prevent qemu from emitting confusing error messages.

> that. For now, I'll turn it into a check against the memory limit, which 
> directly translates to the gmap ASCE used.

sounds good

> 
> Thanks!

no problem :)
 

[PATCH v1 11/14] s390x/s390-virtio-ccw: prepare for memory devices

[David Hildenbrand]

Let's prepare our address space for memory devices if enabled via
"maxmem" and if we have CONFIG_MEM_DEVICE enabled at all. Note that
CONFIG_MEM_DEVICE will be selected automatically once we add support
for devices.

Just like on other architectures, the region container for memory devices
is placed directly above our initial memory. For now, we only align the
start address of the region up to 1 GiB, but we won't add any additional
space to the region for internal alignment purposes; this can be done in
the future if really required.

The RAM size returned via SCLP is not modified, as this only
covers initial RAM (and standby memory we don't implement) and not memory
devices; clarify that in the docs of read_SCP_info(). Existing OSes without
support for memory devices will keep working as is, even when memory
devices would be attached the VM.

Guest OSs which support memory devices, such as virtio-mem, will
consult diag500(), to find out the maximum possible pfn. Guest OSes that
don't support memory devices, don't have to be changed and will continue
relying on information provided by SCLP.

There are no remaining maxram_size users in s390x code, and the remaining
ram_size users only care about initial RAM:
* hw/s390x/ipl.c
* hw/s390x/s390-hypercall.c
* hw/s390x/sclp.c
* target/s390x/kvm/pv.c

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

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 08156f0682..0a9d25620d 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -126,7 +126,7 @@ static void s390_memory_init(MachineState *machine)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = machine->ram;
     uint64_t ram_size = memory_region_size(ram);
-    uint64_t hw_limit;
+    uint64_t hw_limit, devmem_base, devmem_size;
     int ret;
 
     if (!QEMU_IS_ALIGNED(ram_size, 1 * MiB)) {
@@ -140,7 +140,21 @@ static void s390_memory_init(MachineState *machine)
         exit(EXIT_FAILURE);
     }
 
-    ret = s390_set_memory_limit(ram_size, &hw_limit);
+    devmem_size = 0;
+    devmem_base = ram_size;
+#ifdef CONFIG_MEM_DEVICE
+    if (machine->ram_size < machine->maxram_size) {
+
+        /*
+         * Make sure memory devices have a sane default alignment, even
+         * when weird initial memory sizes are specified.
+         */
+        devmem_base = QEMU_ALIGN_UP(devmem_base, 1 * GiB);
+        devmem_size = machine->maxram_size - machine->ram_size;
+    }
+#endif
+
+    ret = s390_set_memory_limit(devmem_base + devmem_size, &hw_limit);
     if (ret == -E2BIG) {
         error_report("host supports a maximum of %" PRIu64 " GB",
                      hw_limit / GiB);
@@ -153,6 +167,13 @@ static void s390_memory_init(MachineState *machine)
     /* Map the initial memory. Must happen after setting the memory limit. */
     memory_region_add_subregion(sysmem, 0, ram);
 
+    /* Initialize address space for memory devices. */
+#ifdef CONFIG_MEM_DEVICE
+    if (devmem_size) {
+        machine_memory_devices_init(machine, devmem_base, devmem_size);
+    }
+#endif /* CONFIG_MEM_DEVICE */
+
     /*
      * Configure the maximum page size. As no memory devices were created
      * yet, this is the page size of initial memory only.
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index fac09816bf..fe4216a10d 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -162,7 +162,11 @@ 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. maxram_size is used for sizing the
+     * memory device region, which is not exposed through SCLP but through
+     * diag500.
+     */
     rnmax = machine->ram_size >> sclp->increment_size;
     if (rnmax < 0x10000) {
         read_info->rnmax = cpu_to_be16(rnmax);
-- 
2.46.0

[PATCH v1 12/14] s390x: introduce s390_get_max_pagesize()

[David Hildenbrand]

Let's add a way to return the value (successfully) set via
s390_set_max_pagesize() previously. This will be helpful to reject
hotplugged memory devices that would exceed this initially set page size.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu-sysemu.c | 16 ++++++++++++++++
 target/s390x/cpu.h        |  1 +
 2 files changed, 17 insertions(+)

diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 1915567b3a..bee5d6c2ee 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -278,11 +278,27 @@ uint64_t s390_get_memory_limit(void)
     return memory_limit;
 }
 
+static uint64_t max_pagesize;
+
 void s390_set_max_pagesize(uint64_t pagesize, Error **errp)
 {
+    ERRP_GUARD();
+
+    if (max_pagesize) {
+        error_setg(errp, "Maximum page size can only be set once");
+        return;
+    }
     if (kvm_enabled()) {
         kvm_s390_set_max_pagesize(pagesize, errp);
     }
+    if (!*errp) {
+        max_pagesize = pagesize;
+    }
+}
+
+uint64_t s390_get_max_pagesize(void)
+{
+    return max_pagesize;
 }
 
 void s390_cmma_reset(void)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7a51b606ed..37845e0d9d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -897,6 +897,7 @@ void s390_crypto_reset(void);
 int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit);
 uint64_t s390_get_memory_limit(void);
 void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
+uint64_t s390_get_max_pagesize(void);
 void s390_cmma_reset(void);
 void s390_enable_css_support(S390CPU *cpu);
 void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
-- 
2.46.0

Re: [PATCH v1 12/14] s390x: introduce s390_get_max_pagesize()

[David Hildenbrand]

On 10.09.24 19:58, David Hildenbrand wrote:
> Let's add a way to return the value (successfully) set via
> s390_set_max_pagesize() previously. This will be helpful to reject
> hotplugged memory devices that would exceed this initially set page size.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

I'll send a v2 shortly, this patch will become:

 From 687f72e488ebed59e57793699d26feb4eebdc9e3 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Sun, 8 Sep 2024 23:25:27 +0200
Subject: [PATCH] s390x: remember the maximum page size

Let's remember the value (successfully) set via s390_set_max_pagesize().
This will be helpful to reject hotplugged memory devices that would exceed
this initially set page size.

Handle it just like how we handle s390_get_memory_limit(), storing it in
the machine, and moving the handling to machine code.

Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
  hw/s390x/s390-virtio-ccw.c         | 12 +++++++++++-
  include/hw/s390x/s390-virtio-ccw.h |  1 +
  target/s390x/cpu-sysemu.c          |  7 -------
  target/s390x/cpu.h                 |  1 -
  4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 2031c4cf29..65f266a78f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -150,6 +150,16 @@ uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
      return s390ms->memory_limit;
  }
  
+static void s390_set_max_pagesize(S390CcwMachineState *s390ms,
+                                  uint64_t pagesize)
+{
+    assert(!s390ms->max_pagesize && pagesize);
+    if (kvm_enabled()) {
+        kvm_s390_set_max_pagesize(pagesize, &error_fatal);
+    }
+    s390ms->max_pagesize = pagesize;
+}
+
  static void s390_memory_init(MachineState *machine)
  {
      S390CcwMachineState *s390ms = S390_CCW_MACHINE(machine);
@@ -198,7 +208,7 @@ static void s390_memory_init(MachineState *machine)
       * Configure the maximum page size. As no memory devices were created
       * yet, this is the page size of initial memory only.
       */
-    s390_set_max_pagesize(qemu_maxrampagesize(), &error_fatal);
+    s390_set_max_pagesize(s390ms, qemu_maxrampagesize());
      /* Initialize storage key device */
      s390_skeys_init();
      /* Initialize storage attributes device */
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index eb04542979..5a730f5d07 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -30,6 +30,7 @@ struct S390CcwMachineState {
      bool pv;
      uint8_t loadparm[8];
      uint64_t memory_limit;
+    uint64_t max_pagesize;
  
      SCLPDevice *sclp;
  };
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 3118a25fee..706a5c53e2 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -255,13 +255,6 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
      return s390_count_running_cpus();
  }
  
-void s390_set_max_pagesize(uint64_t pagesize, Error **errp)
-{
-    if (kvm_enabled()) {
-        kvm_s390_set_max_pagesize(pagesize, errp);
-    }
-}
-
  void s390_cmma_reset(void)
  {
      if (kvm_enabled()) {
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index b4506539f0..5b7992deda 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -881,7 +881,6 @@ static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg)
  
  /* cpu.c */
  void s390_crypto_reset(void);
-void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
  void s390_cmma_reset(void);
  void s390_enable_css_support(S390CPU *cpu);
  void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
-- 
2.46.1



-- 
Cheers,

David / dhildenb

[PATCH v1 13/14] s390x/virtio-ccw: add support for virtio based memory devices

[David Hildenbrand]

Let's implement support for abstract virtio based memory devices, using
the virtio-pci implementation as an orientation.

As we neither support virtio-mem or virtio-pmem yet, the code is
effectively unused and not wired up. We'll implement support for
virtio-mem based on this next.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 MAINTAINERS              |   2 +
 hw/s390x/meson.build     |   1 +
 hw/s390x/virtio-ccw-md.c | 153 +++++++++++++++++++++++++++++++++++++++
 hw/s390x/virtio-ccw-md.h |  44 +++++++++++
 hw/virtio/Kconfig        |   1 +
 5 files changed, 201 insertions(+)
 create mode 100644 hw/s390x/virtio-ccw-md.c
 create mode 100644 hw/s390x/virtio-ccw-md.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 0c1bc69828..53ed2c5f0f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2400,6 +2400,8 @@ F: include/hw/virtio/virtio-crypto.h
 virtio based memory device
 M: David Hildenbrand <david@redhat.com>
 S: Supported
+F: hw/s390x/virtio-ccw-md.c
+F: hw/s390x/virtio-ccw-md.h
 F: hw/virtio/virtio-md-pci.c
 F: include/hw/virtio/virtio-md-pci.h
 F: stubs/virtio-md-pci.c
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 71ec747f4c..4df40da855 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -48,6 +48,7 @@ endif
 virtio_ss.add(when: 'CONFIG_VHOST_SCSI', if_true: files('vhost-scsi-ccw.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-ccw.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'))
+virtio_ss.add(when: 'CONFIG_VIRTIO_MD', if_true: files('virtio-ccw-md.c'))
 s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
 
 hw_arch += {'s390x': s390x_ss}
diff --git a/hw/s390x/virtio-ccw-md.c b/hw/s390x/virtio-ccw-md.c
new file mode 100644
index 0000000000..de333282df
--- /dev/null
+++ b/hw/s390x/virtio-ccw-md.c
@@ -0,0 +1,153 @@
+/*
+ * Virtio CCW support for abstract virtio based memory device
+ *
+ * Copyright (C) 2024 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/s390x/virtio-ccw-md.h"
+#include "hw/mem/memory-device.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+
+void virtio_ccw_md_pre_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+    DeviceState *dev = DEVICE(vmd);
+    HotplugHandler *bus_handler = qdev_get_bus_hotplug_handler(dev);
+    MemoryDeviceState *md = MEMORY_DEVICE(vmd);
+    Error *local_err = NULL;
+
+    if (!bus_handler && dev->hotplugged) {
+        /*
+         * Without a bus hotplug handler, we cannot control the plug/unplug
+         * order. We should never reach this point when hotplugging, but
+         * better add a safety net.
+         */
+        error_setg(errp, "hotplug of virtio based memory devices not supported"
+                   " on this bus.");
+        return;
+    }
+
+    /*
+     * 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, ms, &local_err);
+    if (!local_err && bus_handler) {
+        hotplug_handler_pre_plug(bus_handler, dev, &local_err);
+    }
+    error_propagate(errp, local_err);
+}
+
+void virtio_ccw_md_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+    DeviceState *dev = DEVICE(vmd);
+    HotplugHandler *bus_handler = qdev_get_bus_hotplug_handler(dev);
+    MemoryDeviceState *md = MEMORY_DEVICE(vmd);
+    Error *local_err = NULL;
+
+    /*
+     * 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(md, ms);
+    if (bus_handler) {
+        hotplug_handler_plug(bus_handler, dev, &local_err);
+        if (local_err) {
+            memory_device_unplug(md, ms);
+        }
+    }
+    error_propagate(errp, local_err);
+}
+
+void virtio_ccw_md_unplug_request(VirtIOMDCcw *vmd, MachineState *ms,
+                                  Error **errp)
+{
+    VirtIOMDCcwClass *vmdc = VIRTIO_MD_CCW_GET_CLASS(vmd);
+    DeviceState *dev = DEVICE(vmd);
+    HotplugHandler *bus_handler = qdev_get_bus_hotplug_handler(dev);
+    HotplugHandlerClass *hdc;
+    Error *local_err = NULL;
+
+    if (!vmdc->unplug_request_check) {
+        error_setg(errp,
+                   "this virtio based memory devices cannot be unplugged");
+        return;
+    }
+
+    if (!bus_handler) {
+        error_setg(errp, "hotunplug of virtio based memory devices not"
+                   "supported on this bus");
+        return;
+    }
+
+    vmdc->unplug_request_check(vmd, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    /*
+     * Forward the async request or turn it into a sync request (handling it
+     * like qdev_unplug()).
+     */
+    hdc = HOTPLUG_HANDLER_GET_CLASS(bus_handler);
+    if (hdc->unplug_request) {
+        hotplug_handler_unplug_request(bus_handler, dev, &local_err);
+    } else {
+        virtio_ccw_md_unplug(vmd, ms, &local_err);
+        if (!local_err) {
+            object_unparent(OBJECT(dev));
+        }
+    }
+}
+
+void virtio_ccw_md_unplug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp)
+{
+    DeviceState *dev = DEVICE(vmd);
+    HotplugHandler *bus_handler = qdev_get_bus_hotplug_handler(dev);
+    MemoryDeviceState *md = MEMORY_DEVICE(vmd);
+    Error *local_err = NULL;
+
+    /* Unplug the memory device while it is still realized. */
+    memory_device_unplug(md, ms);
+
+    if (bus_handler) {
+        hotplug_handler_unplug(bus_handler, dev, &local_err);
+        if (local_err) {
+            /* Not expected to fail ... but still try to recover. */
+            memory_device_plug(md, ms);
+            error_propagate(errp, local_err);
+            return;
+        }
+    } else {
+        /* Very unexpected, but let's just try to do the right thing. */
+        warn_report("Unexpected unplug of virtio based memory device");
+        qdev_unrealize(dev);
+    }
+}
+
+static const TypeInfo virtio_ccw_md_info = {
+    .name = TYPE_VIRTIO_MD_CCW,
+    .parent = TYPE_VIRTIO_CCW_DEVICE,
+    .instance_size = sizeof(VirtIOMDCcw),
+    .class_size = sizeof(VirtIOMDCcwClass),
+    .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_MEMORY_DEVICE },
+        { }
+    },
+};
+
+static void virtio_ccw_md_register(void)
+{
+    type_register_static(&virtio_ccw_md_info);
+}
+type_init(virtio_ccw_md_register)
diff --git a/hw/s390x/virtio-ccw-md.h b/hw/s390x/virtio-ccw-md.h
new file mode 100644
index 0000000000..e83d40f6c4
--- /dev/null
+++ b/hw/s390x/virtio-ccw-md.h
@@ -0,0 +1,44 @@
+/*
+ * Virtio CCW support for abstract virtio based memory device
+ *
+ * Copyright (C) 2024 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.
+ */
+
+#ifndef HW_S390X_VIRTIO_CCW_MD_H
+#define HW_S390X_VIRTIO_CCW_MD_H
+
+#include "virtio-ccw.h"
+#include "qom/object.h"
+
+/*
+ * virtio-md-ccw: This extends VirtioCcwDevice.
+ */
+#define TYPE_VIRTIO_MD_CCW "virtio-md-ccw"
+
+OBJECT_DECLARE_TYPE(VirtIOMDCcw, VirtIOMDCcwClass, VIRTIO_MD_CCW)
+
+struct VirtIOMDCcwClass {
+    /* private */
+    VirtIOCCWDeviceClass parent;
+
+    /* public */
+    void (*unplug_request_check)(VirtIOMDCcw *vmd, Error **errp);
+};
+
+struct VirtIOMDCcw {
+    VirtioCcwDevice parent_obj;
+};
+
+void virtio_ccw_md_pre_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp);
+void virtio_ccw_md_plug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp);
+void virtio_ccw_md_unplug_request(VirtIOMDCcw *vmd, MachineState *ms,
+                                  Error **errp);
+void virtio_ccw_md_unplug(VirtIOMDCcw *vmd, MachineState *ms, Error **errp);
+
+#endif
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index 0afec2ae92..f4b14e1a44 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -25,6 +25,7 @@ config VIRTIO_MMIO
 config VIRTIO_CCW
     bool
     select VIRTIO
+    select VIRTIO_MD_SUPPORTED
 
 config VIRTIO_BALLOON
     bool
-- 
2.46.0

[PATCH v1 14/14] s390x: virtio-mem support

[David Hildenbrand]

Let's add our virtio-mem-ccw proxy device and wire it up. We should
be supporting everything (e.g., device unplug, "dynamic-memslots") that
we already support for the virtio-pci variant.

With a Linux guest that supports virtio-mem (and has automatic memory
onlining properly configured, the following example will work:

1. Start a VM with 4G initial memory and a virtio-mem device with a maximum
   capacity of 16GB:

   qemu/build/qemu-system-s390x \
    --enable-kvm \
    -m 4G,maxmem=20G \
    -nographic \
    -smp 8 \
    -hda Fedora-Server-KVM-40-1.14.s390x.qcow2 \
    -chardev socket,id=monitor,path=/var/tmp/monitor,server,nowait \
    -mon chardev=monitor,mode=readline \
    -object memory-backend-ram,id=mem0,size=16G,reserve=off \
    -device virtio-mem-ccw,id=vmem0,memdev=mem0,dynamic-memslots=on

2. Query the current size of virtio-mem device:

    (qemu) info memory-devices
    Memory device [virtio-mem]: "vmem0"
      memaddr: 0x100000000
      node: 0
      requested-size: 0
      size: 0
      max-size: 17179869184
      block-size: 1048576
      memdev: /objects/mem0

3. Request to grow it to 8GB (hotplug 8GB):

    (qemu) qom-set vmem0 requested-size 8G
    (qemu) info memory-devices
    Memory device [virtio-mem]: "vmem0"
      memaddr: 0x100000000
      node: 0
      requested-size: 8589934592
      size: 8589934592
      max-size: 17179869184
      block-size: 1048576
      memdev: /objects/mem0

4. Request to grow to 16GB (hotplug another 8GB):

    (qemu) qom-set vmem0 requested-size 16G
    (qemu) info memory-devices
    Memory device [virtio-mem]: "vmem0"
      memaddr: 0x100000000
      node: 0
      requested-size: 17179869184
      size: 17179869184
      max-size: 17179869184
      block-size: 1048576
      memdev: /objects/mem0

5. Try to hotunplug all memory again, shrinking to 0GB:

    (qemu) qom-set vmem0 requested-size 0G
    (qemu) info memory-devices
    Memory device [virtio-mem]: "vmem0"
      memaddr: 0x100000000
      node: 0
      requested-size: 0
      size: 0
      max-size: 17179869184
      block-size: 1048576
      memdev: /objects/mem0

6. If it worked, unplug the device

    (qemu) device_del vmem0
    (qemu) info memory-devices
    (qemu) object_del mem0

7. Hotplug a new device with a smaller capacity and directly size it to 1GB

    (qemu) object_add memory-backend-ram,id=mem0,size=8G,reserve=off
    (qemu) device_add virtio-mem-ccw,id=vmem0,memdev=mem0,\
                      dynamic-memslots=on,requested-size=1G
    (qemu) info memory-devices
    Memory device [virtio-mem]: "vmem0"
      memaddr: 0x100000000
      node: 0
      requested-size: 1073741824
      size: 1073741824
      max-size: 8589934592
      block-size: 1048576
      memdev: /objects/mem0

Trying to use a virtio-mem device backed by hugetlb into a !hugetlb VM
correctly results in the error:
   ... Memory device uses a bigger page size than initial memory

Note that the virtio-mem driver in Linux will supports 1 MiB (pageblock)
granularity.

Note that we won't wire up virtio-mem-pci (should currently be
impossible due to lack of support for MSI-X), but we'll add a safety net
to reject plugging them.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 MAINTAINERS                |   2 +
 hw/s390x/Kconfig           |   1 +
 hw/s390x/meson.build       |   1 +
 hw/s390x/s390-virtio-ccw.c |  45 +++++++-
 hw/s390x/virtio-ccw-mem.c  | 226 +++++++++++++++++++++++++++++++++++++
 hw/s390x/virtio-ccw-mem.h  |  34 ++++++
 hw/virtio/virtio-mem.c     |   4 +-
 7 files changed, 311 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/virtio-ccw-mem.c
 create mode 100644 hw/s390x/virtio-ccw-mem.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 53ed2c5f0f..f8e0b6c8e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2413,6 +2413,8 @@ W: https://virtio-mem.gitlab.io/
 F: hw/virtio/virtio-mem.c
 F: hw/virtio/virtio-mem-pci.h
 F: hw/virtio/virtio-mem-pci.c
+F: hw/s390x/virtio-ccw-mem.c
+F: hw/s390x/virtio-ccw-mem.h
 F: include/hw/virtio/virtio-mem.h
 
 virtio-snd
diff --git a/hw/s390x/Kconfig b/hw/s390x/Kconfig
index 3bbf4ae56e..5d57daff77 100644
--- a/hw/s390x/Kconfig
+++ b/hw/s390x/Kconfig
@@ -15,3 +15,4 @@ config S390_CCW_VIRTIO
     select SCLPCONSOLE
     select VIRTIO_CCW
     select MSI_NONBROKEN
+    select VIRTIO_MEM_SUPPORTED
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 4df40da855..a8434e7918 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -49,6 +49,7 @@ virtio_ss.add(when: 'CONFIG_VHOST_SCSI', if_true: files('vhost-scsi-ccw.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-ccw.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-ccw.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_MD', if_true: files('virtio-ccw-md.c'))
+virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-ccw-mem.c'))
 s390x_ss.add_all(when: 'CONFIG_VIRTIO_CCW', if_true: virtio_ss)
 
 hw_arch += {'s390x': s390x_ss}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 0a9d25620d..8d4a23db7d 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -45,6 +45,8 @@
 #include "migration/blocker.h"
 #include "qapi/visitor.h"
 #include "hw/s390x/cpu-topology.h"
+#include "hw/virtio/virtio-md-pci.h"
+#include "hw/s390x/virtio-ccw-md.h"
 #include CONFIG_DEVICES
 
 static Error *pv_mig_blocker;
@@ -529,11 +531,37 @@ static void s390_machine_reset(MachineState *machine, ShutdownCause reason)
     s390_ipl_clear_reset_request();
 }
 
+static void s390_machine_device_pre_plug(HotplugHandler *hotplug_dev,
+                                         DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
+        virtio_ccw_md_pre_plug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
+        error_setg(errp,
+                   "PCI-attached virtio based memory device is not supported");
+    }
+}
+
 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_MD_CCW)) {
+        /*
+         * At this point, the device is realized and set all memdevs mapped, so
+         * qemu_maxrampagesize() will pick up the page sizes of these memdevs
+         * as well. Before we plug the device and expose any RAM memory regions
+         * to the system, make sure we don't exceed the previously set max page
+         * size. While only relevant for KVM, there is not really any use case
+         * for this with TCG, so we'll unconditionally reject it.
+         */
+        if (qemu_maxrampagesize() != s390_get_max_pagesize()) {
+            error_setg(errp, "Memory device uses a bigger page size than"
+                       " initial memory");
+            return;
+        }
+        virtio_ccw_md_plug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), errp);
     }
 }
 
@@ -543,6 +571,17 @@ 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_MD_CCW)) {
+        virtio_ccw_md_unplug_request(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev),
+                                     errp);
+    }
+}
+
+static void s390_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_CCW)) {
+        virtio_ccw_md_unplug(VIRTIO_MD_CCW(dev), MACHINE(hotplug_dev), errp);
     }
 }
 
@@ -592,7 +631,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_MD_CCW) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
         return HOTPLUG_HANDLER(machine);
     }
     return NULL;
@@ -768,8 +809,10 @@ 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;
+    hc->unplug = s390_machine_device_unplug;
     nc->nmi_monitor_handler = s390_nmi;
     mc->default_ram_id = "s390.ram";
     mc->default_nic = "virtio-net-ccw";
diff --git a/hw/s390x/virtio-ccw-mem.c b/hw/s390x/virtio-ccw-mem.c
new file mode 100644
index 0000000000..bee0d560cb
--- /dev/null
+++ b/hw/s390x/virtio-ccw-mem.c
@@ -0,0 +1,226 @@
+/*
+ * virtio-mem CCW implementation
+ *
+ * Copyright (C) 2024 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 "qapi/error.h"
+#include "qemu/module.h"
+#include "virtio-ccw-mem.h"
+#include "hw/mem/memory-device.h"
+#include "qapi/qapi-events-machine.h"
+#include "qapi/qapi-events-misc.h"
+
+static void virtio_ccw_mem_realize(VirtioCcwDevice *ccw_dev, Error **errp)
+{
+    VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(ccw_dev);
+    DeviceState *vdev = DEVICE(&dev->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 *dev = VIRTIO_MEM_CCW(md);
+    VirtIOMEM *vmem = &dev->vdev;
+    VirtIOMEMClass *vmc = VIRTIO_MEM_GET_CLASS(vmem);
+
+    return vmc->get_memory_region(vmem, errp);
+}
+
+static void virtio_ccw_mem_decide_memslots(MemoryDeviceState *md,
+                                           unsigned int limit)
+{
+    VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(md);
+    VirtIOMEM *vmem = VIRTIO_MEM(&dev->vdev);
+    VirtIOMEMClass *vmc = VIRTIO_MEM_GET_CLASS(vmem);
+
+    vmc->decide_memslots(vmem, limit);
+}
+
+static unsigned int virtio_ccw_mem_get_memslots(MemoryDeviceState *md)
+{
+    VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(md);
+    VirtIOMEM *vmem = VIRTIO_MEM(&dev->vdev);
+    VirtIOMEMClass *vmc = VIRTIO_MEM_GET_CLASS(vmem);
+
+    return vmc->get_memslots(vmem);
+}
+
+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 *dev = VIRTIO_MEM_CCW(md);
+    VirtIOMEM *vmem = &dev->vdev;
+    VirtIOMEMClass *vpc = VIRTIO_MEM_GET_CLASS(vmem);
+    DeviceState *vdev = DEVICE(md);
+
+    if (vdev->id) {
+        vi->id = g_strdup(vdev->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 uint64_t virtio_ccw_mem_get_min_alignment(const MemoryDeviceState *md)
+{
+    return object_property_get_uint(OBJECT(md), VIRTIO_MEM_BLOCK_SIZE_PROP,
+                                    &error_abort);
+}
+
+static void virtio_ccw_mem_size_change_notify(Notifier *notifier, void *data)
+{
+    VirtIOMEMCcw *dev = container_of(notifier, VirtIOMEMCcw,
+                                         size_change_notifier);
+    DeviceState *vdev = DEVICE(dev);
+    char *qom_path = object_get_canonical_path(OBJECT(dev));
+    const uint64_t * const size_p = data;
+
+    qapi_event_send_memory_device_size_change(vdev->id, *size_p, qom_path);
+    g_free(qom_path);
+}
+
+static void virtio_ccw_mem_unplug_request_check(VirtIOMDCcw *vmd, Error **errp)
+{
+    VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(vmd);
+    VirtIOMEM *vmem = &dev->vdev;
+    VirtIOMEMClass *vpc = VIRTIO_MEM_GET_CLASS(vmem);
+
+    vpc->unplug_request_check(vmem, errp);
+}
+
+static void virtio_ccw_mem_get_requested_size(Object *obj, Visitor *v,
+                                              const char *name, void *opaque,
+                                              Error **errp)
+{
+    VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(obj);
+
+    object_property_get(OBJECT(&dev->vdev), name, v, errp);
+}
+
+static void virtio_ccw_mem_set_requested_size(Object *obj, Visitor *v,
+                                              const char *name, void *opaque,
+                                              Error **errp)
+{
+    VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(obj);
+    DeviceState *vdev = DEVICE(obj);
+
+    /*
+     * If we passed virtio_ccw_mem_unplug_request_check(), making sure that
+     * the requested size is 0, don't allow modifying the requested size
+     * anymore, otherwise the VM might end up hotplugging memory before
+     * handling the unplug request.
+     */
+    if (vdev->pending_deleted_event) {
+        error_setg(errp, "'%s' cannot be changed if the device is in the"
+                   " process of unplug", name);
+        return;
+    }
+
+    object_property_set(OBJECT(&dev->vdev), name, v, errp);
+}
+
+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);
+    VirtIOMDCcwClass *vmdc = VIRTIO_MD_CCW_CLASS(klass);
+
+    k->realize = virtio_ccw_mem_realize;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    device_class_set_props(dc, virtio_ccw_mem_properties);
+
+    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->decide_memslots = virtio_ccw_mem_decide_memslots;
+    mdc->get_memslots = virtio_ccw_mem_get_memslots;
+    mdc->fill_device_info = virtio_ccw_mem_fill_device_info;
+    mdc->get_min_alignment = virtio_ccw_mem_get_min_alignment;
+
+    vmdc->unplug_request_check = virtio_ccw_mem_unplug_request_check;
+}
+
+static void virtio_ccw_mem_instance_init(Object *obj)
+{
+    VirtIOMEMCcw *dev = VIRTIO_MEM_CCW(obj);
+    VirtIOMEMClass *vmc;
+    VirtIOMEM *vmem;
+
+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+                                TYPE_VIRTIO_MEM);
+
+    dev->size_change_notifier.notify = virtio_ccw_mem_size_change_notify;
+    vmem = &dev->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, &dev->size_change_notifier);
+
+    object_property_add_alias(obj, VIRTIO_MEM_BLOCK_SIZE_PROP,
+                              OBJECT(&dev->vdev), VIRTIO_MEM_BLOCK_SIZE_PROP);
+    object_property_add_alias(obj, VIRTIO_MEM_SIZE_PROP, OBJECT(&dev->vdev),
+                              VIRTIO_MEM_SIZE_PROP);
+    object_property_add(obj, VIRTIO_MEM_REQUESTED_SIZE_PROP, "size",
+                        virtio_ccw_mem_get_requested_size,
+                        virtio_ccw_mem_set_requested_size, NULL, NULL);
+}
+
+static const TypeInfo virtio_ccw_mem = {
+    .name = TYPE_VIRTIO_MEM_CCW,
+    .parent = TYPE_VIRTIO_MD_CCW,
+    .instance_size = sizeof(VirtIOMEMCcw),
+    .instance_init = virtio_ccw_mem_instance_init,
+    .class_init = virtio_ccw_mem_class_init,
+};
+
+static void virtio_ccw_mem_register_types(void)
+{
+    type_register_static(&virtio_ccw_mem);
+}
+type_init(virtio_ccw_mem_register_types)
diff --git a/hw/s390x/virtio-ccw-mem.h b/hw/s390x/virtio-ccw-mem.h
new file mode 100644
index 0000000000..730cd9fcd7
--- /dev/null
+++ b/hw/s390x/virtio-ccw-mem.h
@@ -0,0 +1,34 @@
+/*
+ * Virtio MEM CCW device
+ *
+ * Copyright (C) 2024 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.
+ */
+
+#ifndef HW_S390X_VIRTIO_CCW_MEM_H
+#define HW_S390X_VIRTIO_CCW_MEM_H
+
+#include "virtio-ccw-md.h"
+#include "hw/virtio/virtio-mem.h"
+#include "qom/object.h"
+
+typedef struct VirtIOMEMCcw VirtIOMEMCcw;
+
+/*
+ * virtio-mem-ccw: This extends VirtIOMDCcw
+ */
+#define TYPE_VIRTIO_MEM_CCW "virtio-mem-ccw"
+DECLARE_INSTANCE_CHECKER(VirtIOMEMCcw, VIRTIO_MEM_CCW, TYPE_VIRTIO_MEM_CCW)
+
+struct VirtIOMEMCcw {
+    VirtIOMDCcw parent_obj;
+    VirtIOMEM vdev;
+    Notifier size_change_notifier;
+};
+
+#endif /* QEMU_VIRTIO_MEM_PCI_H */
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index ef64bf1b4a..988101783f 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -61,6 +61,8 @@ static uint32_t virtio_mem_default_thp_size(void)
     } else if (qemu_real_host_page_size() == 64 * KiB) {
         default_thp_size = 512 * MiB;
     }
+#elif defined(__s390x__)
+    default_thp_size = 1 * MiB;
 #endif
 
     return default_thp_size;
@@ -161,7 +163,7 @@ static bool virtio_mem_has_shared_zeropage(RAMBlock *rb)
  * necessary (as the section size can change). But it's more likely that the
  * section size will rather get smaller and not bigger over time.
  */
-#if defined(TARGET_X86_64) || defined(TARGET_I386)
+#if defined(TARGET_X86_64) || defined(TARGET_I386) || defined(TARGET_S390X)
 #define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB))
 #elif defined(TARGET_ARM)
 #define VIRTIO_MEM_USABLE_EXTENT (2 * (512 * MiB))
-- 
2.46.0

Re: [PATCH v1 00/14] s390x: virtio-mem support

[Michael S. Tsirkin]

On Tue, Sep 10, 2024 at 07:57:55PM +0200, David Hildenbrand wrote:
> This series is based on:
>     [PATCH v2] virtio: kconfig: memory devices are PCI only [1]
> 
> I finally found the time (IOW forced myself) to finish virtio-mem
> support on s390x. The last RFC was from 2020, so I won't talk about
> what changed -- a lot changed in the meantime :)
> 
> There is really not much left to do on s390x, because virtio-mem already
> implements most things we need today (e.g., early-migration,
> unplugged-inaccessible). The biggest part of this series is just doing what
> we do with virtio-pci, wiring it up in the machine hotplug handler and ...
> well, messing with the physical memory layout where we can now exceed
> initial RAM size and have sparsity (memory holes).
> 
> I tested a lot of things, including:
>  * Memory hotplug/unplug
>  * Device hotplug/unplug
>  * System resets / reboots
>  * Migrate to/from file (including storage attributes under KVM)
>  * Basic live migration
>  * Basic postcopy live migration
> 
> More details on how to use it on s390x -- which is pretty much how
> we use it on other architectures, except
> s/virtio-mem-pci/virtio-mem-ccw/ --- is in the last patch.
> 
> This series introduces a new diag(500) "STORAGE LIMIT" subcode that will
> be documented at [2] once this+kernel part go upstream.
> 
> There are not many s390x-specific virtio-mem future work items, except:
> * Storage attribute migration might be improved
> * We might want to reset storage attributes of unplugged memory
>   (might or might not be required for upcoming page table reclaim in
>    Linux; TBD)


I don't see anything needing virtio specific here, let me know if
I missed anything.
A quick look is fine so I guess you can add
Acked-by: Michael S. Tsirkin <mst@redhat.com>



> I'll send out the kernel driver bits soon.
> 
> [1] https://lkml.kernel.org/r/20240906101658.514470-1-pbonzini@redhat.com
> [2] https://gitlab.com/davidhildenbrand/s390x-os-virt-spec
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
> Cc: Eric Farman <farman@linux.ibm.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Ilya Leoshkevich <iii@linux.ibm.com>
> Cc: Janosch Frank <frankja@linux.ibm.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> 
> David Hildenbrand (14):
>   s390x/s390-virtio-ccw: don't crash on weird RAM sizes
>   s390x/s390-virtio-hcall: remove hypercall registration mechanism
>   s390x/s390-virtio-hcall: prepare for more diag500 hypercalls
>   s390x: rename s390-virtio-hcall* to s390-hypercall*
>   s390x/s390-virtio-ccw: move setting the maximum guest size from sclp
>     to machine code
>   s390x: introduce s390_get_memory_limit()
>   s390x/s390-hypercall: introduce DIAG500 STORAGE_LIMIT
>   s390x/s390-stattrib-kvm: prepare memory devices and sparse memory
>     layouts
>   s390x/s390-skeys: prepare for memory devices
>   s390x/pv: check initial, not maximum RAM size
>   s390x/s390-virtio-ccw: prepare for memory devices
>   s390x: introduce s390_get_max_pagesize()
>   s390x/virtio-ccw: add support for virtio based memory devices
>   s390x: virtio-mem support
> 
>  MAINTAINERS                    |   4 +
>  hw/s390x/Kconfig               |   1 +
>  hw/s390x/meson.build           |   4 +-
>  hw/s390x/s390-hypercall.c      |  77 +++++++++++
>  hw/s390x/s390-hypercall.h      |  25 ++++
>  hw/s390x/s390-skeys.c          |   4 +-
>  hw/s390x/s390-stattrib-kvm.c   |  63 +++++----
>  hw/s390x/s390-virtio-ccw.c     | 143 +++++++++++++--------
>  hw/s390x/s390-virtio-hcall.c   |  41 ------
>  hw/s390x/s390-virtio-hcall.h   |  25 ----
>  hw/s390x/sclp.c                |  17 +--
>  hw/s390x/virtio-ccw-md.c       | 153 ++++++++++++++++++++++
>  hw/s390x/virtio-ccw-md.h       |  44 +++++++
>  hw/s390x/virtio-ccw-mem.c      | 226 +++++++++++++++++++++++++++++++++
>  hw/s390x/virtio-ccw-mem.h      |  34 +++++
>  hw/virtio/Kconfig              |   1 +
>  hw/virtio/virtio-mem.c         |   4 +-
>  target/s390x/cpu-sysemu.c      |  35 ++++-
>  target/s390x/cpu.h             |   2 +
>  target/s390x/kvm/kvm.c         |  12 +-
>  target/s390x/kvm/pv.c          |   2 +-
>  target/s390x/tcg/misc_helper.c |   6 +-
>  22 files changed, 746 insertions(+), 177 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-md.c
>  create mode 100644 hw/s390x/virtio-ccw-md.h
>  create mode 100644 hw/s390x/virtio-ccw-mem.c
>  create mode 100644 hw/s390x/virtio-ccw-mem.h
> 
> -- 
> 2.46.0

Re: [PATCH v1 00/14] s390x: virtio-mem support

[David Hildenbrand]

On 10.09.24 20:33, Michael S. Tsirkin wrote:
> On Tue, Sep 10, 2024 at 07:57:55PM +0200, David Hildenbrand wrote:
>> This series is based on:
>>      [PATCH v2] virtio: kconfig: memory devices are PCI only [1]
>>
>> I finally found the time (IOW forced myself) to finish virtio-mem
>> support on s390x. The last RFC was from 2020, so I won't talk about
>> what changed -- a lot changed in the meantime :)
>>
>> There is really not much left to do on s390x, because virtio-mem already
>> implements most things we need today (e.g., early-migration,
>> unplugged-inaccessible). The biggest part of this series is just doing what
>> we do with virtio-pci, wiring it up in the machine hotplug handler and ...
>> well, messing with the physical memory layout where we can now exceed
>> initial RAM size and have sparsity (memory holes).
>>
>> I tested a lot of things, including:
>>   * Memory hotplug/unplug
>>   * Device hotplug/unplug
>>   * System resets / reboots
>>   * Migrate to/from file (including storage attributes under KVM)
>>   * Basic live migration
>>   * Basic postcopy live migration
>>
>> More details on how to use it on s390x -- which is pretty much how
>> we use it on other architectures, except
>> s/virtio-mem-pci/virtio-mem-ccw/ --- is in the last patch.
>>
>> This series introduces a new diag(500) "STORAGE LIMIT" subcode that will
>> be documented at [2] once this+kernel part go upstream.
>>
>> There are not many s390x-specific virtio-mem future work items, except:
>> * Storage attribute migration might be improved
>> * We might want to reset storage attributes of unplugged memory
>>    (might or might not be required for upcoming page table reclaim in
>>     Linux; TBD)
> 
> 
> I don't see anything needing virtio specific here, let me know if
> I missed anything.

No, it's really just wiring up virtio-mem.

> A quick look is fine so I guess you can add
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 00/14] s390x: virtio-mem support

[Janosch Frank]

On 9/10/24 7:57 PM, David Hildenbrand wrote:
> This series introduces a new diag(500) "STORAGE LIMIT" subcode that will
> be documented at [2] once this+kernel part go upstream.

Why not in Documentation/virt/kvm/s390/?
s390-diag.rst is very similar already.

I'd rather have it in a shared and bigger repo than in your personal 
gitlab one. Maybe there's a space somewhere in QEMU or the Virtio team's 
repos that would be a good fit if the kernel's docu isn't the right place?

Re: [PATCH v1 00/14] s390x: virtio-mem support

[David Hildenbrand]

On 11.09.24 13:49, Janosch Frank wrote:
> On 9/10/24 7:57 PM, David Hildenbrand wrote:
>> This series introduces a new diag(500) "STORAGE LIMIT" subcode that will
>> be documented at [2] once this+kernel part go upstream.
> 
> Why not in Documentation/virt/kvm/s390/?
> s390-diag.rst is very similar already.

I can document it anywhere people fancy, really I don't care. But this 
has some history. The last time we discussed it [1] there was:

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

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

And then

"the kernel's s390-diag.rst should also point to it ... Nothing as 
complicated as an OASIS spec, but maybe a git??b project?" from Conny

> 
> I'd rather have it in a shared and bigger repo than in your personal
> gitlab one. Maybe there's a space somewhere in QEMU or the Virtio team's
> repos that would be a good fit if the kernel's docu isn't the right place?

At this point, outside of kernel/QEMU feels like the right thing to do. 
Conny is already a co-maintainer of my "personal" (;)) gitlab.


And now I realize that I CCed Heiko on the Linux series but not the QEMU 
series. My bad.

[1] https://lore.kernel.org/all/20200727114819.3f816010.cohuck@redhat.com/

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 00/14] s390x: virtio-mem support

[Michael S. Tsirkin]

> > 
> > I'd rather have it in a shared and bigger repo than in your personal
> > gitlab one. Maybe there's a space somewhere in QEMU or the Virtio team's
> > repos that would be a good fit if the kernel's docu isn't the right place?
> 
> At this point, outside of kernel/QEMU feels like the right thing to do.
> Conny is already a co-maintainer of my "personal" (;)) gitlab.
> 
> 
> And now I realize that I CCed Heiko on the Linux series but not the QEMU
> series. My bad.
> 
> [1] https://lore.kernel.org/all/20200727114819.3f816010.cohuck@redhat.com/


No prob. Or if you want it in virtio spec, that's also fine.

Re: [PATCH v1 00/14] s390x: virtio-mem support

[Cornelia Huck]

On Wed, Sep 11 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:

>> > 
>> > I'd rather have it in a shared and bigger repo than in your personal
>> > gitlab one. Maybe there's a space somewhere in QEMU or the Virtio team's
>> > repos that would be a good fit if the kernel's docu isn't the right place?
>> 
>> At this point, outside of kernel/QEMU feels like the right thing to do.
>> Conny is already a co-maintainer of my "personal" (;)) gitlab.
>> 
>> 
>> And now I realize that I CCed Heiko on the Linux series but not the QEMU
>> series. My bad.
>> 
>> [1] https://lore.kernel.org/all/20200727114819.3f816010.cohuck@redhat.com/
>
>
> No prob. Or if you want it in virtio spec, that's also fine.

The virtio spec makes sense for documenting things needed to implement
it, but what I liked about the gitlab project is that it tries to go
into more s390-specific aspects (that feel a bit out of scope for the
virtio spec), and that it provides a place to document non-virtio
related interfaces.

Anyway, if we want to proceed with the gitlab project, would it make
sense to create an org for it, so that it doesn't look like David's
personal project? In any case, while I'm happy to stay on, I'm not that
involved with s390 anymore, and it might make sense to add more people
to it.

Re: [PATCH v1 00/14] s390x: virtio-mem support

[David Hildenbrand]

On 11.09.24 17:38, Cornelia Huck wrote:
> On Wed, Sep 11 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>>>>
>>>> I'd rather have it in a shared and bigger repo than in your personal
>>>> gitlab one. Maybe there's a space somewhere in QEMU or the Virtio team's
>>>> repos that would be a good fit if the kernel's docu isn't the right place?
>>>
>>> At this point, outside of kernel/QEMU feels like the right thing to do.
>>> Conny is already a co-maintainer of my "personal" (;)) gitlab.
>>>
>>>
>>> And now I realize that I CCed Heiko on the Linux series but not the QEMU
>>> series. My bad.
>>>
>>> [1] https://lore.kernel.org/all/20200727114819.3f816010.cohuck@redhat.com/
>>
>>
>> No prob. Or if you want it in virtio spec, that's also fine.
> 
> The virtio spec makes sense for documenting things needed to implement
> it, but what I liked about the gitlab project is that it tries to go
> into more s390-specific aspects (that feel a bit out of scope for the
> virtio spec), and that it provides a place to document non-virtio
> related interfaces.
> 
> Anyway, if we want to proceed with the gitlab project, would it make
> sense to create an org for it, so that it doesn't look like David's
> personal project?

I had that in mind as well, I can move the project.

> In any case, while I'm happy to stay on, I'm not that
> involved with s390 anymore, and it might make sense to add more people
> to it.

Indeed!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 00/14] s390x: virtio-mem support

[Halil Pasic]

On Wed, 11 Sep 2024 21:09:27 +0200
David Hildenbrand <david@redhat.com> wrote:

> > Anyway, if we want to proceed with the gitlab project, would it make
> > sense to create an org for it, so that it doesn't look like David's
> > personal project?

Frankly, I would prefer making Documentation/virt/kvm/s390/s390-diag.rst
the authoritative documentation on DIAGs.

My train of thought is DIAG 500 is a KVM thing, and KVM is a linux
kernel thing, so it just feels right for the documentatio to
live within the linux source tree.

I may have missed some of the discussion: what were the benefits
of having this in its separate project/repository?

Regards,
Halil

Re: [PATCH v1 00/14] s390x: virtio-mem support

[David Hildenbrand]

On 27.09.24 20:20, Halil Pasic wrote:
> On Wed, 11 Sep 2024 21:09:27 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>>> Anyway, if we want to proceed with the gitlab project, would it make
>>> sense to create an org for it, so that it doesn't look like David's
>>> personal project?
> 
> Frankly, I would prefer making Documentation/virt/kvm/s390/s390-diag.rst
> the authoritative documentation on DIAGs.
> 
> My train of thought is DIAG 500 is a KVM thing, and KVM is a linux
> kernel thing, so it just feels right for the documentatio to
> live within the linux source tree.

QEMU/TCG is the proof that KVM is not necessarily involved.

Are you sure that no other OS out there besides Linux implements virtio 
on s390x, or would want to implement it? :)

> 
> I may have missed some of the discussion: what were the benefits
> of having this in its separate project/repository?

Having it independent of the implementation.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 00/14] s390x: virtio-mem support

[Halil Pasic]

On Fri, 27 Sep 2024 20:29:19 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 27.09.24 20:20, Halil Pasic wrote:
> > On Wed, 11 Sep 2024 21:09:27 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >>> Anyway, if we want to proceed with the gitlab project, would it make
> >>> sense to create an org for it, so that it doesn't look like David's
> >>> personal project?  
> > 
> > Frankly, I would prefer making Documentation/virt/kvm/s390/s390-diag.rst
> > the authoritative documentation on DIAGs.
> > 
> > My train of thought is DIAG 500 is a KVM thing, and KVM is a linux
> > kernel thing, so it just feels right for the documentatio to
> > live within the linux source tree.  
> 
> QEMU/TCG is the proof that KVM is not necessarily involved.
> 
> Are you sure that no other OS out there besides Linux implements virtio 
> on s390x, or would want to implement it? :)
>

As Christian has pointed out in another thread DIAG 500 is documented
as the KVM hypervisor call, and that made me argue it is a KVM thing.

You are right KVM is not necessarily involved, and neither is QEMU. For
me it is not about the components involved in the visualization, but
about the people, projects and governance.

IMHO this is basically extending the s390 architecture. We are guaranteed
to not collide with the Architecture because DIAG 500 is reserved for
KVM as a project I guess.

> > 
> > I may have missed some of the discussion: what were the benefits
> > of having this in its separate project/repository?  
> 
> Having it independent of the implementation.
> 

That is a valid point. But IMHO the benefit of having this independent,
does not justify the churn of having a separate project with its
own governance, and communication infrastructure. And I suppose for an
open(?) specification, one would need those things.

No strong opinions though. If Christian, Janosch and Claudio are in
favor of a separate "Specifications for open-source virtualization on
s390x (IBM z Systems)" project, I'm fine with it as well.

Regards,
Halil

Re: [PATCH v1 00/14] s390x: virtio-mem support

[David Hildenbrand]

On 30.09.24 23:49, Halil Pasic wrote:
> On Fri, 27 Sep 2024 20:29:19 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 27.09.24 20:20, Halil Pasic wrote:
>>> On Wed, 11 Sep 2024 21:09:27 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>    
>>>>> Anyway, if we want to proceed with the gitlab project, would it make
>>>>> sense to create an org for it, so that it doesn't look like David's
>>>>> personal project?
>>>
>>> Frankly, I would prefer making Documentation/virt/kvm/s390/s390-diag.rst
>>> the authoritative documentation on DIAGs.
>>>
>>> My train of thought is DIAG 500 is a KVM thing, and KVM is a linux
>>> kernel thing, so it just feels right for the documentatio to
>>> live within the linux source tree.
>>
>> QEMU/TCG is the proof that KVM is not necessarily involved.
>>
>> Are you sure that no other OS out there besides Linux implements virtio
>> on s390x, or would want to implement it? :)
>>
> 
> As Christian has pointed out in another thread DIAG 500 is documented
> as the KVM hypervisor call, and that made me argue it is a KVM thing.
> 
> You are right KVM is not necessarily involved, and neither is QEMU. For
> me it is not about the components involved in the visualization, but
> about the people, projects and governance.
> 
> IMHO this is basically extending the s390 architecture. We are guaranteed
> to not collide with the Architecture because DIAG 500 is reserved for
> KVM as a project I guess.

That's my understanding. I assume because the CCW virtio machine started 
out as KVM-only, documenting that it is "KVM ONLY" may be because of 
historical reasons.

> 
>>>
>>> I may have missed some of the discussion: what were the benefits
>>> of having this in its separate project/repository?
>>
>> Having it independent of the implementation.
>>
> 
> That is a valid point. But IMHO the benefit of having this independent,
> does not justify the churn of having a separate project with its
> own governance, and communication infrastructure. And I suppose for an
> open(?) specification, one would need those things.

I don't see the need to bring in all that bureaucracy. The original idea 
was simple: if QEMU/TCG or QEMU/KVM implement a hypercall (IOW: it was 
acked by the s390x maintainers), we document it somewhere.

Implementing something in QEMU and then modifying a KVM document in the 
kernel tree sounded odd.

It is a valid question to ask: what if any other hypervisor 
(cloud-hypervisor etc.) would want to implement a custom diag500 
hypercall, and who would ack it. But I don't really think that we would 
have to sort this out this at this point in time.

> 
> No strong opinions though. If Christian, Janosch and Claudio are in
> favor of a separate "Specifications for open-source virtualization on
> s390x (IBM z Systems)" project, I'm fine with it as well.

I'm more than happy if we don't need that. As said, I'm happy to 
document wherever people tell me to document.

4 years ago we thought that having a separate repository was a good 
idea, maybe it is no longer. In that case, s390x mainters please let me 
know what to do :)

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 00/14] s390x: virtio-mem support

[Janosch Frank]

On 10/1/24 10:54 AM, David Hildenbrand wrote:
> On 30.09.24 23:49, Halil Pasic wrote:
>> On Fri, 27 Sep 2024 20:29:19 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 27.09.24 20:20, Halil Pasic wrote:
>>>> On Wed, 11 Sep 2024 21:09:27 +0200
>>>> David Hildenbrand <david@redhat.com> wrote:

[...]

>> That is a valid point. But IMHO the benefit of having this independent,
>> does not justify the churn of having a separate project with its
>> own governance, and communication infrastructure. And I suppose for an
>> open(?) specification, one would need those things.
> 
> I don't see the need to bring in all that bureaucracy. The original idea
> was simple: if QEMU/TCG or QEMU/KVM implement a hypercall (IOW: it was
> acked by the s390x maintainers), we document it somewhere.
> 
> Implementing something in QEMU and then modifying a KVM document in the
> kernel tree sounded odd.
> 
> It is a valid question to ask: what if any other hypervisor
> (cloud-hypervisor etc.) would want to implement a custom diag500
> hypercall, and who would ack it. But I don't really think that we would
> have to sort this out this at this point in time.
> 
>>
>> No strong opinions though. If Christian, Janosch and Claudio are in
>> favor of a separate "Specifications for open-source virtualization on
>> s390x (IBM z Systems)" project, I'm fine with it as well.
> 
> I'm more than happy if we don't need that. As said, I'm happy to
> document wherever people tell me to document.
> 
> 4 years ago we thought that having a separate repository was a good
> idea, maybe it is no longer. In that case, s390x mainters please let me
> know what to do :)
> 

I'd like to at least have a partial documentation in the kernel if not a 
full one. You can add a link to your repo to that.

Even if they go out of sync I'd rather have documentation where I'd 
expect it (Linux) than only the repo. IMHO duplication isn't a gigantic 
issue here.

We also have an internal space where KVM architecture is being stored 
(currently a handful of documents) and we'll store it there as well, 
including a link to the repo.

Re: [PATCH v1 00/14] s390x: virtio-mem support

[David Hildenbrand]

On 02.10.24 11:04, Janosch Frank wrote:
> On 10/1/24 10:54 AM, David Hildenbrand wrote:
>> On 30.09.24 23:49, Halil Pasic wrote:
>>> On Fri, 27 Sep 2024 20:29:19 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> On 27.09.24 20:20, Halil Pasic wrote:
>>>>> On Wed, 11 Sep 2024 21:09:27 +0200
>>>>> David Hildenbrand <david@redhat.com> wrote:
> 
> [...]
> 
>>> That is a valid point. But IMHO the benefit of having this independent,
>>> does not justify the churn of having a separate project with its
>>> own governance, and communication infrastructure. And I suppose for an
>>> open(?) specification, one would need those things.
>>
>> I don't see the need to bring in all that bureaucracy. The original idea
>> was simple: if QEMU/TCG or QEMU/KVM implement a hypercall (IOW: it was
>> acked by the s390x maintainers), we document it somewhere.
>>
>> Implementing something in QEMU and then modifying a KVM document in the
>> kernel tree sounded odd.
>>
>> It is a valid question to ask: what if any other hypervisor
>> (cloud-hypervisor etc.) would want to implement a custom diag500
>> hypercall, and who would ack it. But I don't really think that we would
>> have to sort this out this at this point in time.
>>
>>>
>>> No strong opinions though. If Christian, Janosch and Claudio are in
>>> favor of a separate "Specifications for open-source virtualization on
>>> s390x (IBM z Systems)" project, I'm fine with it as well.
>>
>> I'm more than happy if we don't need that. As said, I'm happy to
>> document wherever people tell me to document.
>>
>> 4 years ago we thought that having a separate repository was a good
>> idea, maybe it is no longer. In that case, s390x mainters please let me
>> know what to do :)
>>
> 
> I'd like to at least have a partial documentation in the kernel if not a
> full one. You can add a link to your repo to that.

Makes sense.

> 
> Even if they go out of sync I'd rather have documentation where I'd
> expect it (Linux) than only the repo. IMHO duplication isn't a gigantic
> issue here.

Agreed.

> 
> We also have an internal space where KVM architecture is being stored
> (currently a handful of documents) and we'll store it there as well,
> including a link to the repo.

Perfect. If there is a way to enable that repository, that would be 
great. For the time being I'll document it there as well.

Thanks!

-- 
Cheers,

David / dhildenb