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

[Duan, Zhenzhong]

> -----Original Message-----
> From: Peter Krempa <pkrempa at redhat.com>
> Sent: Friday, June 18, 2021 7:22 PM
> To: Duan, Zhenzhong <zhenzhong.duan at intel.com>
> Cc: libvir-list at redhat.com; Yamahata, Isaku <isaku.yamahata at intel.com>;
> Tian, Jun J <jun.j.tian at intel.com>; Qiang, Chenyi <chenyi.qiang at intel.com>
> 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 at intel.com>
> > ---
> >  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.
Thanks for your suggestion, we will drop this chunk.

> 
> Also it feels very unjustified, it's bad for performance, but is it guaranteed to
> always be like this?
No, we are optimizing the performance continuously, I believe the performance
will be better and better.

> 
> >
> >      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.
Will fix.

Thanks
Zhenzhong