system/confidential-guest-support: Header cleanups

[PATCH 2/2] target/i386/sev: Reduce system specific declarations

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

"system/confidential-guest-support.h" is not needed,
remove it. Reorder #ifdef'ry to reduce declarations
exposed on user emulation.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/sev.h  | 29 ++++++++++++++++-------------
 hw/i386/pc_sysfw.c |  2 +-
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/target/i386/sev.h b/target/i386/sev.h
index 2664c0b1b6c..373669eaace 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -18,7 +18,17 @@
 #include CONFIG_DEVICES /* CONFIG_SEV */
 #endif
 
-#include "system/confidential-guest-support.h"
+#if !defined(CONFIG_SEV) || defined(CONFIG_USER_ONLY)
+#define sev_enabled() 0
+#define sev_es_enabled() 0
+#define sev_snp_enabled() 0
+#else
+bool sev_enabled(void);
+bool sev_es_enabled(void);
+bool sev_snp_enabled(void);
+#endif
+
+#if !defined(CONFIG_USER_ONLY)
 
 #define TYPE_SEV_COMMON "sev-common"
 #define TYPE_SEV_GUEST "sev-guest"
@@ -45,18 +55,6 @@ typedef struct SevKernelLoaderContext {
     size_t cmdline_size;
 } SevKernelLoaderContext;
 
-#ifdef CONFIG_SEV
-bool sev_enabled(void);
-bool sev_es_enabled(void);
-bool sev_snp_enabled(void);
-#else
-#define sev_enabled() 0
-#define sev_es_enabled() 0
-#define sev_snp_enabled() 0
-#endif
-
-uint32_t sev_get_cbit_position(void);
-uint32_t sev_get_reduced_phys_bits(void);
 bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp);
 
 int sev_encrypt_flash(hwaddr gpa, uint8_t *ptr, uint64_t len, Error **errp);
@@ -68,4 +66,9 @@ void sev_es_set_reset_vector(CPUState *cpu);
 
 void pc_system_parse_sev_metadata(uint8_t *flash_ptr, size_t flash_size);
 
+#endif /* !CONFIG_USER_ONLY */
+
+uint32_t sev_get_cbit_position(void);
+uint32_t sev_get_reduced_phys_bits(void);
+
 #endif
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index da7ed121292..1eeb58ab37f 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -36,7 +36,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/block/flash.h"
 #include "system/kvm.h"
-#include "sev.h"
+#include "target/i386/sev.h"
 
 #define FLASH_SECTOR_SIZE 4096
 
-- 
2.45.2

Re: [PATCH 2/2] target/i386/sev: Reduce system specific declarations

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

On 18/12/24 16:59, Philippe Mathieu-Daudé wrote:
> "system/confidential-guest-support.h" is not needed,
> remove it. Reorder #ifdef'ry to reduce declarations
> exposed on user emulation.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/i386/sev.h  | 29 ++++++++++++++++-------------
>   hw/i386/pc_sysfw.c |  2 +-
>   2 files changed, 17 insertions(+), 14 deletions(-)


> +#if !defined(CONFIG_USER_ONLY)
>   
>   #define TYPE_SEV_COMMON "sev-common"
>   #define TYPE_SEV_GUEST "sev-guest"
> @@ -45,18 +55,6 @@ typedef struct SevKernelLoaderContext {
>       size_t cmdline_size;
>   } SevKernelLoaderContext;
>   
> -#ifdef CONFIG_SEV
> -bool sev_enabled(void);
> -bool sev_es_enabled(void);
> -bool sev_snp_enabled(void);
> -#else
> -#define sev_enabled() 0
> -#define sev_es_enabled() 0
> -#define sev_snp_enabled() 0
> -#endif
> -
> -uint32_t sev_get_cbit_position(void);
> -uint32_t sev_get_reduced_phys_bits(void);
>   bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp);
>   
>   int sev_encrypt_flash(hwaddr gpa, uint8_t *ptr, uint64_t len, Error **errp);

The motivation is to reduce system-specific definitions
exposed to user-mode in target/i386/cpu.c, like hwaddr &co,
but I'm not there yet and have too many local patches so
starting to send what's ready.

> @@ -68,4 +66,9 @@ void sev_es_set_reset_vector(CPUState *cpu);
>   
>   void pc_system_parse_sev_metadata(uint8_t *flash_ptr, size_t flash_size);
>   
> +#endif /* !CONFIG_USER_ONLY */
> +
> +uint32_t sev_get_cbit_position(void);
> +uint32_t sev_get_reduced_phys_bits(void);
> +
>   #endif

Re: [PATCH 2/2] target/i386/sev: Reduce system specific declarations

Posted by Daniel P. Berrangé 1 year, 1 month ago

On Wed, Dec 18, 2024 at 04:59:13PM +0100, Philippe Mathieu-Daudé wrote:
> "system/confidential-guest-support.h" is not needed,
> remove it. Reorder #ifdef'ry to reduce declarations
> exposed on user emulation.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/i386/sev.h  | 29 ++++++++++++++++-------------
>  hw/i386/pc_sysfw.c |  2 +-
>  2 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/target/i386/sev.h b/target/i386/sev.h
> index 2664c0b1b6c..373669eaace 100644
> --- a/target/i386/sev.h
> +++ b/target/i386/sev.h
> @@ -18,7 +18,17 @@
>  #include CONFIG_DEVICES /* CONFIG_SEV */
>  #endif
>  
> -#include "system/confidential-guest-support.h"
> +#if !defined(CONFIG_SEV) || defined(CONFIG_USER_ONLY)
> +#define sev_enabled() 0
> +#define sev_es_enabled() 0
> +#define sev_snp_enabled() 0
> +#else
> +bool sev_enabled(void);
> +bool sev_es_enabled(void);
> +bool sev_snp_enabled(void);
> +#endif
> +
> +#if !defined(CONFIG_USER_ONLY)

I'm surprised any of this header file is relevant to
user mode. If something is mistakely calling sev_ functions
from user mode compiled code, I'd be inclined to fix the
caller such that its #include ".../sev.h" can be wrapped
by !CONFIG_USER_ONLY

>  
>  #define TYPE_SEV_COMMON "sev-common"
>  #define TYPE_SEV_GUEST "sev-guest"
> @@ -45,18 +55,6 @@ typedef struct SevKernelLoaderContext {
>      size_t cmdline_size;
>  } SevKernelLoaderContext;
>  
> -#ifdef CONFIG_SEV
> -bool sev_enabled(void);
> -bool sev_es_enabled(void);
> -bool sev_snp_enabled(void);
> -#else
> -#define sev_enabled() 0
> -#define sev_es_enabled() 0
> -#define sev_snp_enabled() 0
> -#endif
> -
> -uint32_t sev_get_cbit_position(void);
> -uint32_t sev_get_reduced_phys_bits(void);
>  bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp);
>  
>  int sev_encrypt_flash(hwaddr gpa, uint8_t *ptr, uint64_t len, Error **errp);
> @@ -68,4 +66,9 @@ void sev_es_set_reset_vector(CPUState *cpu);
>  
>  void pc_system_parse_sev_metadata(uint8_t *flash_ptr, size_t flash_size);
>  
> +#endif /* !CONFIG_USER_ONLY */
> +
> +uint32_t sev_get_cbit_position(void);
> +uint32_t sev_get_reduced_phys_bits(void);
> +
>  #endif
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index da7ed121292..1eeb58ab37f 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -36,7 +36,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/block/flash.h"
>  #include "system/kvm.h"
> -#include "sev.h"
> +#include "target/i386/sev.h"
>  
>  #define FLASH_SECTOR_SIZE 4096
>  
> -- 
> 2.45.2
> 
> 

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: [PATCH 2/2] target/i386/sev: Reduce system specific declarations

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

On 18/12/24 17:17, Daniel P. Berrangé wrote:
> On Wed, Dec 18, 2024 at 04:59:13PM +0100, Philippe Mathieu-Daudé wrote:
>> "system/confidential-guest-support.h" is not needed,
>> remove it. Reorder #ifdef'ry to reduce declarations
>> exposed on user emulation.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/i386/sev.h  | 29 ++++++++++++++++-------------
>>   hw/i386/pc_sysfw.c |  2 +-
>>   2 files changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/i386/sev.h b/target/i386/sev.h
>> index 2664c0b1b6c..373669eaace 100644
>> --- a/target/i386/sev.h
>> +++ b/target/i386/sev.h
>> @@ -18,7 +18,17 @@
>>   #include CONFIG_DEVICES /* CONFIG_SEV */
>>   #endif
>>   
>> -#include "system/confidential-guest-support.h"
>> +#if !defined(CONFIG_SEV) || defined(CONFIG_USER_ONLY)
>> +#define sev_enabled() 0
>> +#define sev_es_enabled() 0
>> +#define sev_snp_enabled() 0
>> +#else
>> +bool sev_enabled(void);
>> +bool sev_es_enabled(void);
>> +bool sev_snp_enabled(void);
>> +#endif
>> +
>> +#if !defined(CONFIG_USER_ONLY)
> 
> I'm surprised any of this header file is relevant to
> user mode. If something is mistakely calling sev_ functions
> from user mode compiled code, I'd be inclined to fix the
> caller such that its #include ".../sev.h" can be wrapped
> by !CONFIG_USER_ONLY

I forgot to mention and just replied in another post:

   The motivation is to reduce system-specific definitions
   exposed to user-mode in target/i386/cpu.c, like hwaddr &co,
   but I'm not there yet and have too many local patches so
   starting to send what's ready.

WRT SEV what is bugging me is in cpu_x86_cpuid():

target/i386/cpu.c-7137-    case 0x8000001F:
target/i386/cpu.c-7138-        *eax = *ebx = *ecx = *edx = 0;
target/i386/cpu.c:7139:        if (sev_enabled()) {
target/i386/cpu.c-7140-            *eax = 0x2;
target/i386/cpu.c-7141-            *eax |= sev_es_enabled() ? 0x8 : 0;
target/i386/cpu.c-7142-            *eax |= sev_snp_enabled() ? 0x10 : 0;
target/i386/cpu.c-7143-            *ebx = sev_get_cbit_position() & 
0x3f; /* EBX[5:0] */
target/i386/cpu.c-7144-            *ebx |= (sev_get_reduced_phys_bits() 
& 0x3f) << 6; /* EBX[11:6] */
target/i386/cpu.c-7145-        }
target/i386/cpu.c-7146-        break;

but maybe I can use #ifdef'ry around CONFIG_USER_ONLY like
with SGX:

     case 0x12:
#ifndef CONFIG_USER_ONLY
         if (count > 1) {
             uint64_t epc_addr, epc_size;

             if (sgx_epc_get_section(count - 2, &epc_addr, &epc_size)) {
                 *eax = *ebx = *ecx = *edx = 0;
                 break;
             }
             ...
#endif
         break;

> 
>>   
>>   #define TYPE_SEV_COMMON "sev-common"
>>   #define TYPE_SEV_GUEST "sev-guest"
>> @@ -45,18 +55,6 @@ typedef struct SevKernelLoaderContext {
>>       size_t cmdline_size;
>>   } SevKernelLoaderContext;
>>   
>> -#ifdef CONFIG_SEV
>> -bool sev_enabled(void);
>> -bool sev_es_enabled(void);
>> -bool sev_snp_enabled(void);
>> -#else
>> -#define sev_enabled() 0
>> -#define sev_es_enabled() 0
>> -#define sev_snp_enabled() 0
>> -#endif
>> -
>> -uint32_t sev_get_cbit_position(void);
>> -uint32_t sev_get_reduced_phys_bits(void);
>>   bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp);
>>   
>>   int sev_encrypt_flash(hwaddr gpa, uint8_t *ptr, uint64_t len, Error **errp);

Re: [PATCH 2/2] target/i386/sev: Reduce system specific declarations

Posted by Zhao Liu 1 year, 1 month ago

On Wed, Dec 18, 2024 at 04:59:13PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 18 Dec 2024 16:59:13 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: [PATCH 2/2] target/i386/sev: Reduce system specific declarations
> X-Mailer: git-send-email 2.45.2
> 
> "system/confidential-guest-support.h" is not needed,
> remove it. Reorder #ifdef'ry to reduce declarations
> exposed on user emulation.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/i386/sev.h  | 29 ++++++++++++++++-------------
>  hw/i386/pc_sysfw.c |  2 +-
>  2 files changed, 17 insertions(+), 14 deletions(-)
> 

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 2/2] target/i386/sev: Reduce system specific declarations

Posted by Thomas Huth 1 year, 1 month ago

On 18/12/2024 16.59, Philippe Mathieu-Daudé wrote:
> "system/confidential-guest-support.h" is not needed,
> remove it. Reorder #ifdef'ry to reduce declarations
> exposed on user emulation.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/i386/sev.h  | 29 ++++++++++++++++-------------
>   hw/i386/pc_sysfw.c |  2 +-
>   2 files changed, 17 insertions(+), 14 deletions(-)

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