Re: [RFC PATCH 3/7] conf: introduce TrustDomain element in domain

> -----Original Message----- > From: Pavel Hrdina <phrdina(a)redhat.com&gt; > Sent: Friday, June 18, 2021 8:09 PM > To: Duan, Zhenzhong <zhenzhong.duan(a)intel.com&gt; > Cc: libvir-list(a)redhat.com; Yamahata, Isaku <isaku.yamahata(a)intel.com>; > Tian, Jun J <jun.j.tian(a)intel.com>; Qiang, Chenyi <chenyi.qiang(a)intel.com&gt; > 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 +-

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.