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

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.