Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On 12/03/18 02:33, Yang, Shunyong wrote:
> Hi, Marc,
> 
> On Sun, 2018-03-11 at 12:17 +, Marc Zyngier wrote:
>> On Sun, 11 Mar 2018 01:55:08 +
>> Christoffer Dall  wrote:
>>
>>>
>>> On Sat, Mar 10, 2018 at 12:20 PM, Marc Zyngier >> m> wrote:

 On Fri, 09 Mar 2018 21:36:12 +,
 Christoffer Dall wrote:  
>
>
> On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:  
>>
>> I'd be more confident if we did forbid P+A for such
>> interrupts
>> altogether, as they really feel like another kind of HW
>> interrupt.  
> How about a slightly bigger hammer:  Can we avoid doing P+A for
> level
> interrupts completely?  I don't think that really makes much
> sense, and
> I think we simply everything if we just come back out and
> resample the
> line.  For an edge, something like a network card, there's a
> potential
> performance win to appending a new pending state, but I doubt
> that this
> is the case for level interrupts.  
 I started implementing the same thing yesterday. Somehow, it
 feels
 slightly better to have the same flow for all level interrupts,
 including the timer, and we only use the MI on EOI as a way to
 trigger
 the next state of injection. Still testing, but looking good so
 far.

 I'm still puzzled that we have this level-but-not-quite behaviour
 for
 VFIO interrupts. At some point, it is going to bite us badly.
  
>>> Where is the departure from level-triggered behavior with VFIO?  As
>>> far as I can tell, the GIC flow of the interrupts will be just a
>>> level
>>> interrupt, 
>> The GIC is fine, I believe. What is not exactly fine is the
>> signalling
>> from the device, which will never be dropped until the EOI has been
>> detected.
>>
>>>
>>> but we just need to make sure the resamplefd mechanism is
>>> supported for both types of interrupts.  Whether or not that's a
>>> decent mechanism seems orthogonal to me, but that's a discussion
>>> for
>>> another day I think.
>> Given that VFIO is built around this mechanism, I don't think we have
>> a
>> choice but to support it. Anyway, I came up with the following patch,
>> which I tested on Seattle with mtty. It also survived my usual
>> hammering of cyclictest, hackbench  and bulk VM installs.
>>
>> Shunyong, could you please give it a go?
>>
>> Thanks,
>>
>>  M.
>>
> 
> I have tested the patch. It works on QDF2400 platform
> and kvm_notify_acked_irq() is called when state is idle.

Thanks a lot for testing.

> 
> BTW, I have following questions when I was debugging the issue.
> Coud you please give me some help?
> 1)what does "mi" mean in gic code? such as lr_signals_eoi_mi();

MI stands for Maintenance Interrupts. Life is too short to write that
all the time ;-)

> 2)In some __hyp_text code where printk() will cause "HYP panic:", such
> as in __kvm_vcpu_run(). How can I output debug information?

You can't. None of the kernel is mapped at EL2 on pre-VHE hardware.
You'll need to find indirect ways of outputting information (store data
in memory, and output it once you're back to EL1).

Thanks again,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Yang, Shunyong]

Hi, Marc,

On Sun, 2018-03-11 at 12:17 +, Marc Zyngier wrote:
> On Sun, 11 Mar 2018 01:55:08 +
> Christoffer Dall  wrote:
> 
> > 
> > On Sat, Mar 10, 2018 at 12:20 PM, Marc Zyngier  > m> wrote:
> > > 
> > > On Fri, 09 Mar 2018 21:36:12 +,
> > > Christoffer Dall wrote:  
> > > > 
> > > > 
> > > > On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:  
> > > > > 
> > > > > I'd be more confident if we did forbid P+A for such
> > > > > interrupts
> > > > > altogether, as they really feel like another kind of HW
> > > > > interrupt.  
> > > > How about a slightly bigger hammer:  Can we avoid doing P+A for
> > > > level
> > > > interrupts completely?  I don't think that really makes much
> > > > sense, and
> > > > I think we simply everything if we just come back out and
> > > > resample the
> > > > line.  For an edge, something like a network card, there's a
> > > > potential
> > > > performance win to appending a new pending state, but I doubt
> > > > that this
> > > > is the case for level interrupts.  
> > > I started implementing the same thing yesterday. Somehow, it
> > > feels
> > > slightly better to have the same flow for all level interrupts,
> > > including the timer, and we only use the MI on EOI as a way to
> > > trigger
> > > the next state of injection. Still testing, but looking good so
> > > far.
> > > 
> > > I'm still puzzled that we have this level-but-not-quite behaviour
> > > for
> > > VFIO interrupts. At some point, it is going to bite us badly.
> > >  
> > Where is the departure from level-triggered behavior with VFIO?  As
> > far as I can tell, the GIC flow of the interrupts will be just a
> > level
> > interrupt, 
> The GIC is fine, I believe. What is not exactly fine is the
> signalling
> from the device, which will never be dropped until the EOI has been
> detected.
> 
> > 
> > but we just need to make sure the resamplefd mechanism is
> > supported for both types of interrupts.  Whether or not that's a
> > decent mechanism seems orthogonal to me, but that's a discussion
> > for
> > another day I think.
> Given that VFIO is built around this mechanism, I don't think we have
> a
> choice but to support it. Anyway, I came up with the following patch,
> which I tested on Seattle with mtty. It also survived my usual
> hammering of cyclictest, hackbench  and bulk VM installs.
> 
> Shunyong, could you please give it a go?
> 
> Thanks,
> 
>   M.
> 

I have tested the patch. It works on QDF2400 platform
and kvm_notify_acked_irq() is called when state is idle.

BTW, I have following questions when I was debugging the issue.
Coud you please give me some help?
1)what does "mi" mean in gic code? such as lr_signals_eoi_mi();
2)In some __hyp_text code where printk() will cause "HYP panic:", such
as in __kvm_vcpu_run(). How can I output debug information?

Thanks.
Shunyong.


> From 9ca96b9fb535cc6ab578bda85c4ecbc4a8c63cd7 Mon Sep 17 00:00:00
> 2001
> From: Marc Zyngier 
> Date: Fri, 9 Mar 2018 14:59:40 +
> Subject: [PATCH] KVM: arm/arm64: vgic: Disallow Active+Pending for
> level
>  interrupts
> 
> It was recently reported that VFIO mediated devices, and anything
> that VFIO exposes as level interrupts, do no strictly follow the
> expected logic of such interrupts as it only lowers the input
> line when the guest has EOId the interrupt at the GIC level, rather
> than when it Acked the interrupt at the device level.
> 
> The GIC's Active+Pending state is fundamentally incompatible with
> this behaviour, as it prevents KVM from observing the EOI, and in
> turn results in VFIO never dropping the line. This results in an
> interrupt storm in the guest, which it really never expected.
> 
> As we cannot really change VFIO to follow the strict rules of level
> signalling, let's forbid the A+P state altogether, as it is in the
> end only an optimization. It ensures that we will transition via
> an invalid state, which we can use to notify VFIO of the EOI.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  virt/kvm/arm/vgic/vgic-v2.c | 47 +++--
> 
>  virt/kvm/arm/vgic/vgic-v3.c | 47 +++--
> 
>  2 files changed, 56 insertions(+), 38 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-
> v2.c
> index 29556f71b691..9356d749da1d 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -153,8 +153,35 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu
> *vcpu)
>  void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq
> *irq, int lr)
>  {
>   u32 val = irq->intid;
> + bool allow_pending = true;
>  
> - if (irq_is_pending(irq)) {
> + if (irq->active)
> + val |= GICH_LR_ACTIVE_BIT;
> +
> + if (irq->hw) {
> + val |= GICH_LR_HW;
> + val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
> + /*
> +  * 

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Christoffer Dall]

On Sat, Mar 10, 2018 at 12:20 PM, Marc Zyngier  wrote:
> On Fri, 09 Mar 2018 21:36:12 +,
> Christoffer Dall wrote:
>>
>> On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:
>> > I'd be more confident if we did forbid P+A for such interrupts
>> > altogether, as they really feel like another kind of HW interrupt.
>>
>> How about a slightly bigger hammer:  Can we avoid doing P+A for level
>> interrupts completely?  I don't think that really makes much sense, and
>> I think we simply everything if we just come back out and resample the
>> line.  For an edge, something like a network card, there's a potential
>> performance win to appending a new pending state, but I doubt that this
>> is the case for level interrupts.
>
> I started implementing the same thing yesterday. Somehow, it feels
> slightly better to have the same flow for all level interrupts,
> including the timer, and we only use the MI on EOI as a way to trigger
> the next state of injection. Still testing, but looking good so far.
>
> I'm still puzzled that we have this level-but-not-quite behaviour for
> VFIO interrupts. At some point, it is going to bite us badly.
>

Where is the departure from level-triggered behavior with VFIO?  As
far as I can tell, the GIC flow of the interrupts will be just a level
interrupt, but we just need to make sure the resamplefd mechanism is
supported for both types of interrupts.  Whether or not that's a
decent mechanism seems orthogonal to me, but that's a discussion for
another day I think.

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On Fri, 09 Mar 2018 21:36:12 +,
Christoffer Dall wrote:
> 
> On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:
> > I'd be more confident if we did forbid P+A for such interrupts
> > altogether, as they really feel like another kind of HW interrupt.
> 
> How about a slightly bigger hammer:  Can we avoid doing P+A for level
> interrupts completely?  I don't think that really makes much sense, and
> I think we simply everything if we just come back out and resample the
> line.  For an edge, something like a network card, there's a potential
> performance win to appending a new pending state, but I doubt that this
> is the case for level interrupts.

I started implementing the same thing yesterday. Somehow, it feels
slightly better to have the same flow for all level interrupts,
including the timer, and we only use the MI on EOI as a way to trigger
the next state of injection. Still testing, but looking good so far.

I'm still puzzled that we have this level-but-not-quite behaviour for
VFIO interrupts. At some point, it is going to bite us badly.

M.

-- 
Jazz is not dead, it just smell funny.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Christoffer Dall]

On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:
> On Thu, 08 Mar 2018 16:19:00 +,
> Christoffer Dall wrote:
> > 
> > On Thu, Mar 08, 2018 at 11:54:27AM +, Marc Zyngier wrote:
> > > On 08/03/18 09:49, Marc Zyngier wrote:

[...]

> > > The state is now pending, we've really EOI'd the interrupt, and
> > > yet lr_signals_eoi_mi() returns false, since the state is not 0.
> > > The result is that we won't signal anything on the corresponding
> > > irqfd, which people complain about. Meh.
> > 
> > So the core of the problem is that when we've entered the guest with
> > PENDING+ACTIVE and when we exit (for some reason) we don't signal the
> > resamplefd, right?  The solution seems to me that we don't ever do
> > PENDING+ACTIVE if you need to resample after each deactivate.  What
> > would be the point of appending a pending state that you only know to be
> > valid after a resample anyway?
> 
> The question is then to identify that a given source needs to be
> signalled back to VFIO. Calling into the eventfd code on the hot path
> is pretty horrid (I'm not sure if we can really call into this with
> interrupts disabled, for example).
> 

This feels like a bad layering violation to me as well.

> > 
> > > 
> > > Example 2:
> > > P+A -> guest EOI -> P -> delayed MI -> guest IAR -> A -> MI fires
> > 
> > We could be more clever and do the following calculation on every exit:
> > 
> > If you enter with P, and exit with either A or 0, then signal.
> > 
> > If you enter with P+A, and you exit with either P, A, or 0, then signal.
> > 
> > Wouldn't that also solve it?  (Although I have a feeling you'd miss some
> > exits in this case).
> 
> I'd be more confident if we did forbid P+A for such interrupts
> altogether, as they really feel like another kind of HW interrupt.

How about a slightly bigger hammer:  Can we avoid doing P+A for level
interrupts completely?  I don't think that really makes much sense, and
I think we simply everything if we just come back out and resample the
line.  For an edge, something like a network card, there's a potential
performance win to appending a new pending state, but I doubt that this
is the case for level interrupts.

The timer would be unaffected, because it's a HW interrupt.

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On 09/03/18 13:10, Auger Eric wrote:
> Hi Marc,
> 
> On 09/03/18 10:40, Marc Zyngier wrote:
>> On 09/03/18 03:14, Yang, Shunyong wrote:
>>
>> [trimming things a bit]
>>
>  static bool lr_signals_eoi_mi(u32 lr_val)
>  {
> - return !(lr_val & GICH_LR_STATE) && (lr_val &
> GICH_LR_EOI) &&
> -!(lr_val & GICH_LR_HW);
> + return !((lr_val & GICH_LR_STATE) ^ GICH_LR_STATE)
> &&
 That feels very wrong. You're now signalling the resampling
 in both
 invalid and pending+active, and the latter state doesn't mean
 you've
 EOIed anything. You're now over-signalling, and signalling
 the
 wrong event.
>>>
>>> I am using XOR GICH_LR_STATE(0b'11), so only 0b'11(P) will be
>>> signaled. Other state will be false.
>>
>> And that's really wrong. P+A is a state where the interrupt is still
>> being processed. The only case where we can reliably detect that an
>> interrupt has been EOId is when state==0.
>>
>>> And I am curious why the EOI bit in LR indicate the end of interrupt
>>> regardless of the state? Please bear with me as I am a newbie in this
>>> part.
>>
>> The EOI bit indicates that we've requested a maintenance interrupt from
>> the HW. It only triggers when state==0. If you have (like you describe
>> further down) a sequence of
>>
>> P -> A -> (exit) -> P+A -> P -> A -> (exit) P+A ...
>>
>> we can never reliably detect that an interrupt has been EOId (because
>> the HW never delivers a maintenance interrupt), other than by tracking
>> the states before and after exit, and hoping that you've done an exit
>> because you're touching the source of the interrupt.
>>
>>> Also, any guideline on how to reproduce this would be much
>>> appreciated.
>>> I never used this mdev/mtty thing, so please bear with me.
>>>
>>> Thanks,
>>>
>>> M.
>>>
>>> The mdev/mtty documentation is at Documentation/vfio-mediated-
>>> device.txt. It docmented how to enable mtty device.
>>> And support for "vfio-pci,sysfsdev" should be availabe in your qemu
>>> version (I compiled the latest version).
>>> Following is my commond to run qemu with mdev support,
>>> "qemu-system-aarch64 -m 1024 -cpu host -M virt,gic_version=3 -nographic
>>> \
>>> -kernel /home/yangsy/up-kvm/arch/arm64/boot/Image.gz \
>>> -initrd /home/yangsy/kvm/ramdisk/initrd.img \
>>> -netdev user,id=eth0 -device virtio-net-device,netdev=eth0 -enable-kvm
>>> \
>>> -append "root=/dev/ram rdinit=/sbin/init" \
>>> -device vfio-pci,sysfsdev=/sys/bus/mdev/devices/83b8f4f2-509f-382f-
>>> 3c1e-e6bfe0fa1001
>>> "
>>> For just test this vgic case, type "cat /dev/ttyS0" in guest. But if
>>> test read/write multiple bytes, please apply following patch also
>>> https://patchwork.kernel.org/patch/10267039/
>>
>> Thanks. I'll have a look.
>>
>>>
>>>
>>> From 66a7c4cfc1029b0169dd771e196e2876ba3f17b1 Mon Sep 17
>>> 00:00:00 2001
>>> From: Marc Zyngier 
>>> Date: Thu, 8 Mar 2018 11:14:06 +
>>> Subject: [PATCH] KVM: arm/arm64: Do not rely on LR state to
>>> guess EOI MI
>>>  status
>>>
>>> We so far rely on the LR state to decide whether the guest has
>>> EOI'd a level interrupt or not. While this looks like a good
>>> idea on the surface, it leads to a couple of annoying corner
>>> cases:
>>>
>>> Example 1: (P = Pending, A = Active, MI = Maintenance
>>> Interrupt)
>>> P -> guest IAR -> A -> exit/entry -> P+A -> guest EOI -> P ->
>>> MI
>> Do we really get an EOI maintenance interrupt here?  Reading the
>> MISR
>> and EISR descriptions make me thing this is not the case...
 Hum yes in EISR it is said that ICH_LR.State = 0b00!
>
>
> Yeah, it looks like I always want EISR to do what I want, and not
> to
> do what it does. Man, this thing is such a piece of crap.
>
> OK, scratch that. We need to do it without the help of the HW.
>>>
>>> If convenient, maybe we can get something from HW gus. :-)
>>>
>>> Hi, Marc,
>>>
>>> Do you need me to test the patch you posted for EISR? As it seems there
>>> are some things need more discussion.
>>
>> Yeah, that approach doesn't work. I'll try and come up with another
>> approach (basically banning P+A for interrupts that require a back
>> notification).
>>
>> [...]
>>
>>> I have added some logs to compare level interrupt between pl011(hwirq =
>>> 33) and mtty (hwirq = 36). In mtty case, vgic_queue_irq_unlock() is
>>> called twice. But only called once in pl011.
>>>
>>> following is the log,
>>> ===Without my patch===
>>> ###PL011###
>>>
>>> <4>[  180.598266] kvm_vgic_inject_irq 453 irq:33 enabled:1 config:1
>>> latch:0 level:1
>>> <4>[  180.604460] ##vgic_queue_irq_unlock 388 irq->intid:33 enable:1
>>> level:1
>>> <4>[  180.604540] ==>90a00221(active)
>>> <4>[  180.614878] ==>d0a00221(P)
>>> <4>[  180.618415] kvm_vgic_inject_irq 453 irq:33 

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Auger Eric]

Hi Marc,

On 09/03/18 10:12, Marc Zyngier wrote:
> On 08/03/18 18:12, Auger Eric wrote:
>> Hi Marc, Christoffer,
>>
>> On 08/03/18 18:28, Marc Zyngier wrote:
>>> On Thu, 08 Mar 2018 16:19:00 +,
>>> Christoffer Dall wrote:

 On Thu, Mar 08, 2018 at 11:54:27AM +, Marc Zyngier wrote:
> On 08/03/18 09:49, Marc Zyngier wrote:
>> [updated Christoffer's email address]
>>
>> Hi Shunyong,
>>
>> On 08/03/18 07:01, Shunyong Yang wrote:
>>> When resampling irqfds is enabled, level interrupt should be
>>> de-asserted when resampling happens. On page 4-47 of GIC v3
>>> specification IHI0069D, it said,
>>> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
>>> interface, the IRI changes the status of the interrupt to active
>>> and pending if:
>>> • It is an edge-triggered interrupt, and another edge has been
>>> detected since the interrupt was acknowledged.
>>> • It is a level-sensitive interrupt, and the level has not been
>>> deasserted since the interrupt was acknowledged."
>>>
>>> GIC v2 specification IHI0048B.b has similar description on page
>>> 3-42 for state machine transition.
>>>
>>> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
>>> in samples/vfio-mdev) triggers a level interrupt, the status
>>> transition in LR is pending-->active-->active and pending.
>>> Then it will wait resampling to de-assert the interrupt.
>>>
>>> Current design of lr_signals_eoi_mi() will return false if state
>>> in LR is not invalid(Inactive). It causes resampling will not happen
>>> in mtty case.
>>
>> Let me rephrase this, and tell me if I understood it correctly:
>>
>> - A level interrupt is injected, activated by the guest (LR state=active)
>> - guest exits, re-enters, (LR state=pending+active)
>> - guest EOIs the interrupt (LR state=pending)
>> - maintenance interrupt
>> - we don't signal the resampling because we're not in an invalid state
>>
>> Is that correct?
>>
>> That's an interesting case, because it seems to invalidate some of the 
>> optimization that went in over a year ago.
>>
>> 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
>> b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary 
>> save_maint_int_state
>> af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary 
>> process_maintenance operation
>>
>> We could compare the value of the LR before the guest entry with
>> the value at exit time, but we still could miss it if we have a
>> transition such as P+A -> P -> A and assume a long enough propagation
>> delay for the maintenance interrupt (which is very likely).
>>
>> In essence, we have lost the benefit of EISR, which was to give us a
>> way to deal with asynchronous signalling.
>>
>>>
>>> This will cause interrupt fired continuously to guest even 8250 IIR
>>> has no interrupt. When 8250's interrupt is configured in shared mode,
>>> it will pass interrupt to other drivers to handle. However, there
>>> is no other driver involved. Then, a "nobody cared" kernel complaint
>>> occurs.
>>>
>>> / # cat /dev/ttyS0
>>> [4.826836] random: crng init done
>>> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
>>> option)
>>> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
>>> [6.378927] Hardware name: linux,dummy-virt (DT)
>>> [6.380876] Call trace:
>>> [6.381937]  dump_backtrace+0x0/0x180
>>> [6.383495]  show_stack+0x14/0x1c
>>> [6.384902]  dump_stack+0x90/0xb4
>>> [6.386312]  __report_bad_irq+0x38/0xe0
>>> [6.387944]  note_interrupt+0x1f4/0x2b8
>>> [6.389568]  handle_irq_event_percpu+0x54/0x7c
>>> [6.391433]  handle_irq_event+0x44/0x74
>>> [6.393056]  handle_fasteoi_irq+0x9c/0x154
>>> [6.394784]  generic_handle_irq+0x24/0x38
>>> [6.396483]  __handle_domain_irq+0x60/0xb4
>>> [6.398207]  gic_handle_irq+0x98/0x1b0
>>> [6.399796]  el1_irq+0xb0/0x128
>>> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
>>> [6.403149]  __setup_irq+0x41c/0x678
>>> [6.404669]  request_threaded_irq+0xe0/0x190
>>> [6.406474]  univ8250_setup_irq+0x208/0x234
>>> [6.408250]  serial8250_do_startup+0x1b4/0x754
>>> [6.410123]  serial8250_startup+0x20/0x28
>>> [6.411826]  uart_startup.part.21+0x78/0x144
>>> [6.413633]  uart_port_activate+0x50/0x68
>>> [6.415328]  tty_port_open+0x84/0xd4
>>> [6.416851]  uart_open+0x34/0x44
>>> [6.418229]  tty_open+0xec/0x3c8
>>> [6.419610]  chrdev_open+0xb0/0x198
>>> [6.421093]  do_dentry_open+0x200/0x310
>>> [6.422714]  vfs_open+0x54/0x84
>>> [6.424054]  path_openat+0x2dc/0xf04
>>> [6.425569]  

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Auger Eric]

Hi Marc,

On 09/03/18 10:40, Marc Zyngier wrote:
> On 09/03/18 03:14, Yang, Shunyong wrote:
> 
> [trimming things a bit]
> 
  static bool lr_signals_eoi_mi(u32 lr_val)
  {
 -  return !(lr_val & GICH_LR_STATE) && (lr_val &
 GICH_LR_EOI) &&
 - !(lr_val & GICH_LR_HW);
 +  return !((lr_val & GICH_LR_STATE) ^ GICH_LR_STATE)
 &&
>>> That feels very wrong. You're now signalling the resampling
>>> in both
>>> invalid and pending+active, and the latter state doesn't mean
>>> you've
>>> EOIed anything. You're now over-signalling, and signalling
>>> the
>>> wrong event.
>>
>> I am using XOR GICH_LR_STATE(0b'11), so only 0b'11(P) will be
>> signaled. Other state will be false.
> 
> And that's really wrong. P+A is a state where the interrupt is still
> being processed. The only case where we can reliably detect that an
> interrupt has been EOId is when state==0.
> 
>> And I am curious why the EOI bit in LR indicate the end of interrupt
>> regardless of the state? Please bear with me as I am a newbie in this
>> part.
> 
> The EOI bit indicates that we've requested a maintenance interrupt from
> the HW. It only triggers when state==0. If you have (like you describe
> further down) a sequence of
> 
> P -> A -> (exit) -> P+A -> P -> A -> (exit) P+A ...
> 
> we can never reliably detect that an interrupt has been EOId (because
> the HW never delivers a maintenance interrupt), other than by tracking
> the states before and after exit, and hoping that you've done an exit
> because you're touching the source of the interrupt.
> 
>> Also, any guideline on how to reproduce this would be much
>> appreciated.
>> I never used this mdev/mtty thing, so please bear with me.
>>
>> Thanks,
>>
>>  M.
>>
>> The mdev/mtty documentation is at Documentation/vfio-mediated-
>> device.txt. It docmented how to enable mtty device.
>> And support for "vfio-pci,sysfsdev" should be availabe in your qemu
>> version (I compiled the latest version).
>> Following is my commond to run qemu with mdev support,
>> "qemu-system-aarch64 -m 1024 -cpu host -M virt,gic_version=3 -nographic
>> \
>> -kernel /home/yangsy/up-kvm/arch/arm64/boot/Image.gz \
>> -initrd /home/yangsy/kvm/ramdisk/initrd.img \
>> -netdev user,id=eth0 -device virtio-net-device,netdev=eth0 -enable-kvm
>> \
>> -append "root=/dev/ram rdinit=/sbin/init" \
>> -device vfio-pci,sysfsdev=/sys/bus/mdev/devices/83b8f4f2-509f-382f-
>> 3c1e-e6bfe0fa1001
>> "
>> For just test this vgic case, type "cat /dev/ttyS0" in guest. But if
>> test read/write multiple bytes, please apply following patch also
>> https://patchwork.kernel.org/patch/10267039/
> 
> Thanks. I'll have a look.
> 
>>
>>
>> From 66a7c4cfc1029b0169dd771e196e2876ba3f17b1 Mon Sep 17
>> 00:00:00 2001
>> From: Marc Zyngier 
>> Date: Thu, 8 Mar 2018 11:14:06 +
>> Subject: [PATCH] KVM: arm/arm64: Do not rely on LR state to
>> guess EOI MI
>>  status
>>
>> We so far rely on the LR state to decide whether the guest has
>> EOI'd a level interrupt or not. While this looks like a good
>> idea on the surface, it leads to a couple of annoying corner
>> cases:
>>
>> Example 1: (P = Pending, A = Active, MI = Maintenance
>> Interrupt)
>> P -> guest IAR -> A -> exit/entry -> P+A -> guest EOI -> P ->
>> MI
> Do we really get an EOI maintenance interrupt here?  Reading the
> MISR
> and EISR descriptions make me thing this is not the case...
>>> Hum yes in EISR it is said that ICH_LR.State = 0b00!


 Yeah, it looks like I always want EISR to do what I want, and not
 to
 do what it does. Man, this thing is such a piece of crap.

 OK, scratch that. We need to do it without the help of the HW.
>>
>> If convenient, maybe we can get something from HW gus. :-)
>>
>> Hi, Marc,
>>
>> Do you need me to test the patch you posted for EISR? As it seems there
>> are some things need more discussion.
> 
> Yeah, that approach doesn't work. I'll try and come up with another
> approach (basically banning P+A for interrupts that require a back
> notification).
> 
> [...]
> 
>> I have added some logs to compare level interrupt between pl011(hwirq =
>> 33) and mtty (hwirq = 36). In mtty case, vgic_queue_irq_unlock() is
>> called twice. But only called once in pl011.
>>
>> following is the log,
>> ===Without my patch===
>> ###PL011###
>>
>> <4>[  180.598266] kvm_vgic_inject_irq 453 irq:33 enabled:1 config:1
>> latch:0 level:1
>> <4>[  180.604460] ##vgic_queue_irq_unlock 388 irq->intid:33 enable:1
>> level:1
>> <4>[  180.604540] ==>90a00221(active)
>> <4>[  180.614878] ==>d0a00221(P)
>> <4>[  180.618415] kvm_vgic_inject_irq 453 irq:33 enabled:1 config:1
>> latch:0 level:0
>> <4>[  180.625508] ==>90a00221(active)
>> <4>[  180.629343] ==>10a00221(inactive)
>>
>> ###mtty-vfio###
>> <4>[  

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On 09/03/18 03:14, Yang, Shunyong wrote:

[trimming things a bit]

>>>  static bool lr_signals_eoi_mi(u32 lr_val)
>>>  {
>>> -   return !(lr_val & GICH_LR_STATE) && (lr_val &
>>> GICH_LR_EOI) &&
>>> -      !(lr_val & GICH_LR_HW);
>>> +   return !((lr_val & GICH_LR_STATE) ^ GICH_LR_STATE)
>>> &&
>> That feels very wrong. You're now signalling the resampling
>> in both
>> invalid and pending+active, and the latter state doesn't mean
>> you've
>> EOIed anything. You're now over-signalling, and signalling
>> the
>> wrong event.
> 
> I am using XOR GICH_LR_STATE(0b'11), so only 0b'11(P) will be
> signaled. Other state will be false.

And that's really wrong. P+A is a state where the interrupt is still
being processed. The only case where we can reliably detect that an
interrupt has been EOId is when state==0.

> And I am curious why the EOI bit in LR indicate the end of interrupt
> regardless of the state? Please bear with me as I am a newbie in this
> part.

The EOI bit indicates that we've requested a maintenance interrupt from
the HW. It only triggers when state==0. If you have (like you describe
further down) a sequence of

P -> A -> (exit) -> P+A -> P -> A -> (exit) P+A ...

we can never reliably detect that an interrupt has been EOId (because
the HW never delivers a maintenance interrupt), other than by tracking
the states before and after exit, and hoping that you've done an exit
because you're touching the source of the interrupt.

> Also, any guideline on how to reproduce this would be much
> appreciated.
> I never used this mdev/mtty thing, so please bear with me.
>
> Thanks,
>
>   M.
> 
> The mdev/mtty documentation is at Documentation/vfio-mediated-
> device.txt. It docmented how to enable mtty device.
> And support for "vfio-pci,sysfsdev" should be availabe in your qemu
> version (I compiled the latest version).
> Following is my commond to run qemu with mdev support,
> "qemu-system-aarch64 -m 1024 -cpu host -M virt,gic_version=3 -nographic
> \
> -kernel /home/yangsy/up-kvm/arch/arm64/boot/Image.gz \
> -initrd /home/yangsy/kvm/ramdisk/initrd.img \
> -netdev user,id=eth0 -device virtio-net-device,netdev=eth0 -enable-kvm
> \
> -append "root=/dev/ram rdinit=/sbin/init" \
> -device vfio-pci,sysfsdev=/sys/bus/mdev/devices/83b8f4f2-509f-382f-
> 3c1e-e6bfe0fa1001
> "
> For just test this vgic case, type "cat /dev/ttyS0" in guest. But if
> test read/write multiple bytes, please apply following patch also
> https://patchwork.kernel.org/patch/10267039/

Thanks. I'll have a look.

> 
>
> From 66a7c4cfc1029b0169dd771e196e2876ba3f17b1 Mon Sep 17
> 00:00:00 2001
> From: Marc Zyngier 
> Date: Thu, 8 Mar 2018 11:14:06 +
> Subject: [PATCH] KVM: arm/arm64: Do not rely on LR state to
> guess EOI MI
>  status
>
> We so far rely on the LR state to decide whether the guest has
> EOI'd a level interrupt or not. While this looks like a good
> idea on the surface, it leads to a couple of annoying corner
> cases:
>
> Example 1: (P = Pending, A = Active, MI = Maintenance
> Interrupt)
> P -> guest IAR -> A -> exit/entry -> P+A -> guest EOI -> P ->
> MI
 Do we really get an EOI maintenance interrupt here?  Reading the
 MISR
 and EISR descriptions make me thing this is not the case...
>> Hum yes in EISR it is said that ICH_LR.State = 0b00!
>>>
>>>
>>> Yeah, it looks like I always want EISR to do what I want, and not
>>> to
>>> do what it does. Man, this thing is such a piece of crap.
>>>
>>> OK, scratch that. We need to do it without the help of the HW.
> 
> If convenient, maybe we can get something from HW gus. :-)
> 
> Hi, Marc,
> 
> Do you need me to test the patch you posted for EISR? As it seems there
> are some things need more discussion.

Yeah, that approach doesn't work. I'll try and come up with another
approach (basically banning P+A for interrupts that require a back
notification).

[...]

> I have added some logs to compare level interrupt between pl011(hwirq =
> 33) and mtty (hwirq = 36). In mtty case, vgic_queue_irq_unlock() is
> called twice. But only called once in pl011.
> 
> following is the log,
> ===Without my patch===
> ###PL011###
> 
> <4>[  180.598266] kvm_vgic_inject_irq 453 irq:33 enabled:1 config:1
> latch:0 level:1
> <4>[  180.604460] ##vgic_queue_irq_unlock 388 irq->intid:33 enable:1
> level:1
> <4>[  180.604540] ==>90a00221(active)
> <4>[  180.614878] ==>d0a00221(P)
> <4>[  180.618415] kvm_vgic_inject_irq 453 irq:33 enabled:1 config:1
> latch:0 level:0
> <4>[  180.625508] ==>90a00221(active)
> <4>[  180.629343] ==>10a00221(inactive)
> 
> ###mtty-vfio###
> <4>[  223.123329] kvm_vgic_inject_irq 453 irq:36 enabled:0 config:1
> latch:0 level:1
> <4>[  223.129736] ##vgic_queue_irq_unlock 388 irq->intid:36 enable:1
> level:1
> <4>[  223.136027] 

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On 08/03/18 18:12, Auger Eric wrote:
> Hi Marc, Christoffer,
> 
> On 08/03/18 18:28, Marc Zyngier wrote:
>> On Thu, 08 Mar 2018 16:19:00 +,
>> Christoffer Dall wrote:
>>>
>>> On Thu, Mar 08, 2018 at 11:54:27AM +, Marc Zyngier wrote:
 On 08/03/18 09:49, Marc Zyngier wrote:
> [updated Christoffer's email address]
>
> Hi Shunyong,
>
> On 08/03/18 07:01, Shunyong Yang wrote:
>> When resampling irqfds is enabled, level interrupt should be
>> de-asserted when resampling happens. On page 4-47 of GIC v3
>> specification IHI0069D, it said,
>> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
>> interface, the IRI changes the status of the interrupt to active
>> and pending if:
>> • It is an edge-triggered interrupt, and another edge has been
>> detected since the interrupt was acknowledged.
>> • It is a level-sensitive interrupt, and the level has not been
>> deasserted since the interrupt was acknowledged."
>>
>> GIC v2 specification IHI0048B.b has similar description on page
>> 3-42 for state machine transition.
>>
>> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
>> in samples/vfio-mdev) triggers a level interrupt, the status
>> transition in LR is pending-->active-->active and pending.
>> Then it will wait resampling to de-assert the interrupt.
>>
>> Current design of lr_signals_eoi_mi() will return false if state
>> in LR is not invalid(Inactive). It causes resampling will not happen
>> in mtty case.
>
> Let me rephrase this, and tell me if I understood it correctly:
>
> - A level interrupt is injected, activated by the guest (LR state=active)
> - guest exits, re-enters, (LR state=pending+active)
> - guest EOIs the interrupt (LR state=pending)
> - maintenance interrupt
> - we don't signal the resampling because we're not in an invalid state
>
> Is that correct?
>
> That's an interesting case, because it seems to invalidate some of the 
> optimization that went in over a year ago.
>
> 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
> b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary 
> save_maint_int_state
> af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary 
> process_maintenance operation
>
> We could compare the value of the LR before the guest entry with
> the value at exit time, but we still could miss it if we have a
> transition such as P+A -> P -> A and assume a long enough propagation
> delay for the maintenance interrupt (which is very likely).
>
> In essence, we have lost the benefit of EISR, which was to give us a
> way to deal with asynchronous signalling.
>
>>
>> This will cause interrupt fired continuously to guest even 8250 IIR
>> has no interrupt. When 8250's interrupt is configured in shared mode,
>> it will pass interrupt to other drivers to handle. However, there
>> is no other driver involved. Then, a "nobody cared" kernel complaint
>> occurs.
>>
>> / # cat /dev/ttyS0
>> [4.826836] random: crng init done
>> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
>> option)
>> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
>> [6.378927] Hardware name: linux,dummy-virt (DT)
>> [6.380876] Call trace:
>> [6.381937]  dump_backtrace+0x0/0x180
>> [6.383495]  show_stack+0x14/0x1c
>> [6.384902]  dump_stack+0x90/0xb4
>> [6.386312]  __report_bad_irq+0x38/0xe0
>> [6.387944]  note_interrupt+0x1f4/0x2b8
>> [6.389568]  handle_irq_event_percpu+0x54/0x7c
>> [6.391433]  handle_irq_event+0x44/0x74
>> [6.393056]  handle_fasteoi_irq+0x9c/0x154
>> [6.394784]  generic_handle_irq+0x24/0x38
>> [6.396483]  __handle_domain_irq+0x60/0xb4
>> [6.398207]  gic_handle_irq+0x98/0x1b0
>> [6.399796]  el1_irq+0xb0/0x128
>> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
>> [6.403149]  __setup_irq+0x41c/0x678
>> [6.404669]  request_threaded_irq+0xe0/0x190
>> [6.406474]  univ8250_setup_irq+0x208/0x234
>> [6.408250]  serial8250_do_startup+0x1b4/0x754
>> [6.410123]  serial8250_startup+0x20/0x28
>> [6.411826]  uart_startup.part.21+0x78/0x144
>> [6.413633]  uart_port_activate+0x50/0x68
>> [6.415328]  tty_port_open+0x84/0xd4
>> [6.416851]  uart_open+0x34/0x44
>> [6.418229]  tty_open+0xec/0x3c8
>> [6.419610]  chrdev_open+0xb0/0x198
>> [6.421093]  do_dentry_open+0x200/0x310
>> [6.422714]  vfs_open+0x54/0x84
>> [6.424054]  path_openat+0x2dc/0xf04
>> [6.425569]  do_filp_open+0x68/0xd8
>> [6.427044]  do_sys_open+0x16c/0x224
>> [6.428563]  SyS_openat+0x10/0x18
>> [6.429972]  el0_svc_naked+0x30/0x34

Re: Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Yang, Shunyong]

Hi, Eric, Marc and Christoffer,

On Thu, 2018-03-08 at 19:12 +0100, Auger Eric wrote:
> Hi Marc, Christoffer,
> 
> On 08/03/18 18:28, Marc Zyngier wrote:
> > 
> > On Thu, 08 Mar 2018 16:19:00 +,
> > Christoffer Dall wrote:
> > > 
> > > 
> > > On Thu, Mar 08, 2018 at 11:54:27AM +, Marc Zyngier wrote:
> > > > 
> > > > On 08/03/18 09:49, Marc Zyngier wrote:
> > > > > 
> > > > > [updated Christoffer's email address]
> > > > > 
> > > > > Hi Shunyong,
> > > > > 
> > > > > On 08/03/18 07:01, Shunyong Yang wrote:
> > > > > > 
> > > > > > When resampling irqfds is enabled, level interrupt should
> > > > > > be
> > > > > > de-asserted when resampling happens. On page 4-47 of GIC v3
> > > > > > specification IHI0069D, it said,
> > > > > > "When the PE acknowledges an SGI, a PPI, or an SPI at the
> > > > > > CPU
> > > > > > interface, the IRI changes the status of the interrupt to
> > > > > > active
> > > > > > and pending if:
> > > > > > • It is an edge-triggered interrupt, and another edge has
> > > > > > been
> > > > > > detected since the interrupt was acknowledged.
> > > > > > • It is a level-sensitive interrupt, and the level has not
> > > > > > been
> > > > > > deasserted since the interrupt was acknowledged."
> > > > > > 
> > > > > > GIC v2 specification IHI0048B.b has similar description on
> > > > > > page
> > > > > > 3-42 for state machine transition.
> > > > > > 
> > > > > > When some VFIO device, like mtty(8250 VFIO mdev emulation
> > > > > > driver
> > > > > > in samples/vfio-mdev) triggers a level interrupt, the
> > > > > > status
> > > > > > transition in LR is pending-->active-->active and pending.
> > > > > > Then it will wait resampling to de-assert the interrupt.
> > > > > > 
> > > > > > Current design of lr_signals_eoi_mi() will return false if
> > > > > > state
> > > > > > in LR is not invalid(Inactive). It causes resampling will
> > > > > > not happen
> > > > > > in mtty case.
> > > > > Let me rephrase this, and tell me if I understood it
> > > > > correctly:
> > > > > 
> > > > > - A level interrupt is injected, activated by the guest (LR
> > > > > state=active)
> > > > > - guest exits, re-enters, (LR state=pending+active)
> > > > > - guest EOIs the interrupt (LR state=pending)
> > > > > - maintenance interrupt
> > > > > - we don't signal the resampling because we're not in an
> > > > > invalid state
> > > > > 
> > > > > Is that correct?
> > > > > 
> > > > > That's an interesting case, because it seems to invalidate
> > > > > some of the 
> > > > > optimization that went in over a year ago.
> > > > > 
> > > > > 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR
> > > > > fields
> > > > > b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary
> > > > > save_maint_int_state
> > > > > af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary
> > > > > process_maintenance operation
> > > > > 
> > > > > We could compare the value of the LR before the guest entry
> > > > > with
> > > > > the value at exit time, but we still could miss it if we have
> > > > > a
> > > > > transition such as P+A -> P -> A and assume a long enough
> > > > > propagation
> > > > > delay for the maintenance interrupt (which is very likely).
> > > > > 
> > > > > In essence, we have lost the benefit of EISR, which was to
> > > > > give us a
> > > > > way to deal with asynchronous signalling.
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > This will cause interrupt fired continuously to guest even
> > > > > > 8250 IIR
> > > > > > has no interrupt. When 8250's interrupt is configured in
> > > > > > shared mode,
> > > > > > it will pass interrupt to other drivers to handle. However,
> > > > > > there
> > > > > > is no other driver involved. Then, a "nobody cared" kernel
> > > > > > complaint
> > > > > > occurs.
> > > > > > 
> > > > > > / # cat /dev/ttyS0
> > > > > > [4.826836] random: crng init done
> > > > > > [6.373620] irq 41: nobody cared (try booting with the
> > > > > > "irqpoll"
> > > > > > option)
> > > > > > [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted
> > > > > > 4.16.0-rc4 #4
> > > > > > [6.378927] Hardware name: linux,dummy-virt (DT)
> > > > > > [6.380876] Call trace:
> > > > > > [6.381937]  dump_backtrace+0x0/0x180
> > > > > > [6.383495]  show_stack+0x14/0x1c
> > > > > > [6.384902]  dump_stack+0x90/0xb4
> > > > > > [6.386312]  __report_bad_irq+0x38/0xe0
> > > > > > [6.387944]  note_interrupt+0x1f4/0x2b8
> > > > > > [6.389568]  handle_irq_event_percpu+0x54/0x7c
> > > > > > [6.391433]  handle_irq_event+0x44/0x74
> > > > > > [6.393056]  handle_fasteoi_irq+0x9c/0x154
> > > > > > [6.394784]  generic_handle_irq+0x24/0x38
> > > > > > [6.396483]  __handle_domain_irq+0x60/0xb4
> > > > > > [6.398207]  gic_handle_irq+0x98/0x1b0
> > > > > > [6.399796]  el1_irq+0xb0/0x128
> > > > > > [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
> > > > > > [6.403149]  __setup_irq+0x41c/0x678
> > > > > > [6.404669]  

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Auger Eric]

Hi Marc, Christoffer,

On 08/03/18 18:28, Marc Zyngier wrote:
> On Thu, 08 Mar 2018 16:19:00 +,
> Christoffer Dall wrote:
>>
>> On Thu, Mar 08, 2018 at 11:54:27AM +, Marc Zyngier wrote:
>>> On 08/03/18 09:49, Marc Zyngier wrote:
 [updated Christoffer's email address]

 Hi Shunyong,

 On 08/03/18 07:01, Shunyong Yang wrote:
> When resampling irqfds is enabled, level interrupt should be
> de-asserted when resampling happens. On page 4-47 of GIC v3
> specification IHI0069D, it said,
> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
> interface, the IRI changes the status of the interrupt to active
> and pending if:
> • It is an edge-triggered interrupt, and another edge has been
> detected since the interrupt was acknowledged.
> • It is a level-sensitive interrupt, and the level has not been
> deasserted since the interrupt was acknowledged."
>
> GIC v2 specification IHI0048B.b has similar description on page
> 3-42 for state machine transition.
>
> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
> in samples/vfio-mdev) triggers a level interrupt, the status
> transition in LR is pending-->active-->active and pending.
> Then it will wait resampling to de-assert the interrupt.
>
> Current design of lr_signals_eoi_mi() will return false if state
> in LR is not invalid(Inactive). It causes resampling will not happen
> in mtty case.

 Let me rephrase this, and tell me if I understood it correctly:

 - A level interrupt is injected, activated by the guest (LR state=active)
 - guest exits, re-enters, (LR state=pending+active)
 - guest EOIs the interrupt (LR state=pending)
 - maintenance interrupt
 - we don't signal the resampling because we're not in an invalid state

 Is that correct?

 That's an interesting case, because it seems to invalidate some of the 
 optimization that went in over a year ago.

 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
 b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary 
 save_maint_int_state
 af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary 
 process_maintenance operation

 We could compare the value of the LR before the guest entry with
 the value at exit time, but we still could miss it if we have a
 transition such as P+A -> P -> A and assume a long enough propagation
 delay for the maintenance interrupt (which is very likely).

 In essence, we have lost the benefit of EISR, which was to give us a
 way to deal with asynchronous signalling.

>
> This will cause interrupt fired continuously to guest even 8250 IIR
> has no interrupt. When 8250's interrupt is configured in shared mode,
> it will pass interrupt to other drivers to handle. However, there
> is no other driver involved. Then, a "nobody cared" kernel complaint
> occurs.
>
> / # cat /dev/ttyS0
> [4.826836] random: crng init done
> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
> option)
> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
> [6.378927] Hardware name: linux,dummy-virt (DT)
> [6.380876] Call trace:
> [6.381937]  dump_backtrace+0x0/0x180
> [6.383495]  show_stack+0x14/0x1c
> [6.384902]  dump_stack+0x90/0xb4
> [6.386312]  __report_bad_irq+0x38/0xe0
> [6.387944]  note_interrupt+0x1f4/0x2b8
> [6.389568]  handle_irq_event_percpu+0x54/0x7c
> [6.391433]  handle_irq_event+0x44/0x74
> [6.393056]  handle_fasteoi_irq+0x9c/0x154
> [6.394784]  generic_handle_irq+0x24/0x38
> [6.396483]  __handle_domain_irq+0x60/0xb4
> [6.398207]  gic_handle_irq+0x98/0x1b0
> [6.399796]  el1_irq+0xb0/0x128
> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
> [6.403149]  __setup_irq+0x41c/0x678
> [6.404669]  request_threaded_irq+0xe0/0x190
> [6.406474]  univ8250_setup_irq+0x208/0x234
> [6.408250]  serial8250_do_startup+0x1b4/0x754
> [6.410123]  serial8250_startup+0x20/0x28
> [6.411826]  uart_startup.part.21+0x78/0x144
> [6.413633]  uart_port_activate+0x50/0x68
> [6.415328]  tty_port_open+0x84/0xd4
> [6.416851]  uart_open+0x34/0x44
> [6.418229]  tty_open+0xec/0x3c8
> [6.419610]  chrdev_open+0xb0/0x198
> [6.421093]  do_dentry_open+0x200/0x310
> [6.422714]  vfs_open+0x54/0x84
> [6.424054]  path_openat+0x2dc/0xf04
> [6.425569]  do_filp_open+0x68/0xd8
> [6.427044]  do_sys_open+0x16c/0x224
> [6.428563]  SyS_openat+0x10/0x18
> [6.429972]  el0_svc_naked+0x30/0x34
> [6.431494] handlers:
> [6.432479] [<0e9fb4bb>] serial8250_interrupt
> [6.434597] Disabling IRQ #41
>
> This 

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On Thu, 08 Mar 2018 16:19:00 +,
Christoffer Dall wrote:
> 
> On Thu, Mar 08, 2018 at 11:54:27AM +, Marc Zyngier wrote:
> > On 08/03/18 09:49, Marc Zyngier wrote:
> > > [updated Christoffer's email address]
> > > 
> > > Hi Shunyong,
> > > 
> > > On 08/03/18 07:01, Shunyong Yang wrote:
> > >> When resampling irqfds is enabled, level interrupt should be
> > >> de-asserted when resampling happens. On page 4-47 of GIC v3
> > >> specification IHI0069D, it said,
> > >> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
> > >> interface, the IRI changes the status of the interrupt to active
> > >> and pending if:
> > >> • It is an edge-triggered interrupt, and another edge has been
> > >> detected since the interrupt was acknowledged.
> > >> • It is a level-sensitive interrupt, and the level has not been
> > >> deasserted since the interrupt was acknowledged."
> > >>
> > >> GIC v2 specification IHI0048B.b has similar description on page
> > >> 3-42 for state machine transition.
> > >>
> > >> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
> > >> in samples/vfio-mdev) triggers a level interrupt, the status
> > >> transition in LR is pending-->active-->active and pending.
> > >> Then it will wait resampling to de-assert the interrupt.
> > >>
> > >> Current design of lr_signals_eoi_mi() will return false if state
> > >> in LR is not invalid(Inactive). It causes resampling will not happen
> > >> in mtty case.
> > > 
> > > Let me rephrase this, and tell me if I understood it correctly:
> > > 
> > > - A level interrupt is injected, activated by the guest (LR state=active)
> > > - guest exits, re-enters, (LR state=pending+active)
> > > - guest EOIs the interrupt (LR state=pending)
> > > - maintenance interrupt
> > > - we don't signal the resampling because we're not in an invalid state
> > > 
> > > Is that correct?
> > > 
> > > That's an interesting case, because it seems to invalidate some of the 
> > > optimization that went in over a year ago.
> > > 
> > > 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
> > > b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary 
> > > save_maint_int_state
> > > af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary 
> > > process_maintenance operation
> > > 
> > > We could compare the value of the LR before the guest entry with
> > > the value at exit time, but we still could miss it if we have a
> > > transition such as P+A -> P -> A and assume a long enough propagation
> > > delay for the maintenance interrupt (which is very likely).
> > > 
> > > In essence, we have lost the benefit of EISR, which was to give us a
> > > way to deal with asynchronous signalling.
> > > 
> > >>
> > >> This will cause interrupt fired continuously to guest even 8250 IIR
> > >> has no interrupt. When 8250's interrupt is configured in shared mode,
> > >> it will pass interrupt to other drivers to handle. However, there
> > >> is no other driver involved. Then, a "nobody cared" kernel complaint
> > >> occurs.
> > >>
> > >> / # cat /dev/ttyS0
> > >> [4.826836] random: crng init done
> > >> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
> > >> option)
> > >> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
> > >> [6.378927] Hardware name: linux,dummy-virt (DT)
> > >> [6.380876] Call trace:
> > >> [6.381937]  dump_backtrace+0x0/0x180
> > >> [6.383495]  show_stack+0x14/0x1c
> > >> [6.384902]  dump_stack+0x90/0xb4
> > >> [6.386312]  __report_bad_irq+0x38/0xe0
> > >> [6.387944]  note_interrupt+0x1f4/0x2b8
> > >> [6.389568]  handle_irq_event_percpu+0x54/0x7c
> > >> [6.391433]  handle_irq_event+0x44/0x74
> > >> [6.393056]  handle_fasteoi_irq+0x9c/0x154
> > >> [6.394784]  generic_handle_irq+0x24/0x38
> > >> [6.396483]  __handle_domain_irq+0x60/0xb4
> > >> [6.398207]  gic_handle_irq+0x98/0x1b0
> > >> [6.399796]  el1_irq+0xb0/0x128
> > >> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
> > >> [6.403149]  __setup_irq+0x41c/0x678
> > >> [6.404669]  request_threaded_irq+0xe0/0x190
> > >> [6.406474]  univ8250_setup_irq+0x208/0x234
> > >> [6.408250]  serial8250_do_startup+0x1b4/0x754
> > >> [6.410123]  serial8250_startup+0x20/0x28
> > >> [6.411826]  uart_startup.part.21+0x78/0x144
> > >> [6.413633]  uart_port_activate+0x50/0x68
> > >> [6.415328]  tty_port_open+0x84/0xd4
> > >> [6.416851]  uart_open+0x34/0x44
> > >> [6.418229]  tty_open+0xec/0x3c8
> > >> [6.419610]  chrdev_open+0xb0/0x198
> > >> [6.421093]  do_dentry_open+0x200/0x310
> > >> [6.422714]  vfs_open+0x54/0x84
> > >> [6.424054]  path_openat+0x2dc/0xf04
> > >> [6.425569]  do_filp_open+0x68/0xd8
> > >> [6.427044]  do_sys_open+0x16c/0x224
> > >> [6.428563]  SyS_openat+0x10/0x18
> > >> [6.429972]  el0_svc_naked+0x30/0x34
> > >> [6.431494] handlers:
> > >> [6.432479] [<0e9fb4bb>] serial8250_interrupt
> > >> [

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Christoffer Dall]

On Thu, Mar 08, 2018 at 11:54:27AM +, Marc Zyngier wrote:
> On 08/03/18 09:49, Marc Zyngier wrote:
> > [updated Christoffer's email address]
> > 
> > Hi Shunyong,
> > 
> > On 08/03/18 07:01, Shunyong Yang wrote:
> >> When resampling irqfds is enabled, level interrupt should be
> >> de-asserted when resampling happens. On page 4-47 of GIC v3
> >> specification IHI0069D, it said,
> >> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
> >> interface, the IRI changes the status of the interrupt to active
> >> and pending if:
> >> • It is an edge-triggered interrupt, and another edge has been
> >> detected since the interrupt was acknowledged.
> >> • It is a level-sensitive interrupt, and the level has not been
> >> deasserted since the interrupt was acknowledged."
> >>
> >> GIC v2 specification IHI0048B.b has similar description on page
> >> 3-42 for state machine transition.
> >>
> >> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
> >> in samples/vfio-mdev) triggers a level interrupt, the status
> >> transition in LR is pending-->active-->active and pending.
> >> Then it will wait resampling to de-assert the interrupt.
> >>
> >> Current design of lr_signals_eoi_mi() will return false if state
> >> in LR is not invalid(Inactive). It causes resampling will not happen
> >> in mtty case.
> > 
> > Let me rephrase this, and tell me if I understood it correctly:
> > 
> > - A level interrupt is injected, activated by the guest (LR state=active)
> > - guest exits, re-enters, (LR state=pending+active)
> > - guest EOIs the interrupt (LR state=pending)
> > - maintenance interrupt
> > - we don't signal the resampling because we're not in an invalid state
> > 
> > Is that correct?
> > 
> > That's an interesting case, because it seems to invalidate some of the 
> > optimization that went in over a year ago.
> > 
> > 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
> > b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary 
> > save_maint_int_state
> > af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary 
> > process_maintenance operation
> > 
> > We could compare the value of the LR before the guest entry with
> > the value at exit time, but we still could miss it if we have a
> > transition such as P+A -> P -> A and assume a long enough propagation
> > delay for the maintenance interrupt (which is very likely).
> > 
> > In essence, we have lost the benefit of EISR, which was to give us a
> > way to deal with asynchronous signalling.
> > 
> >>
> >> This will cause interrupt fired continuously to guest even 8250 IIR
> >> has no interrupt. When 8250's interrupt is configured in shared mode,
> >> it will pass interrupt to other drivers to handle. However, there
> >> is no other driver involved. Then, a "nobody cared" kernel complaint
> >> occurs.
> >>
> >> / # cat /dev/ttyS0
> >> [4.826836] random: crng init done
> >> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
> >> option)
> >> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
> >> [6.378927] Hardware name: linux,dummy-virt (DT)
> >> [6.380876] Call trace:
> >> [6.381937]  dump_backtrace+0x0/0x180
> >> [6.383495]  show_stack+0x14/0x1c
> >> [6.384902]  dump_stack+0x90/0xb4
> >> [6.386312]  __report_bad_irq+0x38/0xe0
> >> [6.387944]  note_interrupt+0x1f4/0x2b8
> >> [6.389568]  handle_irq_event_percpu+0x54/0x7c
> >> [6.391433]  handle_irq_event+0x44/0x74
> >> [6.393056]  handle_fasteoi_irq+0x9c/0x154
> >> [6.394784]  generic_handle_irq+0x24/0x38
> >> [6.396483]  __handle_domain_irq+0x60/0xb4
> >> [6.398207]  gic_handle_irq+0x98/0x1b0
> >> [6.399796]  el1_irq+0xb0/0x128
> >> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
> >> [6.403149]  __setup_irq+0x41c/0x678
> >> [6.404669]  request_threaded_irq+0xe0/0x190
> >> [6.406474]  univ8250_setup_irq+0x208/0x234
> >> [6.408250]  serial8250_do_startup+0x1b4/0x754
> >> [6.410123]  serial8250_startup+0x20/0x28
> >> [6.411826]  uart_startup.part.21+0x78/0x144
> >> [6.413633]  uart_port_activate+0x50/0x68
> >> [6.415328]  tty_port_open+0x84/0xd4
> >> [6.416851]  uart_open+0x34/0x44
> >> [6.418229]  tty_open+0xec/0x3c8
> >> [6.419610]  chrdev_open+0xb0/0x198
> >> [6.421093]  do_dentry_open+0x200/0x310
> >> [6.422714]  vfs_open+0x54/0x84
> >> [6.424054]  path_openat+0x2dc/0xf04
> >> [6.425569]  do_filp_open+0x68/0xd8
> >> [6.427044]  do_sys_open+0x16c/0x224
> >> [6.428563]  SyS_openat+0x10/0x18
> >> [6.429972]  el0_svc_naked+0x30/0x34
> >> [6.431494] handlers:
> >> [6.432479] [<0e9fb4bb>] serial8250_interrupt
> >> [6.434597] Disabling IRQ #41
> >>
> >> This patch changes the lr state condition in lr_signals_eoi_mi() from
> >> invalid(Inactive) to active and pending to avoid this.
> >>
> >> I am not sure about the original design of the condition of
> >> invalid(active). So, This RFC is 

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Christoffer Dall]

On Thu, Mar 08, 2018 at 09:49:43AM +, Marc Zyngier wrote:
> [updated Christoffer's email address]
> 
> Hi Shunyong,
> 
> On 08/03/18 07:01, Shunyong Yang wrote:
> > When resampling irqfds is enabled, level interrupt should be
> > de-asserted when resampling happens. On page 4-47 of GIC v3
> > specification IHI0069D, it said,
> > "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
> > interface, the IRI changes the status of the interrupt to active
> > and pending if:
> > • It is an edge-triggered interrupt, and another edge has been
> > detected since the interrupt was acknowledged.
> > • It is a level-sensitive interrupt, and the level has not been
> > deasserted since the interrupt was acknowledged."
> > 
> > GIC v2 specification IHI0048B.b has similar description on page
> > 3-42 for state machine transition.
> > 
> > When some VFIO device, like mtty(8250 VFIO mdev emulation driver
> > in samples/vfio-mdev) triggers a level interrupt, the status
> > transition in LR is pending-->active-->active and pending.
> > Then it will wait resampling to de-assert the interrupt.
> > 
> > Current design of lr_signals_eoi_mi() will return false if state
> > in LR is not invalid(Inactive). It causes resampling will not happen
> > in mtty case.
> 
> Let me rephrase this, and tell me if I understood it correctly:
> 
> - A level interrupt is injected, activated by the guest (LR state=active)
> - guest exits, re-enters, (LR state=pending+active)
> - guest EOIs the interrupt (LR state=pending)
> - maintenance interrupt
> - we don't signal the resampling because we're not in an invalid state
> 
> Is that correct?
> 
> That's an interesting case, because it seems to invalidate some of the 
> optimization that went in over a year ago.
> 
> 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
> b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary save_maint_int_state
> af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary process_maintenance 
> operation
> 
> We could compare the value of the LR before the guest entry with
> the value at exit time, but we still could miss it if we have a
> transition such as P+A -> P -> A and assume a long enough propagation
> delay for the maintenance interrupt (which is very likely).
> 
> In essence, we have lost the benefit of EISR, which was to give us a
> way to deal with asynchronous signalling.
> 

I don't understand why EISR gives us anything beyond looking at the LR
and evaluating if the state is 00.  My reading of the spec is that the
EISR is merely a shortcut to knowing the state of the LRs but contains
not record or information beyond what you can read from the LRs.

What am I missing?

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Auger Eric]

Hi Marc,

On 08/03/18 12:54, Marc Zyngier wrote:
> On 08/03/18 09:49, Marc Zyngier wrote:
>> [updated Christoffer's email address]
>>
>> Hi Shunyong,
>>
>> On 08/03/18 07:01, Shunyong Yang wrote:
>>> When resampling irqfds is enabled, level interrupt should be
>>> de-asserted when resampling happens. On page 4-47 of GIC v3
>>> specification IHI0069D, it said,
>>> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
>>> interface, the IRI changes the status of the interrupt to active
>>> and pending if:
>>> • It is an edge-triggered interrupt, and another edge has been
>>> detected since the interrupt was acknowledged.
>>> • It is a level-sensitive interrupt, and the level has not been
>>> deasserted since the interrupt was acknowledged."
>>>
>>> GIC v2 specification IHI0048B.b has similar description on page
>>> 3-42 for state machine transition.
>>>
>>> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
>>> in samples/vfio-mdev) triggers a level interrupt, the status
>>> transition in LR is pending-->active-->active and pending.
>>> Then it will wait resampling to de-assert the interrupt.
>>>
>>> Current design of lr_signals_eoi_mi() will return false if state
>>> in LR is not invalid(Inactive). It causes resampling will not happen
>>> in mtty case.
>>
>> Let me rephrase this, and tell me if I understood it correctly:
>>
>> - A level interrupt is injected, activated by the guest (LR state=active)
>> - guest exits, re-enters, (LR state=pending+active)
>> - guest EOIs the interrupt (LR state=pending)
>> - maintenance interrupt
>> - we don't signal the resampling because we're not in an invalid state
>>
>> Is that correct?
>>
>> That's an interesting case, because it seems to invalidate some of the 
>> optimization that went in over a year ago.
>>
>> 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
>> b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary 
>> save_maint_int_state
>> af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary 
>> process_maintenance operation
>>
>> We could compare the value of the LR before the guest entry with
>> the value at exit time, but we still could miss it if we have a
>> transition such as P+A -> P -> A and assume a long enough propagation
>> delay for the maintenance interrupt (which is very likely).
>>
>> In essence, we have lost the benefit of EISR, which was to give us a
>> way to deal with asynchronous signalling.
>>
>>>
>>> This will cause interrupt fired continuously to guest even 8250 IIR
>>> has no interrupt. When 8250's interrupt is configured in shared mode,
>>> it will pass interrupt to other drivers to handle. However, there
>>> is no other driver involved. Then, a "nobody cared" kernel complaint
>>> occurs.
>>>
>>> / # cat /dev/ttyS0
>>> [4.826836] random: crng init done
>>> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
>>> option)
>>> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
>>> [6.378927] Hardware name: linux,dummy-virt (DT)
>>> [6.380876] Call trace:
>>> [6.381937]  dump_backtrace+0x0/0x180
>>> [6.383495]  show_stack+0x14/0x1c
>>> [6.384902]  dump_stack+0x90/0xb4
>>> [6.386312]  __report_bad_irq+0x38/0xe0
>>> [6.387944]  note_interrupt+0x1f4/0x2b8
>>> [6.389568]  handle_irq_event_percpu+0x54/0x7c
>>> [6.391433]  handle_irq_event+0x44/0x74
>>> [6.393056]  handle_fasteoi_irq+0x9c/0x154
>>> [6.394784]  generic_handle_irq+0x24/0x38
>>> [6.396483]  __handle_domain_irq+0x60/0xb4
>>> [6.398207]  gic_handle_irq+0x98/0x1b0
>>> [6.399796]  el1_irq+0xb0/0x128
>>> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
>>> [6.403149]  __setup_irq+0x41c/0x678
>>> [6.404669]  request_threaded_irq+0xe0/0x190
>>> [6.406474]  univ8250_setup_irq+0x208/0x234
>>> [6.408250]  serial8250_do_startup+0x1b4/0x754
>>> [6.410123]  serial8250_startup+0x20/0x28
>>> [6.411826]  uart_startup.part.21+0x78/0x144
>>> [6.413633]  uart_port_activate+0x50/0x68
>>> [6.415328]  tty_port_open+0x84/0xd4
>>> [6.416851]  uart_open+0x34/0x44
>>> [6.418229]  tty_open+0xec/0x3c8
>>> [6.419610]  chrdev_open+0xb0/0x198
>>> [6.421093]  do_dentry_open+0x200/0x310
>>> [6.422714]  vfs_open+0x54/0x84
>>> [6.424054]  path_openat+0x2dc/0xf04
>>> [6.425569]  do_filp_open+0x68/0xd8
>>> [6.427044]  do_sys_open+0x16c/0x224
>>> [6.428563]  SyS_openat+0x10/0x18
>>> [6.429972]  el0_svc_naked+0x30/0x34
>>> [6.431494] handlers:
>>> [6.432479] [<0e9fb4bb>] serial8250_interrupt
>>> [6.434597] Disabling IRQ #41
>>>
>>> This patch changes the lr state condition in lr_signals_eoi_mi() from
>>> invalid(Inactive) to active and pending to avoid this.
>>>
>>> I am not sure about the original design of the condition of
>>> invalid(active). So, This RFC is sent out for comments.
>>>
>>> Cc: Joey Zheng 
>>> Signed-off-by: Shunyong Yang 

Re: [此邮件可能存在风险] Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Yang, Shunyong]

Hi, Eric,

First, please let me change Christoffer's email to cd...@kernel.org. I
add more information about my test below, please check.

On Thu, 2018-03-08 at 09:57 +0100, Auger Eric wrote:
> Hi,
> 
> On 08/03/18 08:01, Shunyong Yang wrote:
> > 
> > When resampling irqfds is enabled, level interrupt should be
> > de-asserted when resampling happens. On page 4-47 of GIC v3
> > specification IHI0069D, it said,
> > "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
> > interface, the IRI changes the status of the interrupt to active
> > and pending if:
> > • It is an edge-triggered interrupt, and another edge has been
> > detected since the interrupt was acknowledged.
> > • It is a level-sensitive interrupt, and the level has not been
> > deasserted since the interrupt was acknowledged."
> > 
> > GIC v2 specification IHI0048B.b has similar description on page
> > 3-42 for state machine transition.
> > 
> > When some VFIO device, like mtty(8250 VFIO mdev emulation driver
> > in samples/vfio-mdev) triggers a level interrupt, the status
> > transition in LR is pending-->active-->active and pending.
> > Then it will wait resampling to de-assert the interrupt.
> > 
> > Current design of lr_signals_eoi_mi() will return false if state
> > in LR is not invalid(Inactive). It causes resampling will not
> > happen
> > in mtty case.
> > 
> > This will cause interrupt fired continuously to guest even 8250 IIR
> > has no interrupt. When 8250's interrupt is configured in shared
> > mode,
> > it will pass interrupt to other drivers to handle. However, there
> > is no other driver involved. Then, a "nobody cared" kernel
> > complaint
> > occurs.
> > 
> > / # cat /dev/ttyS0
> > [4.826836] random: crng init done
> > [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
> > option)
> > [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
> > [6.378927] Hardware name: linux,dummy-virt (DT)
> > [6.380876] Call trace:
> > [6.381937]  dump_backtrace+0x0/0x180
> > [6.383495]  show_stack+0x14/0x1c
> > [6.384902]  dump_stack+0x90/0xb4
> > [6.386312]  __report_bad_irq+0x38/0xe0
> > [6.387944]  note_interrupt+0x1f4/0x2b8
> > [6.389568]  handle_irq_event_percpu+0x54/0x7c
> > [6.391433]  handle_irq_event+0x44/0x74
> > [6.393056]  handle_fasteoi_irq+0x9c/0x154
> > [6.394784]  generic_handle_irq+0x24/0x38
> > [6.396483]  __handle_domain_irq+0x60/0xb4
> > [6.398207]  gic_handle_irq+0x98/0x1b0
> > [6.399796]  el1_irq+0xb0/0x128
> > [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
> > [6.403149]  __setup_irq+0x41c/0x678
> > [6.404669]  request_threaded_irq+0xe0/0x190
> > [6.406474]  univ8250_setup_irq+0x208/0x234
> > [6.408250]  serial8250_do_startup+0x1b4/0x754
> > [6.410123]  serial8250_startup+0x20/0x28
> > [6.411826]  uart_startup.part.21+0x78/0x144
> > [6.413633]  uart_port_activate+0x50/0x68
> > [6.415328]  tty_port_open+0x84/0xd4
> > [6.416851]  uart_open+0x34/0x44
> > [6.418229]  tty_open+0xec/0x3c8
> > [6.419610]  chrdev_open+0xb0/0x198
> > [6.421093]  do_dentry_open+0x200/0x310
> > [6.422714]  vfs_open+0x54/0x84
> > [6.424054]  path_openat+0x2dc/0xf04
> > [6.425569]  do_filp_open+0x68/0xd8
> > [6.427044]  do_sys_open+0x16c/0x224
> > [6.428563]  SyS_openat+0x10/0x18
> > [6.429972]  el0_svc_naked+0x30/0x34
> > [6.431494] handlers:
> > [6.432479] [<0e9fb4bb>] serial8250_interrupt
> > [6.434597] Disabling IRQ #41
> > 
> > This patch changes the lr state condition in lr_signals_eoi_mi()
> > from
> > invalid(Inactive) to active and pending to avoid this.
> > 
> > I am not sure about the original design of the condition of
> > invalid(active). So, This RFC is sent out for comments.
> > 
> > Cc: Joey Zheng 
> > Signed-off-by: Shunyong Yang 
> > ---
> >  virt/kvm/arm/vgic/vgic-v2.c | 4 ++--
> >  virt/kvm/arm/vgic/vgic-v3.c | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-
> > v2.c
> > index e9d840a75e7b..740ee9a5f551 100644
> > --- a/virt/kvm/arm/vgic/vgic-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-v2.c
> > @@ -46,8 +46,8 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
> >  
> >  static bool lr_signals_eoi_mi(u32 lr_val)
> >  {
> > -   return !(lr_val & GICH_LR_STATE) && (lr_val & GICH_LR_EOI)
> > &&
> > -      !(lr_val & GICH_LR_HW);
> > +   return !((lr_val & GICH_LR_STATE) ^ GICH_LR_STATE) &&
> > +      (lr_val & GICH_LR_EOI) && !(lr_val & GICH_LR_HW);
> >  }
> >  
> >  /*
> > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-
> > v3.c
> > index 6b329414e57a..43111bba7af9 100644
> > --- a/virt/kvm/arm/vgic/vgic-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-v3.c
> > @@ -35,8 +35,8 @@ void vgic_v3_set_underflow(struct kvm_vcpu *vcpu)
> >  
> >  static bool lr_signals_eoi_mi(u64 lr_val)

Re: [此邮件可能存在风险] Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Auger Eric]

Hi Shunyong,
On 08/03/18 10:31, Yang, Shunyong wrote:
> Hi, Eric,
> 
> First, please let me change Christoffer's email to cd...@kernel.org. I
> add more information about my test below, please check.
> 
> On Thu, 2018-03-08 at 09:57 +0100, Auger Eric wrote:
>> Hi,
>>
>> On 08/03/18 08:01, Shunyong Yang wrote:
>>>
>>> When resampling irqfds is enabled, level interrupt should be
>>> de-asserted when resampling happens. On page 4-47 of GIC v3
>>> specification IHI0069D, it said,
>>> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
>>> interface, the IRI changes the status of the interrupt to active
>>> and pending if:
>>> • It is an edge-triggered interrupt, and another edge has been
>>> detected since the interrupt was acknowledged.
>>> • It is a level-sensitive interrupt, and the level has not been
>>> deasserted since the interrupt was acknowledged."
>>>
>>> GIC v2 specification IHI0048B.b has similar description on page
>>> 3-42 for state machine transition.
>>>
>>> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
>>> in samples/vfio-mdev) triggers a level interrupt, the status
>>> transition in LR is pending-->active-->active and pending.
>>> Then it will wait resampling to de-assert the interrupt.
>>>
>>> Current design of lr_signals_eoi_mi() will return false if state
>>> in LR is not invalid(Inactive). It causes resampling will not
>>> happen
>>> in mtty case.
>>>
>>> This will cause interrupt fired continuously to guest even 8250 IIR
>>> has no interrupt. When 8250's interrupt is configured in shared
>>> mode,
>>> it will pass interrupt to other drivers to handle. However, there
>>> is no other driver involved. Then, a "nobody cared" kernel
>>> complaint
>>> occurs.
>>>
>>> / # cat /dev/ttyS0
>>> [4.826836] random: crng init done
>>> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
>>> option)
>>> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
>>> [6.378927] Hardware name: linux,dummy-virt (DT)
>>> [6.380876] Call trace:
>>> [6.381937]  dump_backtrace+0x0/0x180
>>> [6.383495]  show_stack+0x14/0x1c
>>> [6.384902]  dump_stack+0x90/0xb4
>>> [6.386312]  __report_bad_irq+0x38/0xe0
>>> [6.387944]  note_interrupt+0x1f4/0x2b8
>>> [6.389568]  handle_irq_event_percpu+0x54/0x7c
>>> [6.391433]  handle_irq_event+0x44/0x74
>>> [6.393056]  handle_fasteoi_irq+0x9c/0x154
>>> [6.394784]  generic_handle_irq+0x24/0x38
>>> [6.396483]  __handle_domain_irq+0x60/0xb4
>>> [6.398207]  gic_handle_irq+0x98/0x1b0
>>> [6.399796]  el1_irq+0xb0/0x128
>>> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
>>> [6.403149]  __setup_irq+0x41c/0x678
>>> [6.404669]  request_threaded_irq+0xe0/0x190
>>> [6.406474]  univ8250_setup_irq+0x208/0x234
>>> [6.408250]  serial8250_do_startup+0x1b4/0x754
>>> [6.410123]  serial8250_startup+0x20/0x28
>>> [6.411826]  uart_startup.part.21+0x78/0x144
>>> [6.413633]  uart_port_activate+0x50/0x68
>>> [6.415328]  tty_port_open+0x84/0xd4
>>> [6.416851]  uart_open+0x34/0x44
>>> [6.418229]  tty_open+0xec/0x3c8
>>> [6.419610]  chrdev_open+0xb0/0x198
>>> [6.421093]  do_dentry_open+0x200/0x310
>>> [6.422714]  vfs_open+0x54/0x84
>>> [6.424054]  path_openat+0x2dc/0xf04
>>> [6.425569]  do_filp_open+0x68/0xd8
>>> [6.427044]  do_sys_open+0x16c/0x224
>>> [6.428563]  SyS_openat+0x10/0x18
>>> [6.429972]  el0_svc_naked+0x30/0x34
>>> [6.431494] handlers:
>>> [6.432479] [<0e9fb4bb>] serial8250_interrupt
>>> [6.434597] Disabling IRQ #41
>>>
>>> This patch changes the lr state condition in lr_signals_eoi_mi()
>>> from
>>> invalid(Inactive) to active and pending to avoid this.
>>>
>>> I am not sure about the original design of the condition of
>>> invalid(active). So, This RFC is sent out for comments.
>>>
>>> Cc: Joey Zheng 
>>> Signed-off-by: Shunyong Yang 
>>> ---
>>>  virt/kvm/arm/vgic/vgic-v2.c | 4 ++--
>>>  virt/kvm/arm/vgic/vgic-v3.c | 4 ++--
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-
>>> v2.c
>>> index e9d840a75e7b..740ee9a5f551 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v2.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
>>> @@ -46,8 +46,8 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
>>>  
>>>  static bool lr_signals_eoi_mi(u32 lr_val)
>>>  {
>>> -   return !(lr_val & GICH_LR_STATE) && (lr_val & GICH_LR_EOI)
>>> &&
>>> -  !(lr_val & GICH_LR_HW);
>>> +   return !((lr_val & GICH_LR_STATE) ^ GICH_LR_STATE) &&
>>> +  (lr_val & GICH_LR_EOI) && !(lr_val & GICH_LR_HW);
>>>  }
>>>  
>>>  /*
>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-
>>> v3.c
>>> index 6b329414e57a..43111bba7af9 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>>> @@ -35,8 +35,8 @@ void vgic_v3_set_underflow(struct kvm_vcpu 

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On 08/03/18 09:49, Marc Zyngier wrote:
> [updated Christoffer's email address]
> 
> Hi Shunyong,
> 
> On 08/03/18 07:01, Shunyong Yang wrote:
>> When resampling irqfds is enabled, level interrupt should be
>> de-asserted when resampling happens. On page 4-47 of GIC v3
>> specification IHI0069D, it said,
>> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
>> interface, the IRI changes the status of the interrupt to active
>> and pending if:
>> • It is an edge-triggered interrupt, and another edge has been
>> detected since the interrupt was acknowledged.
>> • It is a level-sensitive interrupt, and the level has not been
>> deasserted since the interrupt was acknowledged."
>>
>> GIC v2 specification IHI0048B.b has similar description on page
>> 3-42 for state machine transition.
>>
>> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
>> in samples/vfio-mdev) triggers a level interrupt, the status
>> transition in LR is pending-->active-->active and pending.
>> Then it will wait resampling to de-assert the interrupt.
>>
>> Current design of lr_signals_eoi_mi() will return false if state
>> in LR is not invalid(Inactive). It causes resampling will not happen
>> in mtty case.
> 
> Let me rephrase this, and tell me if I understood it correctly:
> 
> - A level interrupt is injected, activated by the guest (LR state=active)
> - guest exits, re-enters, (LR state=pending+active)
> - guest EOIs the interrupt (LR state=pending)
> - maintenance interrupt
> - we don't signal the resampling because we're not in an invalid state
> 
> Is that correct?
> 
> That's an interesting case, because it seems to invalidate some of the 
> optimization that went in over a year ago.
> 
> 096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
> b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary save_maint_int_state
> af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary process_maintenance 
> operation
> 
> We could compare the value of the LR before the guest entry with
> the value at exit time, but we still could miss it if we have a
> transition such as P+A -> P -> A and assume a long enough propagation
> delay for the maintenance interrupt (which is very likely).
> 
> In essence, we have lost the benefit of EISR, which was to give us a
> way to deal with asynchronous signalling.
> 
>>
>> This will cause interrupt fired continuously to guest even 8250 IIR
>> has no interrupt. When 8250's interrupt is configured in shared mode,
>> it will pass interrupt to other drivers to handle. However, there
>> is no other driver involved. Then, a "nobody cared" kernel complaint
>> occurs.
>>
>> / # cat /dev/ttyS0
>> [4.826836] random: crng init done
>> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
>> option)
>> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
>> [6.378927] Hardware name: linux,dummy-virt (DT)
>> [6.380876] Call trace:
>> [6.381937]  dump_backtrace+0x0/0x180
>> [6.383495]  show_stack+0x14/0x1c
>> [6.384902]  dump_stack+0x90/0xb4
>> [6.386312]  __report_bad_irq+0x38/0xe0
>> [6.387944]  note_interrupt+0x1f4/0x2b8
>> [6.389568]  handle_irq_event_percpu+0x54/0x7c
>> [6.391433]  handle_irq_event+0x44/0x74
>> [6.393056]  handle_fasteoi_irq+0x9c/0x154
>> [6.394784]  generic_handle_irq+0x24/0x38
>> [6.396483]  __handle_domain_irq+0x60/0xb4
>> [6.398207]  gic_handle_irq+0x98/0x1b0
>> [6.399796]  el1_irq+0xb0/0x128
>> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
>> [6.403149]  __setup_irq+0x41c/0x678
>> [6.404669]  request_threaded_irq+0xe0/0x190
>> [6.406474]  univ8250_setup_irq+0x208/0x234
>> [6.408250]  serial8250_do_startup+0x1b4/0x754
>> [6.410123]  serial8250_startup+0x20/0x28
>> [6.411826]  uart_startup.part.21+0x78/0x144
>> [6.413633]  uart_port_activate+0x50/0x68
>> [6.415328]  tty_port_open+0x84/0xd4
>> [6.416851]  uart_open+0x34/0x44
>> [6.418229]  tty_open+0xec/0x3c8
>> [6.419610]  chrdev_open+0xb0/0x198
>> [6.421093]  do_dentry_open+0x200/0x310
>> [6.422714]  vfs_open+0x54/0x84
>> [6.424054]  path_openat+0x2dc/0xf04
>> [6.425569]  do_filp_open+0x68/0xd8
>> [6.427044]  do_sys_open+0x16c/0x224
>> [6.428563]  SyS_openat+0x10/0x18
>> [6.429972]  el0_svc_naked+0x30/0x34
>> [6.431494] handlers:
>> [6.432479] [<0e9fb4bb>] serial8250_interrupt
>> [6.434597] Disabling IRQ #41
>>
>> This patch changes the lr state condition in lr_signals_eoi_mi() from
>> invalid(Inactive) to active and pending to avoid this.
>>
>> I am not sure about the original design of the condition of
>> invalid(active). So, This RFC is sent out for comments.
>>
>> Cc: Joey Zheng 
>> Signed-off-by: Shunyong Yang 
>> ---
>>  virt/kvm/arm/vgic/vgic-v2.c | 4 ++--
>>  virt/kvm/arm/vgic/vgic-v3.c | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> 

Re: [此邮件可能存在风险] Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On 08/03/18 09:31, Yang, Shunyong wrote:
> Hi, Eric,
> 
> First, please let me change Christoffer's email to cd...@kernel.org. I
> add more information about my test below, please check.
> 
> On Thu, 2018-03-08 at 09:57 +0100, Auger Eric wrote:
>> Hi,
>>
>> On 08/03/18 08:01, Shunyong Yang wrote:
>>>
>>> When resampling irqfds is enabled, level interrupt should be
>>> de-asserted when resampling happens. On page 4-47 of GIC v3
>>> specification IHI0069D, it said,
>>> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
>>> interface, the IRI changes the status of the interrupt to active
>>> and pending if:
>>> • It is an edge-triggered interrupt, and another edge has been
>>> detected since the interrupt was acknowledged.
>>> • It is a level-sensitive interrupt, and the level has not been
>>> deasserted since the interrupt was acknowledged."
>>>
>>> GIC v2 specification IHI0048B.b has similar description on page
>>> 3-42 for state machine transition.
>>>
>>> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
>>> in samples/vfio-mdev) triggers a level interrupt, the status
>>> transition in LR is pending-->active-->active and pending.
>>> Then it will wait resampling to de-assert the interrupt.
>>>
>>> Current design of lr_signals_eoi_mi() will return false if state
>>> in LR is not invalid(Inactive). It causes resampling will not
>>> happen
>>> in mtty case.
>>>
>>> This will cause interrupt fired continuously to guest even 8250 IIR
>>> has no interrupt. When 8250's interrupt is configured in shared
>>> mode,
>>> it will pass interrupt to other drivers to handle. However, there
>>> is no other driver involved. Then, a "nobody cared" kernel
>>> complaint
>>> occurs.
>>>
>>> / # cat /dev/ttyS0
>>> [4.826836] random: crng init done
>>> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
>>> option)
>>> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
>>> [6.378927] Hardware name: linux,dummy-virt (DT)
>>> [6.380876] Call trace:
>>> [6.381937]  dump_backtrace+0x0/0x180
>>> [6.383495]  show_stack+0x14/0x1c
>>> [6.384902]  dump_stack+0x90/0xb4
>>> [6.386312]  __report_bad_irq+0x38/0xe0
>>> [6.387944]  note_interrupt+0x1f4/0x2b8
>>> [6.389568]  handle_irq_event_percpu+0x54/0x7c
>>> [6.391433]  handle_irq_event+0x44/0x74
>>> [6.393056]  handle_fasteoi_irq+0x9c/0x154
>>> [6.394784]  generic_handle_irq+0x24/0x38
>>> [6.396483]  __handle_domain_irq+0x60/0xb4
>>> [6.398207]  gic_handle_irq+0x98/0x1b0
>>> [6.399796]  el1_irq+0xb0/0x128
>>> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
>>> [6.403149]  __setup_irq+0x41c/0x678
>>> [6.404669]  request_threaded_irq+0xe0/0x190
>>> [6.406474]  univ8250_setup_irq+0x208/0x234
>>> [6.408250]  serial8250_do_startup+0x1b4/0x754
>>> [6.410123]  serial8250_startup+0x20/0x28
>>> [6.411826]  uart_startup.part.21+0x78/0x144
>>> [6.413633]  uart_port_activate+0x50/0x68
>>> [6.415328]  tty_port_open+0x84/0xd4
>>> [6.416851]  uart_open+0x34/0x44
>>> [6.418229]  tty_open+0xec/0x3c8
>>> [6.419610]  chrdev_open+0xb0/0x198
>>> [6.421093]  do_dentry_open+0x200/0x310
>>> [6.422714]  vfs_open+0x54/0x84
>>> [6.424054]  path_openat+0x2dc/0xf04
>>> [6.425569]  do_filp_open+0x68/0xd8
>>> [6.427044]  do_sys_open+0x16c/0x224
>>> [6.428563]  SyS_openat+0x10/0x18
>>> [6.429972]  el0_svc_naked+0x30/0x34
>>> [6.431494] handlers:
>>> [6.432479] [<0e9fb4bb>] serial8250_interrupt
>>> [6.434597] Disabling IRQ #41
>>>
>>> This patch changes the lr state condition in lr_signals_eoi_mi()
>>> from
>>> invalid(Inactive) to active and pending to avoid this.
>>>
>>> I am not sure about the original design of the condition of
>>> invalid(active). So, This RFC is sent out for comments.
>>>
>>> Cc: Joey Zheng 
>>> Signed-off-by: Shunyong Yang 
>>> ---
>>>  virt/kvm/arm/vgic/vgic-v2.c | 4 ++--
>>>  virt/kvm/arm/vgic/vgic-v3.c | 4 ++--
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-
>>> v2.c
>>> index e9d840a75e7b..740ee9a5f551 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v2.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
>>> @@ -46,8 +46,8 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
>>>  
>>>  static bool lr_signals_eoi_mi(u32 lr_val)
>>>  {
>>> -   return !(lr_val & GICH_LR_STATE) && (lr_val & GICH_LR_EOI)
>>> &&
>>> -      !(lr_val & GICH_LR_HW);
>>> +   return !((lr_val & GICH_LR_STATE) ^ GICH_LR_STATE) &&
>>> +      (lr_val & GICH_LR_EOI) && !(lr_val & GICH_LR_HW);
>>>  }
>>>  
>>>  /*
>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-
>>> v3.c
>>> index 6b329414e57a..43111bba7af9 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>>> @@ -35,8 +35,8 @@ void vgic_v3_set_underflow(struct kvm_vcpu *vcpu)
>>>  

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

[updated Christoffer's email address]

Hi Shunyong,

On 08/03/18 07:01, Shunyong Yang wrote:
> When resampling irqfds is enabled, level interrupt should be
> de-asserted when resampling happens. On page 4-47 of GIC v3
> specification IHI0069D, it said,
> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
> interface, the IRI changes the status of the interrupt to active
> and pending if:
> • It is an edge-triggered interrupt, and another edge has been
> detected since the interrupt was acknowledged.
> • It is a level-sensitive interrupt, and the level has not been
> deasserted since the interrupt was acknowledged."
> 
> GIC v2 specification IHI0048B.b has similar description on page
> 3-42 for state machine transition.
> 
> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
> in samples/vfio-mdev) triggers a level interrupt, the status
> transition in LR is pending-->active-->active and pending.
> Then it will wait resampling to de-assert the interrupt.
> 
> Current design of lr_signals_eoi_mi() will return false if state
> in LR is not invalid(Inactive). It causes resampling will not happen
> in mtty case.

Let me rephrase this, and tell me if I understood it correctly:

- A level interrupt is injected, activated by the guest (LR state=active)
- guest exits, re-enters, (LR state=pending+active)
- guest EOIs the interrupt (LR state=pending)
- maintenance interrupt
- we don't signal the resampling because we're not in an invalid state

Is that correct?

That's an interesting case, because it seems to invalidate some of the 
optimization that went in over a year ago.

096f31c4360f KVM: arm/arm64: vgic: Get rid of MISR and EISR fields
b6095b084d87 KVM: arm/arm64: vgic: Get rid of unnecessary save_maint_int_state
af0614991ab6 KVM: arm/arm64: vgic: Get rid of unnecessary process_maintenance 
operation

We could compare the value of the LR before the guest entry with
the value at exit time, but we still could miss it if we have a
transition such as P+A -> P -> A and assume a long enough propagation
delay for the maintenance interrupt (which is very likely).

In essence, we have lost the benefit of EISR, which was to give us a
way to deal with asynchronous signalling.

> 
> This will cause interrupt fired continuously to guest even 8250 IIR
> has no interrupt. When 8250's interrupt is configured in shared mode,
> it will pass interrupt to other drivers to handle. However, there
> is no other driver involved. Then, a "nobody cared" kernel complaint
> occurs.
> 
> / # cat /dev/ttyS0
> [4.826836] random: crng init done
> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
> option)
> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
> [6.378927] Hardware name: linux,dummy-virt (DT)
> [6.380876] Call trace:
> [6.381937]  dump_backtrace+0x0/0x180
> [6.383495]  show_stack+0x14/0x1c
> [6.384902]  dump_stack+0x90/0xb4
> [6.386312]  __report_bad_irq+0x38/0xe0
> [6.387944]  note_interrupt+0x1f4/0x2b8
> [6.389568]  handle_irq_event_percpu+0x54/0x7c
> [6.391433]  handle_irq_event+0x44/0x74
> [6.393056]  handle_fasteoi_irq+0x9c/0x154
> [6.394784]  generic_handle_irq+0x24/0x38
> [6.396483]  __handle_domain_irq+0x60/0xb4
> [6.398207]  gic_handle_irq+0x98/0x1b0
> [6.399796]  el1_irq+0xb0/0x128
> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
> [6.403149]  __setup_irq+0x41c/0x678
> [6.404669]  request_threaded_irq+0xe0/0x190
> [6.406474]  univ8250_setup_irq+0x208/0x234
> [6.408250]  serial8250_do_startup+0x1b4/0x754
> [6.410123]  serial8250_startup+0x20/0x28
> [6.411826]  uart_startup.part.21+0x78/0x144
> [6.413633]  uart_port_activate+0x50/0x68
> [6.415328]  tty_port_open+0x84/0xd4
> [6.416851]  uart_open+0x34/0x44
> [6.418229]  tty_open+0xec/0x3c8
> [6.419610]  chrdev_open+0xb0/0x198
> [6.421093]  do_dentry_open+0x200/0x310
> [6.422714]  vfs_open+0x54/0x84
> [6.424054]  path_openat+0x2dc/0xf04
> [6.425569]  do_filp_open+0x68/0xd8
> [6.427044]  do_sys_open+0x16c/0x224
> [6.428563]  SyS_openat+0x10/0x18
> [6.429972]  el0_svc_naked+0x30/0x34
> [6.431494] handlers:
> [6.432479] [<0e9fb4bb>] serial8250_interrupt
> [6.434597] Disabling IRQ #41
> 
> This patch changes the lr state condition in lr_signals_eoi_mi() from
> invalid(Inactive) to active and pending to avoid this.
> 
> I am not sure about the original design of the condition of
> invalid(active). So, This RFC is sent out for comments.
> 
> Cc: Joey Zheng 
> Signed-off-by: Shunyong Yang 
> ---
>  virt/kvm/arm/vgic/vgic-v2.c | 4 ++--
>  virt/kvm/arm/vgic/vgic-v3.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index e9d840a75e7b..740ee9a5f551 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ 

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Auger Eric]

Hi,

On 08/03/18 08:01, Shunyong Yang wrote:
> When resampling irqfds is enabled, level interrupt should be
> de-asserted when resampling happens. On page 4-47 of GIC v3
> specification IHI0069D, it said,
> "When the PE acknowledges an SGI, a PPI, or an SPI at the CPU
> interface, the IRI changes the status of the interrupt to active
> and pending if:
> • It is an edge-triggered interrupt, and another edge has been
> detected since the interrupt was acknowledged.
> • It is a level-sensitive interrupt, and the level has not been
> deasserted since the interrupt was acknowledged."
> 
> GIC v2 specification IHI0048B.b has similar description on page
> 3-42 for state machine transition.
> 
> When some VFIO device, like mtty(8250 VFIO mdev emulation driver
> in samples/vfio-mdev) triggers a level interrupt, the status
> transition in LR is pending-->active-->active and pending.
> Then it will wait resampling to de-assert the interrupt.
> 
> Current design of lr_signals_eoi_mi() will return false if state
> in LR is not invalid(Inactive). It causes resampling will not happen
> in mtty case.
> 
> This will cause interrupt fired continuously to guest even 8250 IIR
> has no interrupt. When 8250's interrupt is configured in shared mode,
> it will pass interrupt to other drivers to handle. However, there
> is no other driver involved. Then, a "nobody cared" kernel complaint
> occurs.
> 
> / # cat /dev/ttyS0
> [4.826836] random: crng init done
> [6.373620] irq 41: nobody cared (try booting with the "irqpoll"
> option)
> [6.376414] CPU: 0 PID: 1307 Comm: cat Not tainted 4.16.0-rc4 #4
> [6.378927] Hardware name: linux,dummy-virt (DT)
> [6.380876] Call trace:
> [6.381937]  dump_backtrace+0x0/0x180
> [6.383495]  show_stack+0x14/0x1c
> [6.384902]  dump_stack+0x90/0xb4
> [6.386312]  __report_bad_irq+0x38/0xe0
> [6.387944]  note_interrupt+0x1f4/0x2b8
> [6.389568]  handle_irq_event_percpu+0x54/0x7c
> [6.391433]  handle_irq_event+0x44/0x74
> [6.393056]  handle_fasteoi_irq+0x9c/0x154
> [6.394784]  generic_handle_irq+0x24/0x38
> [6.396483]  __handle_domain_irq+0x60/0xb4
> [6.398207]  gic_handle_irq+0x98/0x1b0
> [6.399796]  el1_irq+0xb0/0x128
> [6.401138]  _raw_spin_unlock_irqrestore+0x18/0x40
> [6.403149]  __setup_irq+0x41c/0x678
> [6.404669]  request_threaded_irq+0xe0/0x190
> [6.406474]  univ8250_setup_irq+0x208/0x234
> [6.408250]  serial8250_do_startup+0x1b4/0x754
> [6.410123]  serial8250_startup+0x20/0x28
> [6.411826]  uart_startup.part.21+0x78/0x144
> [6.413633]  uart_port_activate+0x50/0x68
> [6.415328]  tty_port_open+0x84/0xd4
> [6.416851]  uart_open+0x34/0x44
> [6.418229]  tty_open+0xec/0x3c8
> [6.419610]  chrdev_open+0xb0/0x198
> [6.421093]  do_dentry_open+0x200/0x310
> [6.422714]  vfs_open+0x54/0x84
> [6.424054]  path_openat+0x2dc/0xf04
> [6.425569]  do_filp_open+0x68/0xd8
> [6.427044]  do_sys_open+0x16c/0x224
> [6.428563]  SyS_openat+0x10/0x18
> [6.429972]  el0_svc_naked+0x30/0x34
> [6.431494] handlers:
> [6.432479] [<0e9fb4bb>] serial8250_interrupt
> [6.434597] Disabling IRQ #41
> 
> This patch changes the lr state condition in lr_signals_eoi_mi() from
> invalid(Inactive) to active and pending to avoid this.
> 
> I am not sure about the original design of the condition of
> invalid(active). So, This RFC is sent out for comments.
> 
> Cc: Joey Zheng 
> Signed-off-by: Shunyong Yang 
> ---
>  virt/kvm/arm/vgic/vgic-v2.c | 4 ++--
>  virt/kvm/arm/vgic/vgic-v3.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index e9d840a75e7b..740ee9a5f551 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -46,8 +46,8 @@ void vgic_v2_set_underflow(struct kvm_vcpu *vcpu)
>  
>  static bool lr_signals_eoi_mi(u32 lr_val)
>  {
> - return !(lr_val & GICH_LR_STATE) && (lr_val & GICH_LR_EOI) &&
> -!(lr_val & GICH_LR_HW);
> + return !((lr_val & GICH_LR_STATE) ^ GICH_LR_STATE) &&
> +(lr_val & GICH_LR_EOI) && !(lr_val & GICH_LR_HW);
>  }
>  
>  /*
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 6b329414e57a..43111bba7af9 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -35,8 +35,8 @@ void vgic_v3_set_underflow(struct kvm_vcpu *vcpu)
>  
>  static bool lr_signals_eoi_mi(u64 lr_val)
>  {
> - return !(lr_val & ICH_LR_STATE) && (lr_val & ICH_LR_EOI) &&
> -!(lr_val & ICH_LR_HW);
> + return !((lr_val & ICH_LR_STATE) ^ ICH_LR_STATE) &&
> +(lr_val & ICH_LR_EOI) && !(lr_val & ICH_LR_HW);


In general don't we have this state transition

inactive -> pending -> pending + active (1) -> active -> inactive.

In that case won't we lower the virt irq level when folding the LR on
Pending +