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

[David Hildenbrand]

On 14.07.20 12:17, Claudio Imbrenda wrote:
> 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?

We only indicate boot memory (e.g., -m 2G), never any memory part of the
device memory address space, because such memory has different semantics.

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

No, that would be dangerous and wrong. Memory ranges part of memory
devices never must be indicated as part of hw/firmware interfaces to
indicate valid boot memory. Memory provided via memory devices
(virtio-mem, virtio-pmem, ...) has different semantics than ordinary
hotplugged memory, and unmodified OSs (esp., older Linux versions)
should not silently try to make use of any such memory. It's not just
some hotplugged memory a guest OS should detect+use during boot as
system ram. Thanks!

-- 
Thanks,

David / dhildenb