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

[Stefan Berger]

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.


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


> > > > +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...


   Stefan