[PATCH v2 0/3] dump: Arch info function pointer addition and cleanup

[PATCH v2 0/3] dump: Arch info function pointer addition and cleanup

[Janosch Frank]

Small cleanups/fixes to the dump info function pointer assignments as
well as a new function pointer for cleanup of residual state.

This has come up because test managed to dump a s390 PV vm onto a disk
that was too small for the dump. After the dump failed, the vm wasn't
able to resume running since KVM was still in dump mode which blocks
vcpu entry.

The new function pointer allows cleanup of such a situation.

v2:
	- Usage of g_autofree
	- Dropped explicit NULLing of function pointers

Janosch Frank (3):
  target/s390x/dump: Remove unneeded dump info function pointer init
  dump: Add arch cleanup function
  target/s390x/arch_dump: Add arch cleanup function for PV dumps

 dump/dump.c                |  4 ++++
 include/sysemu/dump-arch.h |  1 +
 target/s390x/arch_dump.c   | 21 +++++++++++++++++----
 3 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] target/s390x/dump: Remove unneeded dump info function pointer init

[Janosch Frank]

dump_state_prepare() now sets the function pointers to NULL so we only
need to touch them if we're going to use them.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 target/s390x/arch_dump.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index 51a2116515..bdb0bfa0e7 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -448,10 +448,6 @@ int cpu_get_dump_info(ArchDumpInfo *info,
         info->arch_sections_add_fn = *arch_sections_add;
         info->arch_sections_write_hdr_fn = *arch_sections_write_hdr;
         info->arch_sections_write_fn = *arch_sections_write;
-    } else {
-        info->arch_sections_add_fn = NULL;
-        info->arch_sections_write_hdr_fn = NULL;
-        info->arch_sections_write_fn = NULL;
     }
     return 0;
 }
-- 
2.34.1

[PATCH v2 2/3] dump: Add arch cleanup function

[Janosch Frank]

Some architectures (s390x) need to cleanup after a failed dump to be
able to continue to run the vm. Add a cleanup function pointer and
call it if it's set.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 dump/dump.c                | 4 ++++
 include/sysemu/dump-arch.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/dump/dump.c b/dump/dump.c
index d355ada62e..9eeb7ab453 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -96,6 +96,10 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
 
 static int dump_cleanup(DumpState *s)
 {
+    if (s->dump_info.arch_cleanup_fn) {
+        s->dump_info.arch_cleanup_fn(s);
+    }
+
     guest_phys_blocks_free(&s->guest_phys_blocks);
     memory_mapping_list_free(&s->list);
     close(s->fd);
diff --git a/include/sysemu/dump-arch.h b/include/sysemu/dump-arch.h
index 59bbc9be38..743916e46c 100644
--- a/include/sysemu/dump-arch.h
+++ b/include/sysemu/dump-arch.h
@@ -24,6 +24,7 @@ typedef struct ArchDumpInfo {
     void (*arch_sections_add_fn)(DumpState *s);
     uint64_t (*arch_sections_write_hdr_fn)(DumpState *s, uint8_t *buff);
     int (*arch_sections_write_fn)(DumpState *s, uint8_t *buff);
+    void (*arch_cleanup_fn)(DumpState *s);
 } ArchDumpInfo;
 
 struct GuestPhysBlockList; /* memory_mapping.h */
-- 
2.34.1

[PATCH v2 3/3] target/s390x/arch_dump: Add arch cleanup function for PV dumps

[Janosch Frank]

PV dumps block vcpu runs until dump end is reached. If there's an
error between PV dump init and PV dump end the vm will never be able
to run again. One example of such an error is insufficient disk space
for the dump file.

Let's add a cleanup function that tries to do a dump end. The dump
completion data is discarded but there's no point in writing it to a
file anyway if there's a possibility that other PV dump data is
missing.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/arch_dump.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index bdb0bfa0e7..7e8a1b4fc0 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -433,6 +433,22 @@ static int arch_sections_write(DumpState *s, uint8_t *buff)
     return 0;
 }
 
+static void arch_cleanup(DumpState *s)
+{
+    g_autofree uint8_t *buff = NULL;
+    int rc;
+
+    if (!pv_dump_initialized) {
+        return;
+    }
+
+    buff = g_malloc(kvm_s390_pv_dmp_get_size_completion_data());
+    rc = kvm_s390_dump_completion_data(buff);
+    if (!rc) {
+            pv_dump_initialized = false;
+    }
+}
+
 int cpu_get_dump_info(ArchDumpInfo *info,
                       const struct GuestPhysBlockList *guest_phys_blocks)
 {
@@ -448,6 +464,7 @@ int cpu_get_dump_info(ArchDumpInfo *info,
         info->arch_sections_add_fn = *arch_sections_add;
         info->arch_sections_write_hdr_fn = *arch_sections_write_hdr;
         info->arch_sections_write_fn = *arch_sections_write;
+        info->arch_cleanup_fn = *arch_cleanup;
     }
     return 0;
 }
-- 
2.34.1

Re: [PATCH v2 3/3] target/s390x/arch_dump: Add arch cleanup function for PV dumps

[Claudio Imbrenda]

On Thu,  9 Nov 2023 12:04:43 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> PV dumps block vcpu runs until dump end is reached. If there's an
> error between PV dump init and PV dump end the vm will never be able
> to run again. One example of such an error is insufficient disk space
> for the dump file.
> 
> Let's add a cleanup function that tries to do a dump end. The dump
> completion data is discarded but there's no point in writing it to a
> file anyway if there's a possibility that other PV dump data is
> missing.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  target/s390x/arch_dump.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index bdb0bfa0e7..7e8a1b4fc0 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -433,6 +433,22 @@ static int arch_sections_write(DumpState *s, uint8_t *buff)
>      return 0;
>  }
>  
> +static void arch_cleanup(DumpState *s)
> +{
> +    g_autofree uint8_t *buff = NULL;
> +    int rc;
> +
> +    if (!pv_dump_initialized) {
> +        return;
> +    }
> +
> +    buff = g_malloc(kvm_s390_pv_dmp_get_size_completion_data());
> +    rc = kvm_s390_dump_completion_data(buff);
> +    if (!rc) {
> +            pv_dump_initialized = false;
> +    }
> +}
> +
>  int cpu_get_dump_info(ArchDumpInfo *info,
>                        const struct GuestPhysBlockList *guest_phys_blocks)
>  {
> @@ -448,6 +464,7 @@ int cpu_get_dump_info(ArchDumpInfo *info,
>          info->arch_sections_add_fn = *arch_sections_add;
>          info->arch_sections_write_hdr_fn = *arch_sections_write_hdr;
>          info->arch_sections_write_fn = *arch_sections_write;
> +        info->arch_cleanup_fn = *arch_cleanup;
>      }
>      return 0;
>  }

Re: [PATCH v2 1/3] target/s390x/dump: Remove unneeded dump info function pointer init

[Thomas Huth]

On 09/11/2023 13.04, Janosch Frank wrote:
> dump_state_prepare() now sets the function pointers to NULL so we only
> need to touch them if we're going to use them.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   target/s390x/arch_dump.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index 51a2116515..bdb0bfa0e7 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -448,10 +448,6 @@ int cpu_get_dump_info(ArchDumpInfo *info,
>           info->arch_sections_add_fn = *arch_sections_add;
>           info->arch_sections_write_hdr_fn = *arch_sections_write_hdr;
>           info->arch_sections_write_fn = *arch_sections_write;
> -    } else {
> -        info->arch_sections_add_fn = NULL;
> -        info->arch_sections_write_hdr_fn = NULL;
> -        info->arch_sections_write_fn = NULL;
>       }
>       return 0;
>   }

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

Re: [PATCH v2 2/3] dump: Add arch cleanup function

[Thomas Huth]

On 09/11/2023 13.04, Janosch Frank wrote:
> Some architectures (s390x) need to cleanup after a failed dump to be
> able to continue to run the vm. Add a cleanup function pointer and
> call it if it's set.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   dump/dump.c                | 4 ++++
>   include/sysemu/dump-arch.h | 1 +
>   2 files changed, 5 insertions(+)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index d355ada62e..9eeb7ab453 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -96,6 +96,10 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>   
>   static int dump_cleanup(DumpState *s)
>   {
> +    if (s->dump_info.arch_cleanup_fn) {
> +        s->dump_info.arch_cleanup_fn(s);
> +    }
> +
>       guest_phys_blocks_free(&s->guest_phys_blocks);
>       memory_mapping_list_free(&s->list);
>       close(s->fd);
> diff --git a/include/sysemu/dump-arch.h b/include/sysemu/dump-arch.h
> index 59bbc9be38..743916e46c 100644
> --- a/include/sysemu/dump-arch.h
> +++ b/include/sysemu/dump-arch.h
> @@ -24,6 +24,7 @@ typedef struct ArchDumpInfo {
>       void (*arch_sections_add_fn)(DumpState *s);
>       uint64_t (*arch_sections_write_hdr_fn)(DumpState *s, uint8_t *buff);
>       int (*arch_sections_write_fn)(DumpState *s, uint8_t *buff);
> +    void (*arch_cleanup_fn)(DumpState *s);
>   } ArchDumpInfo;
>   
>   struct GuestPhysBlockList; /* memory_mapping.h */

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

Re: [PATCH v2 3/3] target/s390x/arch_dump: Add arch cleanup function for PV dumps

[Thomas Huth]

On 09/11/2023 13.04, Janosch Frank wrote:
> PV dumps block vcpu runs until dump end is reached. If there's an
> error between PV dump init and PV dump end the vm will never be able
> to run again. One example of such an error is insufficient disk space
> for the dump file.
> 
> Let's add a cleanup function that tries to do a dump end. The dump
> completion data is discarded but there's no point in writing it to a
> file anyway if there's a possibility that other PV dump data is
> missing.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   target/s390x/arch_dump.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index bdb0bfa0e7..7e8a1b4fc0 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -433,6 +433,22 @@ static int arch_sections_write(DumpState *s, uint8_t *buff)
>       return 0;
>   }
>   
> +static void arch_cleanup(DumpState *s)
> +{
> +    g_autofree uint8_t *buff = NULL;
> +    int rc;
> +
> +    if (!pv_dump_initialized) {
> +        return;
> +    }

Maybe add a comment here why we still try the completion (e.g. something 
like the first paragraph of the commit description)

> +    buff = g_malloc(kvm_s390_pv_dmp_get_size_completion_data());
> +    rc = kvm_s390_dump_completion_data(buff);
> +    if (!rc) {
> +            pv_dump_initialized = false;
> +    }
> +}
> +
>   int cpu_get_dump_info(ArchDumpInfo *info,
>                         const struct GuestPhysBlockList *guest_phys_blocks)
>   {
> @@ -448,6 +464,7 @@ int cpu_get_dump_info(ArchDumpInfo *info,
>           info->arch_sections_add_fn = *arch_sections_add;
>           info->arch_sections_write_hdr_fn = *arch_sections_write_hdr;
>           info->arch_sections_write_fn = *arch_sections_write;
> +        info->arch_cleanup_fn = *arch_cleanup;
>       }
>       return 0;
>   }

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

Re: [PATCH v2 0/3] dump: Arch info function pointer addition and cleanup

[Marc-André Lureau]

Hi

On Thu, Nov 9, 2023 at 4:05 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> Small cleanups/fixes to the dump info function pointer assignments as
> well as a new function pointer for cleanup of residual state.
>
> This has come up because test managed to dump a s390 PV vm onto a disk
> that was too small for the dump. After the dump failed, the vm wasn't
> able to resume running since KVM was still in dump mode which blocks
> vcpu entry.
>
> The new function pointer allows cleanup of such a situation.
>
> v2:
>         - Usage of g_autofree
>         - Dropped explicit NULLing of function pointers
>
> Janosch Frank (3):
>   target/s390x/dump: Remove unneeded dump info function pointer init
>   dump: Add arch cleanup function
>   target/s390x/arch_dump: Add arch cleanup function for PV dumps
>
>  dump/dump.c                |  4 ++++
>  include/sysemu/dump-arch.h |  1 +
>  target/s390x/arch_dump.c   | 21 +++++++++++++++++----
>  3 files changed, 22 insertions(+), 4 deletions(-)
>

Series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



-- 
Marc-André Lureau