Re: [PATCH RFCv2 3/6] s390x/diag: implement diag260

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri, 10 Jul 2020 17:12:36 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's implement diag260 - "Access Certain Virtual Machine
> Information", used under z/VM to expose the storage configuration
> (especially, layout of storage extends and thereby holes). For now,
> the returned information is completely redundant to the information
> exposed via SCLP.
> 
> We want to reuse diag260 in QEMU to implement memory devices - to
> have a mechanism to indicate to the guest OS that the initial ram
> size and the maximum possible physical address differ.
> 
> The Linux kernel supports diag260 (0x10) to query the available memory
> since v4.20. Ancient Linux versions used diag 260 (0xc), but stopped
> doing so a while ago.
> 
> Let's unconditionally implement the new diag, without any migration
> checks (e.g., compatibility machine, CPU model). Although a guest OS
> could observe this when migrating between QEMU evrsions, it's somewhat
> unlikely to ever trigger due to the way diag260 is used within a guest
> OS - called only once or twice during boot.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/diag.c        | 51
> ++++++++++++++++++++++++++++++++++++++ target/s390x/internal.h    |
> 2 ++ target/s390x/kvm.c         | 11 ++++++++
>  target/s390x/misc_helper.c |  6 +++++
>  target/s390x/translate.c   |  7 ++++++
>  5 files changed, 77 insertions(+)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index be70aecd72..5378fcf582 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -23,6 +23,57 @@
>  #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];
> +
> +    switch (subcode) {
> +    case 0xc:
> +        /* The first storage extent maps to our initial ram. */
> +        env->regs[r1] = initial_ram_size - 1;
> +        /* The highest addressable byte maps to the initial ram size
> for now. */
> +        env->regs[r3] = initial_ram_size - 1;
> +        break;
> +    case 0x10: {
> +        ram_addr_t addr, length;
> +        uint64_t tmp;
> +
> +        if (r1 & 1) {
> +            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +            return;
> +        }
> +
> +        addr = env->regs[r1];
> +        length = env->regs[r1 + 1];
> +        if (!QEMU_IS_ALIGNED(addr, 16) || !QEMU_IS_ALIGNED(length,
> 16) ||
> +            !length) {
> +            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +            return;
> +        }
> +        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, it always fits into the
> area. */

maybe I missed something, but I have the impression that your
implementation of DIAG 260 always only returns the first extent?

shouldn't it return all the hotplugged areas once hotplugging is
enabled?

> +        env->regs[r3] = 1;
> +        setcc(env_archcpu(env), 0);
> +        break;
> +    }
> +    default:
> +        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +    }
> +}
> +
>  int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>  {
>      uint64_t func = env->regs[r1];
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index b1e0ebf67f..a7a3df9a3b 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -372,6 +372,8 @@ int mmu_translate_real(CPUS390XState *env,
> target_ulong raddr, int rw, 
>  
>  /* misc_helper.c */
> +void handle_diag_260(CPUS390XState *env, uint64_t r1, uint64_t r3,
> +                     uintptr_t ra);
>  int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3);
>  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3,
>                       uintptr_t ra);
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index f2f75d2a57..d6de3ad86c 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1565,6 +1565,14 @@ static int handle_hypercall(S390CPU *cpu,
> struct kvm_run *run) return ret;
>  }
>  
> +static void kvm_handle_diag_260(S390CPU *cpu, struct kvm_run *run)
> +{
> +    const uint64_t r1 = (run->s390_sieic.ipa & 0x00f0) >> 4;
> +    const uint64_t r3 = run->s390_sieic.ipa & 0x000f;
> +
> +    handle_diag_260(&cpu->env, r1, r3, 0);
> +}
> +
>  static void kvm_handle_diag_288(S390CPU *cpu, struct kvm_run *run)
>  {
>      uint64_t r1, r3;
> @@ -1614,6 +1622,9 @@ static int handle_diag(S390CPU *cpu, struct
> kvm_run *run, uint32_t ipb) */
>      func_code = decode_basedisp_rs(&cpu->env, ipb, NULL) &
> DIAG_KVM_CODE_MASK; switch (func_code) {
> +    case 0x260:
> +        kvm_handle_diag_260(cpu, run);
> +        break;
>      case DIAG_TIMEREVENT:
>          kvm_handle_diag_288(cpu, run);
>          break;
> 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();
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 4f6f1e31cd..e3358395f0 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -2397,6 +2397,13 @@ static DisasJumpType op_diag(DisasContext *s,
> DisasOps *o) TCGv_i32 r3 = tcg_const_i32(get_field(s, r3));
>      TCGv_i32 func_code = tcg_const_i32(get_field(s, i2));
>  
> +    /*
> +     * Diag 0x260 updates the CC - only for some subcodes. Calculate
> the
> +     * current cc, such that the helper can simply overwrite it
> conditionally.
> +     */
> +    if (get_field(s, i2) == 0x260) {
> +        gen_op_calc_cc(s);
> +    }
>      gen_helper_diag(cpu_env, r1, r3, func_code);
>  
>      tcg_temp_free_i32(func_code);