On Wed, Jul 08, 2020 at 10:41:11 -0400, Stefan Berger wrote:
> On 7/8/20 10:22 AM, Peter Krempa wrote:
> > On Wed, Jul 08, 2020 at 10:19:11 -0400, Stefan Berger wrote:
> > > On 7/8/20 10:10 AM, Peter Krempa wrote:
> > > > On Wed, Jul 08, 2020 at 09:57:56 -0400, Stefan Berger wrote:
> > > > > From: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > > > >
> > > > > The firmware (SLOF) on QEMU for ppc64 does not support TPM 1.2, so
> > > > > prevent the choice of TPM 1.2 when the SPAPR device model is chosen
> > > > > and use a default of '2.0' (TPM 2) for the backend.
> > > > >
> > > > > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > > > > ---
> > > > > src/qemu/qemu_validate.c | 14 ++++++++++++--
> > > > > 1 file changed, 12 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> > > > > index bd7590a00a..f4d71aebf5 100644
> > > > > --- a/src/qemu/qemu_validate.c
> > > > > +++ b/src/qemu/qemu_validate.c
> > > > > @@ -3645,8 +3645,12 @@ qemuValidateDomainDeviceDefTPM(virDomainTPMDef *tpm,
> > > > > virQEMUCapsFlags flag;
> > > > > /* TPM 1.2 and 2 are not compatible, so we choose a specific version here */
> > > > > - if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT)
> > > > > - tpm->version = VIR_DOMAIN_TPM_VERSION_1_2;
> > > > > + if (tpm->version == VIR_DOMAIN_TPM_VERSION_DEFAULT) {
> > > > > + if (tpm->model == VIR_DOMAIN_TPM_MODEL_SPAPR)
> > > > > + tpm->version = VIR_DOMAIN_TPM_VERSION_2_0;
> > > > > + else
> > > > > + tpm->version = VIR_DOMAIN_TPM_VERSION_1_2;
> > > > > + }
> > > > This is the validation callback, which must not modify the config.
> > > >
> > > > qemuDomainDefTPMsPostParse is the correct place. Beaware that the
> > > > function has a short-circuit condition at the beginning.
> > >
> > > So should I remove the existing code that already set the version to
> > > VIR_DOMAIN_TPM_VERSION_1_2 and move this into that other function?
> > Ideally yes as a refactor prior to your change. The XML parser shouldn't
> > really be setting defaults or validating any form of interdependance of
> > values.
> >
> Well, the issue is that the validation function works on a single device
> (TPM) config but for validating a correct configuration for the SPAPR proxy
> device we would need to see both configs. So this is presumably the reason
> why this was done in the post parse function that has visibility into all
> TPM configs.
It is okay to do validation in PostParse, if it's being added as new
code, which never worked before.
The difference between PostParse and Validate is that PostParse is
called always after parsing an XML. This means even those meant for
internal use such as status XMLs. Failing to parse an existing one would
be bad as a VM would vanish after libvirtd restart.
The Validate callbacks are selectively used only in cases when a new XML
is used. This is e.g. when defining via the API or any other place where
we can return reasonable error to the user. In the Validate code we can
reject even previously accepted configs as there is no problem with
vanishing VMs.
Thus as per the above, you can leave the validation in the PostParse
callback, but you must not add any new one which was accepted previously
and there might be VMs running.
