Re: [PATCH 5/5] s390x: pv: Add dump support

[Richard Henderson]

On 3/10/22 03:25, Janosch Frank wrote:
> Sometimes dumping a guest from the outside is the only way to get the
> data that is needed. This can be the case if a dumping mechanism like
> KDUMP hasn't been configured or data needs to be fetched at a specific
> point. Dumping a protected guest from the outside without help from
> fw/hw doesn't yield sufficient data to be useful. Hence we now
> introduce PV dump support.
>
> The PV dump support works by integrating the firmware into the dump
> process. New Ultravisor calls are used to initiate the dump process,
> dump cpu data, dump memory state and lastly complete the dump process.
> The UV calls are exposed by KVM via the new KVM_PV_DUMP command and
> its subcommands. The guest's data is fully encrypted and can only be
> decrypted by the entity that owns the customer communication key for
> the dumped guest. Also dumping needs to be allowed via a flag in the
> SE header.
>
> On the QEMU side of things we store the PV dump data in the newly
> introduced architecture ELF sections (storage state and completion
> data) and the cpu notes (for cpu dump data).
>
> Users can use a tool to convert the encrypted QEMU dump to an
> unencrypted one.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   dump/dump.c              |  27 -----
>   include/elf.h            |   1 +
>   include/sysemu/dump.h    |  27 +++++
>   target/s390x/arch_dump.c | 247 ++++++++++++++++++++++++++++++++++-----
>   4 files changed, 244 insertions(+), 58 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index a9d9ea514d..4b7d333866 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -708,33 +708,6 @@ static void dump_begin(DumpState *s, Error **errp)
>       write_elf_notes(s, errp);
>   }
>   
> -static int get_next_block(DumpState *s, GuestPhysBlock *block)
> -{
> -    while (1) {
> -        block = QTAILQ_NEXT(block, next);
> -        if (!block) {
> -            /* no more block */
> -            return 1;
> -        }
> -
> -        s->start = 0;
> -        s->next_block = block;
> -        if (s->has_filter) {
> -            if (block->target_start >= s->begin + s->length ||
> -                block->target_end <= s->begin) {
> -                /* This block is out of the range */
> -                continue;
> -            }
> -
> -            if (s->begin > block->target_start) {
> -                s->start = s->begin - block->target_start;
> -            }
> -        }
> -
> -        return 0;
> -    }
> -}

No reason to make this inline, surely.  Just export it.
Anyway, this should be it's own patch.

> @@ -76,9 +77,12 @@ typedef struct noteStruct {
>           uint64_t todcmp;
>           uint32_t todpreg;
>           uint64_t ctrs[16];
> +        uint8_t dynamic;  /* Real size comes from PV info interface */

This needs a better comment.  Possibly

    uint8_t dynamic[1];  /* Would be a flexible array member, if that was legal inside a 
union. Real size comes from PV info interface. */

Alternately, rewrite everything to work with

typedef struct noteStruct {
    Elf64_Nhdr hdr;
    char name[8];
    uint8_t contents[];
} QEMU_PACKED Note;

which is a better description of what's going on in this file anyway.


r~