Re: [PATCH v5 1/5] tpm_spapr: Support TPM for ppc64 using CRQ based interface

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3604 bytes --]

On Tue, Dec 17, 2019 at 02:44:04PM -0500, Stefan Berger wrote:
> On 12/16/19 7:29 PM, David Gibson wrote:
> > On Fri, Dec 13, 2019 at 08:03:36AM -0500, Stefan Berger wrote:
> > > On 12/13/19 12:34 AM, David Gibson wrote:
> > > 
> > > The existing one looks like this:
> > > 
> > > typedef struct SpaprVioCrq {
> > >      uint64_t qladdr;
> > >      uint32_t qsize;
> > >      uint32_t qnext;
> > >      int(*SendFunc)(struct SpaprVioDevice *vdev, uint8_t *crq);
> > > } SpaprVioCrq;
> > > 
> > > I don't seem to find the fields there that we need for vTPM support.
> > Yeah, I can see the difference in the structures.  What I'm after is
> > what is the difference in purpose which means they have different
> > content.
> > 
> > Having read through the whole series now, I *think* the answer is that
> > the tpm specific structure is one entry in the request queue for the
> > vtpm, whereas the VioCrq structure is a handle on an entire queue.
> > 
> > I think the tpm one needs a rename to reflect that a) it's vtpm
> > specific and b) it's not actually a queue, just part of it.
> 
> 
> v6 has it as TpmCrq. It's local to the file, so from that perspective it's
> specific to (v)TPM.

Ok.

> > > This is a 1:1 copy from the existing TIS driver.
> > Hm, right.  Probably not a bad idea to move that out as a helper
> > function then.
> 
> 
> In V7 then.

Ok.

> > > > > +static void tpm_spapr_update_deviceclass(SpaprVioDevice *dev)
> > > > > +{
> > > > > +    SPAPRvTPMState *s = VIO_SPAPR_VTPM(dev);
> > > > > +    SpaprVioDeviceClass *k = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> > > > > +
> > > > > +    switch (s->be_tpm_version) {
> > > > > +    case TPM_VERSION_UNSPEC:
> > > > > +        assert(false);
> > > > > +        break;
> > > > > +    case TPM_VERSION_1_2:
> > > > > +        k->dt_name = "vtpm";
> > > > > +        k->dt_type = "IBM,vtpm";
> > > > > +        k->dt_compatible = "IBM,vtpm";
> > > > > +        break;
> > > > > +    case TPM_VERSION_2_0:
> > > > > +        k->dt_name = "vtpm";
> > > > > +        k->dt_type = "IBM,vtpm";
> > > > > +        k->dt_compatible = "IBM,vtpm20";
> > > > > +        break;
> > > > Erk.  Updating DeviceClass structures on the fly is hideously ugly.
> > > > We might need to take a different approach for this.
> > > Make a suggestion... Obviously, we can hard-initialize dt_name and dt_type
> > > but dt_compatible can only be set after we have determined the version of
> > > TPM.
> > As you say name and type can just be put into the class statically.
> 
> 
> I did this in v6.
> 
> 
> > Since you need to change compatible based on an internal variable,
> > we'd need to replace the static dt_compatible in the class with a
> > callback.
> 
> 
> Why can we not initialize it once we know the version of TPM? From the
> perspective of SLOF at least this seems to be building the device tree fine
> since it sees the proper value...

Because it's a serious layering / isolation violation.  You're
modifying QOM type information from the runtime code of a specific
instance.  You get away with it (now) because there's only one
instance and the ordering of things happens to let it work, but that's
assuming way too much about QOM's implementation details.

As a rule, once the QOM classes are set up with their class_init
function, they should never be modified.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]