From: "Yang, Shunyong" <shunyong.yang@hxt-semitech.com> To: "eric.auger@redhat.com" <eric.auger@redhat.com>, "cdall@kernel.org" <cdall@kernel.org> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>, "kvmarm@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>, "Zheng, Joey" <yu.zheng@hxt-semitech.com>, "will.deacon@arm.com" <will.deacon@arm.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "david.daney@cavium.com" <david.daney@cavium.com>, "marc.zyngier@arm.com" <marc.zyngier@arm.com> Subject: Re: [此邮件可能存在风险] Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling Date: Thu, 8 Mar 2018 09:31:36 +0000 [thread overview] Message-ID: <1520501495.2583.19.camel@hxt-semitech.com> (raw) In-Reply-To: <9b3763f2-1dfa-5506-d7f2-93389647111c@redhat.com> 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 <yu.zheng@hxt-semitech.com> > > Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com> > > --- > > 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) > >
WARNING: multiple messages have this Message-ID (diff)
From: shunyong.yang@hxt-semitech.com (Yang, Shunyong) To: linux-arm-kernel@lists.infradead.org Subject: Re: [此邮件可能存在风险] Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling Date: Thu, 8 Mar 2018 09:31:36 +0000 [thread overview] Message-ID: <1520501495.2583.19.camel@hxt-semitech.com> (raw) In-Reply-To: <9b3763f2-1dfa-5506-d7f2-93389647111c@redhat.com> Hi, Eric, First, please let me change?Christoffer's email to?cdall at 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 <yu.zheng@hxt-semitech.com> > > Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com> > > --- > > ?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) > >
next prev parent reply other threads:[~2018-03-08 9:31 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-08 7:01 [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling Shunyong Yang 2018-03-08 7:01 ` Shunyong Yang 2018-03-08 7:01 ` Shunyong Yang 2018-03-08 8:57 ` Auger Eric 2018-03-08 8:57 ` Auger Eric 2018-03-08 9:31 ` Yang, Shunyong [this message] 2018-03-08 9:31 ` [此邮件可能存在风险] " Yang, Shunyong 2018-03-08 11:01 ` Marc Zyngier 2018-03-08 11:01 ` Marc Zyngier 2018-03-08 15:29 ` Auger Eric 2018-03-08 15:29 ` Auger Eric 2018-03-08 9:49 ` Marc Zyngier 2018-03-08 9:49 ` Marc Zyngier 2018-03-08 9:49 ` Marc Zyngier 2018-03-08 11:54 ` Marc Zyngier 2018-03-08 11:54 ` Marc Zyngier 2018-03-08 16:09 ` Auger Eric 2018-03-08 16:09 ` Auger Eric 2018-03-08 16:19 ` Christoffer Dall 2018-03-08 16:19 ` Christoffer Dall 2018-03-08 17:28 ` Marc Zyngier 2018-03-08 17:28 ` Marc Zyngier 2018-03-08 18:12 ` Auger Eric 2018-03-08 18:12 ` Auger Eric 2018-03-09 3:14 ` Yang, Shunyong 2018-03-09 3:14 ` Yang, Shunyong 2018-03-09 9:40 ` Marc Zyngier 2018-03-09 9:40 ` Marc Zyngier 2018-03-09 13:10 ` Auger Eric 2018-03-09 13:10 ` Auger Eric 2018-03-09 13:37 ` Marc Zyngier 2018-03-09 13:37 ` Marc Zyngier 2018-03-09 9:12 ` Marc Zyngier 2018-03-09 9:12 ` Marc Zyngier 2018-03-09 13:18 ` Auger Eric 2018-03-09 13:18 ` Auger Eric 2018-03-09 21:36 ` Christoffer Dall 2018-03-09 21:36 ` Christoffer Dall 2018-03-10 12:20 ` Marc Zyngier 2018-03-10 12:20 ` Marc Zyngier 2018-03-11 1:55 ` Christoffer Dall 2018-03-11 1:55 ` Christoffer Dall 2018-03-11 12:17 ` Marc Zyngier 2018-03-11 12:17 ` Marc Zyngier 2018-03-12 2:33 ` Yang, Shunyong 2018-03-12 2:33 ` Yang, Shunyong 2018-03-12 10:09 ` Marc Zyngier 2018-03-12 10:09 ` Marc Zyngier 2018-03-08 16:10 ` Christoffer Dall 2018-03-08 16:10 ` Christoffer Dall
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1520501495.2583.19.camel@hxt-semitech.com \ --to=shunyong.yang@hxt-semitech.com \ --cc=ard.biesheuvel@linaro.org \ --cc=cdall@kernel.org \ --cc=david.daney@cavium.com \ --cc=eric.auger@redhat.com \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=will.deacon@arm.com \ --cc=yu.zheng@hxt-semitech.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.