From: James Morse <james.morse@arm.com> To: Marc Zyngier <maz@kernel.org> Cc: Andre Przywara <andre.przywara@arm.com>, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] KVM: arm64: arch_timer shouldn't assume the vcpu is loaded Date: Thu, 9 Apr 2020 17:53:28 +0100 [thread overview] Message-ID: <20498855-352b-ed7a-c851-8ecf8b77e503@arm.com> (raw) In-Reply-To: <20200409092706.74e6bd1d@why> Hi Marc, On 09/04/2020 09:27, Marc Zyngier wrote: > On Wed, 8 Apr 2020 12:16:01 +0100 > James Morse <james.morse@arm.com> wrote: >> On 08/04/2020 11:07, Marc Zyngier wrote: >>> I don't fully agree with the analysis, Remember we are looking at the >>> state of the physical interrupt associated with a virtual interrupt, so >>> the vcpu doesn't quite make sense here if it isn't loaded. >>> >>> What does it mean to look at the HW timer when we are not in the right >>> context? For all we know, it is completely random (the only guarantee >>> we have is that it is disabled, actually). >> >>> My gut feeling is that this is another instance where we should provide >>> specific userspace accessors that would only deal with the virtual >>> state, and leave anything that deals with the physical state of the >>> interrupt to be exercised only by the guest. >> >>> Does it make sense? >> >> Broadly, yes. Specifically ... I'm not familiar enough with this code to work out where >> such a change should go! >> >> ~20 mins of grepping later~ >> >> Remove REGISTER_DESC_WITH_LENGTH() so that uaccess helpers have to be provided, and forbid >> NULL for the ur/uw values in REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED()...? > > I'd suggest something like this (untested, of course): [...] >> Or if that is too invasive, something like, (totally, untested): >> ----------------%<---------------- >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >> index 97fb2a40e6ba..30ae5f29e429 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >> @@ -113,10 +113,11 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu, >> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >> >> raw_spin_lock_irqsave(&irq->irq_lock, flags); >> - if (vgic_irq_is_mapped_level(irq)) { >> + if (kvm_running_vcpu() && vgic_irq_is_mapped_level(irq)) { >> bool was_high = irq->line_level; >> >> /* >> + * Unless we are running due to a user-space access, >> * We need to update the state of the interrupt because >> * the guest might have changed the state of the device >> * while the interrupt was disabled at the VGIC level. >> ----------------%<---------------- > > Huh, nice try! ;-) Unfortunately, I think there is more than the enable > register that suffers from a similar problem (see how the pending > register write is also accessing the HW state, even if accessed from > userspace). Yeah, I'd expect to play wack-a-mole if I actually tested it. It was just the smallest, er, hack I could get my head round given your explanation. I've blindly tested your version, it works for me on a gicv2 machine: Tested-by: James Morse <james.morse@arm.com> I'll test on the gicv3 espressobin that I originally saw this on with rc1 on Tuesday. Do you want me to post it back to you as a tested patch? You can judge whether I understand it from the commit message... (I'd need your Signed-off-by...) Have a good extended weekend! Thanks, James _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
WARNING: multiple messages have this Message-ID (diff)
From: James Morse <james.morse@arm.com> To: Marc Zyngier <maz@kernel.org> Cc: Andre Przywara <andre.przywara@arm.com>, Julien Thierry <julien.thierry.kdev@gmail.com>, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, Suzuki K Poulose <suzuki.poulose@arm.com> Subject: Re: [PATCH] KVM: arm64: arch_timer shouldn't assume the vcpu is loaded Date: Thu, 9 Apr 2020 17:53:28 +0100 [thread overview] Message-ID: <20498855-352b-ed7a-c851-8ecf8b77e503@arm.com> (raw) In-Reply-To: <20200409092706.74e6bd1d@why> Hi Marc, On 09/04/2020 09:27, Marc Zyngier wrote: > On Wed, 8 Apr 2020 12:16:01 +0100 > James Morse <james.morse@arm.com> wrote: >> On 08/04/2020 11:07, Marc Zyngier wrote: >>> I don't fully agree with the analysis, Remember we are looking at the >>> state of the physical interrupt associated with a virtual interrupt, so >>> the vcpu doesn't quite make sense here if it isn't loaded. >>> >>> What does it mean to look at the HW timer when we are not in the right >>> context? For all we know, it is completely random (the only guarantee >>> we have is that it is disabled, actually). >> >>> My gut feeling is that this is another instance where we should provide >>> specific userspace accessors that would only deal with the virtual >>> state, and leave anything that deals with the physical state of the >>> interrupt to be exercised only by the guest. >> >>> Does it make sense? >> >> Broadly, yes. Specifically ... I'm not familiar enough with this code to work out where >> such a change should go! >> >> ~20 mins of grepping later~ >> >> Remove REGISTER_DESC_WITH_LENGTH() so that uaccess helpers have to be provided, and forbid >> NULL for the ur/uw values in REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED()...? > > I'd suggest something like this (untested, of course): [...] >> Or if that is too invasive, something like, (totally, untested): >> ----------------%<---------------- >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >> index 97fb2a40e6ba..30ae5f29e429 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >> @@ -113,10 +113,11 @@ void vgic_mmio_write_senable(struct kvm_vcpu *vcpu, >> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >> >> raw_spin_lock_irqsave(&irq->irq_lock, flags); >> - if (vgic_irq_is_mapped_level(irq)) { >> + if (kvm_running_vcpu() && vgic_irq_is_mapped_level(irq)) { >> bool was_high = irq->line_level; >> >> /* >> + * Unless we are running due to a user-space access, >> * We need to update the state of the interrupt because >> * the guest might have changed the state of the device >> * while the interrupt was disabled at the VGIC level. >> ----------------%<---------------- > > Huh, nice try! ;-) Unfortunately, I think there is more than the enable > register that suffers from a similar problem (see how the pending > register write is also accessing the HW state, even if accessed from > userspace). Yeah, I'd expect to play wack-a-mole if I actually tested it. It was just the smallest, er, hack I could get my head round given your explanation. I've blindly tested your version, it works for me on a gicv2 machine: Tested-by: James Morse <james.morse@arm.com> I'll test on the gicv3 espressobin that I originally saw this on with rc1 on Tuesday. Do you want me to post it back to you as a tested patch? You can judge whether I understand it from the commit message... (I'd need your Signed-off-by...) Have a good extended weekend! Thanks, James _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-04-09 16:53 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-06 15:03 [PATCH] KVM: arm64: arch_timer shouldn't assume the vcpu is loaded James Morse 2020-04-06 15:03 ` James Morse 2020-04-08 10:07 ` Marc Zyngier 2020-04-08 10:07 ` Marc Zyngier 2020-04-08 11:16 ` James Morse 2020-04-08 11:16 ` James Morse 2020-04-09 8:27 ` Marc Zyngier 2020-04-09 8:27 ` Marc Zyngier 2020-04-09 16:53 ` James Morse [this message] 2020-04-09 16:53 ` James Morse 2020-04-08 12:13 ` André Przywara 2020-04-08 12:13 ` André Przywara 2020-04-08 14:19 ` Marc Zyngier 2020-04-08 14:19 ` Marc Zyngier 2020-04-08 16:50 ` André Przywara 2020-04-08 16:50 ` André Przywara 2020-04-09 8:08 ` Marc Zyngier 2020-04-09 8:08 ` Marc Zyngier 2020-04-09 16:04 ` André Przywara 2020-04-09 16:04 ` André Przywara
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=20498855-352b-ed7a-c851-8ecf8b77e503@arm.com \ --to=james.morse@arm.com \ --cc=andre.przywara@arm.com \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=maz@kernel.org \ /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.