On Fri, Jul 02, 2021 at 03:46:43PM +0200, Boris Fiuczynski wrote:
> On 6/25/21 12:00 PM, Pavel Hrdina wrote:
> > On Tue, Jun 22, 2021 at 03:10:48PM +0200, Boris Fiuczynski wrote:
> > > Add launch security type 's390-pv' as well as some tests.
> > >
> > > Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> > > Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>
> > > ---
> > > 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_firmware.c | 1 +
> > > src/qemu/qemu_namespace.c | 1 +
> > > src/qemu/qemu_process.c | 1 +
> > > src/qemu/qemu_validate.c | 9 +++++
> > > .../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 ++
> > > 17 files changed, 229 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
> > >
>
> <snip>
>
> > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > > index 4135a8444a..3ab803f7ce 100644
> > > --- a/src/qemu/qemu_command.c
> > > +++ b/src/qemu/qemu_command.c
> > > @@ -6975,6 +6975,9 @@ qemuBuildMachineCommandLine(virCommand *cmd,
> > > virBufferAddLit(&buf, ",memory-encryption=sev0");
> > > }
> > > break;
> > > + case VIR_DOMAIN_LAUNCH_SECURITY_PV:
> > > + virBufferAddLit(&buf, ",confidential-guest-support=pv0");
> > > + break;
> >
> > This could be possible shared for all launchSecurity types as well but
> > it can be done as followup. That would mean using for example lsec0
> > instead of sev0, pv0, somethingelse0 and so on. It's just an id which
> > can be anything.
> >
>
> I will add an follow-up patch which changes the id to a common id 'lsec0'.
>
>
> > > case VIR_DOMAIN_LAUNCH_SECURITY_NONE:
> > > break;
> > > case VIR_DOMAIN_LAUNCH_SECURITY_LAST:
>
> <snip/>
>
> > > diff --git a/tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml b/tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml
> > > new file mode 100644
> > > index 0000000000..0c398cced8
> > > --- /dev/null
> > > +++ b/tests/genericxml2xmlindata/launch-security-s390-pv-ignore-policy.xml
> > > @@ -0,0 +1,24 @@
> > > +<domain type='kvm'>
> > > + <name>QEMUGuest1</name>
> > > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > > + <memory unit='KiB'>219100</memory>
> > > + <currentMemory unit='KiB'>219100</currentMemory>
> > > + <vcpu placement='static'>1</vcpu>
> > > + <os>
> > > + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
> > > + <boot dev='hd'/>
> > > + </os>
> > > + <clock offset='utc'/>
> > > + <on_poweroff>destroy</on_poweroff>
> > > + <on_reboot>restart</on_reboot>
> > > + <on_crash>destroy</on_crash>
> > > + <devices>
> > > + </devices>
> > > + <launchSecurity type='s390-pv'>
> > > + <cbitpos>47</cbitpos>
> > > + <reducedPhysBits>1</reducedPhysBits>
> > > + <policy>0x0001</policy>
> > > + <dhCert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dhCert>
> > > + <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session>
> >
> > I thing we should not ignore invalid XML bits and error out instead.
>
>
> This would result in s390-pv checking for the existence of any child
> elements and if there is a child fail.
> If other launchSecurity types come along with new or shared child elements
> checking for SEV and the new types would have to added/changed.
> Wouldn't that get messy quickly?
No necessarily, one approach that we use is to do a while() loop over
all elements, match only known elements and for unknown print error.
For each launchSecurity type we will have dedicated parser function
where we would do the while loop where we would error out for any
unknown sub-element. In case of s390-pv-guest we would simply error
out if there is any sub-element.
Looking at the current code we don't use this approach for SEV so keep
the code as it is. In this XML-TO-XML test it makes sense to validate
that the unknown sub-elements are ignored, but ... [1]
> >
> > > + </launchSecurity>
> > > +</domain>
> > > diff --git a/tests/genericxml2xmlindata/launch-security-s390-pv.xml b/tests/genericxml2xmlindata/launch-security-s390-pv.xml
> > > new file mode 100644
> > > index 0000000000..29c7fc152d
> > > --- /dev/null
> > > +++ b/tests/genericxml2xmlindata/launch-security-s390-pv.xml
> > > @@ -0,0 +1,18 @@
> > > +<domain type='kvm'>
> > > + <name>QEMUGuest1</name>
> > > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > > + <memory unit='KiB'>219100</memory>
> > > + <currentMemory unit='KiB'>219100</currentMemory>
> > > + <vcpu placement='static'>1</vcpu>
> > > + <os>
> > > + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
> > > + <boot dev='hd'/>
> > > + </os>
> > > + <clock offset='utc'/>
> > > + <on_poweroff>destroy</on_poweroff>
> > > + <on_reboot>restart</on_reboot>
> > > + <on_crash>destroy</on_crash>
> > > + <devices>
> > > + </devices>
> > > + <launchSecurity type='s390-pv'/>
> > > +</domain>
> > > diff --git a/tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml b/tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml
> > > new file mode 120000
> > > index 0000000000..075c72603d
> > > --- /dev/null
> > > +++ b/tests/genericxml2xmloutdata/launch-security-s390-pv-ignore-policy.xml
> > > @@ -0,0 +1 @@
> > > +../genericxml2xmlindata/launch-security-s390-pv.xml
> > > \ No newline at end of file
> > > diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c
> > > index ac89422a32..eb15f66c3c 100644
> > > --- a/tests/genericxml2xmltest.c
> > > +++ b/tests/genericxml2xmltest.c
> > > @@ -233,6 +233,8 @@ mymain(void)
> > > DO_TEST("tseg");
> > > DO_TEST("launch-security-sev");
> > > + DO_TEST("launch-security-s390-pv");
> > > + DO_TEST_DIFFERENT("launch-security-s390-pv-ignore-policy");
> > > DO_TEST_DIFFERENT("cputune");
> > > DO_TEST("device-backenddomain");
> > > diff --git a/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args b/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args
> > > new file mode 100644
> > > index 0000000000..c9d9b84dd3
> > > --- /dev/null
> > > +++ b/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.s390x-latest.args
> > > @@ -0,0 +1,35 @@
> > > +LC_ALL=C \
> > > +PATH=/bin \
> > > +HOME=/tmp/lib/domain--1-QEMUGuest1 \
> > > +USER=test \
> > > +LOGNAME=test \
> > > +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
> > > +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
> > > +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
> > > +/usr/bin/qemu-system-s390x \
> > > +-name guest=QEMUGuest1,debug-threads=on \
> > > +-S \
> > > +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/tmp/lib/domain--1-QEMUGuest1/master-key.aes"}' \
> > > +-machine s390-ccw-virtio,accel=kvm,usb=off,dump-guest-core=off,confidential-guest-support=pv0,memory-backend=s390.ram \
> > > +-cpu gen15a-base,aen=on,cmmnt=on,vxpdeh=on,aefsi=on,diag318=on,csske=on,mepoch=on,msa9=on,msa8=on,msa7=on,msa6=on,msa5=on,msa4=on,msa3=on,msa2=on,msa1=on,sthyi=on,edat=on,ri=on,deflate=on,edat2=on,etoken=on,vx=on,ipter=on,mepochptff=on,ap=on,vxeh=on,vxpd=on,esop=on,msa9_pckmo=on,vxeh2=on,esort=on,apqi=on,apft=on,els=on,iep=on,apqci=on,cte=on,ais=on,bpb=on,gs=on,ppa15=on,zpci=on,sea_esop2=on,te=on,cmm=on \
> > > +-m 214 \
> > > +-object '{"qom-type":"memory-backend-ram","id":"s390.ram","size":224395264}' \
> > > +-overcommit mem-lock=off \
> > > +-smp 1,sockets=1,cores=1,threads=1 \
> > > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> > > +-display none \
> > > +-no-user-config \
> > > +-nodefaults \
> > > +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
> > > +-mon chardev=charmonitor,id=monitor,mode=control \
> > > +-rtc base=utc \
> > > +-no-shutdown \
> > > +-boot strict=on \
> > > +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
> > > +-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"raw","file":"libvirt-1-storage"}' \
> > > +-device virtio-blk-ccw,devno=fe.0.0000,drive=libvirt-1-format,id=virtio-disk0,bootindex=1 \
> > > +-audiodev id=audio1,driver=none \
> > > +-device virtio-balloon-ccw,id=balloon0,devno=fe.0.0001 \
> > > +-object '{"qom-type":"s390-pv-guest","id":"pv0"}' \
> > > +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
> > > +-msg timestamp=on
> > > diff --git a/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml b/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml
> > > new file mode 100644
> > > index 0000000000..052d96dedb
> > > --- /dev/null
> > > +++ b/tests/qemuxml2argvdata/launch-security-s390-pv-ignore-policy.xml
> > > @@ -0,0 +1,33 @@
> > > +<domain type='kvm'>
> > > + <name>QEMUGuest1</name>
> > > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > > + <memory unit='KiB'>219100</memory>
> > > + <currentMemory unit='KiB'>219100</currentMemory>
> > > + <vcpu placement='static'>1</vcpu>
> > > + <os>
> > > + <type arch='s390x' machine='s390-ccw-virtio'>hvm</type>
> > > + <boot dev='hd'/>
> > > + </os>
> > > + <clock offset='utc'/>
> > > + <on_poweroff>destroy</on_poweroff>
> > > + <on_reboot>restart</on_reboot>
> > > + <on_crash>destroy</on_crash>
> > > + <devices>
> > > + <emulator>/usr/bin/qemu-system-s390x</emulator>
> > > + <disk type='block' device='disk'>
> > > + <driver name='qemu' type='raw'/>
> > > + <source dev='/dev/HostVG/QEMUGuest1'/>
> > > + <target dev='hda' bus='virtio'/>
> > > + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/>
> > > + </disk>
> > > + <controller type='pci' index='0' model='pci-root'/>
> > > + <memballoon model='virtio'>
> > > + <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0001'/>
> > > + </memballoon>
> > > + <panic model='s390'/>
> > > + </devices>
> > > + <launchSecurity type='s390-pv'>
> > > + <dhCert>AQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAAAQAAAAAOAAA</dhCert>
> > > + <session>IHAVENOIDEABUTJUSTPROVIDINGASTRING</session>
> >
> > This doesn't look correct, we should not format dhCert or session with
> > s390-pv because based on the patches they are not used at all.
>
> Isn't this the same issue as before about unsued elements being ignored?
[1] ... here it is useless as it is already tested by XML-TO-XML, so the
ignore variant can be dropped from XML-TO-ARGV.
Pavel
Attachment:
signature.asc
Description: PGP signature
