* Enabling interrupts in QEMU TPM TIS
@ 2020-06-25 14:56 Stefan Berger
2020-06-25 17:28 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Berger @ 2020-06-25 14:56 UTC (permalink / raw)
To: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen; +Cc: LSM List, LKML
Hello!
I want to enable IRQs now in QEMU's TPM TIS device model and I need to
work with the following patch to Linux TIS. I am wondering whether the
changes there look reasonable to you? Windows works with the QEMU
modifications as-is, so maybe it's a bug in the TIS code (which I had
not run into before).
The point of the loop I need to introduce in the interrupt handler is
that while the interrupt handler is running another interrupt may
occur/be posted that then does NOT cause the interrupt handler to be
invoked again but causes a stall, unless the loop is there.
The 'o' in the dmesg log indicates the original IRQ for which the
handler was invoked. The interrupt values have the following meaning.
0x2: STS valid
0x4: locality changed
0x80: command ready
So the first 'looping entry' [in log below] indicates that a locality
change interrupt occurred while the interrupt handler was running due to
STS_valid + command ready. This sounds reasonable considering that we
are frequently acquiring and releasing the locality. The loop then deals
with the locality change interrupt and the interrupts then settle.
[ 210.365129] tpm_tis MSFT0101:00: 2.0 TPM (device-id 0x1, rev-id 1)
[ 210.367124] looping: 0x4 (o: 0x82)
[ 212.375045] looping: 0x80 (o: 0x2)
[ 212.389218] looping: 0x4 (o: 0x82)
[ 212.404161] looping: 0x80 (o: 0x2)
[ 212.526427] looping: 0x4 (o: 0x82)
[ 212.595488] looping: 0x4 (o: 0x82)
[ 212.614357] looping: 0x80 (o: 0x2)
diff --git a/drivers/char/tpm/tpm_tis_core.c
b/drivers/char/tpm/tpm_tis_core.c
index 65ab1b027949..f77544563fb1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -704,7 +704,7 @@ static irqreturn_t tis_int_handler(int dummy, void
*dev_id)
{
struct tpm_chip *chip = dev_id;
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
- u32 interrupt;
+ u32 interrupt, o;
int i, rc;
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
@@ -715,6 +715,7 @@ static irqreturn_t tis_int_handler(int dummy, void
*dev_id)
return IRQ_NONE;
priv->irq_tested = true;
+again:
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&priv->read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -731,7 +732,12 @@ static irqreturn_t tis_int_handler(int dummy, void
*dev_id)
if (rc < 0)
return IRQ_NONE;
+ o = interrupt;
tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
+ if (interrupt != 0) {
+ printk("looping: 0x%x (o: 0x%x)\n", interrupt, o);
+ goto again;
+ }
return IRQ_HANDLED;
}
@@ -1062,6 +1068,8 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
goto out_err;
}
+ tpm_chip_start(chip);
+ chip->flags |= TPM_CHIP_FLAG_IRQ;
if (irq) {
tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
irq);
@@ -1074,6 +1082,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
} else {
tpm_tis_probe_irq(chip, intmask);
}
+ tpm_chip_stop(chip);
}
rc = tpm_chip_register(chip);
--
2.26.2
Stefan
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Enabling interrupts in QEMU TPM TIS
2020-06-25 14:56 Enabling interrupts in QEMU TPM TIS Stefan Berger
@ 2020-06-25 17:28 ` Jason Gunthorpe
2020-06-25 21:26 ` Stefan Berger
0 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2020-06-25 17:28 UTC (permalink / raw)
To: Stefan Berger
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List, LKML
On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
> Hello!
>
> I want to enable IRQs now in QEMU's TPM TIS device model and I need to work
> with the following patch to Linux TIS. I am wondering whether the changes
> there look reasonable to you? Windows works with the QEMU modifications
> as-is, so maybe it's a bug in the TIS code (which I had not run into
> before).
>
>
> The point of the loop I need to introduce in the interrupt handler is that
> while the interrupt handler is running another interrupt may occur/be posted
> that then does NOT cause the interrupt handler to be invoked again but
> causes a stall, unless the loop is there.
That seems like a qemu bug, TPM interrupts are supposed to be level
interrupts, not edge.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Enabling interrupts in QEMU TPM TIS
2020-06-25 17:28 ` Jason Gunthorpe
@ 2020-06-25 21:26 ` Stefan Berger
2020-06-25 22:48 ` Stefan Berger
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Berger @ 2020-06-25 21:26 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List, LKML
On 6/25/20 1:28 PM, Jason Gunthorpe wrote:
> On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
>> Hello!
>>
>> I want to enable IRQs now in QEMU's TPM TIS device model and I need to work
>> with the following patch to Linux TIS. I am wondering whether the changes
>> there look reasonable to you? Windows works with the QEMU modifications
>> as-is, so maybe it's a bug in the TIS code (which I had not run into
>> before).
>>
>>
>> The point of the loop I need to introduce in the interrupt handler is that
>> while the interrupt handler is running another interrupt may occur/be posted
>> that then does NOT cause the interrupt handler to be invoked again but
>> causes a stall, unless the loop is there.
> That seems like a qemu bug, TPM interrupts are supposed to be level
> interrupts, not edge.
Following this document here the hardware may choose to support
different types of interrutps:
https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf
Table 23. Edge falling or rising, level low or level high.
So with different steps in the driver causing different types of
interrupts, we may get into such situations where we process some
interrupt 'reasons' but then another one gets posted, I guess due to
parallel processing.
Stefan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Enabling interrupts in QEMU TPM TIS
2020-06-25 21:26 ` Stefan Berger
@ 2020-06-25 22:48 ` Stefan Berger
2020-06-25 23:19 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Berger @ 2020-06-25 22:48 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List, LKML
On 6/25/20 5:26 PM, Stefan Berger wrote:
> On 6/25/20 1:28 PM, Jason Gunthorpe wrote:
>> On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
>>> Hello!
>>>
>>> I want to enable IRQs now in QEMU's TPM TIS device model and I
>>> need to work
>>> with the following patch to Linux TIS. I am wondering whether the
>>> changes
>>> there look reasonable to you? Windows works with the QEMU modifications
>>> as-is, so maybe it's a bug in the TIS code (which I had not run into
>>> before).
>>>
>>>
>>> The point of the loop I need to introduce in the interrupt handler
>>> is that
>>> while the interrupt handler is running another interrupt may
>>> occur/be posted
>>> that then does NOT cause the interrupt handler to be invoked again but
>>> causes a stall, unless the loop is there.
>> That seems like a qemu bug, TPM interrupts are supposed to be level
>> interrupts, not edge.
>
>
> Following this document here the hardware may choose to support
> different types of interrutps:
>
> https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf
>
>
> Table 23. Edge falling or rising, level low or level high.
>
> So with different steps in the driver causing different types of
> interrupts, we may get into such situations where we process some
> interrupt 'reasons' but then another one gets posted, I guess due to
> parallel processing.
Another data point: I had the TIS driver working on IRQ 5 (festeoi)
without the introduction of this loop. There are additional bits being
set while the interrupt handler is running, but the handler deals with
them in the next invocation. On IRQ 13 (edge), it does need the loop
since the next interrupt handler invocation is not happening. That IRQ
13 is an edge interrupt, is this a hard-configured PC 'thing'? Windows
drove this to IRQ 13, which seemed to be the only one accepted by it and
iirc it wouldn't even touch the TIS (found via tracing) if another IRQ
than 13 was declared in the ACPI table.
>
> Stefan
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Enabling interrupts in QEMU TPM TIS
2020-06-25 22:48 ` Stefan Berger
@ 2020-06-25 23:19 ` Jason Gunthorpe
2020-06-26 12:25 ` Stefan Berger
0 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2020-06-25 23:19 UTC (permalink / raw)
To: Stefan Berger
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List, LKML
On Thu, Jun 25, 2020 at 06:48:09PM -0400, Stefan Berger wrote:
> On 6/25/20 5:26 PM, Stefan Berger wrote:
> > On 6/25/20 1:28 PM, Jason Gunthorpe wrote:
> > > On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
> > > > Hello!
> > > >
> > > > I want to enable IRQs now in QEMU's TPM TIS device model and I
> > > > need to work
> > > > with the following patch to Linux TIS. I am wondering whether
> > > > the changes
> > > > there look reasonable to you? Windows works with the QEMU modifications
> > > > as-is, so maybe it's a bug in the TIS code (which I had not run into
> > > > before).
> > > >
> > > >
> > > > The point of the loop I need to introduce in the interrupt
> > > > handler is that
> > > > while the interrupt handler is running another interrupt may
> > > > occur/be posted
> > > > that then does NOT cause the interrupt handler to be invoked again but
> > > > causes a stall, unless the loop is there.
> > > That seems like a qemu bug, TPM interrupts are supposed to be level
> > > interrupts, not edge.
> >
> >
> > Following this document here the hardware may choose to support
> > different types of interrutps:
> >
> > https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf
> >
> >
> > Table 23. Edge falling or rising, level low or level high.
> >
> > So with different steps in the driver causing different types of
> > interrupts, we may get into such situations where we process some
> > interrupt 'reasons' but then another one gets posted, I guess due to
> > parallel processing.
>
>
> Another data point: I had the TIS driver working on IRQ 5 (festeoi) without
> the introduction of this loop. There are additional bits being set while the
> interrupt handler is running, but the handler deals with them in the next
> invocation. On IRQ 13 (edge), it does need the loop since the next interrupt
> handler invocation is not happening.
A loop like that is never the correct way to handle edge interrupts.
I don't think the tpm driver was ever designed for edge, so most
likely the structure and order of the hard irq is not correct.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Enabling interrupts in QEMU TPM TIS
2020-06-25 23:19 ` Jason Gunthorpe
@ 2020-06-26 12:25 ` Stefan Berger
2020-06-26 13:15 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Berger @ 2020-06-26 12:25 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List, LKML
On 6/25/20 7:19 PM, Jason Gunthorpe wrote:
> On Thu, Jun 25, 2020 at 06:48:09PM -0400, Stefan Berger wrote:
>> On 6/25/20 5:26 PM, Stefan Berger wrote:
>>> On 6/25/20 1:28 PM, Jason Gunthorpe wrote:
>>>> On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
>>>>> Hello!
>>>>>
>>>>> I want to enable IRQs now in QEMU's TPM TIS device model and I
>>>>> need to work
>>>>> with the following patch to Linux TIS. I am wondering whether
>>>>> the changes
>>>>> there look reasonable to you? Windows works with the QEMU modifications
>>>>> as-is, so maybe it's a bug in the TIS code (which I had not run into
>>>>> before).
>>>>>
>>>>>
>>>>> The point of the loop I need to introduce in the interrupt
>>>>> handler is that
>>>>> while the interrupt handler is running another interrupt may
>>>>> occur/be posted
>>>>> that then does NOT cause the interrupt handler to be invoked again but
>>>>> causes a stall, unless the loop is there.
>>>> That seems like a qemu bug, TPM interrupts are supposed to be level
>>>> interrupts, not edge.
>>>
>>> Following this document here the hardware may choose to support
>>> different types of interrutps:
>>>
>>> https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf
>>>
>>>
>>> Table 23. Edge falling or rising, level low or level high.
>>>
>>> So with different steps in the driver causing different types of
>>> interrupts, we may get into such situations where we process some
>>> interrupt 'reasons' but then another one gets posted, I guess due to
>>> parallel processing.
>>
>> Another data point: I had the TIS driver working on IRQ 5 (festeoi) without
>> the introduction of this loop. There are additional bits being set while the
>> interrupt handler is running, but the handler deals with them in the next
>> invocation. On IRQ 13 (edge), it does need the loop since the next interrupt
>> handler invocation is not happening.
> A loop like that is never the correct way to handle edge interrupts.
Right, we can just miss the update of the interrupt flags and miss the
loop and then afterwards the new flag gets set and we'd stall.
>
> I don't think the tpm driver was ever designed for edge, so most
> likely the structure and order of the hard irq is not correct.
Right. For edge support I think we would need to avoid causing another
interrupt (like locality change interrupt) before the interrupt handler
hasn't finished dealing with an existing interrupt. Considering that
Windows works on IRQ 13 (egde) and Linux driver cannot, I guess this is
a good reason not to move QEMU TIS to IRQ 13 and try to support
interrupts via ACPI table declaration.
Stefan
>
> Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Enabling interrupts in QEMU TPM TIS
2020-06-26 12:25 ` Stefan Berger
@ 2020-06-26 13:15 ` Jason Gunthorpe
0 siblings, 0 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2020-06-26 13:15 UTC (permalink / raw)
To: Stefan Berger
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List, LKML
On Fri, Jun 26, 2020 at 08:25:57AM -0400, Stefan Berger wrote:
> > I don't think the tpm driver was ever designed for edge, so most
> > likely the structure and order of the hard irq is not correct.
>
> Right. For edge support I think we would need to avoid causing another
> interrupt (like locality change interrupt) before the interrupt handler
> hasn't finished dealing with an existing interrupt. Considering that Windows
> works on IRQ 13 (egde) and Linux driver cannot, I guess this is a good
> reason not to move QEMU TIS to IRQ 13 and try to support interrupts via ACPI
> table declaration.
Generaly clearing the IRQ needs to be done before testing for pending
IRQs - ie as the first thing
Move the write to status up higher:
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), interrupt);
[handle 'interrupt']
Then if new events set a status bit they will generate an edge and
re-enter here.
I don't know why there is an extra read at the end of the handler
either, seems sketchy.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-26 13:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 14:56 Enabling interrupts in QEMU TPM TIS Stefan Berger
2020-06-25 17:28 ` Jason Gunthorpe
2020-06-25 21:26 ` Stefan Berger
2020-06-25 22:48 ` Stefan Berger
2020-06-25 23:19 ` Jason Gunthorpe
2020-06-26 12:25 ` Stefan Berger
2020-06-26 13:15 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).