From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755566AbeCHJbq (ORCPT ); Thu, 8 Mar 2018 04:31:46 -0500 Received: from mx01.hxt-semitech.com.96.203.223.in-addr.arpa ([223.203.96.7]:47057 "EHLO barracuda.hxt-semitech.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1755537AbeCHJbl (ORCPT ); Thu, 8 Mar 2018 04:31:41 -0500 X-ASG-Debug-ID: 1520501497-093b7e4ca912b60001-xx1T2L X-Barracuda-Envelope-From: shunyong.yang@hxt-semitech.com From: "Yang, Shunyong" To: "eric.auger@redhat.com" , "cdall@kernel.org" CC: "linux-kernel@vger.kernel.org" , "ard.biesheuvel@linaro.org" , "kvmarm@lists.cs.columbia.edu" , "Zheng, Joey" , "will.deacon@arm.com" , "linux-arm-kernel@lists.infradead.org" , "david.daney@cavium.com" , "marc.zyngier@arm.com" Subject: =?utf-8?B?UmU6IFvmraTpgq7ku7blj6/og73lrZjlnKjpo47pmaldICBSZTogW1JGQyBQ?= =?utf-8?B?QVRDSF0gS1ZNOiBhcm0vYXJtNjQ6IHZnaWM6IGNoYW5nZSBjb25kaXRpb24g?= =?utf-8?Q?for_level_interrupt_resampling?= Thread-Topic: =?utf-8?B?W+atpOmCruS7tuWPr+iDveWtmOWcqOmjjumZqV0gIFJlOiBbUkZDIFBBVENI?= =?utf-8?B?XSBLVk06IGFybS9hcm02NDogdmdpYzogY2hhbmdlIGNvbmRpdGlvbiBmb3Ig?= =?utf-8?Q?level_interrupt_resampling?= X-ASG-Orig-Subj: =?utf-8?B?UmU6IFvmraTpgq7ku7blj6/og73lrZjlnKjpo47pmaldICBSZTogW1JGQyBQ?= =?utf-8?B?QVRDSF0gS1ZNOiBhcm0vYXJtNjQ6IHZnaWM6IGNoYW5nZSBjb25kaXRpb24g?= =?utf-8?Q?for_level_interrupt_resampling?= Thread-Index: AQHTtquoFGdOJ9stn0iMRL6AbYCzvaPFg4+AgAAJfYA= Date: Thu, 8 Mar 2018 09:31:36 +0000 Message-ID: <1520501495.2583.19.camel@hxt-semitech.com> References: <1520492490-7943-1-git-send-email-shunyong.yang@hxt-semitech.com> <9b3763f2-1dfa-5506-d7f2-93389647111c@redhat.com> In-Reply-To: <9b3763f2-1dfa-5506-d7f2-93389647111c@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.64.6.48] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 X-Barracuda-Connect: UNKNOWN[10.128.0.15] X-Barracuda-Start-Time: 1520501497 X-Barracuda-Encrypted: ECDHE-RSA-AES256-SHA X-Barracuda-URL: https://192.168.50.101:443/cgi-mod/mark.cgi X-Barracuda-BRTS-Status: 1 X-Barracuda-Bayes: INNOCENT GLOBAL 0.5104 1.0000 0.7500 X-Barracuda-Spam-Score: 0.75 X-Barracuda-Spam-Status: No, SCORE=0.75 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=9.0 tests=BSF_SC0_MV0113c X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.48722 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.00 BSF_SC0_MV0113c BSF_SC0_MV0113c Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id w289Vpcx007750 Hi, Eric, First, please let me change Christoffer's email to cdall@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] [<000000000e9fb4bb>] 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 + Active state, which is not was we want? > > Thanks > > Eric In current code, in my test, when I output LR value of the mtty IRQ 41 (hwirq = 36) in vgic_v3_fold_lr_state(). The LR's transition starts like following, 0-->50a0020000000024-->90a0020000000024-->d0a0020000000024 That is inactive-->pending-->active-->pending + active. Then it keep running cyclic pending-->active-->pending + active. The level interrupt de-assert should happen in following code /* Notify fds when the guest EOI'ed a level-triggered IRQ */ if (lr_signals_eoi_mi(val) && vgic_valid_spi(vcpu->kvm, intid)) kvm_notify_acked_irq(vcpu->kvm, 0,      intid - VGIC_NR_PRIVATE_IRQS); But as addressed in commit message, lr_signals_eoi_mi() will return false if state in LR is not invalid(inactive), so it has no chance to de-assert the level interrupt in my test.  Thanks. Shunyong. > > > > >  } > >   > >  void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu) > >