1. Patchew
  2. QEMU
  3. [PATCH v3 00/22] target/i386/sev: Housekeeping SEV + measured Linux SEV guest
  4. View patch

[PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision

Philippe Mathieu-Daudé posted 22 patches 4 years, 4 months ago
There is a newer version of this series
[PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision
Posted by Philippe Mathieu-Daudé 4 years, 4 months ago 
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@redhat.com>
---
 include/sysemu/sev.h    | 14 +++++++++++++-
 target/i386/sev_i386.h  |  3 ---
 target/i386/cpu.c       | 16 +++++++++-------
 target/i386/sev-stub.c  | 36 ------------------------------------
 target/i386/meson.build |  2 +-
 5 files changed, 23 insertions(+), 48 deletions(-)
 delete mode 100644 target/i386/sev-stub.c

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index a329ed75c1c..f5c625bb3b3 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -14,9 +14,21 @@
 #ifndef QEMU_SEV_H
 #define QEMU_SEV_H
 
-#include "sysemu/kvm.h"
+#ifndef CONFIG_USER_ONLY
+#include CONFIG_DEVICES /* CONFIG_SEV */
+#endif
 
+#ifdef CONFIG_SEV
 bool sev_enabled(void);
+bool sev_es_enabled(void);
+#else
+#define sev_enabled() 0
+#define sev_es_enabled() 0
+#endif
+
+uint32_t sev_get_cbit_position(void);
+uint32_t sev_get_reduced_phys_bits(void);
+
 int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
 
 #endif
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 0798ab3519a..2d9a1a0112e 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -24,10 +24,7 @@
 #define SEV_POLICY_DOMAIN       0x10
 #define SEV_POLICY_SEV          0x20
 
-extern bool sev_es_enabled(void);
 extern SevInfo *sev_get_info(void);
-extern uint32_t sev_get_cbit_position(void);
-extern uint32_t sev_get_reduced_phys_bits(void);
 extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(Error **errp);
 extern SevAttestationReport *
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index e169a01713d..27992bdc9f8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -25,8 +25,8 @@
 #include "tcg/helper-tcg.h"
 #include "sysemu/reset.h"
 #include "sysemu/hvf.h"
+#include "sysemu/sev.h"
 #include "kvm/kvm_i386.h"
-#include "sev_i386.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-machine.h"
 #include "qapi/qmp/qerror.h"
@@ -38,6 +38,7 @@
 #include "exec/address-spaces.h"
 #include "hw/boards.h"
 #include "hw/i386/sgx-epc.h"
+#include "sev_i386.h"
 #endif
 
 #include "disas/capstone.h"
@@ -5764,12 +5765,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 8eae5d2fa8d..00000000000
--- a/target/i386/sev-stub.c
+++ /dev/null
@@ -1,36 +0,0 @@
-/*
- * QEMU SEV stub
- *
- * Copyright Advanced Micro Devices 2018
- *
- * Authors:
- *      Brijesh Singh <brijesh.singh@amd.com>
- *
- * 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_i386.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;
-}
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
Re: [PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision
Posted by Paolo Bonzini 4 years, 4 months ago 
On 02/10/21 14:53, Philippe Mathieu-Daudé 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@redhat.com>
> ---
>   include/sysemu/sev.h    | 14 +++++++++++++-
>   target/i386/sev_i386.h  |  3 ---
>   target/i386/cpu.c       | 16 +++++++++-------
>   target/i386/sev-stub.c  | 36 ------------------------------------
>   target/i386/meson.build |  2 +-
>   5 files changed, 23 insertions(+), 48 deletions(-)
>   delete mode 100644 target/i386/sev-stub.c
> 
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index a329ed75c1c..f5c625bb3b3 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -14,9 +14,21 @@
>   #ifndef QEMU_SEV_H
>   #define QEMU_SEV_H
>   
> -#include "sysemu/kvm.h"
> +#ifndef CONFIG_USER_ONLY
> +#include CONFIG_DEVICES /* CONFIG_SEV */
> +#endif
>   
> +#ifdef CONFIG_SEV
>   bool sev_enabled(void);
> +bool sev_es_enabled(void);
> +#else
> +#define sev_enabled() 0
> +#define sev_es_enabled() 0
> +#endif

This means that sev.h can only be included from target-specific files.

An alternative could be:

#ifdef NEED_CPU_H
# include CONFIG_DEVICES
#endif

#if defined NEED_CPU_H && !defined CONFIG_SEV
# define sev_enabled() 0
# define sev_es_enabled() 0
#else
bool sev_enabled(void);
bool sev_es_enabled(void);
#endif

... but in fact sysemu/sev.h _is_ only used from x86-specific files. So 
should it be moved to include/hw/i386, and even merged with 
target/i386/sev_i386.h?  Do we need two files?

Thanks,

Paolo

> +uint32_t sev_get_cbit_position(void);
> +uint32_t sev_get_reduced_phys_bits(void);
> +
>   int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
>   
>   #endif
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 0798ab3519a..2d9a1a0112e 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -24,10 +24,7 @@
>   #define SEV_POLICY_DOMAIN       0x10
>   #define SEV_POLICY_SEV          0x20
>   
> -extern bool sev_es_enabled(void);
>   extern SevInfo *sev_get_info(void);
> -extern uint32_t sev_get_cbit_position(void);
> -extern uint32_t sev_get_reduced_phys_bits(void);
>   extern char *sev_get_launch_measurement(void);
>   extern SevCapability *sev_get_capabilities(Error **errp);
>   extern SevAttestationReport *
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index e169a01713d..27992bdc9f8 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -25,8 +25,8 @@
>   #include "tcg/helper-tcg.h"
>   #include "sysemu/reset.h"
>   #include "sysemu/hvf.h"
> +#include "sysemu/sev.h"
>   #include "kvm/kvm_i386.h"
> -#include "sev_i386.h"
>   #include "qapi/error.h"
>   #include "qapi/qapi-visit-machine.h"
>   #include "qapi/qmp/qerror.h"
> @@ -38,6 +38,7 @@
>   #include "exec/address-spaces.h"
>   #include "hw/boards.h"
>   #include "hw/i386/sgx-epc.h"
> +#include "sev_i386.h"
>   #endif
>   
>   #include "disas/capstone.h"
> @@ -5764,12 +5765,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 8eae5d2fa8d..00000000000
> --- a/target/i386/sev-stub.c
> +++ /dev/null
> @@ -1,36 +0,0 @@
> -/*
> - * QEMU SEV stub
> - *
> - * Copyright Advanced Micro Devices 2018
> - *
> - * Authors:
> - *      Brijesh Singh <brijesh.singh@amd.com>
> - *
> - * 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_i386.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;
> -}
> 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'))
>
Re: [PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision
Posted by Philippe Mathieu-Daudé 4 years, 4 months ago 
On 10/4/21 10:19, Paolo Bonzini wrote:
> On 02/10/21 14:53, Philippe Mathieu-Daudé 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@redhat.com>
>> ---
>>   include/sysemu/sev.h    | 14 +++++++++++++-
>>   target/i386/sev_i386.h  |  3 ---
>>   target/i386/cpu.c       | 16 +++++++++-------
>>   target/i386/sev-stub.c  | 36 ------------------------------------
>>   target/i386/meson.build |  2 +-
>>   5 files changed, 23 insertions(+), 48 deletions(-)
>>   delete mode 100644 target/i386/sev-stub.c
>>
>> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
>> index a329ed75c1c..f5c625bb3b3 100644
>> --- a/include/sysemu/sev.h
>> +++ b/include/sysemu/sev.h
>> @@ -14,9 +14,21 @@
>>   #ifndef QEMU_SEV_H
>>   #define QEMU_SEV_H
>>   -#include "sysemu/kvm.h"
>> +#ifndef CONFIG_USER_ONLY
>> +#include CONFIG_DEVICES /* CONFIG_SEV */
>> +#endif
>>   +#ifdef CONFIG_SEV
>>   bool sev_enabled(void);
>> +bool sev_es_enabled(void);
>> +#else
>> +#define sev_enabled() 0
>> +#define sev_es_enabled() 0
>> +#endif
> 
> This means that sev.h can only be included from target-specific files.
> 
> An alternative could be:
> 
> #ifdef NEED_CPU_H
> # include CONFIG_DEVICES

<command-line>: fatal error: x86_64-linux-user-config-devices.h: No such
file or directory

> #endif
> 
> #if defined NEED_CPU_H && !defined CONFIG_SEV
> # define sev_enabled() 0
> # define sev_es_enabled() 0
> #else
> bool sev_enabled(void);
> bool sev_es_enabled(void);
> #endif
> 
> ... but in fact sysemu/sev.h _is_ only used from x86-specific files. So
> should it be moved to include/hw/i386, and even merged with
> target/i386/sev_i386.h?  Do we need two files?

No clue, I don't think we need. Brijesh?

> 
> Thanks,
> 
> Paolo
> 
>> +uint32_t sev_get_cbit_position(void);
>> +uint32_t sev_get_reduced_phys_bits(void);
>> +
>>   int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
>>     #endif
>> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
>> index 0798ab3519a..2d9a1a0112e 100644
>> --- a/target/i386/sev_i386.h
>> +++ b/target/i386/sev_i386.h
>> @@ -24,10 +24,7 @@
>>   #define SEV_POLICY_DOMAIN       0x10
>>   #define SEV_POLICY_SEV          0x20
>>   -extern bool sev_es_enabled(void);
>>   extern SevInfo *sev_get_info(void);
>> -extern uint32_t sev_get_cbit_position(void);
>> -extern uint32_t sev_get_reduced_phys_bits(void);
>>   extern char *sev_get_launch_measurement(void);
>>   extern SevCapability *sev_get_capabilities(Error **errp);
>>   extern SevAttestationReport *
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index e169a01713d..27992bdc9f8 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -25,8 +25,8 @@
>>   #include "tcg/helper-tcg.h"
>>   #include "sysemu/reset.h"
>>   #include "sysemu/hvf.h"
>> +#include "sysemu/sev.h"
>>   #include "kvm/kvm_i386.h"
>> -#include "sev_i386.h"
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-visit-machine.h"
>>   #include "qapi/qmp/qerror.h"
>> @@ -38,6 +38,7 @@
>>   #include "exec/address-spaces.h"
>>   #include "hw/boards.h"
>>   #include "hw/i386/sgx-epc.h"
>> +#include "sev_i386.h"
>>   #endif
>>     #include "disas/capstone.h"
>> @@ -5764,12 +5765,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;
Re: [PATCH v3 13/22] target/i386/sev: Remove stubs by using code elision
Posted by Brijesh Singh 4 years, 4 months ago 
On 10/6/21 11:55 AM, Philippe Mathieu-Daudé wrote:
> On 10/4/21 10:19, Paolo Bonzini wrote:
>> On 02/10/21 14:53, Philippe Mathieu-Daudé 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@redhat.com>
>>> ---
>>>   include/sysemu/sev.h    | 14 +++++++++++++-
>>>   target/i386/sev_i386.h  |  3 ---
>>>   target/i386/cpu.c       | 16 +++++++++-------
>>>   target/i386/sev-stub.c  | 36 ------------------------------------
>>>   target/i386/meson.build |  2 +-
>>>   5 files changed, 23 insertions(+), 48 deletions(-)
>>>   delete mode 100644 target/i386/sev-stub.c
>>>
>>> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
>>> index a329ed75c1c..f5c625bb3b3 100644
>>> --- a/include/sysemu/sev.h
>>> +++ b/include/sysemu/sev.h
>>> @@ -14,9 +14,21 @@
>>>   #ifndef QEMU_SEV_H
>>>   #define QEMU_SEV_H
>>>   -#include "sysemu/kvm.h"
>>> +#ifndef CONFIG_USER_ONLY
>>> +#include CONFIG_DEVICES /* CONFIG_SEV */
>>> +#endif
>>>   +#ifdef CONFIG_SEV
>>>   bool sev_enabled(void);
>>> +bool sev_es_enabled(void);
>>> +#else
>>> +#define sev_enabled() 0
>>> +#define sev_es_enabled() 0
>>> +#endif
>> This means that sev.h can only be included from target-specific files.
>>
>> An alternative could be:
>>
>> #ifdef NEED_CPU_H
>> # include CONFIG_DEVICES
> <command-line>: fatal error: x86_64-linux-user-config-devices.h: No such
> file or directory
>
>> #endif
>>
>> #if defined NEED_CPU_H && !defined CONFIG_SEV
>> # define sev_enabled() 0
>> # define sev_es_enabled() 0
>> #else
>> bool sev_enabled(void);
>> bool sev_es_enabled(void);
>> #endif
>>
>> ... but in fact sysemu/sev.h _is_ only used from x86-specific files. So
>> should it be moved to include/hw/i386, and even merged with
>> target/i386/sev_i386.h?  Do we need two files?
> No clue, I don't think we need. Brijesh?


Sorry for the late reply, we do not need two files and it can be easily
merged.

thanks

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.