qapi/qom.json | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
ObjectType and ObjectOptions are defined in a target-independent file,
therefore they do not have access to target-specific configuration
symbols such as CONFIG_PSERIES or CONFIG_SEV. For this reason,
pef-guest and sev-guest are currently omitted when compiling the
generated QAPI files. In addition, this causes ObjectType to have
different definitions depending on the file that is including
qapi-types-qom.h (currently this is not causing any issues, but it
is wrong).
Define the two enum entries and the SevGuestProperties type
unconditionally to avoid the issue. We do not expect to have
many target-dependent user-creatable classes, so it is not
particularly problematic.
Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qapi/qom.json | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/qapi/qom.json b/qapi/qom.json
index 2056edc072..db5ac419b1 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -733,8 +733,7 @@
'*policy': 'uint32',
'*handle': 'uint32',
'*cbitpos': 'uint32',
- 'reduced-phys-bits': 'uint32' },
- 'if': 'defined(CONFIG_SEV)' }
+ 'reduced-phys-bits': 'uint32' } }
##
# @ObjectType:
@@ -768,14 +767,14 @@
{ 'name': 'memory-backend-memfd',
'if': 'defined(CONFIG_LINUX)' },
'memory-backend-ram',
- {'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
+ 'pef-guest',
'pr-manager-helper',
'rng-builtin',
'rng-egd',
'rng-random',
'secret',
'secret_keyring',
- {'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
+ 'sev-guest',
's390-pv-guest',
'throttle-group',
'tls-creds-anon',
@@ -831,8 +830,7 @@
'rng-random': 'RngRandomProperties',
'secret': 'SecretProperties',
'secret_keyring': 'SecretKeyringProperties',
- 'sev-guest': { 'type': 'SevGuestProperties',
- 'if': 'defined(CONFIG_SEV)' },
+ 'sev-guest': 'SevGuestProperties',
'throttle-group': 'ThrottleGroupProperties',
'tls-creds-anon': 'TlsCredsAnonProperties',
'tls-creds-psk': 'TlsCredsPskProperties',
--
2.26.2
Paolo Bonzini <pbonzini@redhat.com> writes:
> ObjectType and ObjectOptions are defined in a target-independent file,
> therefore they do not have access to target-specific configuration
> symbols such as CONFIG_PSERIES or CONFIG_SEV. For this reason,
> pef-guest and sev-guest are currently omitted when compiling the
> generated QAPI files. In addition, this causes ObjectType to have
> different definitions depending on the file that is including
> qapi-types-qom.h (currently this is not causing any issues, but it
> is wrong).
>
> Define the two enum entries and the SevGuestProperties type
> unconditionally to avoid the issue. We do not expect to have
> many target-dependent user-creatable classes, so it is not
> particularly problematic.
>
> Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qapi/qom.json | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 2056edc072..db5ac419b1 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -733,8 +733,7 @@
> '*policy': 'uint32',
> '*handle': 'uint32',
> '*cbitpos': 'uint32',
> - 'reduced-phys-bits': 'uint32' },
> - 'if': 'defined(CONFIG_SEV)' }
> + 'reduced-phys-bits': 'uint32' } }
>
> ##
> # @ObjectType:
> @@ -768,14 +767,14 @@
> { 'name': 'memory-backend-memfd',
> 'if': 'defined(CONFIG_LINUX)' },
> 'memory-backend-ram',
> - {'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
> + 'pef-guest',
> 'pr-manager-helper',
> 'rng-builtin',
> 'rng-egd',
> 'rng-random',
> 'secret',
> 'secret_keyring',
> - {'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
> + 'sev-guest',
> 's390-pv-guest',
> 'throttle-group',
> 'tls-creds-anon',
> @@ -831,8 +830,7 @@
> 'rng-random': 'RngRandomProperties',
> 'secret': 'SecretProperties',
> 'secret_keyring': 'SecretKeyringProperties',
> - 'sev-guest': { 'type': 'SevGuestProperties',
> - 'if': 'defined(CONFIG_SEV)' },
> + 'sev-guest': 'SevGuestProperties',
> 'throttle-group': 'ThrottleGroupProperties',
> 'tls-creds-anon': 'TlsCredsAnonProperties',
> 'tls-creds-psk': 'TlsCredsPskProperties',
No branch for tag value 'pef-guest', i.e. no tag-specific members.
There are two more: can_bus, s390_pv_guest. I assume this is
intentional.
Links a bit of dead code into the other qemu-system-FOO, but that's
okay.
If we genuinely needed (or wanted) target-dependent -object, we'd split
qom-target.json off qom.json, and put the target-dependent parts there,
including the enum and the union, with the obvious ripple effects. Not
now, maybe not ever.
Would adding "only for CONFIG_MUMBLE" to the doc comments make sense?
It's what we did before we had 'if'.
On 26/03/21 11:57, Markus Armbruster wrote:
>> 'rng-random': 'RngRandomProperties',
>> 'secret': 'SecretProperties',
>> 'secret_keyring': 'SecretKeyringProperties',
>> - 'sev-guest': { 'type': 'SevGuestProperties',
>> - 'if': 'defined(CONFIG_SEV)' },
>> + 'sev-guest': 'SevGuestProperties',
>> 'throttle-group': 'ThrottleGroupProperties',
>> 'tls-creds-anon': 'TlsCredsAnonProperties',
>> 'tls-creds-psk': 'TlsCredsPskProperties',
>
> No branch for tag value 'pef-guest', i.e. no tag-specific members.
> There are two more: can_bus, s390_pv_guest. I assume this is
> intentional.
Yes, they have no properties.
> Links a bit of dead code into the other qemu-system-FOO, but that's
> okay.
>
> If we genuinely needed (or wanted) target-dependent -object, we'd split
> qom-target.json off qom.json, and put the target-dependent parts there,
> including the enum and the union, with the obvious ripple effects. Not
> now, maybe not ever.
>
> Would adding "only for CONFIG_MUMBLE" to the doc comments make sense?
> It's what we did before we had 'if'.
In this specific case we had not documentation at all for objects. We
can add the information on relevant targets in the documentation for the
*Properties types.
Paolo
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 26/03/21 11:57, Markus Armbruster wrote:
>>> 'rng-random': 'RngRandomProperties',
>>> 'secret': 'SecretProperties',
>>> 'secret_keyring': 'SecretKeyringProperties',
>>> - 'sev-guest': { 'type': 'SevGuestProperties',
>>> - 'if': 'defined(CONFIG_SEV)' },
>>> + 'sev-guest': 'SevGuestProperties',
>>> 'throttle-group': 'ThrottleGroupProperties',
>>> 'tls-creds-anon': 'TlsCredsAnonProperties',
>>> 'tls-creds-psk': 'TlsCredsPskProperties',
>> No branch for tag value 'pef-guest', i.e. no tag-specific members.
>> There are two more: can_bus, s390_pv_guest. I assume this is
>> intentional.
>
> Yes, they have no properties.
>
>> Links a bit of dead code into the other qemu-system-FOO, but that's
>> okay.
>> If we genuinely needed (or wanted) target-dependent -object, we'd
>> split
>> qom-target.json off qom.json, and put the target-dependent parts there,
>> including the enum and the union, with the obvious ripple effects. Not
>> now, maybe not ever.
>> Would adding "only for CONFIG_MUMBLE" to the doc comments make
>> sense?
>> It's what we did before we had 'if'.
>
> In this specific case we had not documentation at all for objects.
... until Kevin added some when he QAPIfied. Looks like this (copied
from qemu-qmp-ref.7)[*]:
SevGuestProperties (Object)
Properties for sev-guest objects.
Members
sev-device: string (optional)
SEV device to use (default: "/dev/sev")
dh-cert-file: string (optional)
guest owners DH certificate (encoded with base64)
session-file: string (optional)
guest owners session parameters (encoded with base64)
policy: int (optional)
SEV policy value (default: 0x1)
handle: int (optional)
SEV firmware handle (default: 0)
cbitpos: int (optional)
C-bit location in page table entry (default: 0)
reduced-phys-bits: int
number of bits in physical addresses that become unavailable
when SEV is enabled
Since
2.12
If
defined(CONFIG_SEV)
Your patch drops the last three lines, without a replacement.
> We
> can add the information on relevant targets in the documentation for
> the *Properties types.
Yes, please.
Preferably with that squashed in:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[*] I lied. It actually looks like
If
defined(CONFIG_SEV).SS ObjectType (Enum)
The running together of "defined(CONFIG_SEV)" and ".SS ObjectType
(Enum)" is a bug. I'll investigate.
On 26/03/21 13:27, Markus Armbruster wrote: > ... until Kevin added some when he QAPIfied. Looks like this (copied > from qemu-qmp-ref.7)[*]: > > SevGuestProperties (Object) > Properties for sev-guest objects. > > Members > sev-device: string (optional) > SEV device to use (default: "/dev/sev") > > dh-cert-file: string (optional) > guest owners DH certificate (encoded with base64) > > session-file: string (optional) > guest owners session parameters (encoded with base64) > > policy: int (optional) > SEV policy value (default: 0x1) > > handle: int (optional) > SEV firmware handle (default: 0) > > cbitpos: int (optional) > C-bit location in page table entry (default: 0) > > reduced-phys-bits: int > number of bits in physical addresses that become unavailable > when SEV is enabled > > Since > 2.12 > > If > defined(CONFIG_SEV) > > Your patch drops the last three lines, without a replacement. Yes, I mean the regression is not from 5.2. Paolo
On 3/26/21 5:03 AM, Paolo Bonzini wrote:
> ObjectType and ObjectOptions are defined in a target-independent file,
> therefore they do not have access to target-specific configuration
> symbols such as CONFIG_PSERIES or CONFIG_SEV. For this reason,
> pef-guest and sev-guest are currently omitted when compiling the
> generated QAPI files. In addition, this causes ObjectType to have
> different definitions depending on the file that is including
> qapi-types-qom.h (currently this is not causing any issues, but it
> is wrong).
>
> Define the two enum entries and the SevGuestProperties type
> unconditionally to avoid the issue. We do not expect to have
> many target-dependent user-creatable classes, so it is not
> particularly problematic.
>
> Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
I'm once again able to launch SEV guests.
Tested-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> qapi/qom.json | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 2056edc072..db5ac419b1 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -733,8 +733,7 @@
> '*policy': 'uint32',
> '*handle': 'uint32',
> '*cbitpos': 'uint32',
> - 'reduced-phys-bits': 'uint32' },
> - 'if': 'defined(CONFIG_SEV)' }
> + 'reduced-phys-bits': 'uint32' } }
>
> ##
> # @ObjectType:
> @@ -768,14 +767,14 @@
> { 'name': 'memory-backend-memfd',
> 'if': 'defined(CONFIG_LINUX)' },
> 'memory-backend-ram',
> - {'name': 'pef-guest', 'if': 'defined(CONFIG_PSERIES)' },
> + 'pef-guest',
> 'pr-manager-helper',
> 'rng-builtin',
> 'rng-egd',
> 'rng-random',
> 'secret',
> 'secret_keyring',
> - {'name': 'sev-guest', 'if': 'defined(CONFIG_SEV)' },
> + 'sev-guest',
> 's390-pv-guest',
> 'throttle-group',
> 'tls-creds-anon',
> @@ -831,8 +830,7 @@
> 'rng-random': 'RngRandomProperties',
> 'secret': 'SecretProperties',
> 'secret_keyring': 'SecretKeyringProperties',
> - 'sev-guest': { 'type': 'SevGuestProperties',
> - 'if': 'defined(CONFIG_SEV)' },
> + 'sev-guest': 'SevGuestProperties',
> 'throttle-group': 'ThrottleGroupProperties',
> 'tls-creds-anon': 'TlsCredsAnonProperties',
> 'tls-creds-psk': 'TlsCredsPskProperties',
>
© 2016 - 2024 Red Hat, Inc.