* [PATCH for-6.0] qapi: qom: do not use target-specific conditionals
@ 2021-03-26 10:03 Paolo Bonzini
2021-03-26 10:57 ` Markus Armbruster
2021-03-26 13:45 ` Tom Lendacky
0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-03-26 10:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Tom Lendacky, armbru
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals
2021-03-26 10:03 [PATCH for-6.0] qapi: qom: do not use target-specific conditionals Paolo Bonzini
@ 2021-03-26 10:57 ` Markus Armbruster
2021-03-26 11:49 ` Paolo Bonzini
2021-03-26 13:45 ` Tom Lendacky
1 sibling, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2021-03-26 10:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Tom Lendacky, qemu-devel
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'.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals
2021-03-26 10:57 ` Markus Armbruster
@ 2021-03-26 11:49 ` Paolo Bonzini
2021-03-26 12:27 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2021-03-26 11:49 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Tom Lendacky, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals
2021-03-26 11:49 ` Paolo Bonzini
@ 2021-03-26 12:27 ` Markus Armbruster
2021-03-26 14:50 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2021-03-26 12:27 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Tom Lendacky, qemu-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals
2021-03-26 12:27 ` Markus Armbruster
@ 2021-03-26 14:50 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2021-03-26 14:50 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Tom Lendacky, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-6.0] qapi: qom: do not use target-specific conditionals
2021-03-26 10:03 [PATCH for-6.0] qapi: qom: do not use target-specific conditionals Paolo Bonzini
2021-03-26 10:57 ` Markus Armbruster
@ 2021-03-26 13:45 ` Tom Lendacky
1 sibling, 0 replies; 6+ messages in thread
From: Tom Lendacky @ 2021-03-26 13:45 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: armbru
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',
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-26 14:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-26 10:03 [PATCH for-6.0] qapi: qom: do not use target-specific conditionals Paolo Bonzini
2021-03-26 10:57 ` Markus Armbruster
2021-03-26 11:49 ` Paolo Bonzini
2021-03-26 12:27 ` Markus Armbruster
2021-03-26 14:50 ` Paolo Bonzini
2021-03-26 13:45 ` Tom Lendacky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).