Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
On Fri, 7 Sep 2012, Kent Yoder wrote: > > > James did accept my pull request, so these are already in > > > security-next... > > > > For the driver itself, it's not a big issue (though I did found issue > > while reviewing it so it will need another round of updates). For the > > code that changes arch/powerpc, especially prom_init.c, that stuff must > > at the very least be acked by me (or the acting powerpc person if I'm > > away) if it's going to go via a different tree. > > Sorry about that. Hopefully there won't be any changes there and we > can amend with your ack. > > As for the driver updates, I'd hate to see everyone else's code in the > pull request get delayed yet again. James, will it be ok to apply the > update on top of security-next? I guess? -- James Morris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
On Fri, 7 Sep 2012, Kent Yoder wrote: James did accept my pull request, so these are already in security-next... For the driver itself, it's not a big issue (though I did found issue while reviewing it so it will need another round of updates). For the code that changes arch/powerpc, especially prom_init.c, that stuff must at the very least be acked by me (or the acting powerpc person if I'm away) if it's going to go via a different tree. Sorry about that. Hopefully there won't be any changes there and we can amend with your ack. As for the driver updates, I'd hate to see everyone else's code in the pull request get delayed yet again. James, will it be ok to apply the update on top of security-next? I guess? -- James Morris jmor...@namei.org -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
> > James did accept my pull request, so these are already in > > security-next... > > For the driver itself, it's not a big issue (though I did found issue > while reviewing it so it will need another round of updates). For the > code that changes arch/powerpc, especially prom_init.c, that stuff must > at the very least be acked by me (or the acting powerpc person if I'm > away) if it's going to go via a different tree. Sorry about that. Hopefully there won't be any changes there and we can amend with your ack. As for the driver updates, I'd hate to see everyone else's code in the pull request get delayed yet again. James, will it be ok to apply the update on top of security-next? Thanks, Kent > Cheers, > Ben. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
James did accept my pull request, so these are already in security-next... For the driver itself, it's not a big issue (though I did found issue while reviewing it so it will need another round of updates). For the code that changes arch/powerpc, especially prom_init.c, that stuff must at the very least be acked by me (or the acting powerpc person if I'm away) if it's going to go via a different tree. Sorry about that. Hopefully there won't be any changes there and we can amend with your ack. As for the driver updates, I'd hate to see everyone else's code in the pull request get delayed yet again. James, will it be ok to apply the update on top of security-next? Thanks, Kent Cheers, Ben. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
On Wed, 2012-09-05 at 10:46 -0500, Kent Yoder wrote: > On Wed, Sep 05, 2012 at 01:40:07PM +1000, Benjamin Herrenschmidt wrote: > > 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 ? > > James did accept my pull request, so these are already in > security-next... For the driver itself, it's not a big issue (though I did found issue while reviewing it so it will need another round of updates). For the code that changes arch/powerpc, especially prom_init.c, that stuff must at the very least be acked by me (or the acting powerpc person if I'm away) if it's going to go via a different tree. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
Hi Ben.,
Thank you so much for the comments. Please see my response below.
> > > + */
> > > +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();
>
> 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.
Good catch. I agreed len should be written last and adding memory
barrier is a good idea.
>
> 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 ?
The TPM layer above allows only one request at any given point in time.
Therefore we don't need to wake_up_interruptible_all(). I can move wq
to the private structure.
>
> Or is the above TPM layer making sure only one command/response happens
> at a given point in time ?
You are right. See above.
>
> 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).
>
Good point. I will check for signal in the next version.
> 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(>lock, flags);
> > > + vio_disable_interrupts(ibmvtpm->vdev);
> > > + tasklet_schedule(>tasklet);
> > > + spin_unlock_irqrestore(>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.
My original thought was to have the bottom half process the crq data
while the top half can service another request. There is some work in
processing crq data but not that bad. You are right the
vio_disable/enable_interrupt() can have significant overhead. I will
remove tasklet in the next version.
>
> Cheers,
> Ben.
>
>
Thanks,
--Ashley Lai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
On Wed, Sep 05, 2012 at 01:40:07PM +1000, Benjamin Herrenschmidt wrote: > 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 ? James did accept my pull request, so these are already in security-next... Kent -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
On Wed, Sep 05, 2012 at 01:40:07PM +1000, Benjamin Herrenschmidt wrote: 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 ? James did accept my pull request, so these are already in security-next... Kent -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
Hi Ben.,
Thank you so much for the comments. Please see my response below.
+ */
+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.
Good catch. I agreed len should be written last and adding memory
barrier is a good idea.
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 ?
The TPM layer above allows only one request at any given point in time.
Therefore we don't need to wake_up_interruptible_all(). I can move wq
to the private structure.
Or is the above TPM layer making sure only one command/response happens
at a given point in time ?
You are right. See above.
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).
Good point. I will check for signal in the next version.
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.
My original thought was to have the bottom half process the crq data
while the top half can service another request. There is some work in
processing crq data but not that bad. You are right the
vio_disable/enable_interrupt() can have significant overhead. I will
remove tasklet in the next version.
Cheers,
Ben.
Thanks,
--Ashley Lai
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
On Wed, 2012-09-05 at 10:46 -0500, Kent Yoder wrote: On Wed, Sep 05, 2012 at 01:40:07PM +1000, Benjamin Herrenschmidt wrote: 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 ? James did accept my pull request, so these are already in security-next... For the driver itself, it's not a big issue (though I did found issue while reviewing it so it will need another round of updates). For the code that changes arch/powerpc, especially prom_init.c, that stuff must at the very least be acked by me (or the acting powerpc person if I'm away) if it's going to go via a different tree. Cheers, Ben. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
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();
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(>lock, flags);
> > + vio_disable_interrupts(ibmvtpm->vdev);
> > + tasklet_schedule(>tasklet);
> > + spin_unlock_irqrestore(>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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
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.
Kent
> Signed-off-by: Ashley Lai
> ---
>
> Oopps... I forgot to remove some invalid comments. Please use this patch for
> 1/3.
>
> drivers/char/tpm/Kconfig |8 +
> drivers/char/tpm/Makefile |1 +
> drivers/char/tpm/tpm.h |1 +
> drivers/char/tpm/tpm_ibmvtpm.c | 749
>
> drivers/char/tpm/tpm_ibmvtpm.h | 77
> 5 files changed, 836 insertions(+), 0 deletions(-)
> create mode 100644 drivers/char/tpm/tpm_ibmvtpm.c
> create mode 100644 drivers/char/tpm/tpm_ibmvtpm.h
>
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index c4aac48..915875e 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -73,4 +73,12 @@ config TCG_INFINEON
> Further information on this driver and the supported hardware
> can be found at
> http://www.trust.rub.de/projects/linux-device-driver-infineon-tpm/
>
> +config TCG_IBMVTPM
> + tristate "IBM VTPM Interface"
> + depends on PPC64
> + ---help---
> + If you have IBM virtual TPM (VTPM) support say Yes and it
> + will be accessible from within Linux. To compile this driver
> + as a module, choose M here; the module will be called tpm_ibmvtpm.
> +
> endif # TCG_TPM
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index beac52f..547509d 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
> obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
> obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
> obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
> +obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 645136e..870fde7 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -100,6 +100,7 @@ struct tpm_vendor_specific {
> bool timeout_adjusted;
> unsigned long duration[3]; /* jiffies */
> bool duration_adjusted;
> + void *data;
>
> wait_queue_head_t read_queue;
> wait_queue_head_t int_queue;
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> new file mode 100644
> index 000..efc4ab3
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -0,0 +1,749 @@
> +/*
> + * Copyright (C) 2012 IBM Corporation
> + *
> + * Author: Ashley Lai
> + *
> + * Maintained by:
> + *
> + * Device driver for TCG/TCPA TPM (trusted platform module).
> + * Specifications at www.trustedcomputinggroup.org
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2 of the
> + * License.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "tpm.h"
> +#include "tpm_ibmvtpm.h"
> +
> +static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm";
> +
> +static struct vio_device_id tpm_ibmvtpm_device_table[] __devinitdata = {
> + { "IBM,vtpm", "IBM,vtpm"},
> + { "", "" }
> +};
> +MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
> +
> +DECLARE_WAIT_QUEUE_HEAD(wq);
> +
> +/**
> + * ibmvtpm_send_crq - Send a CRQ request
> + * @vdev:vio device struct
> + * @w1: first word
> + * @w2: second word
> + *
> + * Return value:
> + * 0 -Sucess
> + * Non-zero - Failure
> + */
> +static int ibmvtpm_send_crq(struct vio_dev *vdev, u64 w1, u64 w2)
> +{
> + return plpar_hcall_norets(H_SEND_CRQ, vdev->unit_address, w1, w2);
> +}
> +
> +/**
> + * ibmvtpm_get_data - Retrieve ibm vtpm data
> + * @dev: device struct
> + *
> + * Return value:
> + * vtpm device struct
> + */
> +static struct ibmvtpm_dev *ibmvtpm_get_data(const struct device *dev)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> + if (chip)
> + return (struct ibmvtpm_dev *)chip->vendor.data;
> + return NULL;
> +}
> +
> +/**
> + * tpm_ibmvtpm_recv - Receive data after send
> + * @chip:tpm chip struct
> + * @buf: buffer to read
> + * count:size of buffer
> + *
> + * Return value:
> + * Number of bytes read
> + */
> +static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> + struct ibmvtpm_dev *ibmvtpm;
> + u16
Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver to support IBM vTPM
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.
Kent
Signed-off-by: Ashley Lai ad...@us.ibm.com
---
Oopps... I forgot to remove some invalid comments. Please use this patch for
1/3.
drivers/char/tpm/Kconfig |8 +
drivers/char/tpm/Makefile |1 +
drivers/char/tpm/tpm.h |1 +
drivers/char/tpm/tpm_ibmvtpm.c | 749
drivers/char/tpm/tpm_ibmvtpm.h | 77
5 files changed, 836 insertions(+), 0 deletions(-)
create mode 100644 drivers/char/tpm/tpm_ibmvtpm.c
create mode 100644 drivers/char/tpm/tpm_ibmvtpm.h
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index c4aac48..915875e 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -73,4 +73,12 @@ config TCG_INFINEON
Further information on this driver and the supported hardware
can be found at
http://www.trust.rub.de/projects/linux-device-driver-infineon-tpm/
+config TCG_IBMVTPM
+ tristate IBM VTPM Interface
+ depends on PPC64
+ ---help---
+ If you have IBM virtual TPM (VTPM) support say Yes and it
+ will be accessible from within Linux. To compile this driver
+ as a module, choose M here; the module will be called tpm_ibmvtpm.
+
endif # TCG_TPM
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index beac52f..547509d 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
obj-$(CONFIG_TCG_NSC) += tpm_nsc.o
obj-$(CONFIG_TCG_ATMEL) += tpm_atmel.o
obj-$(CONFIG_TCG_INFINEON) += tpm_infineon.o
+obj-$(CONFIG_TCG_IBMVTPM) += tpm_ibmvtpm.o
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 645136e..870fde7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -100,6 +100,7 @@ struct tpm_vendor_specific {
bool timeout_adjusted;
unsigned long duration[3]; /* jiffies */
bool duration_adjusted;
+ void *data;
wait_queue_head_t read_queue;
wait_queue_head_t int_queue;
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
new file mode 100644
index 000..efc4ab3
--- /dev/null
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -0,0 +1,749 @@
+/*
+ * Copyright (C) 2012 IBM Corporation
+ *
+ * Author: Ashley Lai ad...@us.ibm.com
+ *
+ * Maintained by: tpmdd-de...@lists.sourceforge.net
+ *
+ * Device driver for TCG/TCPA TPM (trusted platform module).
+ * Specifications at www.trustedcomputinggroup.org
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2 of the
+ * License.
+ *
+ */
+
+#include linux/dma-mapping.h
+#include linux/dmapool.h
+#include linux/slab.h
+#include asm/vio.h
+#include asm/irq.h
+#include linux/types.h
+#include linux/list.h
+#include linux/spinlock.h
+#include linux/interrupt.h
+#include linux/wait.h
+#include asm/prom.h
+
+#include tpm.h
+#include tpm_ibmvtpm.h
+
+static const char tpm_ibmvtpm_driver_name[] = tpm_ibmvtpm;
+
+static struct vio_device_id tpm_ibmvtpm_device_table[] __devinitdata = {
+ { IBM,vtpm, IBM,vtpm},
+ { , }
+};
+MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
+
+DECLARE_WAIT_QUEUE_HEAD(wq);
+
+/**
+ * ibmvtpm_send_crq - Send a CRQ request
+ * @vdev:vio device struct
+ * @w1: first word
+ * @w2: second word
+ *
+ * Return value:
+ * 0 -Sucess
+ * Non-zero - Failure
+ */
+static int ibmvtpm_send_crq(struct vio_dev *vdev, u64 w1, u64 w2)
+{
+ return plpar_hcall_norets(H_SEND_CRQ, vdev-unit_address, w1, w2);
+}
+
+/**
+ * ibmvtpm_get_data - Retrieve ibm vtpm data
+ * @dev: device struct
+ *
+ * Return value:
+ * vtpm device struct
+ */
+static struct ibmvtpm_dev *ibmvtpm_get_data(const struct device *dev)
+{
+ struct tpm_chip *chip = dev_get_drvdata(dev);
+ if (chip)
+ return (struct ibmvtpm_dev *)chip-vendor.data;
+ return NULL;
+}
+
+/**
+ * tpm_ibmvtpm_recv - Receive data after send
+ * @chip:tpm chip struct
+ * @buf: buffer to read
+ * count:size of buffer
+ *
+ * Return value:
+ * Number of bytes read
+ */
+static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+ struct
