RE: [RFC PATCH 6/7] qemu: force special features enabled for TDX guest

-----Original Message----- From: Peter Krempa <pkrempa(a)redhat.com&gt; Sent: Friday, June 18, 2021 7:22 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 6/7] qemu: force special features enabled for TDX guest On Fri, Jun 18, 2021 at 16:50:51 +0800, Zhenzhong Duan wrote: > TDX guest requires some special parameters in qemu command line. > They are "pic=no,kernel_irqchip=split" without which guest fails to > bootup. > > PMU has a big impact to the performance of TDX guest. So always > disable PMU except it's forcely enabled. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan(a)intel.com&gt; > --- > src/qemu/qemu_command.c | 6 +++++- > src/qemu/qemu_validate.c | 6 ++++++ > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index > 891d795b02..bffa3fdf10 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -6599,6 +6599,10 @@ qemuBuildCpuCommandLine(virCommand *cmd, > virTristateSwitch pmu = def->features[VIR_DOMAIN_FEATURE_PMU]; > virBufferAsprintf(&buf, ",pmu=%s", > virTristateSwitchTypeToString(pmu)); > + } else if (!def->features[VIR_DOMAIN_FEATURE_PMU] && def->tdx) { > + /* PMU lead to performance drop if TDX enabled, disable PMU by default */ > + virBufferAsprintf(&buf, ",pmu=%s", > + > + virTristateSwitchTypeToString(VIR_TRISTATE_SWITCH_OFF)); > } This must be done in a way that will be visible in the XML rather than silently hiding it in the command line generator code.

Also it feels very unjustified, it's bad for performance, but is it guaranteed to always be like this?

> > if (def->cpu && def->cpu->cache) { [...] > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index > 2efd011cc0..3c3a00c7e8 100644 > --- a/src/qemu/qemu_validate.c > +++ b/src/qemu/qemu_validate.c > @@ -202,6 +202,12 @@ qemuValidateDomainDefFeatures(const virDomainDef *def, > return -1; > } > } > + if (def->tdx && (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP) > + || def->features[i] != > + VIR_DOMAIN_IOAPIC_QEMU)) { Our coding style usually has boolean operators at the end of the previous line in multi-line statements.