Re: [PATCH 3/4] conf: add s390-pv as launch security type

On 5/20/21 4:05 AM, Boris Fiuczynski wrote: > On 5/19/21 9:34 PM, Daniel Henrique Barboza wrote: >> >> >> On 5/19/21 2:40 PM, Boris Fiuczynski wrote: >>> Add launch security type 's390-pv' as well as some tests. >>> >>> Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; >>> --- >>>   docs/schemas/domaincommon.rng                 |  1 + >>>   src/conf/domain_conf.c                        |  8 +++++ >>>   src/conf/domain_conf.h                        |  1 + >>>   src/qemu/qemu_command.c                       | 26 ++++++++++++++ >>>   src/qemu/qemu_namespace.c                     |  1 + >>>   src/qemu/qemu_process.c                       |  1 + >>>   src/qemu/qemu_validate.c                      |  8 +++++ >>>   .../launch-security-s390-pv-ignore-policy.xml | 24 +++++++++++++ >>>   .../launch-security-s390-pv.xml               | 18 ++++++++++ >>>   .../launch-security-s390-pv-ignore-policy.xml |  1 + >>>   tests/genericxml2xmltest.c                    |  2 ++ >>>   ...ty-s390-pv-ignore-policy.s390x-latest.args | 35 >>> +++++++++++++++++++ >>>   .../launch-security-s390-pv-ignore-policy.xml | 33 +++++++++++++++++ >>>   .../launch-security-s390-pv.s390x-latest.args | 35 >>> +++++++++++++++++++ >>>   .../launch-security-s390-pv.xml               | 30 ++++++++++++++++ >>>   tests/qemuxml2argvtest.c                      |  3 ++ >>>   16 files changed, 227 insertions(+) >>>   create mode 100644 >>> tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml >>>   create mode 100644 >>> tests/genericxml2xmlindata/launch-security-s390-pv.xml >>>   create mode 120000 >>> tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml >>>   create mode 100644 >>> tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args >>> >>>   create mode 100644 >>> tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml >>>   create mode 100644 >>> tests/qemuxml2argvdata/launch-security-s390-pv.s390x-latest.args >>>   create mode 100644 tests/qemuxml2argvdata/launch-security-s390-pv.xml >>> >>> diff --git a/docs/schemas/domaincommon.rng >>> b/docs/schemas/domaincommon.rng >>> index 3df13a0cf1..7c92e4c812 100644 >>> --- a/docs/schemas/domaincommon.rng >>> +++ b/docs/schemas/domaincommon.rng >>> @@ -485,6 +485,7 @@ >>>         <attribute name="type"> >>>           <choice> >>>             <value>sev</value> >>> +          <value>s390-pv</value> >>>           </choice> >>>         </attribute> >>>         <interleave> >> >> You added a new 's390-pv' security type, but down there you're using >> the new confidential-guest-support feature from QEMU 6.0 which is also >> valid for AMD and pSeries. I think you can do a little change in the >> idea >> of these patches while keeping most of it. Instead of calling this new >> support 's390-pv', call it 'confidential-guest-support' or 'CGS'. >> >> My reasoning is that the QEMU community (namely David Gibson, qemu-ppc >> maintainer) went into a lot of discussions back and forth to develop the >> confidential-guest-support machine option, based on what was at first >> AMD-SEV >> specific code, with the intention of make it easier for users to enable >> secure guests across machine types. I believe Libvirt should follow suit >> and do the same - a single option to enable secure guest supports for >> all guests, with any differences in the support being handled by each >> arch >> deep down in the driver. >> >> Otherwise, what will end up happening is that when someone (probably >> myself) >> come along with the secure guest support for pSeries (PEF), I will >> need to >> create yet another launch type 'ppc64-pef' to do basically the same >> thing you're >> already doing for s390x, which is adding '-machine >> confidential-guest-support=<>' >> in the QEMU command line. Same thing with AMD SEV, and with any other >> arch that QEMU might support with the confidential-guest-support >> option. We're >> going to add extra XML parsing code and docs to handle the same thing. >> >> Note that I'm not asking you to go ahead and implement the Libvirt >> support for >> all the 3 archs. What I'm asking is to change the name of the launch >> security >> type in the domain XML and docs to reflect that this will be the same >> type >> that all other archs that has confidential-guest-support will end up >> using. >> >> >> Thanks, >> >> >> Daniel > > Daniel, > thanks for your review and feedback. > When I looked at the QEMU commit 590466f056c4f > https://git.qemu.org/?p=qemu.git;a=commit;h=590466f056c4f2a7ff87ed751cece... > > I did not get the impression that there is a common type for > confidential guest support possible as the requiered data per type > differs. Also I got the impression that the different types are not > necessarily architecture bound. I may have gotten the wrong impression. The point I tried to make is that, although there are different types of secure VMs in QEMU, this doesn't mean that we need different launch security types in Libvirt since they're all tied in the same QEMU machine option. If you check the QEMU confidential-guest-support docs: https://gitlab.com/qemu-project/qemu/-/blob/master/docs/confidential-gues... ---- To run a confidential guest you need to add two command line parameters: 1. Use "-object" to create a "confidential guest support" object.  The    type and parameters will vary with the specific mechanism to be    used 2. Set the "confidential-guest-support" machine parameter to the ID of    the object from (1). ---- This is true for all 3 currently supported types. For s390x PV:     -object s390-pv-guest,id=pv0 \     -machine confidential-guest-support=pv0 \ AMD SEV:     -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1 \     -machine confidential-guest-support=sev0 \ PAPR PEF:     -object pef-guest,id=pef0 \     -machine confidential-guest-support=pef0 \

The difference between them, aside from the object types, is that AMD has configurable arguments to deal with. So my proposal is to, instead of creating a s390-pv launch type, you create a confidential-guest-support type:   <launchSecurity type='confidential-guest-support' /> This will translate, for s390x guests, into s390-pv. All validations and constraints specific to s390-pv applies. This same XML for a PAPR guest will trigger papr-pef code, and AMD-SEV code for AMD guests. AMD will probably want to add more arguments, so they would want to extend it like it is already done with their current 'sev' launcher:   <launchSecurity type='confidential-guest-support'>     <policy>0x0001</policy>     <cbitpos>47</cbitpos>     <reducedPhysBits>1</reducedPhysBits>     <dhCert>RBBBSDDD=FDDCCCDDDG</dhCert>     <session>AAACCCDD=FFFCCCDSDS</session>   </launchSecurity> The advantage is that the same launchSecurity type will enable confidential-guest-support for everyone, sparing a bit of XML parsing and docs we would have if each arch adds a new launch type that maps to its own secure VM technology. Note that you can (and should) clarify in the docs that the confidential-guest-support for s390x guests is the s390-pf technology and mention all applicable features/limitations of it. This also will make for an easier time for other Libvirt users that has to deal with multiple archs (not sure there are many, but ...) since the same option enables secure VM for all archs. I think you can argue that it's a matter of taste/design and that we should instead add a launcherSecurity type for each secure VM technology, regardless of how QEMU see all of them as a 'secure VM knob'. I would like to hear other opinions (specially from AMD) about what they feel about it. I'd rather have the same launcherSecurity type for everyone. Thanks, Daniel >