1. Patchew
  2. QEMU
  3. View series

[RFC PATCH] target/i386: restrict SEV to 64 bit host builds

Alex Bennée posted 1 patch 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240626140307.1026816-1-alex.bennee@linaro.org
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>
target/i386/sev.h       | 2 +-
target/i386/meson.build | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
[RFC PATCH] target/i386: restrict SEV to 64 bit host builds
Posted by Alex Bennée 5 months ago 
Re-enabling the 32 bit host build on i686 showed the recently merged
SEV code doesn't take enough care over its types. While the format
strings could use more portable types there isn't much we can do about
casting uint64_t into a pointer. The easiest solution seems to be just
to disable SEV for a 32 bit build. It's highly unlikely anyone would
want this functionality anyway.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/i386/sev.h       | 2 +-
 target/i386/meson.build | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/i386/sev.h b/target/i386/sev.h
index 858005a119..b0cb9dd7ed 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -45,7 +45,7 @@ typedef struct SevKernelLoaderContext {
     size_t cmdline_size;
 } SevKernelLoaderContext;
 
-#ifdef CONFIG_SEV
+#if defined(CONFIG_SEV) && defined(HOST_X86_64)
 bool sev_enabled(void);
 bool sev_es_enabled(void);
 bool sev_snp_enabled(void);
diff --git a/target/i386/meson.build b/target/i386/meson.build
index 075117989b..d2a008926c 100644
--- a/target/i386/meson.build
+++ b/target/i386/meson.build
@@ -6,7 +6,7 @@ i386_ss.add(files(
   'xsave_helper.c',
   'cpu-dump.c',
 ))
-i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c', 'confidential-guest.c'))
+i386_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('host-cpu.c', 'confidential-guest.c'))
 
 # x86 cpu type
 i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
@@ -21,7 +21,7 @@ i386_system_ss.add(files(
   'cpu-apic.c',
   'cpu-sysemu.c',
 ))
-i386_system_ss.add(when: 'CONFIG_SEV', if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
+i386_system_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
 
 i386_user_ss = ss.source_set()
 
-- 
2.39.2
Re: [RFC PATCH] target/i386: restrict SEV to 64 bit host builds
Posted by Paolo Bonzini 5 months ago 
On 6/26/24 16:03, Alex Bennée wrote:
> Re-enabling the 32 bit host build on i686 showed the recently merged
> SEV code doesn't take enough care over its types. While the format
> strings could use more portable types there isn't much we can do about
> casting uint64_t into a pointer. The easiest solution seems to be just
> to disable SEV for a 32 bit build. It's highly unlikely anyone would
> want this functionality anyway.

It's better style to just fix the compilation issues.  I'll send a small 
series once I test it.

Paolo

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/i386/sev.h       | 2 +-
>   target/i386/meson.build | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/sev.h b/target/i386/sev.h
> index 858005a119..b0cb9dd7ed 100644
> --- a/target/i386/sev.h
> +++ b/target/i386/sev.h
> @@ -45,7 +45,7 @@ typedef struct SevKernelLoaderContext {
>       size_t cmdline_size;
>   } SevKernelLoaderContext;
>   
> -#ifdef CONFIG_SEV
> +#if defined(CONFIG_SEV) && defined(HOST_X86_64)
>   bool sev_enabled(void);
>   bool sev_es_enabled(void);
>   bool sev_snp_enabled(void);
> diff --git a/target/i386/meson.build b/target/i386/meson.build
> index 075117989b..d2a008926c 100644
> --- a/target/i386/meson.build
> +++ b/target/i386/meson.build
> @@ -6,7 +6,7 @@ i386_ss.add(files(
>     'xsave_helper.c',
>     'cpu-dump.c',
>   ))
> -i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c', 'confidential-guest.c'))
> +i386_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('host-cpu.c', 'confidential-guest.c'))
>   
>   # x86 cpu type
>   i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
> @@ -21,7 +21,7 @@ i386_system_ss.add(files(
>     'cpu-apic.c',
>     'cpu-sysemu.c',
>   ))
> -i386_system_ss.add(when: 'CONFIG_SEV', if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
> +i386_system_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
>   
>   i386_user_ss = ss.source_set()
>
Re: [RFC PATCH] target/i386: restrict SEV to 64 bit host builds
Posted by Richard Henderson 5 months ago 
On 6/26/24 07:03, Alex Bennée wrote:
> While the format
> strings could use more portable types there isn't much we can do about
> casting uint64_t into a pointer.

Use uintptr_t, obviously.


r~
Re: [RFC PATCH] target/i386: restrict SEV to 64 bit host builds
Posted by Daniel P. Berrangé 5 months ago 
On Wed, Jun 26, 2024 at 03:03:07PM +0100, Alex Bennée wrote:
> Re-enabling the 32 bit host build on i686 showed the recently merged
> SEV code doesn't take enough care over its types. While the format
> strings could use more portable types there isn't much we can do about
> casting uint64_t into a pointer. The easiest solution seems to be just
> to disable SEV for a 32 bit build. It's highly unlikely anyone would
> want this functionality anyway.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/i386/sev.h       | 2 +-
>  target/i386/meson.build | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/sev.h b/target/i386/sev.h
> index 858005a119..b0cb9dd7ed 100644
> --- a/target/i386/sev.h
> +++ b/target/i386/sev.h
> @@ -45,7 +45,7 @@ typedef struct SevKernelLoaderContext {
>      size_t cmdline_size;
>  } SevKernelLoaderContext;
>  
> -#ifdef CONFIG_SEV
> +#if defined(CONFIG_SEV) && defined(HOST_X86_64)
>  bool sev_enabled(void);
>  bool sev_es_enabled(void);
>  bool sev_snp_enabled(void);
> diff --git a/target/i386/meson.build b/target/i386/meson.build
> index 075117989b..d2a008926c 100644
> --- a/target/i386/meson.build
> +++ b/target/i386/meson.build
> @@ -6,7 +6,7 @@ i386_ss.add(files(
>    'xsave_helper.c',
>    'cpu-dump.c',
>  ))
> -i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c', 'confidential-guest.c'))
> +i386_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('host-cpu.c', 'confidential-guest.c'))
>  
>  # x86 cpu type
>  i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
> @@ -21,7 +21,7 @@ i386_system_ss.add(files(
>    'cpu-apic.c',
>    'cpu-sysemu.c',
>  ))
> -i386_system_ss.add(when: 'CONFIG_SEV', if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
> +i386_system_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
>  
>  i386_user_ss = ss.source_set()

Instead of changing each usage of CONFIG_SEV, is it better to
prevent it getting enabled in the first place ?

eg. move

  #CONFIG_SEV=n

From

  configs/devices/i386-softmmu/default.mak

to

  configs/devices/x86_64-softmmu/default.mak

And then also change

  hw/i386/Kconfig

to say

  config SEV
      bool
      select X86_FW_OVMF
      depends on KVM && X86_64


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [RFC PATCH] target/i386: restrict SEV to 64 bit host builds
Posted by Philippe Mathieu-Daudé 5 months ago 
On 26/6/24 16:20, Daniel P. Berrangé wrote:
> On Wed, Jun 26, 2024 at 03:03:07PM +0100, Alex Bennée wrote:
>> Re-enabling the 32 bit host build on i686 showed the recently merged
>> SEV code doesn't take enough care over its types. While the format
>> strings could use more portable types there isn't much we can do about
>> casting uint64_t into a pointer. The easiest solution seems to be just
>> to disable SEV for a 32 bit build. It's highly unlikely anyone would
>> want this functionality anyway.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   target/i386/sev.h       | 2 +-
>>   target/i386/meson.build | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/i386/sev.h b/target/i386/sev.h
>> index 858005a119..b0cb9dd7ed 100644
>> --- a/target/i386/sev.h
>> +++ b/target/i386/sev.h
>> @@ -45,7 +45,7 @@ typedef struct SevKernelLoaderContext {
>>       size_t cmdline_size;
>>   } SevKernelLoaderContext;
>>   
>> -#ifdef CONFIG_SEV
>> +#if defined(CONFIG_SEV) && defined(HOST_X86_64)
>>   bool sev_enabled(void);
>>   bool sev_es_enabled(void);
>>   bool sev_snp_enabled(void);
>> diff --git a/target/i386/meson.build b/target/i386/meson.build
>> index 075117989b..d2a008926c 100644
>> --- a/target/i386/meson.build
>> +++ b/target/i386/meson.build
>> @@ -6,7 +6,7 @@ i386_ss.add(files(
>>     'xsave_helper.c',
>>     'cpu-dump.c',
>>   ))
>> -i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c', 'confidential-guest.c'))
>> +i386_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('host-cpu.c', 'confidential-guest.c'))
>>   
>>   # x86 cpu type
>>   i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
>> @@ -21,7 +21,7 @@ i386_system_ss.add(files(
>>     'cpu-apic.c',
>>     'cpu-sysemu.c',
>>   ))
>> -i386_system_ss.add(when: 'CONFIG_SEV', if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
>> +i386_system_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
>>   
>>   i386_user_ss = ss.source_set()
> 
> Instead of changing each usage of CONFIG_SEV, is it better to
> prevent it getting enabled in the first place ?
> 
> eg. move
> 
>    #CONFIG_SEV=n
> 
> From
> 
>    configs/devices/i386-softmmu/default.mak
> 
> to
> 
>    configs/devices/x86_64-softmmu/default.mak
> 
> And then also change
> 
>    hw/i386/Kconfig
> 
> to say
> 
>    config SEV
>        bool
>        select X86_FW_OVMF
>        depends on KVM && X86_64

Both are *targets*, IIUC we want to disable on *hosts*.
Re: [RFC PATCH] target/i386: restrict SEV to 64 bit host builds
Posted by Alex Bennée 5 months ago 
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Jun 26, 2024 at 03:03:07PM +0100, Alex Bennée wrote:
>> Re-enabling the 32 bit host build on i686 showed the recently merged
>> SEV code doesn't take enough care over its types. While the format
>> strings could use more portable types there isn't much we can do about
>> casting uint64_t into a pointer. The easiest solution seems to be just
>> to disable SEV for a 32 bit build. It's highly unlikely anyone would
>> want this functionality anyway.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  target/i386/sev.h       | 2 +-
>>  target/i386/meson.build | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/target/i386/sev.h b/target/i386/sev.h
>> index 858005a119..b0cb9dd7ed 100644
>> --- a/target/i386/sev.h
>> +++ b/target/i386/sev.h
>> @@ -45,7 +45,7 @@ typedef struct SevKernelLoaderContext {
>>      size_t cmdline_size;
>>  } SevKernelLoaderContext;
>>  
>> -#ifdef CONFIG_SEV
>> +#if defined(CONFIG_SEV) && defined(HOST_X86_64)
>>  bool sev_enabled(void);
>>  bool sev_es_enabled(void);
>>  bool sev_snp_enabled(void);
>> diff --git a/target/i386/meson.build b/target/i386/meson.build
>> index 075117989b..d2a008926c 100644
>> --- a/target/i386/meson.build
>> +++ b/target/i386/meson.build
>> @@ -6,7 +6,7 @@ i386_ss.add(files(
>>    'xsave_helper.c',
>>    'cpu-dump.c',
>>  ))
>> -i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c', 'confidential-guest.c'))
>> +i386_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('host-cpu.c', 'confidential-guest.c'))
>>  
>>  # x86 cpu type
>>  i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
>> @@ -21,7 +21,7 @@ i386_system_ss.add(files(
>>    'cpu-apic.c',
>>    'cpu-sysemu.c',
>>  ))
>> -i386_system_ss.add(when: 'CONFIG_SEV', if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
>> +i386_system_ss.add(when: ['CONFIG_SEV', 'HOST_X86_64'], if_true: files('sev.c'), if_false: files('sev-sysemu-stub.c'))
>>  
>>  i386_user_ss = ss.source_set()
>
> Instead of changing each usage of CONFIG_SEV, is it better to
> prevent it getting enabled in the first place ?
>
> eg. move
>
>   #CONFIG_SEV=n
>
> From
>
>   configs/devices/i386-softmmu/default.mak
>
> to
>
>   configs/devices/x86_64-softmmu/default.mak
>
> And then also change
>
>   hw/i386/Kconfig
>
> to say
>
>   config SEV
>       bool
>       select X86_FW_OVMF
>       depends on KVM && X86_64

I was wondering if I could do it all with Kconfig. Will respin thanks.

>
>
> With regards,
> Daniel

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.