Re: [PATCH v1 10/12] hw/s390x/s390-skeys: use memory mapping to detect which storage keys to dump

[Thomas Huth <thuth@redhat.com>]

On 05/08/2021 17.28, David Hildenbrand wrote:
> Handle it similar to migration. Assert that we're holding the BQL, to
> make sure we don't see concurrent modifications.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/s390x/s390-skeys.c | 50 ++++++++++++++++++++++++++-----------------
>   1 file changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 250685a95a..56a47fe180 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -110,11 +110,10 @@ void qmp_dump_skeys(const char *filename, Error **errp)
>   {
>       S390SKeysState *ss = s390_get_skeys_device();
>       S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
> -    MachineState *ms = MACHINE(qdev_get_machine());
> -    const uint64_t total_count = ms->ram_size / TARGET_PAGE_SIZE;
> -    uint64_t handled_count = 0, cur_count;
> +    GuestPhysBlockList guest_phys_blocks;
> +    GuestPhysBlock *block;
> +    uint64_t pages, gfn;
>       Error *lerr = NULL;
> -    vaddr cur_gfn = 0;
>       uint8_t *buf;
>       int ret;
>       int fd;
> @@ -145,28 +144,39 @@ void qmp_dump_skeys(const char *filename, Error **errp)
>           goto out;
>       }
>   
> -    /* we'll only dump initial memory for now */
> -    while (handled_count < total_count) {
> -        /* Calculate how many keys to ask for & handle overflow case */
> -        cur_count = MIN(total_count - handled_count, S390_SKEYS_BUFFER_SIZE);
> +    assert(qemu_mutex_iothread_locked());
> +    guest_phys_blocks_init(&guest_phys_blocks);
> +    guest_phys_blocks_append(&guest_phys_blocks);
>   
> -        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
> -        if (ret < 0) {
> -            error_setg(errp, "get_keys error %d", ret);
> -            goto out_free;
> -        }
> +    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));
>   
> -        /* write keys to stream */
> -        write_keys(f, buf, cur_gfn, cur_count, &lerr);
> -        if (lerr) {
> -            goto out_free;
> -        }
> +        gfn = block->target_start / TARGET_PAGE_SIZE;
> +        pages = (block->target_end - block->target_start) / TARGET_PAGE_SIZE;
>   
> -        cur_gfn += cur_count;
> -        handled_count += cur_count;
> +        while (pages) {
> +            const uint64_t cur_pages = MIN(pages, S390_SKEYS_BUFFER_SIZE);
> +
> +            ret = skeyclass->get_skeys(ss, gfn, cur_pages, buf);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "get_keys error");
> +                goto out_free;
> +            }
> +
> +            /* write keys to stream */
> +            write_keys(f, buf, gfn, cur_pages, &lerr);
> +            if (lerr) {
> +                goto out_free;
> +            }
> +
> +            gfn += cur_pages;
> +            pages -= cur_pages;
> +        }
>       }
>   
>   out_free:
> +    guest_phys_blocks_free(&guest_phys_blocks);
>       error_propagate(errp, lerr);
>       g_free(buf);
>   out:
> 

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