[PATCH 0/5] target/i386: SEV: fix compiler warnings on 32-bit hosts

[PATCH 0/5] target/i386: SEV: fix compiler warnings on 32-bit hosts

[Paolo Bonzini]

While SEV in practice is unlikely to be used when compiling for 32-bit
environments (it is not even supported by 32-bit kernels), it is easier
and/or nicer to clean up the warts that block compilation, than to add
conditionals in the build system to limit it to 64-bit hosts.

Paolo

Paolo Bonzini (5):
  target/i386: SEV: rename sev_snp_guest->id_block
  target/i386: SEV: store pointer to decoded id_block in SevSnpGuest
  target/i386: SEV: rename sev_snp_guest->id_auth
  target/i386: SEV: store pointer to decoded id_auth in SevSnpGuest
  target/i386: SEV: fix format strings for 32-bit hosts

 target/i386/sev.c | 52 +++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

-- 
2.45.2

[PATCH 1/5] target/i386: SEV: rename sev_snp_guest->id_block

[Paolo Bonzini]

Free the "id_block" name for the binary version of the data.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/sev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 30b83f1d77d..6daa8c264cd 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -152,7 +152,7 @@ struct SevSnpGuestState {
 
     /* configuration parameters */
     char *guest_visible_workarounds;
-    char *id_block;
+    char *id_block_base64;
     char *id_auth;
     char *host_data;
 
@@ -1296,7 +1296,7 @@ sev_snp_launch_finish(SevCommonState *sev_common)
         }
     }
 
-    trace_kvm_sev_snp_launch_finish(sev_snp->id_block, sev_snp->id_auth,
+    trace_kvm_sev_snp_launch_finish(sev_snp->id_block_base64, sev_snp->id_auth,
                                     sev_snp->host_data);
     ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_SNP_LAUNCH_FINISH,
                     finish, &error);
@@ -2159,7 +2159,7 @@ sev_snp_guest_get_id_block(Object *obj, Error **errp)
 {
     SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(obj);
 
-    return g_strdup(sev_snp_guest->id_block);
+    return g_strdup(sev_snp_guest->id_block_base64);
 }
 
 static void
@@ -2170,14 +2170,14 @@ sev_snp_guest_set_id_block(Object *obj, const char *value, Error **errp)
     gsize len;
 
     finish->id_block_en = 0;
-    g_free(sev_snp_guest->id_block);
+    g_free(sev_snp_guest->id_block_base64);
     g_free((guchar *)finish->id_block_uaddr);
 
     /* store the base64 str so we don't need to re-encode in getter */
-    sev_snp_guest->id_block = g_strdup(value);
+    sev_snp_guest->id_block_base64 = g_strdup(value);
 
     finish->id_block_uaddr =
-        (uint64_t)qbase64_decode(sev_snp_guest->id_block, -1, &len, errp);
+        (uint64_t)qbase64_decode(sev_snp_guest->id_block_base64, -1, &len, errp);
 
     if (!finish->id_block_uaddr) {
         return;
-- 
2.45.2

Re: [PATCH 1/5] target/i386: SEV: rename sev_snp_guest->id_block

[Richard Henderson]

On 6/27/24 07:53, Paolo Bonzini wrote:
> Free the "id_block" name for the binary version of the data.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/sev.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/5] target/i386: SEV: store pointer to decoded id_block in SevSnpGuest

[Paolo Bonzini]

Do not rely on finish->id_block_uaddr, so that there are no casts from
pointer to uint64_t.  They break on 32-bit hosts.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/sev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 6daa8c264cd..2d4cfd41e83 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -153,6 +153,7 @@ struct SevSnpGuestState {
     /* configuration parameters */
     char *guest_visible_workarounds;
     char *id_block_base64;
+    uint8_t *id_block;
     char *id_auth;
     char *host_data;
 
@@ -2170,16 +2171,15 @@ sev_snp_guest_set_id_block(Object *obj, const char *value, Error **errp)
     gsize len;
 
     finish->id_block_en = 0;
+    g_free(sev_snp_guest->id_block);
     g_free(sev_snp_guest->id_block_base64);
-    g_free((guchar *)finish->id_block_uaddr);
 
     /* store the base64 str so we don't need to re-encode in getter */
     sev_snp_guest->id_block_base64 = g_strdup(value);
+    sev_snp_guest->id_block =
+        qbase64_decode(sev_snp_guest->id_block_base64, -1, &len, errp);
 
-    finish->id_block_uaddr =
-        (uint64_t)qbase64_decode(sev_snp_guest->id_block_base64, -1, &len, errp);
-
-    if (!finish->id_block_uaddr) {
+    if (!sev_snp_guest->id_block) {
         return;
     }
 
@@ -2190,6 +2190,7 @@ sev_snp_guest_set_id_block(Object *obj, const char *value, Error **errp)
     }
 
     finish->id_block_en = 1;
+    finish->id_block_uaddr = (uintptr_t)sev_snp_guest->id_block;
 }
 
 static char *
-- 
2.45.2

Re: [PATCH 2/5] target/i386: SEV: store pointer to decoded id_block in SevSnpGuest

[Richard Henderson]

On 6/27/24 07:53, Paolo Bonzini wrote:
> Do not rely on finish->id_block_uaddr, so that there are no casts from
> pointer to uint64_t.  They break on 32-bit hosts.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/sev.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)

Yes, that is better than my patch 1.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 6daa8c264cd..2d4cfd41e83 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -153,6 +153,7 @@ struct SevSnpGuestState {
>       /* configuration parameters */
>       char *guest_visible_workarounds;
>       char *id_block_base64;
> +    uint8_t *id_block;
>       char *id_auth;
>       char *host_data;
>   
> @@ -2170,16 +2171,15 @@ sev_snp_guest_set_id_block(Object *obj, const char *value, Error **errp)
>       gsize len;
>   
>       finish->id_block_en = 0;
> +    g_free(sev_snp_guest->id_block);
>       g_free(sev_snp_guest->id_block_base64);
> -    g_free((guchar *)finish->id_block_uaddr);
>   
>       /* store the base64 str so we don't need to re-encode in getter */
>       sev_snp_guest->id_block_base64 = g_strdup(value);
> +    sev_snp_guest->id_block =
> +        qbase64_decode(sev_snp_guest->id_block_base64, -1, &len, errp);
>   
> -    finish->id_block_uaddr =
> -        (uint64_t)qbase64_decode(sev_snp_guest->id_block_base64, -1, &len, errp);
> -
> -    if (!finish->id_block_uaddr) {
> +    if (!sev_snp_guest->id_block) {
>           return;
>       }
>   
> @@ -2190,6 +2190,7 @@ sev_snp_guest_set_id_block(Object *obj, const char *value, Error **errp)
>       }
>   
>       finish->id_block_en = 1;
> +    finish->id_block_uaddr = (uintptr_t)sev_snp_guest->id_block;
>   }
>   
>   static char *

[PATCH 3/5] target/i386: SEV: rename sev_snp_guest->id_auth

[Paolo Bonzini]

Free the "id_auth" name for the binary version of the data.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/sev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 2d4cfd41e83..a6b063b762c 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -154,7 +154,7 @@ struct SevSnpGuestState {
     char *guest_visible_workarounds;
     char *id_block_base64;
     uint8_t *id_block;
-    char *id_auth;
+    char *id_auth_base64;
     char *host_data;
 
     struct kvm_sev_snp_launch_start kvm_start_conf;
@@ -1297,7 +1297,7 @@ sev_snp_launch_finish(SevCommonState *sev_common)
         }
     }
 
-    trace_kvm_sev_snp_launch_finish(sev_snp->id_block_base64, sev_snp->id_auth,
+    trace_kvm_sev_snp_launch_finish(sev_snp->id_block_base64, sev_snp->id_auth_base64,
                                     sev_snp->host_data);
     ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_SNP_LAUNCH_FINISH,
                     finish, &error);
@@ -2198,7 +2198,7 @@ sev_snp_guest_get_id_auth(Object *obj, Error **errp)
 {
     SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(obj);
 
-    return g_strdup(sev_snp_guest->id_auth);
+    return g_strdup(sev_snp_guest->id_auth_base64);
 }
 
 static void
@@ -2208,14 +2208,14 @@ sev_snp_guest_set_id_auth(Object *obj, const char *value, Error **errp)
     struct kvm_sev_snp_launch_finish *finish = &sev_snp_guest->kvm_finish_conf;
     gsize len;
 
-    g_free(sev_snp_guest->id_auth);
+    g_free(sev_snp_guest->id_auth_base64);
     g_free((guchar *)finish->id_auth_uaddr);
 
     /* store the base64 str so we don't need to re-encode in getter */
-    sev_snp_guest->id_auth = g_strdup(value);
+    sev_snp_guest->id_auth_base64 = g_strdup(value);
 
     finish->id_auth_uaddr =
-        (uint64_t)qbase64_decode(sev_snp_guest->id_auth, -1, &len, errp);
+        (uint64_t)qbase64_decode(sev_snp_guest->id_auth_base64, -1, &len, errp);
 
     if (!finish->id_auth_uaddr) {
         return;
-- 
2.45.2

Re: [PATCH 3/5] target/i386: SEV: rename sev_snp_guest->id_auth

[Richard Henderson]

On 6/27/24 07:53, Paolo Bonzini wrote:
> Free the "id_auth" name for the binary version of the data.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/sev.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/5] target/i386: SEV: store pointer to decoded id_auth in SevSnpGuest

[Paolo Bonzini]

Do not rely on finish->id_auth_uaddr, so that there are no casts from
pointer to uint64_t.  They break on 32-bit hosts.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/sev.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index a6b063b762c..28d6bd3adfa 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -155,6 +155,7 @@ struct SevSnpGuestState {
     char *id_block_base64;
     uint8_t *id_block;
     char *id_auth_base64;
+    uint8_t *id_auth;
     char *host_data;
 
     struct kvm_sev_snp_launch_start kvm_start_conf;
@@ -2208,16 +2209,16 @@ sev_snp_guest_set_id_auth(Object *obj, const char *value, Error **errp)
     struct kvm_sev_snp_launch_finish *finish = &sev_snp_guest->kvm_finish_conf;
     gsize len;
 
+    finish->id_auth_uaddr = 0;
+    g_free(sev_snp_guest->id_auth);
     g_free(sev_snp_guest->id_auth_base64);
-    g_free((guchar *)finish->id_auth_uaddr);
 
     /* store the base64 str so we don't need to re-encode in getter */
     sev_snp_guest->id_auth_base64 = g_strdup(value);
+    sev_snp_guest->id_auth =
+        qbase64_decode(sev_snp_guest->id_auth_base64, -1, &len, errp);
 
-    finish->id_auth_uaddr =
-        (uint64_t)qbase64_decode(sev_snp_guest->id_auth_base64, -1, &len, errp);
-
-    if (!finish->id_auth_uaddr) {
+    if (!sev_snp_guest->id_auth) {
         return;
     }
 
@@ -2226,6 +2227,8 @@ sev_snp_guest_set_id_auth(Object *obj, const char *value, Error **errp)
                    len, KVM_SEV_SNP_ID_AUTH_SIZE);
         return;
     }
+
+    finish->id_auth_uaddr = (uintptr_t)sev_snp_guest->id_auth;
 }
 
 static bool
-- 
2.45.2

Re: [PATCH 4/5] target/i386: SEV: store pointer to decoded id_auth in SevSnpGuest

[Richard Henderson]

On 6/27/24 07:53, Paolo Bonzini wrote:
> Do not rely on finish->id_auth_uaddr, so that there are no casts from
> pointer to uint64_t.  They break on 32-bit hosts.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/sev.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 5/5] target/i386: SEV: fix format strings for 32-bit hosts

[Paolo Bonzini]

Use PRIx64 for uint64_t and %zu for size_t/gsize.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/sev.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 28d6bd3adfa..77ff908ab17 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -886,7 +886,7 @@ sev_snp_launch_update(SevSnpGuestState *sev_snp_guest,
 
     if (!data->hva || !data->len) {
         error_report("SNP_LAUNCH_UPDATE called with invalid address"
-                     "/ length: %p / %lx",
+                     "/ length: %p / %" PRIx64,
                      data->hva, data->len);
         return 1;
     }
@@ -934,8 +934,8 @@ sev_snp_launch_update(SevSnpGuestState *sev_snp_guest,
 
 out:
     if (!ret && update.gfn_start << TARGET_PAGE_BITS != data->gpa + data->len) {
-        error_report("SEV-SNP: expected update of GPA range %lx-%lx,"
-                     "got GPA range %lx-%llx",
+        error_report("SEV-SNP: expected update of GPA range %" PRIx64 "-%" PRIx64 ","
+                     "got GPA range %" PRIx64 "-%llx",
                      data->gpa, data->gpa + data->len, data->gpa,
                      update.gfn_start << TARGET_PAGE_BITS);
         ret = -EIO;
@@ -2148,7 +2148,7 @@ sev_snp_guest_set_guest_visible_workarounds(Object *obj, const char *value,
     }
 
     if (len != sizeof(start->gosvw)) {
-        error_setg(errp, "parameter length of %lu exceeds max of %lu",
+        error_setg(errp, "parameter length of %zu exceeds max of %zu",
                    len, sizeof(start->gosvw));
         return;
     }
@@ -2185,7 +2185,7 @@ sev_snp_guest_set_id_block(Object *obj, const char *value, Error **errp)
     }
 
     if (len != KVM_SEV_SNP_ID_BLOCK_SIZE) {
-        error_setg(errp, "parameter length of %lu not equal to %u",
+        error_setg(errp, "parameter length of %zu not equal to %u",
                    len, KVM_SEV_SNP_ID_BLOCK_SIZE);
         return;
     }
@@ -2223,7 +2223,7 @@ sev_snp_guest_set_id_auth(Object *obj, const char *value, Error **errp)
     }
 
     if (len > KVM_SEV_SNP_ID_AUTH_SIZE) {
-        error_setg(errp, "parameter length:ID_AUTH %lu exceeds max of %u",
+        error_setg(errp, "parameter length:ID_AUTH %zu exceeds max of %u",
                    len, KVM_SEV_SNP_ID_AUTH_SIZE);
         return;
     }
@@ -2291,7 +2291,7 @@ sev_snp_guest_set_host_data(Object *obj, const char *value, Error **errp)
     }
 
     if (len != sizeof(finish->host_data)) {
-        error_setg(errp, "parameter length of %lu not equal to %lu",
+        error_setg(errp, "parameter length of %zu not equal to %zu",
                    len, sizeof(finish->host_data));
         return;
     }
-- 
2.45.2