Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/03/2018 10:34 AM, Dmitry Safonov wrote:
> On Thu, 2018-05-03 at 10:16 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 05/03/2018 09:59 AM, Dmitry Safonov wrote:
>>> On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
 Hi,

 On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
> AFAICS, we're doing fault-clearing in a loop inside irq
> handler.
> That means that while we're clearing if a fault raises, it'll
> make
> an irq level triggered (or on edge) on lapic. So, whenever we
> return
> from the irq handler, irq will raise again.
>
 Uhm, double checked with the spec. Interrupts should be generated
 since we always clear the fault overflow bit.

 Anyway, we can't clear faults in a limited loop, as the spec says
 in
 7.3.1:
>>> Mind to elaborate?
>>> ITOW, I do not see a contradiction. We're still clearing faults in
>>> FIFO
>>> fashion. There is no limitation to do some spare work in between
>>> clearings (return from interrupt, then fault again and continue).
>> Hardware maintains an internal index to reference the fault recording
>> register in which the next fault can be recorded. When a fault comes,
>> hardware will check the Fault bit (bit 31 of the 4th 32-bit register
>> recording
>> register) referenced by the internal index. If this bit is set,
>> hardware will
>> not record the fault.
>>
>> Since we now don't clear the F bit until a register entry which has
>> the F bit
>> cleared, we might exit the fault handling with some register entries
>> still
>> have the F bit set.
>>
>>   F
>>> 0 |  x|
>>> 0 |  x|
>>> 0 |  x|  <--- Fault record index in fault status
>>> register
>>> 0 |  x|
>>> 1 |  x|  <--- hardware maintained index
>>> 1 |  x|
>>> 1 |  x|
>>> 0 |  x|
>>> 0 |  x|
>>> 0 |  x|
>>> 0 |  x|
>> Take an example as above, hardware could only record 2 more faults
>> with
>> others all dropped.
> Ugh, yeah, I got what you're saying.. Thanks for explanations.
> So, we shouldn't mark faults as cleared until we've actually processed
> them here:
> :writel(DMA_FSTS_PFO | DMA_FSTS_PPF | DMA_FSTS_PRO,
> :   iommu->reg + DMAR_FSTS_REG);
>
> As Joerg mentioned, we do care about latency here, so this fault work
> can't be moved entirely into workqueue.. but we might limit loop and
> check if we've hit the limit - to proceed servicing faults in a wq,
> as in that case we should care about being too long in irq-disabled
> section more than about latencies.
> Does that makes any sense, what do you think?
>
> I can possibly re-write 2/2 with idea above..

Very appreciated. I am open to the idea. :-)

Best regards,
Lu Baolu

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/03/2018 10:34 AM, Dmitry Safonov wrote:
> On Thu, 2018-05-03 at 10:16 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 05/03/2018 09:59 AM, Dmitry Safonov wrote:
>>> On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
 Hi,

 On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
> AFAICS, we're doing fault-clearing in a loop inside irq
> handler.
> That means that while we're clearing if a fault raises, it'll
> make
> an irq level triggered (or on edge) on lapic. So, whenever we
> return
> from the irq handler, irq will raise again.
>
 Uhm, double checked with the spec. Interrupts should be generated
 since we always clear the fault overflow bit.

 Anyway, we can't clear faults in a limited loop, as the spec says
 in
 7.3.1:
>>> Mind to elaborate?
>>> ITOW, I do not see a contradiction. We're still clearing faults in
>>> FIFO
>>> fashion. There is no limitation to do some spare work in between
>>> clearings (return from interrupt, then fault again and continue).
>> Hardware maintains an internal index to reference the fault recording
>> register in which the next fault can be recorded. When a fault comes,
>> hardware will check the Fault bit (bit 31 of the 4th 32-bit register
>> recording
>> register) referenced by the internal index. If this bit is set,
>> hardware will
>> not record the fault.
>>
>> Since we now don't clear the F bit until a register entry which has
>> the F bit
>> cleared, we might exit the fault handling with some register entries
>> still
>> have the F bit set.
>>
>>   F
>>> 0 |  x|
>>> 0 |  x|
>>> 0 |  x|  <--- Fault record index in fault status
>>> register
>>> 0 |  x|
>>> 1 |  x|  <--- hardware maintained index
>>> 1 |  x|
>>> 1 |  x|
>>> 0 |  x|
>>> 0 |  x|
>>> 0 |  x|
>>> 0 |  x|
>> Take an example as above, hardware could only record 2 more faults
>> with
>> others all dropped.
> Ugh, yeah, I got what you're saying.. Thanks for explanations.
> So, we shouldn't mark faults as cleared until we've actually processed
> them here:
> :writel(DMA_FSTS_PFO | DMA_FSTS_PPF | DMA_FSTS_PRO,
> :   iommu->reg + DMAR_FSTS_REG);
>
> As Joerg mentioned, we do care about latency here, so this fault work
> can't be moved entirely into workqueue.. but we might limit loop and
> check if we've hit the limit - to proceed servicing faults in a wq,
> as in that case we should care about being too long in irq-disabled
> section more than about latencies.
> Does that makes any sense, what do you think?
>
> I can possibly re-write 2/2 with idea above..

Very appreciated. I am open to the idea. :-)

Best regards,
Lu Baolu

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Dmitry Safonov]

On Thu, 2018-05-03 at 10:16 +0800, Lu Baolu wrote:
> Hi,
> 
> On 05/03/2018 09:59 AM, Dmitry Safonov wrote:
> > On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
> > > Hi,
> > > 
> > > On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
> > > > AFAICS, we're doing fault-clearing in a loop inside irq
> > > > handler.
> > > > That means that while we're clearing if a fault raises, it'll
> > > > make
> > > > an irq level triggered (or on edge) on lapic. So, whenever we
> > > > return
> > > > from the irq handler, irq will raise again.
> > > > 
> > > 
> > > Uhm, double checked with the spec. Interrupts should be generated
> > > since we always clear the fault overflow bit.
> > > 
> > > Anyway, we can't clear faults in a limited loop, as the spec says
> > > in
> > > 7.3.1:
> > 
> > Mind to elaborate?
> > ITOW, I do not see a contradiction. We're still clearing faults in
> > FIFO
> > fashion. There is no limitation to do some spare work in between
> > clearings (return from interrupt, then fault again and continue).
> 
> Hardware maintains an internal index to reference the fault recording
> register in which the next fault can be recorded. When a fault comes,
> hardware will check the Fault bit (bit 31 of the 4th 32-bit register
> recording
> register) referenced by the internal index. If this bit is set,
> hardware will
> not record the fault.
> 
> Since we now don't clear the F bit until a register entry which has
> the F bit
> cleared, we might exit the fault handling with some register entries
> still
> have the F bit set.
> 
>   F
> > 0 |  x|
> > 0 |  x|
> > 0 |  x|  <--- Fault record index in fault status
> > register
> > 0 |  x|
> > 1 |  x|  <--- hardware maintained index
> > 1 |  x|
> > 1 |  x|
> > 0 |  x|
> > 0 |  x|
> > 0 |  x|
> > 0 |  x|
> 
> Take an example as above, hardware could only record 2 more faults
> with
> others all dropped.

Ugh, yeah, I got what you're saying.. Thanks for explanations.
So, we shouldn't mark faults as cleared until we've actually processed
them here:
:writel(DMA_FSTS_PFO | DMA_FSTS_PPF | DMA_FSTS_PRO,
:   iommu->reg + DMAR_FSTS_REG);

As Joerg mentioned, we do care about latency here, so this fault work
can't be moved entirely into workqueue.. but we might limit loop and
check if we've hit the limit - to proceed servicing faults in a wq,
as in that case we should care about being too long in irq-disabled
section more than about latencies.
Does that makes any sense, what do you think?

I can possibly re-write 2/2 with idea above..
And it would be a bit joy to have 1/1 applied, as it's independent fix
and fixes an issue that happens for real on our devices, heh.

-- 
Thanks,
 Dmitry

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Dmitry Safonov]

On Thu, 2018-05-03 at 10:16 +0800, Lu Baolu wrote:
> Hi,
> 
> On 05/03/2018 09:59 AM, Dmitry Safonov wrote:
> > On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
> > > Hi,
> > > 
> > > On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
> > > > AFAICS, we're doing fault-clearing in a loop inside irq
> > > > handler.
> > > > That means that while we're clearing if a fault raises, it'll
> > > > make
> > > > an irq level triggered (or on edge) on lapic. So, whenever we
> > > > return
> > > > from the irq handler, irq will raise again.
> > > > 
> > > 
> > > Uhm, double checked with the spec. Interrupts should be generated
> > > since we always clear the fault overflow bit.
> > > 
> > > Anyway, we can't clear faults in a limited loop, as the spec says
> > > in
> > > 7.3.1:
> > 
> > Mind to elaborate?
> > ITOW, I do not see a contradiction. We're still clearing faults in
> > FIFO
> > fashion. There is no limitation to do some spare work in between
> > clearings (return from interrupt, then fault again and continue).
> 
> Hardware maintains an internal index to reference the fault recording
> register in which the next fault can be recorded. When a fault comes,
> hardware will check the Fault bit (bit 31 of the 4th 32-bit register
> recording
> register) referenced by the internal index. If this bit is set,
> hardware will
> not record the fault.
> 
> Since we now don't clear the F bit until a register entry which has
> the F bit
> cleared, we might exit the fault handling with some register entries
> still
> have the F bit set.
> 
>   F
> > 0 |  x|
> > 0 |  x|
> > 0 |  x|  <--- Fault record index in fault status
> > register
> > 0 |  x|
> > 1 |  x|  <--- hardware maintained index
> > 1 |  x|
> > 1 |  x|
> > 0 |  x|
> > 0 |  x|
> > 0 |  x|
> > 0 |  x|
> 
> Take an example as above, hardware could only record 2 more faults
> with
> others all dropped.

Ugh, yeah, I got what you're saying.. Thanks for explanations.
So, we shouldn't mark faults as cleared until we've actually processed
them here:
:writel(DMA_FSTS_PFO | DMA_FSTS_PPF | DMA_FSTS_PRO,
:   iommu->reg + DMAR_FSTS_REG);

As Joerg mentioned, we do care about latency here, so this fault work
can't be moved entirely into workqueue.. but we might limit loop and
check if we've hit the limit - to proceed servicing faults in a wq,
as in that case we should care about being too long in irq-disabled
section more than about latencies.
Does that makes any sense, what do you think?

I can possibly re-write 2/2 with idea above..
And it would be a bit joy to have 1/1 applied, as it's independent fix
and fixes an issue that happens for real on our devices, heh.

-- 
Thanks,
 Dmitry

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/03/2018 10:16 AM, Lu Baolu wrote:
> Hi,
>
> On 05/03/2018 09:59 AM, Dmitry Safonov wrote:
>> On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
>>> Hi,
>>>
>>> On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
 AFAICS, we're doing fault-clearing in a loop inside irq handler.
 That means that while we're clearing if a fault raises, it'll make
 an irq level triggered (or on edge) on lapic. So, whenever we
 return
 from the irq handler, irq will raise again.

>>> Uhm, double checked with the spec. Interrupts should be generated
>>> since we always clear the fault overflow bit.
>>>
>>> Anyway, we can't clear faults in a limited loop, as the spec says in
>>> 7.3.1:
>> Mind to elaborate?
>> ITOW, I do not see a contradiction. We're still clearing faults in FIFO
>> fashion. There is no limitation to do some spare work in between
>> clearings (return from interrupt, then fault again and continue).
> Hardware maintains an internal index to reference the fault recording
> register in which the next fault can be recorded. When a fault comes,
> hardware will check the Fault bit (bit 31 of the 4th 32-bit register recording
> register) referenced by the internal index. If this bit is set, hardware will
> not record the fault.
>
> Since we now don't clear the F bit until a register entry which has the F bit
> cleared, we might exit the fault handling with some register entries still
> have the F bit set.
>
>   F
> | 0 |  x|
> | 0 |  x|
> | 0 |  x|  <--- Fault record index in fault status register

Forgot to mention, this fault record index that software reads from
the fault status register is also maintained by hardware. It means
the index of the first fault recording register that hardware records
the faults last time.

Software doesn't maintains its own index, right? So there might some
registers left there with F bit set.

Best regards,
Lu Baolu

> | 0 |  x|
> | 1 |  x|  <--- hardware maintained index
> | 1 |  x|
> | 1 |  x|
> | 0 |  x|
> | 0 |  x|
> | 0 |  x|
> | 0 |  x|
>
> Take an example as above, hardware could only record 2 more faults with
> others all dropped.
>
> Best regards,
> Lu Baolu
>

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/03/2018 10:16 AM, Lu Baolu wrote:
> Hi,
>
> On 05/03/2018 09:59 AM, Dmitry Safonov wrote:
>> On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
>>> Hi,
>>>
>>> On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
 AFAICS, we're doing fault-clearing in a loop inside irq handler.
 That means that while we're clearing if a fault raises, it'll make
 an irq level triggered (or on edge) on lapic. So, whenever we
 return
 from the irq handler, irq will raise again.

>>> Uhm, double checked with the spec. Interrupts should be generated
>>> since we always clear the fault overflow bit.
>>>
>>> Anyway, we can't clear faults in a limited loop, as the spec says in
>>> 7.3.1:
>> Mind to elaborate?
>> ITOW, I do not see a contradiction. We're still clearing faults in FIFO
>> fashion. There is no limitation to do some spare work in between
>> clearings (return from interrupt, then fault again and continue).
> Hardware maintains an internal index to reference the fault recording
> register in which the next fault can be recorded. When a fault comes,
> hardware will check the Fault bit (bit 31 of the 4th 32-bit register recording
> register) referenced by the internal index. If this bit is set, hardware will
> not record the fault.
>
> Since we now don't clear the F bit until a register entry which has the F bit
> cleared, we might exit the fault handling with some register entries still
> have the F bit set.
>
>   F
> | 0 |  x|
> | 0 |  x|
> | 0 |  x|  <--- Fault record index in fault status register

Forgot to mention, this fault record index that software reads from
the fault status register is also maintained by hardware. It means
the index of the first fault recording register that hardware records
the faults last time.

Software doesn't maintains its own index, right? So there might some
registers left there with F bit set.

Best regards,
Lu Baolu

> | 0 |  x|
> | 1 |  x|  <--- hardware maintained index
> | 1 |  x|
> | 1 |  x|
> | 0 |  x|
> | 0 |  x|
> | 0 |  x|
> | 0 |  x|
>
> Take an example as above, hardware could only record 2 more faults with
> others all dropped.
>
> Best regards,
> Lu Baolu
>

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/03/2018 09:59 AM, Dmitry Safonov wrote:
> On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
>>> AFAICS, we're doing fault-clearing in a loop inside irq handler.
>>> That means that while we're clearing if a fault raises, it'll make
>>> an irq level triggered (or on edge) on lapic. So, whenever we
>>> return
>>> from the irq handler, irq will raise again.
>>>
>> Uhm, double checked with the spec. Interrupts should be generated
>> since we always clear the fault overflow bit.
>>
>> Anyway, we can't clear faults in a limited loop, as the spec says in
>> 7.3.1:
> Mind to elaborate?
> ITOW, I do not see a contradiction. We're still clearing faults in FIFO
> fashion. There is no limitation to do some spare work in between
> clearings (return from interrupt, then fault again and continue).

Hardware maintains an internal index to reference the fault recording
register in which the next fault can be recorded. When a fault comes,
hardware will check the Fault bit (bit 31 of the 4th 32-bit register recording
register) referenced by the internal index. If this bit is set, hardware will
not record the fault.

Since we now don't clear the F bit until a register entry which has the F bit
cleared, we might exit the fault handling with some register entries still
have the F bit set.

  F
| 0 |  x|
| 0 |  x|
| 0 |  x|  <--- Fault record index in fault status register
| 0 |  x|
| 1 |  x|  <--- hardware maintained index
| 1 |  x|
| 1 |  x|
| 0 |  x|
| 0 |  x|
| 0 |  x|
| 0 |  x|

Take an example as above, hardware could only record 2 more faults with
others all dropped.

Best regards,
Lu Baolu

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/03/2018 09:59 AM, Dmitry Safonov wrote:
> On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
>>> AFAICS, we're doing fault-clearing in a loop inside irq handler.
>>> That means that while we're clearing if a fault raises, it'll make
>>> an irq level triggered (or on edge) on lapic. So, whenever we
>>> return
>>> from the irq handler, irq will raise again.
>>>
>> Uhm, double checked with the spec. Interrupts should be generated
>> since we always clear the fault overflow bit.
>>
>> Anyway, we can't clear faults in a limited loop, as the spec says in
>> 7.3.1:
> Mind to elaborate?
> ITOW, I do not see a contradiction. We're still clearing faults in FIFO
> fashion. There is no limitation to do some spare work in between
> clearings (return from interrupt, then fault again and continue).

Hardware maintains an internal index to reference the fault recording
register in which the next fault can be recorded. When a fault comes,
hardware will check the Fault bit (bit 31 of the 4th 32-bit register recording
register) referenced by the internal index. If this bit is set, hardware will
not record the fault.

Since we now don't clear the F bit until a register entry which has the F bit
cleared, we might exit the fault handling with some register entries still
have the F bit set.

  F
| 0 |  x|
| 0 |  x|
| 0 |  x|  <--- Fault record index in fault status register
| 0 |  x|
| 1 |  x|  <--- hardware maintained index
| 1 |  x|
| 1 |  x|
| 0 |  x|
| 0 |  x|
| 0 |  x|
| 0 |  x|

Take an example as above, hardware could only record 2 more faults with
others all dropped.

Best regards,
Lu Baolu

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Dmitry Safonov]

On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
> Hi,
> 
> On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
> > AFAICS, we're doing fault-clearing in a loop inside irq handler.
> > That means that while we're clearing if a fault raises, it'll make
> > an irq level triggered (or on edge) on lapic. So, whenever we
> > return
> > from the irq handler, irq will raise again.
> > 
> 
> Uhm, double checked with the spec. Interrupts should be generated
> since we always clear the fault overflow bit.
> 
> Anyway, we can't clear faults in a limited loop, as the spec says in
> 7.3.1:

Mind to elaborate?
ITOW, I do not see a contradiction. We're still clearing faults in FIFO
fashion. There is no limitation to do some spare work in between
clearings (return from interrupt, then fault again and continue).

> Software is expected to process the non-recoverable faults reported
> through the Fault Recording
> Registers in a circular FIFO fashion starting from the Fault
> Recording Register referenced by the Fault
> Recording Index (FRI) field, until it finds a Fault Recording
> Register with no faults (F field Clear).
> 
> Best regards,
> Lu Baolu

-- 
Thanks,
 Dmitry

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Dmitry Safonov]

On Thu, 2018-05-03 at 09:32 +0800, Lu Baolu wrote:
> Hi,
> 
> On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
> > AFAICS, we're doing fault-clearing in a loop inside irq handler.
> > That means that while we're clearing if a fault raises, it'll make
> > an irq level triggered (or on edge) on lapic. So, whenever we
> > return
> > from the irq handler, irq will raise again.
> > 
> 
> Uhm, double checked with the spec. Interrupts should be generated
> since we always clear the fault overflow bit.
> 
> Anyway, we can't clear faults in a limited loop, as the spec says in
> 7.3.1:

Mind to elaborate?
ITOW, I do not see a contradiction. We're still clearing faults in FIFO
fashion. There is no limitation to do some spare work in between
clearings (return from interrupt, then fault again and continue).

> Software is expected to process the non-recoverable faults reported
> through the Fault Recording
> Registers in a circular FIFO fashion starting from the Fault
> Recording Register referenced by the Fault
> Recording Index (FRI) field, until it finds a Fault Recording
> Register with no faults (F field Clear).
> 
> Best regards,
> Lu Baolu

-- 
Thanks,
 Dmitry

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
> On Thu, 2018-05-03 at 07:49 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 05/02/2018 08:38 PM, Dmitry Safonov wrote:
>>> Hi Lu,
>>>
>>> On Wed, 2018-05-02 at 14:34 +0800, Lu Baolu wrote:
 Hi,

 On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
> Theoretically, on some machines faults might be generated
> faster
> than
> they're cleared by CPU.
 Is this a real case?
>>> No. 1/2 is a real case and this one was discussed on v3:
>>> lkml.kernel.org/r/<20180215191729.15777-1-d...@arista.com>
>>>
>>> It's not possible on my hw as far as I tried, but the discussion
>>> result
>>> was to fix this theoretical issue too.
>> If faults are generated faster than CPU can clear them, the PCIe
>> device should be in a very very bad state. How about disabling
>> the PCIe device and ask the administrator to replace it? Anyway,
>> I don't think that's goal of this patch series. :-)
> Uhm, yeah, my point is not about the number of faults, but about
> physical ability of iommu to generate faults faster than cpu processes
> them. I might be wrong that it's not possible (like low cpu freq?)
>
> But the number of interrupts might be high. It's like you've many
> mappings on iommu and PCIe device went off. It could be just a link
> flap. I think it makes sense not lockup on such occasions.
>
>  Let's limit the cleaning-loop by number of hw
> fault registers.
 Will this cause the fault recording registers full of faults,
 hence
 new faults will be dropped without logging?
>>> If faults come faster then they're being cleared - some of them
>>> will be
>>> dropped without logging. Not sure if it's worth to report all
>>> faults in
>>> such theoretical(!) situation.
>>> If amount of reported faults for such situation is not enough and
>>> it's
>>> worth to keep all the faults, then probably we should introduce a
>>> workqueue here (which I did in v1, but it was rejected by the
>>> reason
>>> that it will introduce some latency in fault reporting).
>>>
 And even worse, new faults will not generate interrupts?
>>> They will, we clear page fault overflow outside of the loop, so any
>>> new
>>> fault will raise interrupt, iiuc.
>>>
>> I am afraid that they might not generate interrupts any more.
>>
>> Say, the fault registers are full of events that are not cleared,
>> then a new fault comes. There is no room for this event and
>> hence the hardware might drop it silently.
> AFAICS, we're doing fault-clearing in a loop inside irq handler.
> That means that while we're clearing if a fault raises, it'll make
> an irq level triggered (or on edge) on lapic. So, whenever we return
> from the irq handler, irq will raise again.
>

Uhm, double checked with the spec. Interrupts should be generated
since we always clear the fault overflow bit.

Anyway, we can't clear faults in a limited loop, as the spec says in 7.3.1:

Software is expected to process the non-recoverable faults reported through the 
Fault Recording
Registers in a circular FIFO fashion starting from the Fault Recording Register 
referenced by the Fault
Recording Index (FRI) field, until it finds a Fault Recording Register with no 
faults (F field Clear).

Best regards,
Lu Baolu

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/03/2018 08:52 AM, Dmitry Safonov wrote:
> On Thu, 2018-05-03 at 07:49 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 05/02/2018 08:38 PM, Dmitry Safonov wrote:
>>> Hi Lu,
>>>
>>> On Wed, 2018-05-02 at 14:34 +0800, Lu Baolu wrote:
 Hi,

 On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
> Theoretically, on some machines faults might be generated
> faster
> than
> they're cleared by CPU.
 Is this a real case?
>>> No. 1/2 is a real case and this one was discussed on v3:
>>> lkml.kernel.org/r/<20180215191729.15777-1-d...@arista.com>
>>>
>>> It's not possible on my hw as far as I tried, but the discussion
>>> result
>>> was to fix this theoretical issue too.
>> If faults are generated faster than CPU can clear them, the PCIe
>> device should be in a very very bad state. How about disabling
>> the PCIe device and ask the administrator to replace it? Anyway,
>> I don't think that's goal of this patch series. :-)
> Uhm, yeah, my point is not about the number of faults, but about
> physical ability of iommu to generate faults faster than cpu processes
> them. I might be wrong that it's not possible (like low cpu freq?)
>
> But the number of interrupts might be high. It's like you've many
> mappings on iommu and PCIe device went off. It could be just a link
> flap. I think it makes sense not lockup on such occasions.
>
>  Let's limit the cleaning-loop by number of hw
> fault registers.
 Will this cause the fault recording registers full of faults,
 hence
 new faults will be dropped without logging?
>>> If faults come faster then they're being cleared - some of them
>>> will be
>>> dropped without logging. Not sure if it's worth to report all
>>> faults in
>>> such theoretical(!) situation.
>>> If amount of reported faults for such situation is not enough and
>>> it's
>>> worth to keep all the faults, then probably we should introduce a
>>> workqueue here (which I did in v1, but it was rejected by the
>>> reason
>>> that it will introduce some latency in fault reporting).
>>>
 And even worse, new faults will not generate interrupts?
>>> They will, we clear page fault overflow outside of the loop, so any
>>> new
>>> fault will raise interrupt, iiuc.
>>>
>> I am afraid that they might not generate interrupts any more.
>>
>> Say, the fault registers are full of events that are not cleared,
>> then a new fault comes. There is no room for this event and
>> hence the hardware might drop it silently.
> AFAICS, we're doing fault-clearing in a loop inside irq handler.
> That means that while we're clearing if a fault raises, it'll make
> an irq level triggered (or on edge) on lapic. So, whenever we return
> from the irq handler, irq will raise again.
>

Uhm, double checked with the spec. Interrupts should be generated
since we always clear the fault overflow bit.

Anyway, we can't clear faults in a limited loop, as the spec says in 7.3.1:

Software is expected to process the non-recoverable faults reported through the 
Fault Recording
Registers in a circular FIFO fashion starting from the Fault Recording Register 
referenced by the Fault
Recording Index (FRI) field, until it finds a Fault Recording Register with no 
faults (F field Clear).

Best regards,
Lu Baolu

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Dmitry Safonov]

On Thu, 2018-05-03 at 07:49 +0800, Lu Baolu wrote:
> Hi,
> 
> On 05/02/2018 08:38 PM, Dmitry Safonov wrote:
> > Hi Lu,
> > 
> > On Wed, 2018-05-02 at 14:34 +0800, Lu Baolu wrote:
> > > Hi,
> > > 
> > > On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
> > > > Theoretically, on some machines faults might be generated
> > > > faster
> > > > than
> > > > they're cleared by CPU.
> > > 
> > > Is this a real case?
> > 
> > No. 1/2 is a real case and this one was discussed on v3:
> > lkml.kernel.org/r/<20180215191729.15777-1-d...@arista.com>
> > 
> > It's not possible on my hw as far as I tried, but the discussion
> > result
> > was to fix this theoretical issue too.
> 
> If faults are generated faster than CPU can clear them, the PCIe
> device should be in a very very bad state. How about disabling
> the PCIe device and ask the administrator to replace it? Anyway,
> I don't think that's goal of this patch series. :-)

Uhm, yeah, my point is not about the number of faults, but about
physical ability of iommu to generate faults faster than cpu processes
them. I might be wrong that it's not possible (like low cpu freq?)

But the number of interrupts might be high. It's like you've many
mappings on iommu and PCIe device went off. It could be just a link
flap. I think it makes sense not lockup on such occasions.

> > > >  Let's limit the cleaning-loop by number of hw
> > > > fault registers.
> > > 
> > > Will this cause the fault recording registers full of faults,
> > > hence
> > > new faults will be dropped without logging?
> > 
> > If faults come faster then they're being cleared - some of them
> > will be
> > dropped without logging. Not sure if it's worth to report all
> > faults in
> > such theoretical(!) situation.
> > If amount of reported faults for such situation is not enough and
> > it's
> > worth to keep all the faults, then probably we should introduce a
> > workqueue here (which I did in v1, but it was rejected by the
> > reason
> > that it will introduce some latency in fault reporting).
> > 
> > > And even worse, new faults will not generate interrupts?
> > 
> > They will, we clear page fault overflow outside of the loop, so any
> > new
> > fault will raise interrupt, iiuc.
> > 
> 
> I am afraid that they might not generate interrupts any more.
> 
> Say, the fault registers are full of events that are not cleared,
> then a new fault comes. There is no room for this event and
> hence the hardware might drop it silently.

AFAICS, we're doing fault-clearing in a loop inside irq handler.
That means that while we're clearing if a fault raises, it'll make
an irq level triggered (or on edge) on lapic. So, whenever we return
from the irq handler, irq will raise again.

-- 
Thanks,
 Dmitry

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Dmitry Safonov]

On Thu, 2018-05-03 at 07:49 +0800, Lu Baolu wrote:
> Hi,
> 
> On 05/02/2018 08:38 PM, Dmitry Safonov wrote:
> > Hi Lu,
> > 
> > On Wed, 2018-05-02 at 14:34 +0800, Lu Baolu wrote:
> > > Hi,
> > > 
> > > On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
> > > > Theoretically, on some machines faults might be generated
> > > > faster
> > > > than
> > > > they're cleared by CPU.
> > > 
> > > Is this a real case?
> > 
> > No. 1/2 is a real case and this one was discussed on v3:
> > lkml.kernel.org/r/<20180215191729.15777-1-d...@arista.com>
> > 
> > It's not possible on my hw as far as I tried, but the discussion
> > result
> > was to fix this theoretical issue too.
> 
> If faults are generated faster than CPU can clear them, the PCIe
> device should be in a very very bad state. How about disabling
> the PCIe device and ask the administrator to replace it? Anyway,
> I don't think that's goal of this patch series. :-)

Uhm, yeah, my point is not about the number of faults, but about
physical ability of iommu to generate faults faster than cpu processes
them. I might be wrong that it's not possible (like low cpu freq?)

But the number of interrupts might be high. It's like you've many
mappings on iommu and PCIe device went off. It could be just a link
flap. I think it makes sense not lockup on such occasions.

> > > >  Let's limit the cleaning-loop by number of hw
> > > > fault registers.
> > > 
> > > Will this cause the fault recording registers full of faults,
> > > hence
> > > new faults will be dropped without logging?
> > 
> > If faults come faster then they're being cleared - some of them
> > will be
> > dropped without logging. Not sure if it's worth to report all
> > faults in
> > such theoretical(!) situation.
> > If amount of reported faults for such situation is not enough and
> > it's
> > worth to keep all the faults, then probably we should introduce a
> > workqueue here (which I did in v1, but it was rejected by the
> > reason
> > that it will introduce some latency in fault reporting).
> > 
> > > And even worse, new faults will not generate interrupts?
> > 
> > They will, we clear page fault overflow outside of the loop, so any
> > new
> > fault will raise interrupt, iiuc.
> > 
> 
> I am afraid that they might not generate interrupts any more.
> 
> Say, the fault registers are full of events that are not cleared,
> then a new fault comes. There is no room for this event and
> hence the hardware might drop it silently.

AFAICS, we're doing fault-clearing in a loop inside irq handler.
That means that while we're clearing if a fault raises, it'll make
an irq level triggered (or on edge) on lapic. So, whenever we return
from the irq handler, irq will raise again.

-- 
Thanks,
 Dmitry

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/02/2018 08:38 PM, Dmitry Safonov wrote:
> Hi Lu,
>
> On Wed, 2018-05-02 at 14:34 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
>>> Theoretically, on some machines faults might be generated faster
>>> than
>>> they're cleared by CPU.
>> Is this a real case?
> No. 1/2 is a real case and this one was discussed on v3:
> lkml.kernel.org/r/<20180215191729.15777-1-d...@arista.com>
>
> It's not possible on my hw as far as I tried, but the discussion result
> was to fix this theoretical issue too.

If faults are generated faster than CPU can clear them, the PCIe
device should be in a very very bad state. How about disabling
the PCIe device and ask the administrator to replace it? Anyway,
I don't think that's goal of this patch series. :-)

>
>>>  Let's limit the cleaning-loop by number of hw
>>> fault registers.
>> Will this cause the fault recording registers full of faults, hence
>> new faults will be dropped without logging?
> If faults come faster then they're being cleared - some of them will be
> dropped without logging. Not sure if it's worth to report all faults in
> such theoretical(!) situation.
> If amount of reported faults for such situation is not enough and it's
> worth to keep all the faults, then probably we should introduce a
> workqueue here (which I did in v1, but it was rejected by the reason
> that it will introduce some latency in fault reporting).
>
>> And even worse, new faults will not generate interrupts?
> They will, we clear page fault overflow outside of the loop, so any new
> fault will raise interrupt, iiuc.
>

I am afraid that they might not generate interrupts any more.

Say, the fault registers are full of events that are not cleared,
then a new fault comes. There is no room for this event and
hence the hardware might drop it silently.

Best regards,
Lu Baolu

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 05/02/2018 08:38 PM, Dmitry Safonov wrote:
> Hi Lu,
>
> On Wed, 2018-05-02 at 14:34 +0800, Lu Baolu wrote:
>> Hi,
>>
>> On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
>>> Theoretically, on some machines faults might be generated faster
>>> than
>>> they're cleared by CPU.
>> Is this a real case?
> No. 1/2 is a real case and this one was discussed on v3:
> lkml.kernel.org/r/<20180215191729.15777-1-d...@arista.com>
>
> It's not possible on my hw as far as I tried, but the discussion result
> was to fix this theoretical issue too.

If faults are generated faster than CPU can clear them, the PCIe
device should be in a very very bad state. How about disabling
the PCIe device and ask the administrator to replace it? Anyway,
I don't think that's goal of this patch series. :-)

>
>>>  Let's limit the cleaning-loop by number of hw
>>> fault registers.
>> Will this cause the fault recording registers full of faults, hence
>> new faults will be dropped without logging?
> If faults come faster then they're being cleared - some of them will be
> dropped without logging. Not sure if it's worth to report all faults in
> such theoretical(!) situation.
> If amount of reported faults for such situation is not enough and it's
> worth to keep all the faults, then probably we should introduce a
> workqueue here (which I did in v1, but it was rejected by the reason
> that it will introduce some latency in fault reporting).
>
>> And even worse, new faults will not generate interrupts?
> They will, we clear page fault overflow outside of the loop, so any new
> fault will raise interrupt, iiuc.
>

I am afraid that they might not generate interrupts any more.

Say, the fault registers are full of events that are not cleared,
then a new fault comes. There is no room for this event and
hence the hardware might drop it silently.

Best regards,
Lu Baolu

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Dmitry Safonov]

Hi Lu,

On Wed, 2018-05-02 at 14:34 +0800, Lu Baolu wrote:
> Hi,
> 
> On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
> > Theoretically, on some machines faults might be generated faster
> > than
> > they're cleared by CPU.
> 
> Is this a real case?

No. 1/2 is a real case and this one was discussed on v3:
lkml.kernel.org/r/<20180215191729.15777-1-d...@arista.com>

It's not possible on my hw as far as I tried, but the discussion result
was to fix this theoretical issue too.

> 
> >  Let's limit the cleaning-loop by number of hw
> > fault registers.
> 
> Will this cause the fault recording registers full of faults, hence
> new faults will be dropped without logging?

If faults come faster then they're being cleared - some of them will be
dropped without logging. Not sure if it's worth to report all faults in
such theoretical(!) situation.
If amount of reported faults for such situation is not enough and it's
worth to keep all the faults, then probably we should introduce a
workqueue here (which I did in v1, but it was rejected by the reason
that it will introduce some latency in fault reporting).

> And even worse, new faults will not generate interrupts?

They will, we clear page fault overflow outside of the loop, so any new
fault will raise interrupt, iiuc.

-- 
Thanks,
 Dmitry

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Dmitry Safonov]

Hi Lu,

On Wed, 2018-05-02 at 14:34 +0800, Lu Baolu wrote:
> Hi,
> 
> On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
> > Theoretically, on some machines faults might be generated faster
> > than
> > they're cleared by CPU.
> 
> Is this a real case?

No. 1/2 is a real case and this one was discussed on v3:
lkml.kernel.org/r/<20180215191729.15777-1-d...@arista.com>

It's not possible on my hw as far as I tried, but the discussion result
was to fix this theoretical issue too.

> 
> >  Let's limit the cleaning-loop by number of hw
> > fault registers.
> 
> Will this cause the fault recording registers full of faults, hence
> new faults will be dropped without logging?

If faults come faster then they're being cleared - some of them will be
dropped without logging. Not sure if it's worth to report all faults in
such theoretical(!) situation.
If amount of reported faults for such situation is not enough and it's
worth to keep all the faults, then probably we should introduce a
workqueue here (which I did in v1, but it was rejected by the reason
that it will introduce some latency in fault reporting).

> And even worse, new faults will not generate interrupts?

They will, we clear page fault overflow outside of the loop, so any new
fault will raise interrupt, iiuc.

-- 
Thanks,
 Dmitry

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
> Theoretically, on some machines faults might be generated faster than
> they're cleared by CPU.

Is this a real case?

>  Let's limit the cleaning-loop by number of hw
> fault registers.

Will this cause the fault recording registers full of faults, hence new
faults will be dropped without logging? And even worse, new faults
will not generate interrupts?

Best regards,
Lu Baolu

>
> Cc: Alex Williamson 
> Cc: David Woodhouse 
> Cc: Ingo Molnar 
> Cc: Joerg Roedel 
> Cc: Lu Baolu 
> Cc: io...@lists.linux-foundation.org
> Signed-off-by: Dmitry Safonov 
> ---
>  drivers/iommu/dmar.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 6c4ea32ee6a9..cf1105111209 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -1615,7 +1615,7 @@ static int dmar_fault_do_one(struct intel_iommu *iommu, 
> int type,
>  irqreturn_t dmar_fault(int irq, void *dev_id)
>  {
>   struct intel_iommu *iommu = dev_id;
> - int reg, fault_index;
> + int reg, fault_index, i;
>   u32 fault_status;
>   unsigned long flag;
>   static DEFINE_RATELIMIT_STATE(rs,
> @@ -1633,7 +1633,7 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
>  
>   fault_index = dma_fsts_fault_record_index(fault_status);
>   reg = cap_fault_reg_offset(iommu->cap);
> - while (1) {
> + for (i = 0; i < cap_num_fault_regs(iommu->cap); i++) {
>   /* Disable printing, simply clear the fault when ratelimited */
>   bool ratelimited = !__ratelimit();
>   u8 fault_reason;

Re: [PATCHv4 2/2] iommu/vt-d: Limit number of faults to clear in irq handler

[Lu Baolu]

Hi,

On 03/31/2018 08:33 AM, Dmitry Safonov wrote:
> Theoretically, on some machines faults might be generated faster than
> they're cleared by CPU.

Is this a real case?

>  Let's limit the cleaning-loop by number of hw
> fault registers.

Will this cause the fault recording registers full of faults, hence new
faults will be dropped without logging? And even worse, new faults
will not generate interrupts?

Best regards,
Lu Baolu

>
> Cc: Alex Williamson 
> Cc: David Woodhouse 
> Cc: Ingo Molnar 
> Cc: Joerg Roedel 
> Cc: Lu Baolu 
> Cc: io...@lists.linux-foundation.org
> Signed-off-by: Dmitry Safonov 
> ---
>  drivers/iommu/dmar.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 6c4ea32ee6a9..cf1105111209 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -1615,7 +1615,7 @@ static int dmar_fault_do_one(struct intel_iommu *iommu, 
> int type,
>  irqreturn_t dmar_fault(int irq, void *dev_id)
>  {
>   struct intel_iommu *iommu = dev_id;
> - int reg, fault_index;
> + int reg, fault_index, i;
>   u32 fault_status;
>   unsigned long flag;
>   static DEFINE_RATELIMIT_STATE(rs,
> @@ -1633,7 +1633,7 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
>  
>   fault_index = dma_fsts_fault_record_index(fault_status);
>   reg = cap_fault_reg_offset(iommu->cap);
> - while (1) {
> + for (i = 0; i < cap_num_fault_regs(iommu->cap); i++) {
>   /* Disable printing, simply clear the fault when ratelimited */
>   bool ratelimited = !__ratelimit();
>   u8 fault_reason;