On Mon, Jun 21, 2021 at 04:14:40AM +0000, Duan, Zhenzhong wrote:
-----Original Message----- From: Pavel Hrdina <phrdina@redhat.com> Sent: Friday, June 18, 2021 8:09 PM To: Duan, Zhenzhong <zhenzhong.duan@intel.com> Cc: libvir-list@redhat.com; Yamahata, Isaku <isaku.yamahata@intel.com>; Tian, Jun J <jun.j.tian@intel.com>; Qiang, Chenyi <chenyi.qiang@intel.com> Subject: Re: [RFC PATCH 3/7] conf: introduce TrustDomain element in domain
On Fri, Jun 18, 2021 at 04:50:48PM +0800, Zhenzhong Duan wrote:
The TrustDomain element can be used to define the security model to use when launching a domain. Only type 'tdx' is supported currently.
When 'tdx' is used, the VM will launched with Intel TDX feature enabled. TDX feature supports running encrypted VM (Trust Domain, TD) under the control of KVM. A TD runs in a CPU model which protects the confidentiality of its memory and its CPU state from other software
There is a child element 'policy' in TrustDomain. In 'policy', bit 0 is used to enable TDX debug, other bits are reserved currently.
For example:
<TrustDomain type='tdx'> <policy>0x0001</policy> </TrustDomain>
Any reason why you are adding a new element that basically copies exactly what <launchSecurity> is doing?
In libvirt it will essentially use `confidential-guest-support` which is used for launchSecurity so there is no need to duplicate the code and the XML element.
It could look like this:
<launchSecurity type='tdx'> <policy>0x0001</policy> </launchSecurity>
We would have to reorganize the <launchSecurity> documentation a little bit but otherwise there is nothing blocking us to have single element to specify different type of encrypted/confidential/secure/... VMs.
We had ever made a patch working as you suggested. It has advantage of only using one element for all. The main reason I chose the other way is because this way having quite less code change, as it avoid many mux and branch code. See below:
Using < launchSecurity>:
docs/schemas/domaincommon.rng | 90 +++++---- src/conf/domain_conf.c | 182 +++++++++++++++--- src/conf/domain_conf.h | 19 +- src/conf/virconftypes.h | 6 + src/qemu/qemu_cgroup.c | 3 +- src/qemu/qemu_command.c | 5 +- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_firmware.c | 5 +- src/qemu/qemu_namespace.c | 4 +- src/qemu/qemu_process.c | 12 +- src/qemu/qemu_validate.c | 30 ++- src/security/security_dac.c | 4 +-
This is not an issue and most likely some of these changes would be required with new <TrustDomain> element as well so we would definitely prefer reusing <launchSecurity> element.
vs. Using <TrustDomain>:
docs/schemas/domaincommon.rng | 16 ++++ src/conf/domain_conf.c | 84 +++++++++++++++++++ src/conf/domain_conf.h | 15 ++++ src/conf/virconftypes.h | 2 + src/qemu/qemu_validate.c | 8 ++
I'm also irresolute on which to choose, I'll use <launchSecurity> if you think it's better.
We already have patches for similar feature for s390 which will also reuse this element and in the future any other CPU architecture can reuse it.
Hmm, I didn’t find those patches for s390, aren't they upstream yet? Appreciate if you could show me a link for reference.
v1 series: https://listman.redhat.com/archives/libvir-list/2021-May/msg00570.html v2 series: https://listman.redhat.com/archives/libvir-list/2021-June/msg00583.html Pavel