[PATCH 0/2] target/i386/sev: Replace malloc with local variables

[PATCH 0/2] target/i386/sev: Replace malloc with local variables

[Dov Murik]

In two places in sev.c we use malloc+free to manage memory for small
constant struct variables.  Modify this to use local variables.

This small series can be applied on top of master or on top of Phil's
Housekeeping SEV series [1].

[1] https://lore.kernel.org/qemu-devel/20211007161716.453984-1-philmd@redhat.com/

Dov Murik (2):
  target/i386/sev: Use local variable for kvm_sev_launch_start
  target/i386/sev: Use local variable for kvm_sev_launch_measure

 target/i386/sev.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

-- 
2.25.1

[PATCH 1/2] target/i386/sev: Use local variable for kvm_sev_launch_start

[Dov Murik]

The struct kvm_sev_launch_start has a constant and small size, and
therefore we can use a regular local variable for it instead of
allocating and freeing heap memory for it.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 target/i386/sev.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 4c64c68244..0062566c71 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -647,31 +647,29 @@ sev_launch_start(SevGuestState *sev)
     gsize sz;
     int ret = 1;
     int fw_error, rc;
-    struct kvm_sev_launch_start *start;
+    struct kvm_sev_launch_start start = {
+        .handle = sev->handle, .policy = sev->policy
+    };
     guchar *session = NULL, *dh_cert = NULL;
 
-    start = g_new0(struct kvm_sev_launch_start, 1);
-
-    start->handle = sev->handle;
-    start->policy = sev->policy;
     if (sev->session_file) {
         if (sev_read_file_base64(sev->session_file, &session, &sz) < 0) {
             goto out;
         }
-        start->session_uaddr = (unsigned long)session;
-        start->session_len = sz;
+        start.session_uaddr = (unsigned long)session;
+        start.session_len = sz;
     }
 
     if (sev->dh_cert_file) {
         if (sev_read_file_base64(sev->dh_cert_file, &dh_cert, &sz) < 0) {
             goto out;
         }
-        start->dh_uaddr = (unsigned long)dh_cert;
-        start->dh_len = sz;
+        start.dh_uaddr = (unsigned long)dh_cert;
+        start.dh_len = sz;
     }
 
-    trace_kvm_sev_launch_start(start->policy, session, dh_cert);
-    rc = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_START, start, &fw_error);
+    trace_kvm_sev_launch_start(start.policy, session, dh_cert);
+    rc = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_START, &start, &fw_error);
     if (rc < 0) {
         error_report("%s: LAUNCH_START ret=%d fw_error=%d '%s'",
                 __func__, ret, fw_error, fw_error_to_str(fw_error));
@@ -679,11 +677,10 @@ sev_launch_start(SevGuestState *sev)
     }
 
     sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
-    sev->handle = start->handle;
+    sev->handle = start.handle;
     ret = 0;
 
 out:
-    g_free(start);
     g_free(session);
     g_free(dh_cert);
     return ret;
-- 
2.25.1

Re: [PATCH 1/2] target/i386/sev: Use local variable for kvm_sev_launch_start

[Dr. David Alan Gilbert]

* Dov Murik (dovmurik@linux.ibm.com) wrote:
> The struct kvm_sev_launch_start has a constant and small size, and
> therefore we can use a regular local variable for it instead of
> allocating and freeing heap memory for it.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/sev.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 4c64c68244..0062566c71 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -647,31 +647,29 @@ sev_launch_start(SevGuestState *sev)
>      gsize sz;
>      int ret = 1;
>      int fw_error, rc;
> -    struct kvm_sev_launch_start *start;
> +    struct kvm_sev_launch_start start = {
> +        .handle = sev->handle, .policy = sev->policy
> +    };
>      guchar *session = NULL, *dh_cert = NULL;
>  
> -    start = g_new0(struct kvm_sev_launch_start, 1);
> -
> -    start->handle = sev->handle;
> -    start->policy = sev->policy;
>      if (sev->session_file) {
>          if (sev_read_file_base64(sev->session_file, &session, &sz) < 0) {
>              goto out;
>          }
> -        start->session_uaddr = (unsigned long)session;
> -        start->session_len = sz;
> +        start.session_uaddr = (unsigned long)session;
> +        start.session_len = sz;
>      }
>  
>      if (sev->dh_cert_file) {
>          if (sev_read_file_base64(sev->dh_cert_file, &dh_cert, &sz) < 0) {
>              goto out;
>          }
> -        start->dh_uaddr = (unsigned long)dh_cert;
> -        start->dh_len = sz;
> +        start.dh_uaddr = (unsigned long)dh_cert;
> +        start.dh_len = sz;
>      }
>  
> -    trace_kvm_sev_launch_start(start->policy, session, dh_cert);
> -    rc = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_START, start, &fw_error);
> +    trace_kvm_sev_launch_start(start.policy, session, dh_cert);
> +    rc = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_START, &start, &fw_error);
>      if (rc < 0) {
>          error_report("%s: LAUNCH_START ret=%d fw_error=%d '%s'",
>                  __func__, ret, fw_error, fw_error_to_str(fw_error));
> @@ -679,11 +677,10 @@ sev_launch_start(SevGuestState *sev)
>      }
>  
>      sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE);
> -    sev->handle = start->handle;
> +    sev->handle = start.handle;
>      ret = 0;
>  
>  out:
> -    g_free(start);
>      g_free(session);
>      g_free(dh_cert);
>      return ret;
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 2/2] target/i386/sev: Use local variable for kvm_sev_launch_measure

[Dov Murik]

The struct kvm_sev_launch_measure has a constant and small size, and
therefore we can use a regular local variable for it instead of
allocating and freeing heap memory for it.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 target/i386/sev.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0062566c71..eede07f11d 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -729,7 +729,7 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
     SevGuestState *sev = sev_guest;
     int ret, error;
     g_autofree guchar *data = NULL;
-    g_autofree struct kvm_sev_launch_measure *measurement = NULL;
+    struct kvm_sev_launch_measure measurement = {};
 
     if (!sev_check_state(sev, SEV_STATE_LAUNCH_UPDATE)) {
         return;
@@ -743,23 +743,21 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
         }
     }
 
-    measurement = g_new0(struct kvm_sev_launch_measure, 1);
-
     /* query the measurement blob length */
     ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
-                    measurement, &error);
-    if (!measurement->len) {
+                    &measurement, &error);
+    if (!measurement.len) {
         error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
                      __func__, ret, error, fw_error_to_str(errno));
         return;
     }
 
-    data = g_new0(guchar, measurement->len);
-    measurement->uaddr = (unsigned long)data;
+    data = g_new0(guchar, measurement.len);
+    measurement.uaddr = (unsigned long)data;
 
     /* get the measurement blob */
     ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
-                    measurement, &error);
+                    &measurement, &error);
     if (ret) {
         error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
                      __func__, ret, error, fw_error_to_str(errno));
@@ -769,7 +767,7 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
     sev_set_guest_state(sev, SEV_STATE_LAUNCH_SECRET);
 
     /* encode the measurement value and emit the event */
-    sev->measurement = g_base64_encode(data, measurement->len);
+    sev->measurement = g_base64_encode(data, measurement.len);
     trace_kvm_sev_launch_measurement(sev->measurement);
 }
 
-- 
2.25.1

Re: [PATCH 2/2] target/i386/sev: Use local variable for kvm_sev_launch_measure

[Dr. David Alan Gilbert]

* Dov Murik (dovmurik@linux.ibm.com) wrote:
> The struct kvm_sev_launch_measure has a constant and small size, and
> therefore we can use a regular local variable for it instead of
> allocating and freeing heap memory for it.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/sev.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 0062566c71..eede07f11d 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -729,7 +729,7 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
>      SevGuestState *sev = sev_guest;
>      int ret, error;
>      g_autofree guchar *data = NULL;
> -    g_autofree struct kvm_sev_launch_measure *measurement = NULL;
> +    struct kvm_sev_launch_measure measurement = {};
>  
>      if (!sev_check_state(sev, SEV_STATE_LAUNCH_UPDATE)) {
>          return;
> @@ -743,23 +743,21 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
>          }
>      }
>  
> -    measurement = g_new0(struct kvm_sev_launch_measure, 1);
> -
>      /* query the measurement blob length */
>      ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
> -                    measurement, &error);
> -    if (!measurement->len) {
> +                    &measurement, &error);
> +    if (!measurement.len) {
>          error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
>                       __func__, ret, error, fw_error_to_str(errno));
>          return;
>      }
>  
> -    data = g_new0(guchar, measurement->len);
> -    measurement->uaddr = (unsigned long)data;
> +    data = g_new0(guchar, measurement.len);
> +    measurement.uaddr = (unsigned long)data;
>  
>      /* get the measurement blob */
>      ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_MEASURE,
> -                    measurement, &error);
> +                    &measurement, &error);
>      if (ret) {
>          error_report("%s: LAUNCH_MEASURE ret=%d fw_error=%d '%s'",
>                       __func__, ret, error, fw_error_to_str(errno));
> @@ -769,7 +767,7 @@ sev_launch_get_measure(Notifier *notifier, void *unused)
>      sev_set_guest_state(sev, SEV_STATE_LAUNCH_SECRET);
>  
>      /* encode the measurement value and emit the event */
> -    sev->measurement = g_base64_encode(data, measurement->len);
> +    sev->measurement = g_base64_encode(data, measurement.len);
>      trace_kvm_sev_launch_measurement(sev->measurement);
>  }
>  
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/2] target/i386/sev: Replace malloc with local variables

[Paolo Bonzini]

On 11/10/21 19:30, Dov Murik wrote:
> In two places in sev.c we use malloc+free to manage memory for small
> constant struct variables.  Modify this to use local variables.
> 
> This small series can be applied on top of master or on top of Phil's
> Housekeeping SEV series [1].
> 
> [1] https://lore.kernel.org/qemu-devel/20211007161716.453984-1-philmd@redhat.com/
> 
> Dov Murik (2):
>    target/i386/sev: Use local variable for kvm_sev_launch_start
>    target/i386/sev: Use local variable for kvm_sev_launch_measure
> 
>   target/i386/sev.c | 39 +++++++++++++++++----------------------
>   1 file changed, 17 insertions(+), 22 deletions(-)
> 

Queued, thanks.

Paolo