* Philippe Mathieu-Daudé (philmd@xxxxxxxxxx) wrote:
> On 10/7/21 19:07, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (philmd@xxxxxxxxxx) wrote:
> >> Only declare sev_enabled() and sev_es_enabled() when CONFIG_SEV is
> >> set, to allow the compiler to elide unused code. Remove unnecessary
> >> stubs.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
> >
> > What makes it allowed to *rely* on the compiler eliding calls?
>
> I am not aware of a particular requirement on the compiler for code
> elision, however we already use this syntax:
>
> $ git grep -A4 'ifdef CONFIG_' include/sysemu/
> ...
> include/sysemu/tcg.h:11:#ifdef CONFIG_TCG
> include/sysemu/tcg.h-12-extern bool tcg_allowed;
> include/sysemu/tcg.h-13-#define tcg_enabled() (tcg_allowed)
> include/sysemu/tcg.h-14-#else
> include/sysemu/tcg.h-15-#define tcg_enabled() 0
So that I'm fine with, the bit I'm more worried about is the bit where
inside the if () we call functions (like sev_get_cbit_position ) which
we know the compiler will elide; I'm sure any sane compiler will,
but.....
Looking at your example, in cpu.c there's still places that ifdef around
areas with tcg_enabled.
Dave
> ...
>
> Cc'ing Richard/Eric/Daniel who have more experience with compiler
> features in case they can enlighten me here.
>
> >> ---
> >> target/i386/sev.h | 14 ++++++++++++--
> >> target/i386/cpu.c | 13 +++++++------
> >> target/i386/sev-stub.c | 41 -----------------------------------------
> >> target/i386/meson.build | 2 +-
> >> 4 files changed, 20 insertions(+), 50 deletions(-)
> >> delete mode 100644 target/i386/sev-stub.c
> >>
> >> diff --git a/target/i386/sev.h b/target/i386/sev.h
> >> index c96072bf78d..d9548e3e642 100644
> >> --- a/target/i386/sev.h
> >> +++ b/target/i386/sev.h
> >> @@ -14,6 +14,10 @@
> >> #ifndef QEMU_SEV_I386_H
> >> #define QEMU_SEV_I386_H
> >>
> >> +#ifndef CONFIG_USER_ONLY
> >> +#include CONFIG_DEVICES /* CONFIG_SEV */
> >> +#endif
> >> +
> >> #include "exec/confidential-guest-support.h"
> >> #include "qapi/qapi-types-misc-target.h"
> >>
> >> @@ -35,8 +39,14 @@ typedef struct SevKernelLoaderContext {
> >> size_t cmdline_size;
> >> } SevKernelLoaderContext;
> >>
> >> -bool sev_enabled(void);
> >> -extern bool sev_es_enabled(void);
> >> +#ifdef CONFIG_SEV
> >> + bool sev_enabled(void);
> >> +bool sev_es_enabled(void);
> >> +#else
> >> +#define sev_enabled() 0
> >> +#define sev_es_enabled() 0
> >> +#endif
> >> +
> >> extern SevInfo *sev_get_info(void);
> >> extern uint32_t sev_get_cbit_position(void);
> >> extern uint32_t sev_get_reduced_phys_bits(void);
> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> index 8289dc87bd5..fc3ed80ef1e 100644
> >> --- a/target/i386/cpu.c
> >> +++ b/target/i386/cpu.c
> >> @@ -5764,12 +5764,13 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >> *edx = 0;
> >> break;
> >> case 0x8000001F:
> >> - *eax = sev_enabled() ? 0x2 : 0;
> >> - *eax |= sev_es_enabled() ? 0x8 : 0;
> >> - *ebx = sev_get_cbit_position();
> >> - *ebx |= sev_get_reduced_phys_bits() << 6;
> >> - *ecx = 0;
> >> - *edx = 0;
> >> + *eax = *ebx = *ecx = *edx = 0;
> >> + if (sev_enabled()) {
> >> + *eax = 0x2;
> >> + *eax |= sev_es_enabled() ? 0x8 : 0;
> >> + *ebx = sev_get_cbit_position();
> >> + *ebx |= sev_get_reduced_phys_bits() << 6;
> >> + }
> >> break;
> >> default:
> >> /* reserved values: zero */
> >> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> >> deleted file mode 100644
> >> index 7e8b6f9a259..00000000000
> >> --- a/target/i386/sev-stub.c
> >> +++ /dev/null
> >> @@ -1,41 +0,0 @@
> >> -/*
> >> - * QEMU SEV stub
> >> - *
> >> - * Copyright Advanced Micro Devices 2018
> >> - *
> >> - * Authors:
> >> - * Brijesh Singh <brijesh.singh@xxxxxxx>
> >> - *
> >> - * This work is licensed under the terms of the GNU GPL, version 2 or later.
> >> - * See the COPYING file in the top-level directory.
> >> - *
> >> - */
> >> -
> >> -#include "qemu/osdep.h"
> >> -#include "qapi/error.h"
> >> -#include "sev.h"
> >> -
> >> -bool sev_enabled(void)
> >> -{
> >> - return false;
> >> -}
> >> -
> >> -uint32_t sev_get_cbit_position(void)
> >> -{
> >> - return 0;
> >> -}
> >> -
> >> -uint32_t sev_get_reduced_phys_bits(void)
> >> -{
> >> - return 0;
> >> -}
> >> -
> >> -bool sev_es_enabled(void)
> >> -{
> >> - return false;
> >> -}
> >> -
> >> -bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
> >> -{
> >> - g_assert_not_reached();
> >> -}
> >> diff --git a/target/i386/meson.build b/target/i386/meson.build
> >> index a4f45c3ec1d..ae38dc95635 100644
> >> --- a/target/i386/meson.build
> >> +++ b/target/i386/meson.build
> >> @@ -6,7 +6,7 @@
> >> 'xsave_helper.c',
> >> 'cpu-dump.c',
> >> ))
> >> -i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'), if_false: files('sev-stub.c'))
> >> +i386_ss.add(when: 'CONFIG_SEV', if_true: files('host-cpu.c'))
> >>
> >> # x86 cpu type
> >> i386_ss.add(when: 'CONFIG_KVM', if_true: files('host-cpu.c'))
> >> --
> >> 2.31.1
> >>
>
--
Dr. David Alan Gilbert / dgilbert@xxxxxxxxxx / Manchester, UK
