From: Benjamin Herrenschmidt <benh@kernel.crashing.org> To: Kent Yoder <key@linux.vnet.ibm.com> Cc: Ashley Lai <adlai@linux.vnet.ibm.com>, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, linuxppc-dev@lists.ozlabs.org, rcj@linux.vnet.ibm.com, adlai@us.ibm.com Subject: Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM Date: Wed, 05 Sep 2012 13:40:07 +1000 [thread overview] Message-ID: <1346816407.2257.37.camel@pasglop> (raw) In-Reply-To: <20120822214217.GB13519@linux.vnet.ibm.com> On Wed, 2012-08-22 at 16:42 -0500, Kent Yoder wrote: > On Wed, Aug 22, 2012 at 04:17:43PM -0500, Ashley Lai wrote: > > This patch adds a new device driver to support IBM virtual TPM > > (vTPM) for PPC64. IBM vTPM is supported through the adjunct > > partition with firmware release 740 or higher. With vTPM > > support, each lpar is able to have its own vTPM without the > > physical TPM hardware. > > > > This driver provides TPM functionalities by communicating with > > the vTPM adjunct partition through Hypervisor calls (Hcalls) > > and Command/Response Queue (CRQ) commands. > > Thanks Ashley, I'll include this in my next pull request to James. Oh ? I was about to put it in the powerpc tree ... But yeah, I notice there's a change to tpm.h so it probably should be at least acked by the TPM folks. As for the subsequent patches, they are powerpc specific so I can add them, I don't think there's a strict dependency, is there ? Now, while having a look at the driver, I found a few things that I'm not sure I'm happy with: Mainly: > > + */ > > +static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count) > > +{ > > + struct ibmvtpm_dev *ibmvtpm; > > + u16 len; > > + > > + ibmvtpm = (struct ibmvtpm_dev *)chip->vendor.data; > > + > > + if (!ibmvtpm->rtce_buf) { > > + dev_err(ibmvtpm->dev, "ibmvtpm device is not ready\n"); > > + return 0; > > + } > > + > > + wait_event_interruptible(wq, ibmvtpm->crq_res.len != 0); > > + > > + if (count < ibmvtpm->crq_res.len) { That doesn't look right. The "other side" as far as I can tell is: > > + case VTPM_TPM_COMMAND_RES: > > + ibmvtpm->crq_res.valid = crq->valid; > > + ibmvtpm->crq_res.msg = crq->msg; > > + ibmvtpm->crq_res.len = crq->len; > > + ibmvtpm->crq_res.data = crq->data; > > + wake_up_interruptible(&wq); That looks racy to me. At the very least it should be doing: ibmvtpm->crq_res.data = crq->data; smp_wmb(); ibmvtpm->crq_res.len = crq->len; IE. Ensure that "len" is written last, and possibly have an corresponding smp_rmb() after wait_event_interruptible() in the receive case. Also, I dislike having a global symbol called "wq". You also don't seem to have any synchronization on access to that wq, can't you end up with several clients trying to send messages & wait for responses getting all mixed up ? You might need to make sure that at least you do a wake_up_interruptible_all() to ensure you wake them all up (inefficient but works). Unless you can track per-client ? Or is the above TPM layer making sure only one command/response happens at a given point in time ? You also do an interruptible wait but don't seem to be checking for signals (and not testing the result from wait_event_interruptible which might be returning -ERESTARTSYS in this case). That all sound a bit wrong to me ... > > +static irqreturn_t ibmvtpm_interrupt(int irq, void *vtpm_instance) > > +{ > > + struct ibmvtpm_dev *ibmvtpm = (struct ibmvtpm_dev *) vtpm_instance; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&ibmvtpm->lock, flags); > > + vio_disable_interrupts(ibmvtpm->vdev); > > + tasklet_schedule(&ibmvtpm->tasklet); > > + spin_unlock_irqrestore(&ibmvtpm->lock, flags); > > + > > + return IRQ_HANDLED; > > +} Tasklets ? We still use those things ? That's softirq iirc, do you really need to offload ? I mean, it allows to limit the amount of time an interrupt is disabled, but that's only useful if you assume you'll have quite a lot of traffic here, is that the case ? You might be actually increasing latency here in favor of throughput, is that what you are aiming for ? Also keep in mind that vio_disable/enable_interrupt() being an hcall, it can have significant overhead. So again, only worth it if you're going to process a bulk of data at a time. Cheers, Ben.
WARNING: multiple messages have this Message-ID (diff)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> To: Kent Yoder <key@linux.vnet.ibm.com> Cc: linux-kernel@vger.kernel.org, Ashley Lai <adlai@linux.vnet.ibm.com>, linux-security-module@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, adlai@us.ibm.com, rcj@linux.vnet.ibm.com, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM Date: Wed, 05 Sep 2012 13:40:07 +1000 [thread overview] Message-ID: <1346816407.2257.37.camel@pasglop> (raw) In-Reply-To: <20120822214217.GB13519@linux.vnet.ibm.com> On Wed, 2012-08-22 at 16:42 -0500, Kent Yoder wrote: > On Wed, Aug 22, 2012 at 04:17:43PM -0500, Ashley Lai wrote: > > This patch adds a new device driver to support IBM virtual TPM > > (vTPM) for PPC64. IBM vTPM is supported through the adjunct > > partition with firmware release 740 or higher. With vTPM > > support, each lpar is able to have its own vTPM without the > > physical TPM hardware. > > > > This driver provides TPM functionalities by communicating with > > the vTPM adjunct partition through Hypervisor calls (Hcalls) > > and Command/Response Queue (CRQ) commands. > > Thanks Ashley, I'll include this in my next pull request to James. Oh ? I was about to put it in the powerpc tree ... But yeah, I notice there's a change to tpm.h so it probably should be at least acked by the TPM folks. As for the subsequent patches, they are powerpc specific so I can add them, I don't think there's a strict dependency, is there ? Now, while having a look at the driver, I found a few things that I'm not sure I'm happy with: Mainly: > > + */ > > +static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count) > > +{ > > + struct ibmvtpm_dev *ibmvtpm; > > + u16 len; > > + > > + ibmvtpm = (struct ibmvtpm_dev *)chip->vendor.data; > > + > > + if (!ibmvtpm->rtce_buf) { > > + dev_err(ibmvtpm->dev, "ibmvtpm device is not ready\n"); > > + return 0; > > + } > > + > > + wait_event_interruptible(wq, ibmvtpm->crq_res.len != 0); > > + > > + if (count < ibmvtpm->crq_res.len) { That doesn't look right. The "other side" as far as I can tell is: > > + case VTPM_TPM_COMMAND_RES: > > + ibmvtpm->crq_res.valid = crq->valid; > > + ibmvtpm->crq_res.msg = crq->msg; > > + ibmvtpm->crq_res.len = crq->len; > > + ibmvtpm->crq_res.data = crq->data; > > + wake_up_interruptible(&wq); That looks racy to me. At the very least it should be doing: ibmvtpm->crq_res.data = crq->data; smp_wmb(); ibmvtpm->crq_res.len = crq->len; IE. Ensure that "len" is written last, and possibly have an corresponding smp_rmb() after wait_event_interruptible() in the receive case. Also, I dislike having a global symbol called "wq". You also don't seem to have any synchronization on access to that wq, can't you end up with several clients trying to send messages & wait for responses getting all mixed up ? You might need to make sure that at least you do a wake_up_interruptible_all() to ensure you wake them all up (inefficient but works). Unless you can track per-client ? Or is the above TPM layer making sure only one command/response happens at a given point in time ? You also do an interruptible wait but don't seem to be checking for signals (and not testing the result from wait_event_interruptible which might be returning -ERESTARTSYS in this case). That all sound a bit wrong to me ... > > +static irqreturn_t ibmvtpm_interrupt(int irq, void *vtpm_instance) > > +{ > > + struct ibmvtpm_dev *ibmvtpm = (struct ibmvtpm_dev *) vtpm_instance; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&ibmvtpm->lock, flags); > > + vio_disable_interrupts(ibmvtpm->vdev); > > + tasklet_schedule(&ibmvtpm->tasklet); > > + spin_unlock_irqrestore(&ibmvtpm->lock, flags); > > + > > + return IRQ_HANDLED; > > +} Tasklets ? We still use those things ? That's softirq iirc, do you really need to offload ? I mean, it allows to limit the amount of time an interrupt is disabled, but that's only useful if you assume you'll have quite a lot of traffic here, is that the case ? You might be actually increasing latency here in favor of throughput, is that what you are aiming for ? Also keep in mind that vio_disable/enable_interrupt() being an hcall, it can have significant overhead. So again, only worth it if you're going to process a bulk of data at a time. Cheers, Ben.
next prev parent reply other threads:[~2012-09-05 3:40 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-08-14 23:23 [PATCH V3 0/3] tpm: Add new vTPM device driver for PPC64 Ashley Lai 2012-08-14 23:23 ` Ashley Lai 2012-08-14 23:34 ` [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM Ashley Lai 2012-08-14 23:34 ` Ashley Lai 2012-08-22 21:17 ` Ashley Lai 2012-08-22 21:17 ` Ashley Lai 2012-08-22 21:42 ` Kent Yoder 2012-08-22 21:42 ` Kent Yoder 2012-09-05 3:40 ` Benjamin Herrenschmidt [this message] 2012-09-05 3:40 ` Benjamin Herrenschmidt 2012-09-05 15:46 ` Kent Yoder 2012-09-05 15:46 ` Kent Yoder 2012-09-05 21:11 ` Benjamin Herrenschmidt 2012-09-05 21:11 ` Benjamin Herrenschmidt 2012-09-07 17:38 ` Kent Yoder 2012-09-07 17:38 ` Kent Yoder 2012-09-12 3:16 ` James Morris 2012-09-12 3:16 ` James Morris 2012-09-05 17:25 ` Ashley Lai 2012-09-05 17:25 ` Ashley Lai 2012-09-12 17:49 ` [PATCH 1/1] drivers/char/tpm: remove tasklet and cleanup Ashley Lai 2012-09-12 17:49 ` Ashley Lai 2012-09-24 2:26 ` James Morris 2012-09-24 2:26 ` James Morris 2012-09-24 14:10 ` key 2012-09-24 14:10 ` key 2012-09-24 15:48 ` key 2012-09-24 15:48 ` key 2012-09-26 13:55 ` key 2012-09-26 13:55 ` key 2012-09-27 14:46 ` Ashley Lai 2012-09-27 14:46 ` Ashley Lai 2012-08-14 23:34 ` [PATCH V3 2/3] PPC64: Add support for instantiating SML from Open Firmware Ashley Lai 2012-08-14 23:34 ` Ashley Lai 2012-08-14 23:35 ` [PATCH V3 3/3] drivers/char/tpm: Add securityfs support for event log Ashley Lai 2012-08-14 23:35 ` Ashley Lai 2012-08-15 19:59 ` [PATCH V3 0/3] tpm: Add new vTPM device driver for PPC64 Kent Yoder 2012-08-15 19:59 ` Kent Yoder
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1346816407.2257.37.camel@pasglop \ --to=benh@kernel.crashing.org \ --cc=adlai@linux.vnet.ibm.com \ --cc=adlai@us.ibm.com \ --cc=key@linux.vnet.ibm.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=rcj@linux.vnet.ibm.com \ --cc=tpmdd-devel@lists.sourceforge.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.