| Message ID | 20241218155913.72288-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | system/confidential-guest-support: Header cleanups | expand |
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>
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
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
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);
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>
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
"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(-)