| Message ID | 20170316112030.20419-11-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 03/16/2017 11:20 AM, Andre Przywara wrote: > Upon receiving an LPI, we need to find the right VCPU and virtual IRQ > number to get this IRQ injected. > Iterate our two-level LPI table to find this information quickly when > the host takes an LPI. Call the existing injection function to let the > GIC emulation deal with this interrupt. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-v3-lpi.c | 41 +++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/gic.c | 6 ++++-- > xen/include/asm-arm/irq.h | 8 ++++++++ > 3 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c > index 59d3ba4..0579976 100644 > --- a/xen/arch/arm/gic-v3-lpi.c > +++ b/xen/arch/arm/gic-v3-lpi.c > @@ -104,6 +104,47 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, bool use_pta) > return per_cpu(lpi_redist, cpu).redist_id << 16; > } > > +/* > + * Handle incoming LPIs, which are a bit special, because they are potentially > + * numerous and also only get injected into guests. Treat them specially here, > + * by just looking up their target vCPU and virtual LPI number and hand it > + * over to the injection function. > + */ > +void do_LPI(unsigned int lpi) > +{ > + struct domain *d; > + union host_lpi *hlpip, hlpi; > + struct vcpu *vcpu; > + > + WRITE_SYSREG32(lpi, ICC_EOIR1_EL1); > + > + hlpip = gic_get_host_lpi(lpi); > + if ( !hlpip ) > + return; > + > + hlpi.data = read_u64_atomic(&hlpip->data); > + > + /* We may have mapped more host LPIs than the guest actually asked for. */ Another way, is the interrupt has been received at the same time the guest is configuring it. What will happen if the interrupt is lost? > + if ( !hlpi.virt_lpi ) > + return; > + > + d = get_domain_by_id(hlpi.dom_id); It would be enough to use rcu_lock_domain_by_id here. > + if ( !d ) > + return; > + > + if ( hlpi.vcpu_id >= d->max_vcpus ) A comment would be certainly useful here to explain why this check. > + { > + put_domain(d); > + return; > + } > + > + vcpu = d->vcpu[hlpi.vcpu_id]; > + > + put_domain(d); > + > + vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi); > +} > + > static int gicv3_lpi_allocate_pendtable(uint64_t *reg) > { > uint64_t val; > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index bd3c032..7286e5d 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -700,8 +700,10 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) > local_irq_enable(); > do_IRQ(regs, irq, is_fiq); > local_irq_disable(); > - } > - else if (unlikely(irq < 16)) > + } else if ( is_lpi(irq) ) > + { > + do_LPI(irq); I really don't want to see GICv3 specific code called in common code. Please introduce a specific callback in gic_hw_operations. > + } else if ( unlikely(irq < 16) ) Coding style: } else if (...) { } else if (...) > { > do_sgi(regs, irq); > } > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h > index 8f7a167..ee47de8 100644 > --- a/xen/include/asm-arm/irq.h > +++ b/xen/include/asm-arm/irq.h > @@ -34,6 +34,14 @@ struct irq_desc *__irq_to_desc(int irq); > > void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq); > > +#ifdef CONFIG_HAS_ITS > +void do_LPI(unsigned int irq); > +#else > +static inline void do_LPI(unsigned int irq) > +{ > +} > +#endif > + This would avoid such ugly hack where do_LPI is define in gic-v3-its.c but declared in irq.h. Cheers,
Hi, On 24/03/17 12:03, Julien Grall wrote: > Hi Andre, > > On 03/16/2017 11:20 AM, Andre Przywara wrote: >> Upon receiving an LPI, we need to find the right VCPU and virtual IRQ >> number to get this IRQ injected. >> Iterate our two-level LPI table to find this information quickly when >> the host takes an LPI. Call the existing injection function to let the >> GIC emulation deal with this interrupt. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic-v3-lpi.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/gic.c | 6 ++++-- >> xen/include/asm-arm/irq.h | 8 ++++++++ >> 3 files changed, 53 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c >> index 59d3ba4..0579976 100644 >> --- a/xen/arch/arm/gic-v3-lpi.c >> +++ b/xen/arch/arm/gic-v3-lpi.c >> @@ -104,6 +104,47 @@ uint64_t gicv3_get_redist_address(unsigned int >> cpu, bool use_pta) >> return per_cpu(lpi_redist, cpu).redist_id << 16; >> } >> >> +/* >> + * Handle incoming LPIs, which are a bit special, because they are >> potentially >> + * numerous and also only get injected into guests. Treat them >> specially here, >> + * by just looking up their target vCPU and virtual LPI number and >> hand it >> + * over to the injection function. >> + */ >> +void do_LPI(unsigned int lpi) >> +{ >> + struct domain *d; >> + union host_lpi *hlpip, hlpi; >> + struct vcpu *vcpu; >> + >> + WRITE_SYSREG32(lpi, ICC_EOIR1_EL1); >> + >> + hlpip = gic_get_host_lpi(lpi); >> + if ( !hlpip ) >> + return; >> + >> + hlpi.data = read_u64_atomic(&hlpip->data); >> + >> + /* We may have mapped more host LPIs than the guest actually >> asked for. */ > > Another way, is the interrupt has been received at the same time the > guest is configuring it. What will happen if the interrupt is lost? I don't see how this would be our problem. If the guest is still about to configure an LPI, then any incoming one is pretty clearly a spurious interrupt. I take it that you mean "mapping an LPI using ITS commands" when you say "configuring" here. If this is not what you mean, please correct me. I'd expect a guest to first map the LPI, then enable it and send an INV command. Doing that differently will not result in any sane behavior (as in: a guest cannot expect an LPI to come through), so nothing we need to worry about (from a lost IRQ perspective). Or what do I miss here? > >> + if ( !hlpi.virt_lpi ) >> + return; >> + >> + d = get_domain_by_id(hlpi.dom_id); > > It would be enough to use rcu_lock_domain_by_id here. Done in v3. >> + if ( !d ) >> + return; >> + >> + if ( hlpi.vcpu_id >= d->max_vcpus ) > > A comment would be certainly useful here to explain why this check. Done in v3, though personally I don't see what is unclear about an: if ( id >= max ) return; >> + { >> + put_domain(d); >> + return; >> + } >> + >> + vcpu = d->vcpu[hlpi.vcpu_id]; >> + >> + put_domain(d); >> + >> + vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi); >> +} >> + >> static int gicv3_lpi_allocate_pendtable(uint64_t *reg) >> { >> uint64_t val; >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index bd3c032..7286e5d 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -700,8 +700,10 @@ void gic_interrupt(struct cpu_user_regs *regs, >> int is_fiq) >> local_irq_enable(); >> do_IRQ(regs, irq, is_fiq); >> local_irq_disable(); >> - } >> - else if (unlikely(irq < 16)) >> + } else if ( is_lpi(irq) ) >> + { >> + do_LPI(irq); > > I really don't want to see GICv3 specific code called in common code. > Please introduce a specific callback in gic_hw_operations. OK, you convinced me by persistence ;-) I still don't fully believe it, but well ... > >> + } else if ( unlikely(irq < 16) ) > > Coding style: > > } > else if (...) > { > } > else if (...) Fixed in v3. > >> { >> do_sgi(regs, irq); >> } >> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h >> index 8f7a167..ee47de8 100644 >> --- a/xen/include/asm-arm/irq.h >> +++ b/xen/include/asm-arm/irq.h >> @@ -34,6 +34,14 @@ struct irq_desc *__irq_to_desc(int irq); >> >> void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq); >> >> +#ifdef CONFIG_HAS_ITS >> +void do_LPI(unsigned int irq); >> +#else >> +static inline void do_LPI(unsigned int irq) >> +{ >> +} >> +#endif >> + > > This would avoid such ugly hack where do_LPI is define in gic-v3-its.c > but declared in irq.h. I agree that this doesn't look nice, that's why I came up with a neater version in v3. So doing the indirection comes at the price of additional code, which is hard to chase (because of the function pointer) and probably more costly (because it's harder to speculate on a CPU). But well, as you wish ... Cheers, Andre.
On 03/04/17 15:18, Andre Przywara wrote: > Hi, Hi Andre, > On 24/03/17 12:03, Julien Grall wrote: >>> + /* We may have mapped more host LPIs than the guest actually >>> asked for. */ >> >> Another way, is the interrupt has been received at the same time the >> guest is configuring it. What will happen if the interrupt is lost? > > I don't see how this would be our problem. If the guest is still about > to configure an LPI, then any incoming one is pretty clearly a spurious > interrupt. > I take it that you mean "mapping an LPI using ITS commands" when you say > "configuring" here. If this is not what you mean, please correct me. > > I'd expect a guest to first map the LPI, then enable it and send an INV > command. Doing that differently will not result in any sane behavior (as > in: a guest cannot expect an LPI to come through), so nothing we need to > worry about (from a lost IRQ perspective). > > Or what do I miss here? All host LPIs will be enabled when a device is assigned to a guest. So technically an LPI can come at anytime before the guest is configuring the virtual mapping. Cheers,
On 04/04/17 13:51, Andre Przywara wrote: > On 04/04/17 12:49, Julien Grall wrote: >> >> >> On 03/04/17 15:18, Andre Przywara wrote: >>> Hi, >> >> Hi Andre, >> >>> On 24/03/17 12:03, Julien Grall wrote: >>>>> + /* We may have mapped more host LPIs than the guest actually >>>>> asked for. */ >>>> >>>> Another way, is the interrupt has been received at the same time the >>>> guest is configuring it. What will happen if the interrupt is lost? >>> >>> I don't see how this would be our problem. If the guest is still about >>> to configure an LPI, then any incoming one is pretty clearly a spurious >>> interrupt. >>> I take it that you mean "mapping an LPI using ITS commands" when you say >>> "configuring" here. If this is not what you mean, please correct me. >>> >>> I'd expect a guest to first map the LPI, then enable it and send an INV >>> command. Doing that differently will not result in any sane behavior (as >>> in: a guest cannot expect an LPI to come through), so nothing we need to >>> worry about (from a lost IRQ perspective). >>> >>> Or what do I miss here? >> >> All host LPIs will be enabled when a device is assigned to a guest. So >> technically an LPI can come at anytime before the guest is configuring >> the virtual mapping. > > As I said: what is the problem? Any LPI coming in without being mapped > by a guest would be spurious from a guest point of view, as in not > expected and ignored. But a guest will never receive it, since we > explicitly check for that and on the host side and bail out early. From > do_LPI(): > /* Unmapped events are marked with an invalid LPI ID. */ > if ( hlpi.virt_lpi == INVALID_LPI ) > return; > > And we (atomically) update the entry once the guest has configured it, > so either it's ignored or delivered. And this is a benign race you have > on real hardware too. The document it... if I ask a questions multiple time it is likely the code is not clear enough. Cheers,
Hi, On 04/04/17 12:49, Julien Grall wrote: > > > On 03/04/17 15:18, Andre Przywara wrote: >> Hi, > > Hi Andre, > >> On 24/03/17 12:03, Julien Grall wrote: >>>> + /* We may have mapped more host LPIs than the guest actually >>>> asked for. */ >>> >>> Another way, is the interrupt has been received at the same time the >>> guest is configuring it. What will happen if the interrupt is lost? >> >> I don't see how this would be our problem. If the guest is still about >> to configure an LPI, then any incoming one is pretty clearly a spurious >> interrupt. >> I take it that you mean "mapping an LPI using ITS commands" when you say >> "configuring" here. If this is not what you mean, please correct me. >> >> I'd expect a guest to first map the LPI, then enable it and send an INV >> command. Doing that differently will not result in any sane behavior (as >> in: a guest cannot expect an LPI to come through), so nothing we need to >> worry about (from a lost IRQ perspective). >> >> Or what do I miss here? > > All host LPIs will be enabled when a device is assigned to a guest. So > technically an LPI can come at anytime before the guest is configuring > the virtual mapping. As I said: what is the problem? Any LPI coming in without being mapped by a guest would be spurious from a guest point of view, as in not expected and ignored. But a guest will never receive it, since we explicitly check for that and on the host side and bail out early. From do_LPI(): /* Unmapped events are marked with an invalid LPI ID. */ if ( hlpi.virt_lpi == INVALID_LPI ) return; And we (atomically) update the entry once the guest has configured it, so either it's ignored or delivered. And this is a benign race you have on real hardware too. Cheers, Andre.
diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c index 59d3ba4..0579976 100644 --- a/xen/arch/arm/gic-v3-lpi.c +++ b/xen/arch/arm/gic-v3-lpi.c @@ -104,6 +104,47 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, bool use_pta) return per_cpu(lpi_redist, cpu).redist_id << 16; } +/* + * Handle incoming LPIs, which are a bit special, because they are potentially + * numerous and also only get injected into guests. Treat them specially here, + * by just looking up their target vCPU and virtual LPI number and hand it + * over to the injection function. + */ +void do_LPI(unsigned int lpi) +{ + struct domain *d; + union host_lpi *hlpip, hlpi; + struct vcpu *vcpu; + + WRITE_SYSREG32(lpi, ICC_EOIR1_EL1); + + hlpip = gic_get_host_lpi(lpi); + if ( !hlpip ) + return; + + hlpi.data = read_u64_atomic(&hlpip->data); + + /* We may have mapped more host LPIs than the guest actually asked for. */ + if ( !hlpi.virt_lpi ) + return; + + d = get_domain_by_id(hlpi.dom_id); + if ( !d ) + return; + + if ( hlpi.vcpu_id >= d->max_vcpus ) + { + put_domain(d); + return; + } + + vcpu = d->vcpu[hlpi.vcpu_id]; + + put_domain(d); + + vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi); +} + static int gicv3_lpi_allocate_pendtable(uint64_t *reg) { uint64_t val; diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index bd3c032..7286e5d 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -700,8 +700,10 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) local_irq_enable(); do_IRQ(regs, irq, is_fiq); local_irq_disable(); - } - else if (unlikely(irq < 16)) + } else if ( is_lpi(irq) ) + { + do_LPI(irq); + } else if ( unlikely(irq < 16) ) { do_sgi(regs, irq); } diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h index 8f7a167..ee47de8 100644 --- a/xen/include/asm-arm/irq.h +++ b/xen/include/asm-arm/irq.h @@ -34,6 +34,14 @@ struct irq_desc *__irq_to_desc(int irq); void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq); +#ifdef CONFIG_HAS_ITS +void do_LPI(unsigned int irq); +#else +static inline void do_LPI(unsigned int irq) +{ +} +#endif + #define domain_pirq_to_irq(d, pirq) (pirq) bool_t is_assignable_irq(unsigned int irq);
Upon receiving an LPI, we need to find the right VCPU and virtual IRQ number to get this IRQ injected. Iterate our two-level LPI table to find this information quickly when the host takes an LPI. Call the existing injection function to let the GIC emulation deal with this interrupt. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic-v3-lpi.c | 41 +++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/gic.c | 6 ++++-- xen/include/asm-arm/irq.h | 8 ++++++++ 3 files changed, 53 insertions(+), 2 deletions(-)