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

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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);