Re: [Qemu-devel] [PATCH V20 6/8] Add support for cancelling of a TPM command

[Corey Bryant]

On 02/04/2013 01:48 PM, Stefan Berger wrote:
> On 02/04/2013 11:02 AM, Corey Bryant wrote:
> > @@ -221,7 +243,24 @@ static void
> > tpm_passthrough_deliver_request(TPMBackend *tb)
> > >   static void tpm_passthrough_cancel_cmd(TPMBackend *tb)
> > >   {
> > > -    /* cancelling an ongoing command is known not to work with some
> > > TPMs */
> > > +    TPMPassthruState *tpm_pt = tb->s.tpm_pt;
> > > +    int n;
> > > +
> > > +    /*
> > > +     * As of Linux 3.7 the tpm_tis driver does not properly cancel
> > > +     * commands for all TPMs.
> >
> > Any idea what the plan is for this issue?  Is it an ongoing matter of
> > adding kernel support as unsupported TPMs are identified?
>
>
> I submitted a patch to the tpmdd-devel mailing list fixing the
> cancellation issue with the particular TPM I have on one of my machines.
> Kent Yoder modified it to add a fix for yet another TPM (from a
> different vendor).
>
> https://github.com/shpedoikal/linux/commit/9f11986de7280f999cad342389b48c29002c0f37
>
>
> The spec seems not be clear enough as to what bits must be set in the
> status register when a TPM command is canceled so that the so far
> implemented cancellation detection is insufficient and needs to be
> adapted to TPM vendors' implementations. All TIS interfaces are supposed
> to support cancellation, though.

It sounds like this is a work in progress at the spec and Linux kernel 
level.  It's certainly not ideal, but I think as long as a message is 
issued for unsupported TPMs, it shouldn't hold up integration of this 
support into QEMU.

> > > +     * Only cancel if we're busy so we don't cancel someone else's
> > > +     * command, e.g., a command executed on the host.
> > > +     */
> > > +    if (tpm_pt->cancel_fd >= 0 && tpm_pt->tpm_executing) {
> > > +        n = write(tpm_pt->cancel_fd, "-", 1);
> > > +        if (n != 1) {
> > > +            error_report("Canceling TPM command failed: %s\n",
> > > +                         strerror(errno));
> > > +        } else {
> > > +            tpm_pt->tpm_op_canceled = true;
> > > +        }
> > > +    }
> >
> > Would an informational message make sense here for unsupported TPMs
> > (when tpm_pt->cancel_fd < 0)?
>
> Sure, I can add that.

Great, thanks.

> > > +        snprintf(path, sizeof(path), "/sys/class/misc/tpm%u/device",
> > > i);
> > > +        if (!tpm_passthrough_check_sysfs_cancel(path, sizeof(path))) {
> > > +            continue;
> > > +        }
> > > +        fd = open(path, O_WRONLY);
> > > +        break;
> > > +    }
> > > +
> > > +exit:
> > > +    if (fd >= 0) {
> > > +        tb->cancel_path = g_strdup(path);
> > > +    }
> > > +
> > > +    return fd;
> > > +}
> > > +
> >
> > A general question about the function above.  I see that
> > "/sys/class/misc/tpm%u/device" will be explained in kernel
> > documentation here:
> >
> > https://github.com/shpedoikal/linux/commit/4e21d66c9efbe740b5655bcf66e39873e354f8e9
> >
> >
> > And the following paths apparently have the same behavior.  But are
> > these paths defined somewhere else?
> > "/sys/devices/pnp%u/%s"
> > "/sys/devices/platform/tpm_tis"
> >
> > Also I think a comment pointing to documentation would be a
> > worth-while addition to the code.
>
> The /sys/class/misc/tpm%u/device path seems to be always there
> independent of what type of device the TPM is registered as (pnp or
> platform). So I'll modify above code to always use that path instead.

Great, that would simplify things a bit.

>    Stefan

--
Regards,
Corey Bryant