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

[PATCH 0/4] 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.

Janosch Frank (4):
  dump: Set dump info function pointers to NULL
  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                |  8 ++++++++
 include/sysemu/dump-arch.h |  1 +
 target/s390x/arch_dump.c   | 21 +++++++++++++++++----
 3 files changed, 26 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH 1/4] dump: Set dump info function pointers to NULL

[Janosch Frank]

Better to not rely on the struct zeroing since NULL is not necessarily
0.

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

diff --git a/dump/dump.c b/dump/dump.c
index d355ada62e..1d38274925 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1706,6 +1706,9 @@ static void dump_state_prepare(DumpState *s)
 {
     /* zero the struct, setting status to active */
     *s = (DumpState) { .status = DUMP_STATUS_ACTIVE };
+    s->dump_info.arch_sections_add_fn = NULL;
+    s->dump_info.arch_sections_write_hdr_fn = NULL;
+    s->dump_info.arch_sections_write_fn = NULL;
 }
 
 bool qemu_system_dump_in_progress(void)
-- 
2.34.1

Re: [PATCH 1/4] dump: Set dump info function pointers to NULL

[Marc-André Lureau]

Hi

On Tue, Nov 7, 2023 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> Better to not rely on the struct zeroing since NULL is not necessarily
> 0.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  dump/dump.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index d355ada62e..1d38274925 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1706,6 +1706,9 @@ static void dump_state_prepare(DumpState *s)
>  {
>      /* zero the struct, setting status to active */
>      *s = (DumpState) { .status = DUMP_STATUS_ACTIVE };
> +    s->dump_info.arch_sections_add_fn = NULL;
> +    s->dump_info.arch_sections_write_hdr_fn = NULL;
> +    s->dump_info.arch_sections_write_fn = NULL;
>  }

I think we would be in trouble if NULL is not 0. Do you have a better argument?



-- 
Marc-André Lureau

Re: [PATCH 1/4] dump: Set dump info function pointers to NULL

[Janosch Frank]

On 11/8/23 09:03, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Nov 7, 2023 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>> Better to not rely on the struct zeroing since NULL is not necessarily
>> 0.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   dump/dump.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index d355ada62e..1d38274925 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -1706,6 +1706,9 @@ static void dump_state_prepare(DumpState *s)
>>   {
>>       /* zero the struct, setting status to active */
>>       *s = (DumpState) { .status = DUMP_STATUS_ACTIVE };
>> +    s->dump_info.arch_sections_add_fn = NULL;
>> +    s->dump_info.arch_sections_write_hdr_fn = NULL;
>> +    s->dump_info.arch_sections_write_fn = NULL;
>>   }
> 
> I think we would be in trouble if NULL is not 0. Do you have a better argument?
> 

I'm one of those people who likes to distinguish between pointers and 
non-pointers but I have no problem dropping this.


OT: On s390 0x0 is a valid address but the kernel maps & handles it in a 
way that it will result in a null pointer if read/written to.

[PATCH 2/4] target/s390x/dump: Remove unneeded dump info function pointer init

[Janosch Frank]

dump_state_prepare() now sets the fucntion 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>
---
 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

Re: [PATCH 2/4] target/s390x/dump: Remove unneeded dump info function pointer init

[Claudio Imbrenda]

On Tue,  7 Nov 2023 14:20:46 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> dump_state_prepare() now sets the fucntion 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>

I would merge this and the previous patch

> ---
>  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;
>  }

Re: [PATCH 2/4] target/s390x/dump: Remove unneeded dump info function pointer init

[Marc-André Lureau]

Hi

On Tue, Nov 7, 2023 at 6:22 PM Janosch Frank <frankja@linux.ibm.com> wrote:
>
> dump_state_prepare() now sets the fucntion pointers to NULL so we only

function

> need to touch them if we're going to use them.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

regardless if the previous patch is applied, this patch lgtm

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
>
>


-- 
Marc-André Lureau

[PATCH 3/4] 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                | 5 +++++
 include/sysemu/dump-arch.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/dump/dump.c b/dump/dump.c
index 1d38274925..5c4e405ad2 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);
@@ -1709,6 +1713,7 @@ static void dump_state_prepare(DumpState *s)
     s->dump_info.arch_sections_add_fn = NULL;
     s->dump_info.arch_sections_write_hdr_fn = NULL;
     s->dump_info.arch_sections_write_fn = NULL;
+    s->dump_info.arch_cleanup_fn = NULL;
 }
 
 bool qemu_system_dump_in_progress(void)
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 4/4] 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..70146d7e84 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)
+{
+    uint8_t *buff = g_malloc(kvm_s390_pv_dmp_get_size_completion_data());
+    int rc;
+
+    if (!pv_dump_initialized || !buff) {
+        return;
+    }
+
+    rc = kvm_s390_dump_completion_data(buff);
+    if (!rc) {
+            pv_dump_initialized = false;
+    }
+    g_free(buff);
+}
+
 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 4/4] target/s390x/arch_dump: Add arch cleanup function for PV dumps

[Marc-André Lureau]

Hi

On Tue, Nov 7, 2023 at 6:22 PM 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>
> ---
>  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..70146d7e84 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)
> +{
> +    uint8_t *buff = g_malloc(kvm_s390_pv_dmp_get_size_completion_data());
> +    int rc;
> +
> +    if (!pv_dump_initialized || !buff) {

this may leak if bluff != NULL && !pv_dump_initialized

Better use g_autofree.

No need to check bluff != NULL. (g_malloc abort() if it failed)

> +        return;
> +    }
> +
> +    rc = kvm_s390_dump_completion_data(buff);
> +    if (!rc) {
> +            pv_dump_initialized = false;
> +    }
> +    g_free(buff);
> +}
> +
>  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
>
>

otherwise, seems ok

-- 
Marc-André Lureau