| Message ID | 20170906122612.18050-5-cdall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Christoffer, On 06/09/2017 14:26, Christoffer Dall wrote: > For mapped IRQs (with the HW bit set in the LR) we have to follow some > rules of the architecture. One of these rules is that VM must not be > allowed to deactivate a virtual interrupt with the HW bit set unless the > physical interrupt is also active. > > This works fine when injecting mapped interrupts, because we leave it up > to the injector to either set EOImode==1 or manually set the active > state of the physical interrupt. > > However, the guest can set virtual interrupt to be pending or active by > writing to the virtual distributor, which could lead to deactivating a > virtual interrupt with the HW bit set without the physical interrupt > being active. > > We could set the physical interrupt to active whenever we are about to > enter the VM with a HW interrupt either pending or active, but that > would be really slow, especially on GICv2. So we take the long way > around and do the hard work when needed, which is expected to be > extremely rare. > > When the VM sets the pending state for a HW interrupt on the virtual > distributor we set the active state on the physical distributor, because > the virtual interrupt can become active and then the guest can > deactivate it. > > When the VM clears the pending state we also clear it on the physical > side, because the injector might otherwise raise the interrupt. > > Signed-off-by: Christoffer Dall <cdall@linaro.org> > --- > virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++ > virt/kvm/arm/vgic/vgic.c | 7 +++++++ > virt/kvm/arm/vgic/vgic.h | 1 + > 3 files changed, 41 insertions(+) > > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c > index c1e4bdd..00003ae 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio.c > +++ b/virt/kvm/arm/vgic/vgic-mmio.c > @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu, > struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > > spin_lock(&irq->irq_lock); > + if (irq->hw) > + vgic_irq_set_phys_active(irq, true); > + > irq->pending_latch = true; > > vgic_queue_irq_unlock(vcpu->kvm, irq); > @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu, > struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); > > spin_lock(&irq->irq_lock); > + /* > + * We don't want the guest to effectively mask the physical > + * interrupt by doing a write to SPENDR followed by a write to > + * CPENDR for HW interrupts, so we clear the active state on > + * the physical side here. This may lead to taking an > + * additional interrupt on the host, but that should not be a > + * problem as the worst that can happen is an additional vgic > + * injection. We also clear the pending state to maintain > + * proper semantics for edge HW interrupts. > + */ > + if (irq->hw) { > + vgic_irq_set_phys_pending(irq, false); > + vgic_irq_set_phys_active(irq, false); I fail in understanding why the active state is reset here. Can you provide an example? If the physical dist has an active state can't the virtual IRQ be in active state, in which case you may later on deactivate a phys IRQ which is not active? Thanks Eric > + } > > irq->pending_latch = false; > > @@ -214,6 +231,22 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq, > irq->vcpu->cpu != -1) /* VCPU thread is running */ > cond_resched_lock(&irq->irq_lock); > > + if (irq->hw) { > + /* > + * We cannot support setting the physical active state for > + * private interrupts from another CPU than the one running > + * the VCPU which identifies which private interrupt it is > + * trying to modify. > + */ > + if (irq->intid < VGIC_NR_PRIVATE_IRQS && > + irq->target_vcpu != requester_vcpu) { > + spin_unlock(&irq->irq_lock); > + return; > + } > + > + vgic_irq_set_phys_active(irq, new_active_state); > + } > + > irq->active = new_active_state; > if (new_active_state) > vgic_queue_irq_unlock(vcpu->kvm, irq); > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c > index 8072969..7aec730 100644 > --- a/virt/kvm/arm/vgic/vgic.c > +++ b/virt/kvm/arm/vgic/vgic.c > @@ -140,6 +140,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq) > kfree(irq); > } > > +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending) > +{ > + WARN_ON(irq_set_irqchip_state(irq->host_irq, > + IRQCHIP_STATE_PENDING, > + pending)); > +} > + > /* Get the input level of a mapped IRQ directly from the physical GIC */ > bool vgic_get_phys_line_level(struct vgic_irq *irq) > { > diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h > index 7bdcda2..498ee05 100644 > --- a/virt/kvm/arm/vgic/vgic.h > +++ b/virt/kvm/arm/vgic/vgic.h > @@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu, > u32 intid); > void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq); > bool vgic_get_phys_line_level(struct vgic_irq *irq); > +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending); > void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active); > bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq); > void vgic_kick_vcpus(struct kvm *kvm); >
On 08/09/17 14:04, Auger Eric wrote: > Hi Christoffer, > > On 06/09/2017 14:26, Christoffer Dall wrote: >> For mapped IRQs (with the HW bit set in the LR) we have to follow some >> rules of the architecture. One of these rules is that VM must not be >> allowed to deactivate a virtual interrupt with the HW bit set unless the >> physical interrupt is also active. >> >> This works fine when injecting mapped interrupts, because we leave it up >> to the injector to either set EOImode==1 or manually set the active >> state of the physical interrupt. >> >> However, the guest can set virtual interrupt to be pending or active by >> writing to the virtual distributor, which could lead to deactivating a >> virtual interrupt with the HW bit set without the physical interrupt >> being active. >> >> We could set the physical interrupt to active whenever we are about to >> enter the VM with a HW interrupt either pending or active, but that >> would be really slow, especially on GICv2. So we take the long way >> around and do the hard work when needed, which is expected to be >> extremely rare. >> >> When the VM sets the pending state for a HW interrupt on the virtual >> distributor we set the active state on the physical distributor, because >> the virtual interrupt can become active and then the guest can >> deactivate it. >> >> When the VM clears the pending state we also clear it on the physical >> side, because the injector might otherwise raise the interrupt. >> >> Signed-off-by: Christoffer Dall <cdall@linaro.org> >> --- >> virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic/vgic.c | 7 +++++++ >> virt/kvm/arm/vgic/vgic.h | 1 + >> 3 files changed, 41 insertions(+) >> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >> index c1e4bdd..00003ae 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu, >> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >> >> spin_lock(&irq->irq_lock); >> + if (irq->hw) >> + vgic_irq_set_phys_active(irq, true); >> + >> irq->pending_latch = true; >> >> vgic_queue_irq_unlock(vcpu->kvm, irq); >> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu, >> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >> >> spin_lock(&irq->irq_lock); >> + /* >> + * We don't want the guest to effectively mask the physical >> + * interrupt by doing a write to SPENDR followed by a write to >> + * CPENDR for HW interrupts, so we clear the active state on >> + * the physical side here. This may lead to taking an >> + * additional interrupt on the host, but that should not be a >> + * problem as the worst that can happen is an additional vgic >> + * injection. We also clear the pending state to maintain >> + * proper semantics for edge HW interrupts. >> + */ >> + if (irq->hw) { >> + vgic_irq_set_phys_pending(irq, false); >> + vgic_irq_set_phys_active(irq, false); > I fail in understanding why the active state is reset here. Can you > provide an example? If we're clearing the pending state on the virtual side, we need to be able to let an incoming physical interrupt fire so that it can be made pending again. We may have to check that the virtual active state is clear though. > If the physical dist has an active state can't the virtual IRQ be in > active state, in which case you may later on deactivate a phys IRQ which > is not active? Well, I think we must be able to handle both cases: - CPENDR write: clear physical pending if (virtual active clear) clear physical active - CACTIVER write: clear physical active Does this work for you? Thanks, M.
Hi Marc, On 08/09/2017 15:32, Marc Zyngier wrote: > On 08/09/17 14:04, Auger Eric wrote: >> Hi Christoffer, >> >> On 06/09/2017 14:26, Christoffer Dall wrote: >>> For mapped IRQs (with the HW bit set in the LR) we have to follow some >>> rules of the architecture. One of these rules is that VM must not be >>> allowed to deactivate a virtual interrupt with the HW bit set unless the >>> physical interrupt is also active. >>> >>> This works fine when injecting mapped interrupts, because we leave it up >>> to the injector to either set EOImode==1 or manually set the active >>> state of the physical interrupt. >>> >>> However, the guest can set virtual interrupt to be pending or active by >>> writing to the virtual distributor, which could lead to deactivating a >>> virtual interrupt with the HW bit set without the physical interrupt >>> being active. >>> >>> We could set the physical interrupt to active whenever we are about to >>> enter the VM with a HW interrupt either pending or active, but that >>> would be really slow, especially on GICv2. So we take the long way >>> around and do the hard work when needed, which is expected to be >>> extremely rare. >>> >>> When the VM sets the pending state for a HW interrupt on the virtual >>> distributor we set the active state on the physical distributor, because >>> the virtual interrupt can become active and then the guest can >>> deactivate it. >>> >>> When the VM clears the pending state we also clear it on the physical >>> side, because the injector might otherwise raise the interrupt. >>> >>> Signed-off-by: Christoffer Dall <cdall@linaro.org> >>> --- >>> virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++ >>> virt/kvm/arm/vgic/vgic.c | 7 +++++++ >>> virt/kvm/arm/vgic/vgic.h | 1 + >>> 3 files changed, 41 insertions(+) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >>> index c1e4bdd..00003ae 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >>> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu, >>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >>> >>> spin_lock(&irq->irq_lock); >>> + if (irq->hw) >>> + vgic_irq_set_phys_active(irq, true); >>> + >>> irq->pending_latch = true; >>> >>> vgic_queue_irq_unlock(vcpu->kvm, irq); >>> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu, >>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >>> >>> spin_lock(&irq->irq_lock); >>> + /* >>> + * We don't want the guest to effectively mask the physical >>> + * interrupt by doing a write to SPENDR followed by a write to >>> + * CPENDR for HW interrupts, so we clear the active state on >>> + * the physical side here. This may lead to taking an >>> + * additional interrupt on the host, but that should not be a >>> + * problem as the worst that can happen is an additional vgic >>> + * injection. We also clear the pending state to maintain >>> + * proper semantics for edge HW interrupts. >>> + */ >>> + if (irq->hw) { >>> + vgic_irq_set_phys_pending(irq, false); >>> + vgic_irq_set_phys_active(irq, false); >> I fail in understanding why the active state is reset here. Can you >> provide an example? > > > If we're clearing the pending state on the virtual side, we need to be > able to let an incoming physical interrupt fire so that it can be made > pending again. We may have to check that the virtual active state is > clear though. > >> If the physical dist has an active state can't the virtual IRQ be in >> active state, in which case you may later on deactivate a phys IRQ which >> is not active? > > > Well, I think we must be able to handle both cases: > > - CPENDR write: > clear physical pending > if (virtual active clear) > clear physical active > > - CACTIVER write: > clear physical active > > Does this work for you? yes it does with that change! Thanks Eric > > Thanks, > > M. >
On Fri, Sep 8, 2017 at 4:27 PM, Auger Eric <eric.auger@redhat.com> wrote: > Hi Marc, > > On 08/09/2017 15:32, Marc Zyngier wrote: >> On 08/09/17 14:04, Auger Eric wrote: >>> Hi Christoffer, >>> >>> On 06/09/2017 14:26, Christoffer Dall wrote: >>>> For mapped IRQs (with the HW bit set in the LR) we have to follow some >>>> rules of the architecture. One of these rules is that VM must not be >>>> allowed to deactivate a virtual interrupt with the HW bit set unless the >>>> physical interrupt is also active. >>>> >>>> This works fine when injecting mapped interrupts, because we leave it up >>>> to the injector to either set EOImode==1 or manually set the active >>>> state of the physical interrupt. >>>> >>>> However, the guest can set virtual interrupt to be pending or active by >>>> writing to the virtual distributor, which could lead to deactivating a >>>> virtual interrupt with the HW bit set without the physical interrupt >>>> being active. >>>> >>>> We could set the physical interrupt to active whenever we are about to >>>> enter the VM with a HW interrupt either pending or active, but that >>>> would be really slow, especially on GICv2. So we take the long way >>>> around and do the hard work when needed, which is expected to be >>>> extremely rare. >>>> >>>> When the VM sets the pending state for a HW interrupt on the virtual >>>> distributor we set the active state on the physical distributor, because >>>> the virtual interrupt can become active and then the guest can >>>> deactivate it. >>>> >>>> When the VM clears the pending state we also clear it on the physical >>>> side, because the injector might otherwise raise the interrupt. >>>> >>>> Signed-off-by: Christoffer Dall <cdall@linaro.org> >>>> --- >>>> virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++ >>>> virt/kvm/arm/vgic/vgic.c | 7 +++++++ >>>> virt/kvm/arm/vgic/vgic.h | 1 + >>>> 3 files changed, 41 insertions(+) >>>> >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >>>> index c1e4bdd..00003ae 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >>>> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu, >>>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >>>> >>>> spin_lock(&irq->irq_lock); >>>> + if (irq->hw) >>>> + vgic_irq_set_phys_active(irq, true); >>>> + >>>> irq->pending_latch = true; >>>> >>>> vgic_queue_irq_unlock(vcpu->kvm, irq); >>>> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu, >>>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >>>> >>>> spin_lock(&irq->irq_lock); >>>> + /* >>>> + * We don't want the guest to effectively mask the physical >>>> + * interrupt by doing a write to SPENDR followed by a write to >>>> + * CPENDR for HW interrupts, so we clear the active state on >>>> + * the physical side here. This may lead to taking an >>>> + * additional interrupt on the host, but that should not be a >>>> + * problem as the worst that can happen is an additional vgic >>>> + * injection. We also clear the pending state to maintain >>>> + * proper semantics for edge HW interrupts. >>>> + */ >>>> + if (irq->hw) { >>>> + vgic_irq_set_phys_pending(irq, false); >>>> + vgic_irq_set_phys_active(irq, false); >>> I fail in understanding why the active state is reset here. Can you >>> provide an example? >> >> >> If we're clearing the pending state on the virtual side, we need to be >> able to let an incoming physical interrupt fire so that it can be made >> pending again. We may have to check that the virtual active state is >> clear though. >> >>> If the physical dist has an active state can't the virtual IRQ be in >>> active state, in which case you may later on deactivate a phys IRQ which >>> is not active? >> >> >> Well, I think we must be able to handle both cases: >> >> - CPENDR write: >> clear physical pending >> if (virtual active clear) >> clear physical active >> >> - CACTIVER write: >> clear physical active >> >> Does this work for you? > > yes it does with that change! > I agree, I should fix that. But then I wondered, are we properly handling PPIs for setting/clearing the pending state? Don't we actually need to do something similar like the active cpu where we check the requester CPU and make sure we're not fiddling with some random hardware state when accessed by userspace? Thanks, -Christoffer
On 08/09/17 17:05, Christoffer Dall wrote: > On Fri, Sep 8, 2017 at 4:27 PM, Auger Eric <eric.auger@redhat.com> wrote: >> Hi Marc, >> >> On 08/09/2017 15:32, Marc Zyngier wrote: >>> On 08/09/17 14:04, Auger Eric wrote: >>>> Hi Christoffer, >>>> >>>> On 06/09/2017 14:26, Christoffer Dall wrote: >>>>> For mapped IRQs (with the HW bit set in the LR) we have to follow some >>>>> rules of the architecture. One of these rules is that VM must not be >>>>> allowed to deactivate a virtual interrupt with the HW bit set unless the >>>>> physical interrupt is also active. >>>>> >>>>> This works fine when injecting mapped interrupts, because we leave it up >>>>> to the injector to either set EOImode==1 or manually set the active >>>>> state of the physical interrupt. >>>>> >>>>> However, the guest can set virtual interrupt to be pending or active by >>>>> writing to the virtual distributor, which could lead to deactivating a >>>>> virtual interrupt with the HW bit set without the physical interrupt >>>>> being active. >>>>> >>>>> We could set the physical interrupt to active whenever we are about to >>>>> enter the VM with a HW interrupt either pending or active, but that >>>>> would be really slow, especially on GICv2. So we take the long way >>>>> around and do the hard work when needed, which is expected to be >>>>> extremely rare. >>>>> >>>>> When the VM sets the pending state for a HW interrupt on the virtual >>>>> distributor we set the active state on the physical distributor, because >>>>> the virtual interrupt can become active and then the guest can >>>>> deactivate it. >>>>> >>>>> When the VM clears the pending state we also clear it on the physical >>>>> side, because the injector might otherwise raise the interrupt. >>>>> >>>>> Signed-off-by: Christoffer Dall <cdall@linaro.org> >>>>> --- >>>>> virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++ >>>>> virt/kvm/arm/vgic/vgic.c | 7 +++++++ >>>>> virt/kvm/arm/vgic/vgic.h | 1 + >>>>> 3 files changed, 41 insertions(+) >>>>> >>>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c >>>>> index c1e4bdd..00003ae 100644 >>>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c >>>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c >>>>> @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu, >>>>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >>>>> >>>>> spin_lock(&irq->irq_lock); >>>>> + if (irq->hw) >>>>> + vgic_irq_set_phys_active(irq, true); >>>>> + >>>>> irq->pending_latch = true; >>>>> >>>>> vgic_queue_irq_unlock(vcpu->kvm, irq); >>>>> @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu, >>>>> struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); >>>>> >>>>> spin_lock(&irq->irq_lock); >>>>> + /* >>>>> + * We don't want the guest to effectively mask the physical >>>>> + * interrupt by doing a write to SPENDR followed by a write to >>>>> + * CPENDR for HW interrupts, so we clear the active state on >>>>> + * the physical side here. This may lead to taking an >>>>> + * additional interrupt on the host, but that should not be a >>>>> + * problem as the worst that can happen is an additional vgic >>>>> + * injection. We also clear the pending state to maintain >>>>> + * proper semantics for edge HW interrupts. >>>>> + */ >>>>> + if (irq->hw) { >>>>> + vgic_irq_set_phys_pending(irq, false); >>>>> + vgic_irq_set_phys_active(irq, false); >>>> I fail in understanding why the active state is reset here. Can you >>>> provide an example? >>> >>> >>> If we're clearing the pending state on the virtual side, we need to be >>> able to let an incoming physical interrupt fire so that it can be made >>> pending again. We may have to check that the virtual active state is >>> clear though. >>> >>>> If the physical dist has an active state can't the virtual IRQ be in >>>> active state, in which case you may later on deactivate a phys IRQ which >>>> is not active? >>> >>> >>> Well, I think we must be able to handle both cases: >>> >>> - CPENDR write: >>> clear physical pending >>> if (virtual active clear) >>> clear physical active >>> >>> - CACTIVER write: >>> clear physical active >>> >>> Does this work for you? >> >> yes it does with that change! >> > I agree, I should fix that. > > But then I wondered, are we properly handling PPIs for > setting/clearing the pending state? Don't we actually need to do > something similar like the active cpu where we check the requester CPU > and make sure we're not fiddling with some random hardware state when > accessed by userspace? Hmmm. Yeah, that's another can of worm. We should certainly make sure that whatever userspace does is not directly impacting the HW state (and only the guest does). I think. I'm not quite sure... M. (GIC-ed, again).
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c index c1e4bdd..00003ae 100644 --- a/virt/kvm/arm/vgic/vgic-mmio.c +++ b/virt/kvm/arm/vgic/vgic-mmio.c @@ -131,6 +131,9 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu, struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); spin_lock(&irq->irq_lock); + if (irq->hw) + vgic_irq_set_phys_active(irq, true); + irq->pending_latch = true; vgic_queue_irq_unlock(vcpu->kvm, irq); @@ -149,6 +152,20 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu, struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); spin_lock(&irq->irq_lock); + /* + * We don't want the guest to effectively mask the physical + * interrupt by doing a write to SPENDR followed by a write to + * CPENDR for HW interrupts, so we clear the active state on + * the physical side here. This may lead to taking an + * additional interrupt on the host, but that should not be a + * problem as the worst that can happen is an additional vgic + * injection. We also clear the pending state to maintain + * proper semantics for edge HW interrupts. + */ + if (irq->hw) { + vgic_irq_set_phys_pending(irq, false); + vgic_irq_set_phys_active(irq, false); + } irq->pending_latch = false; @@ -214,6 +231,22 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq, irq->vcpu->cpu != -1) /* VCPU thread is running */ cond_resched_lock(&irq->irq_lock); + if (irq->hw) { + /* + * We cannot support setting the physical active state for + * private interrupts from another CPU than the one running + * the VCPU which identifies which private interrupt it is + * trying to modify. + */ + if (irq->intid < VGIC_NR_PRIVATE_IRQS && + irq->target_vcpu != requester_vcpu) { + spin_unlock(&irq->irq_lock); + return; + } + + vgic_irq_set_phys_active(irq, new_active_state); + } + irq->active = new_active_state; if (new_active_state) vgic_queue_irq_unlock(vcpu->kvm, irq); diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c index 8072969..7aec730 100644 --- a/virt/kvm/arm/vgic/vgic.c +++ b/virt/kvm/arm/vgic/vgic.c @@ -140,6 +140,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq) kfree(irq); } +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending) +{ + WARN_ON(irq_set_irqchip_state(irq->host_irq, + IRQCHIP_STATE_PENDING, + pending)); +} + /* Get the input level of a mapped IRQ directly from the physical GIC */ bool vgic_get_phys_line_level(struct vgic_irq *irq) { diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h index 7bdcda2..498ee05 100644 --- a/virt/kvm/arm/vgic/vgic.h +++ b/virt/kvm/arm/vgic/vgic.h @@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu, u32 intid); void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq); bool vgic_get_phys_line_level(struct vgic_irq *irq); +void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending); void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active); bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq); void vgic_kick_vcpus(struct kvm *kvm);
For mapped IRQs (with the HW bit set in the LR) we have to follow some rules of the architecture. One of these rules is that VM must not be allowed to deactivate a virtual interrupt with the HW bit set unless the physical interrupt is also active. This works fine when injecting mapped interrupts, because we leave it up to the injector to either set EOImode==1 or manually set the active state of the physical interrupt. However, the guest can set virtual interrupt to be pending or active by writing to the virtual distributor, which could lead to deactivating a virtual interrupt with the HW bit set without the physical interrupt being active. We could set the physical interrupt to active whenever we are about to enter the VM with a HW interrupt either pending or active, but that would be really slow, especially on GICv2. So we take the long way around and do the hard work when needed, which is expected to be extremely rare. When the VM sets the pending state for a HW interrupt on the virtual distributor we set the active state on the physical distributor, because the virtual interrupt can become active and then the guest can deactivate it. When the VM clears the pending state we also clear it on the physical side, because the injector might otherwise raise the interrupt. Signed-off-by: Christoffer Dall <cdall@linaro.org> --- virt/kvm/arm/vgic/vgic-mmio.c | 33 +++++++++++++++++++++++++++++++++ virt/kvm/arm/vgic/vgic.c | 7 +++++++ virt/kvm/arm/vgic/vgic.h | 1 + 3 files changed, 41 insertions(+)