Re: [PATCH RFC 2/5] s390x: implement diag260

[Cornelia Huck]

On Wed,  8 Jul 2020 20:51:32 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's implement the "storage configuration" part of diag260. This diag
> is found under z/VM, to indicate usable chunks of memory tot he guest OS.
> As I don't have access to documentation, I have no clue what the actual
> error cases are, and which other stuff we could eventually query using this
> interface. Somebody with access to documentation should fix this. This
> implementation seems to work with Linux guests just fine.
> 
> The Linux kernel supports diag260 to query the available memory since
> v4.20. Older kernels / kvm-unit-tests will later fail to run in such a VM
> (with maxmem being defined and bigger than the memory size, e.g., "-m
>  2G,maxmem=4G"), just as if support for SCLP storage information is not
> implemented. They will fail to detect the actual initial memory size.
> 
> This interface allows us to expose the maximum ramsize via sclp
> and the initial ramsize via diag260 - without having to mess with the
> memory increment size and having to align the initial memory size to it.
> 
> This is a preparation for memory device support. We'll unlock the
> implementation with a new QEMU machine that supports memory devices.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/diag.c        | 57 ++++++++++++++++++++++++++++++++++++++
>  target/s390x/internal.h    |  2 ++
>  target/s390x/kvm.c         | 11 ++++++++
>  target/s390x/misc_helper.c |  6 ++++
>  target/s390x/translate.c   |  4 +++
>  5 files changed, 80 insertions(+)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 1a48429564..c3b1e24b2c 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -23,6 +23,63 @@
>  #include "hw/s390x/pv.h"
>  #include "kvm_s390x.h"
>  
> +void handle_diag_260(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t 
> ra)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    const ram_addr_t initial_ram_size = ms->ram_size;
> +    const uint64_t subcode = env->regs[r3];
> +    S390CPU *cpu = env_archcpu(env);
> +    ram_addr_t addr, length;
> +    uint64_t tmp;
> +
> +    /* TODO: Unlock with new QEMU machine. */
> +    if (false) {
> +        s390_program_interrupt(env, PGM_OPERATION, ra);
> +        return;
> +    }
> +
> +    /*
> +     * There also seems to be subcode "0xc", which stores the size of the
> +     * first chunk and the total size to r1/r2. It's only used by very old
> +     * Linux, so don't implement it.

FWIW,
https://www-01.ibm.com/servers/resourcelink/svc0302a.nsf/pages/zVMV7R1sc246272/$file/hcpb4_v7r1.pdf
seems to list the available subcodes. Anything but 0xc and 0x10 is for
24/31 bit only, so we can safely ignore them. Not sure what we want to
do with 0xc: it is supposed to "Return the highest addressable byte of
virtual storage in the host-primary address space, including named
saved systems and saved segments", so returning the end of the address
space should be easy enough, but not very useful.

> +     */
> +    if ((r1 & 1) || subcode != 0x10) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        return;
> +    }
> +    addr = env->regs[r1];
> +    length = env->regs[r1 + 1];
> +
> +    /* FIXME: Somebody with documentation should fix this. */

Doc mentioned above says for specification exception:

"For subcode X'10':
• Rx is not an even-numbered register.
• The address contained in Rx is not on a quadword boundary.
• The length contained in Rx+1 is not a positive multiple of 16."

> +    if (!QEMU_IS_ALIGNED(addr, 16) || !QEMU_IS_ALIGNED(length, 16)) {
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +        return;
> +    }
> +
> +    /* FIXME: Somebody with documentation should fix this. */
> +    if (!length) {

Probably specification exception as well?

> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    /* FIXME: Somebody with documentation should fix this. */

For access exception:

"For subcode X'10', an error occurred trying to store the extent
information into the guest's output area."

> +    if (!address_space_access_valid(&address_space_memory, addr, length, 
> true,
> +                                    MEMTXATTRS_UNSPECIFIED)) {
> +        s390_program_interrupt(env, PGM_ADDRESSING, ra);
> +        return;
> +    }
> +
> +    /* Indicate our initial memory ([0 .. ram_size - 1]) */
> +    tmp = cpu_to_be64(0);
> +    cpu_physical_memory_write(addr, &tmp, sizeof(tmp));
> +    tmp = cpu_to_be64(initial_ram_size - 1);
> +    cpu_physical_memory_write(addr + sizeof(tmp), &tmp, sizeof(tmp));
> +
> +    /* Exactly one entry was stored. */
> +    env->regs[r3] = 1;
> +    setcc(cpu, 0);
> +}
> +
>  int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>  {
>      uint64_t func = env->regs[r1];

(...)

> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 58dbc023eb..d7274eb320 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -116,6 +116,12 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, 
> uint32_t r3, uint32_t num)
>      uint64_t r;
>  
>      switch (num) {
> +    case 0x260:
> +        qemu_mutex_lock_iothread();
> +        handle_diag_260(env, r1, r3, GETPC());
> +        qemu_mutex_unlock_iothread();
> +        r = 0;
> +        break;
>      case 0x500:
>          /* KVM hypercall */
>          qemu_mutex_lock_iothread();

Looking at the doc referenced above, it seems that we treat every diag
call as privileged under tcg; but it seems that 0x44 isn't? (Unrelated
to your patch; maybe I'm misreading.)

> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 4f6f1e31cd..6bb8b6e513 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -2398,6 +2398,10 @@ static DisasJumpType op_diag(DisasContext *s, DisasOps 
> *o)
>      TCGv_i32 func_code = tcg_const_i32(get_field(s, i2));
>  
>      gen_helper_diag(cpu_env, r1, r3, func_code);
> +    /* Only some diags modify the CC. */
> +    if (get_field(s, i2) == 0x260) {
> +        set_cc_static(s);
> +    }
>  
>      tcg_temp_free_i32(func_code);
>      tcg_temp_free_i32(r3);