| Message ID | 1491957874-31600-6-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 12/04/17 01:44, Andre Przywara wrote: > Upon receiving an LPI on the host, 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. > Also we enhance struct pending_irq to cache the pending bit and the > priority information for LPIs. Reading the information from there is > faster than accessing the property table from guest memory. Also it > use some padding area, so does not require more memory. > This introduces a do_LPI() as a hardware gic_ops and a function to > retrieve the (cached) priority value of an LPI and a vgic_ops. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-v2.c | 7 ++++ > xen/arch/arm/gic-v3-lpi.c | 71 ++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/gic-v3.c | 1 + > xen/arch/arm/gic.c | 8 ++++- > xen/arch/arm/vgic-v2.c | 7 ++++ > xen/arch/arm/vgic-v3.c | 12 +++++++ > xen/arch/arm/vgic.c | 7 +++- > xen/include/asm-arm/domain.h | 3 +- > xen/include/asm-arm/gic.h | 2 ++ > xen/include/asm-arm/gic_v3_its.h | 8 +++++ > xen/include/asm-arm/vgic.h | 2 ++ > 11 files changed, 125 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 270a136..ffbe47c 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -1217,6 +1217,12 @@ static int __init gicv2_init(void) > return 0; > } > > +static void gicv2_do_LPI(unsigned int lpi) > +{ > + /* No LPIs in a GICv2 */ > + BUG(); > +} > + > const static struct gic_hw_operations gicv2_ops = { > .info = &gicv2_info, > .init = gicv2_init, > @@ -1244,6 +1250,7 @@ const static struct gic_hw_operations gicv2_ops = { > .make_hwdom_madt = gicv2_make_hwdom_madt, > .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings, > .iomem_deny_access = gicv2_iomem_deny_access, > + .do_LPI = gicv2_do_LPI, > }; > > /* Set up the GIC */ > diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c > index 292f2d0..44f6315 100644 > --- a/xen/arch/arm/gic-v3-lpi.c > +++ b/xen/arch/arm/gic-v3-lpi.c > @@ -136,6 +136,77 @@ 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. > + * Please note that LPIs are edge-triggered only, also have no active state, > + * so spurious interrupts on the host side are no issue (we can just ignore > + * them). > + * Also a guest cannot expect that firing interrupts that haven't been > + * fully configured yet will reach the CPU, so we don't need to care about > + * this special case. > + */ > +void gicv3_do_LPI(unsigned int lpi) > +{ > + struct domain *d; > + union host_lpi *hlpip, hlpi; > + struct vcpu *vcpu; > + > + irq_enter(); > + > + /* EOI the LPI already. */ > + WRITE_SYSREG32(lpi, ICC_EOIR1_EL1); > + > + /* Find out if a guest mapped something to this physical LPI. */ > + hlpip = gic_get_host_lpi(lpi); > + if ( !hlpip ) > + goto out; > + > + hlpi.data = read_u64_atomic(&hlpip->data); > + > + /* > + * Unmapped events are marked with an invalid LPI ID. We can safely > + * ignore them, as they have no further state and no-one can expect > + * to see them if they have not been mapped. > + */ > + if ( hlpi.virt_lpi == INVALID_LPI ) > + goto out; > + > + d = rcu_lock_domain_by_id(hlpi.dom_id); > + if ( !d ) > + goto out; > + > + /* Make sure we don't step beyond the vcpu array. */ > + if ( hlpi.vcpu_id >= d->max_vcpus ) > + { > + rcu_unlock_domain(d); > + goto out; > + } > + > + vcpu = d->vcpu[hlpi.vcpu_id]; > + > + /* Check if the VCPU is ready to receive LPIs. */ > + if ( vcpu->arch.vgic.flags & VGIC_V3_LPIS_ENABLED ) > + /* > + * TODO: Investigate what to do here for potential interrupt storms. > + * As we keep all host LPIs enabled, for disabling LPIs we would need > + * to queue a ITS host command, which we avoid so far during a guest's > + * runtime. Also re-enabling would trigger a host command upon the > + * guest sending a command, which could be an attack vector for > + * hogging the host command queue. > + * See the thread around here for some background: > + * https://lists.xen.org/archives/html/xen-devel/2016-12/msg00003.html > + */ This TODO should have been listed in the cover letter. > + vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi); > + > + rcu_unlock_domain(d); > + > +out: > + irq_exit(); > +} > + > static int gicv3_lpi_allocate_pendtable(uint64_t *reg) > { > uint64_t val; > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 29c8964..8140c5f 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -1674,6 +1674,7 @@ static const struct gic_hw_operations gicv3_ops = { > .make_hwdom_dt_node = gicv3_make_hwdom_dt_node, > .make_hwdom_madt = gicv3_make_hwdom_madt, > .iomem_deny_access = gicv3_iomem_deny_access, > + .do_LPI = gicv3_do_LPI, > }; > > static int __init gicv3_dt_preinit(struct dt_device_node *node, const void *data) > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 62ae3b8..d752352 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -735,7 +735,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) > do_IRQ(regs, irq, is_fiq); > local_irq_disable(); > } > - else if (unlikely(irq < 16)) > + else if ( is_lpi(irq) ) > + { > + local_irq_enable(); > + gic_hw_ops->do_LPI(irq); > + local_irq_disable(); > + } > + else if ( unlikely(irq < 16) ) > { > do_sgi(regs, irq); > } > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > index 0587569..df91940 100644 > --- a/xen/arch/arm/vgic-v2.c > +++ b/xen/arch/arm/vgic-v2.c > @@ -709,11 +709,18 @@ static struct pending_irq *vgic_v2_lpi_to_pending(struct domain *d, > BUG(); > } > > +static int vgic_v2_lpi_get_priority(struct domain *d, unsigned int vlpi) > +{ > + /* Dummy function, no LPIs on a VGICv2. */ > + BUG(); > +} > + > static const struct vgic_ops vgic_v2_ops = { > .vcpu_init = vgic_v2_vcpu_init, > .domain_init = vgic_v2_domain_init, > .domain_free = vgic_v2_domain_free, > .lpi_to_pending = vgic_v2_lpi_to_pending, > + .lpi_get_priority = vgic_v2_lpi_get_priority, > .max_vcpus = 8, > }; > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index f462610..c059dbd 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -1558,12 +1558,24 @@ static struct pending_irq *vgic_v3_lpi_to_pending(struct domain *d, > return pirq; > } > > +/* Retrieve the priority of an LPI from its struct pending_irq. */ > +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi) > +{ > + struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi); > + > + if ( !p ) > + return GIC_PRI_IRQ; Why the check here? And why returning GIC_PRI_IRQ? AFAICT, vgic_v3_lpi_to_pending should never return NULL when reading the priority. Or else, you are in big trouble. > + > + return p->lpi_priority; > +} > + > static const struct vgic_ops v3_ops = { > .vcpu_init = vgic_v3_vcpu_init, > .domain_init = vgic_v3_domain_init, > .domain_free = vgic_v3_domain_free, > .emulate_reg = vgic_v3_emulate_reg, > .lpi_to_pending = vgic_v3_lpi_to_pending, > + .lpi_get_priority = vgic_v3_lpi_get_priority, > /* > * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU > * that can be supported is up to 4096(==256*16) in theory. > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index c2bfdb1..b6fe34f 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -226,10 +226,15 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq) > > static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) > { > - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq); > + struct vgic_irq_rank *rank; > unsigned long flags; > int priority; > > + /* LPIs don't have a rank, also store their priority separately. */ > + if ( is_lpi(virq) ) > + return v->domain->arch.vgic.handler->lpi_get_priority(v->domain, virq); > + > + rank = vgic_rank_irq(v, virq); > vgic_lock_rank(v, rank, flags); > priority = rank->priority[virq & INTERRUPT_RANK_MASK]; > vgic_unlock_rank(v, rank, flags); > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 3d8e84c..ebaea35 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -260,7 +260,8 @@ struct arch_vcpu > > /* GICv3: redistributor base and flags for this vCPU */ > paddr_t rdist_base; > -#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */ > +#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */ > +#define VGIC_V3_LPIS_ENABLED (1 << 1) > uint8_t flags; > } vgic; > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 836a103..42963c0 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -366,6 +366,8 @@ struct gic_hw_operations { > int (*map_hwdom_extra_mappings)(struct domain *d); > /* Deny access to GIC regions */ > int (*iomem_deny_access)(const struct domain *d); > + /* Handle LPIs, which require special handling */ > + void (*do_LPI)(unsigned int lpi); > }; > > void register_gic_ops(const struct gic_hw_operations *ops); > diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h > index 29559a3..7470779 100644 > --- a/xen/include/asm-arm/gic_v3_its.h > +++ b/xen/include/asm-arm/gic_v3_its.h > @@ -134,6 +134,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node); > > bool gicv3_its_host_has_its(void); > > +void gicv3_do_LPI(unsigned int lpi); > + > int gicv3_lpi_init_rdist(void __iomem * rdist_base); > > /* Initialize the host structures for LPIs and the host ITSes. */ > @@ -175,6 +177,12 @@ static inline bool gicv3_its_host_has_its(void) > return false; > } > > +static inline void gicv3_do_LPI(unsigned int lpi) > +{ > + /* We don't enable LPIs without an ITS. */ > + BUG(); > +} > + > static inline int gicv3_lpi_init_rdist(void __iomem * rdist_base) > { > return -ENODEV; > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index c9075a9..7efa164 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -72,6 +72,7 @@ struct pending_irq > #define GIC_INVALID_LR (uint8_t)~0 > uint8_t lr; > uint8_t priority; > + uint8_t lpi_priority; /* Caches the priority if this is an LPI. */ > /* inflight is used to append instances of pending_irq to > * vgic.inflight_irqs */ > struct list_head inflight; > @@ -136,6 +137,7 @@ struct vgic_ops { > bool (*emulate_reg)(struct cpu_user_regs *regs, union hsr hsr); > /* lookup the struct pending_irq for a given LPI interrupt */ > struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned int vlpi); > + int (*lpi_get_priority)(struct domain *d, uint32_t vlpi); > /* Maximum number of vCPU supported */ > const unsigned int max_vcpus; > }; >
Hi, On 12/04/17 11:44, Julien Grall wrote: > Hi Andre, > > On 12/04/17 01:44, Andre Przywara wrote: >> Upon receiving an LPI on the host, 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. >> Also we enhance struct pending_irq to cache the pending bit and the >> priority information for LPIs. Reading the information from there is >> faster than accessing the property table from guest memory. Also it >> use some padding area, so does not require more memory. >> This introduces a do_LPI() as a hardware gic_ops and a function to >> retrieve the (cached) priority value of an LPI and a vgic_ops. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic-v2.c | 7 ++++ >> xen/arch/arm/gic-v3-lpi.c | 71 >> ++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/gic-v3.c | 1 + >> xen/arch/arm/gic.c | 8 ++++- >> xen/arch/arm/vgic-v2.c | 7 ++++ >> xen/arch/arm/vgic-v3.c | 12 +++++++ >> xen/arch/arm/vgic.c | 7 +++- >> xen/include/asm-arm/domain.h | 3 +- >> xen/include/asm-arm/gic.h | 2 ++ >> xen/include/asm-arm/gic_v3_its.h | 8 +++++ >> xen/include/asm-arm/vgic.h | 2 ++ >> 11 files changed, 125 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >> index 270a136..ffbe47c 100644 >> --- a/xen/arch/arm/gic-v2.c >> +++ b/xen/arch/arm/gic-v2.c >> @@ -1217,6 +1217,12 @@ static int __init gicv2_init(void) >> return 0; >> } >> >> +static void gicv2_do_LPI(unsigned int lpi) >> +{ >> + /* No LPIs in a GICv2 */ >> + BUG(); >> +} >> + >> const static struct gic_hw_operations gicv2_ops = { >> .info = &gicv2_info, >> .init = gicv2_init, >> @@ -1244,6 +1250,7 @@ const static struct gic_hw_operations gicv2_ops = { >> .make_hwdom_madt = gicv2_make_hwdom_madt, >> .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings, >> .iomem_deny_access = gicv2_iomem_deny_access, >> + .do_LPI = gicv2_do_LPI, >> }; >> >> /* Set up the GIC */ >> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c >> index 292f2d0..44f6315 100644 >> --- a/xen/arch/arm/gic-v3-lpi.c >> +++ b/xen/arch/arm/gic-v3-lpi.c >> @@ -136,6 +136,77 @@ 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. >> + * Please note that LPIs are edge-triggered only, also have no active >> state, >> + * so spurious interrupts on the host side are no issue (we can just >> ignore >> + * them). >> + * Also a guest cannot expect that firing interrupts that haven't been >> + * fully configured yet will reach the CPU, so we don't need to care >> about >> + * this special case. >> + */ >> +void gicv3_do_LPI(unsigned int lpi) >> +{ >> + struct domain *d; >> + union host_lpi *hlpip, hlpi; >> + struct vcpu *vcpu; >> + >> + irq_enter(); >> + >> + /* EOI the LPI already. */ >> + WRITE_SYSREG32(lpi, ICC_EOIR1_EL1); >> + >> + /* Find out if a guest mapped something to this physical LPI. */ >> + hlpip = gic_get_host_lpi(lpi); >> + if ( !hlpip ) >> + goto out; >> + >> + hlpi.data = read_u64_atomic(&hlpip->data); >> + >> + /* >> + * Unmapped events are marked with an invalid LPI ID. We can safely >> + * ignore them, as they have no further state and no-one can expect >> + * to see them if they have not been mapped. >> + */ >> + if ( hlpi.virt_lpi == INVALID_LPI ) >> + goto out; >> + >> + d = rcu_lock_domain_by_id(hlpi.dom_id); >> + if ( !d ) >> + goto out; >> + >> + /* Make sure we don't step beyond the vcpu array. */ >> + if ( hlpi.vcpu_id >= d->max_vcpus ) >> + { >> + rcu_unlock_domain(d); >> + goto out; >> + } >> + >> + vcpu = d->vcpu[hlpi.vcpu_id]; >> + >> + /* Check if the VCPU is ready to receive LPIs. */ >> + if ( vcpu->arch.vgic.flags & VGIC_V3_LPIS_ENABLED ) >> + /* >> + * TODO: Investigate what to do here for potential interrupt >> storms. >> + * As we keep all host LPIs enabled, for disabling LPIs we >> would need >> + * to queue a ITS host command, which we avoid so far during >> a guest's >> + * runtime. Also re-enabling would trigger a host command >> upon the >> + * guest sending a command, which could be an attack vector for >> + * hogging the host command queue. >> + * See the thread around here for some background: >> + * >> https://lists.xen.org/archives/html/xen-devel/2016-12/msg00003.html >> + */ > > This TODO should have been listed in the cover letter. Done. >> + vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi); >> + >> + rcu_unlock_domain(d); >> + >> +out: >> + irq_exit(); >> +} >> + >> static int gicv3_lpi_allocate_pendtable(uint64_t *reg) >> { >> uint64_t val; >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c >> index 29c8964..8140c5f 100644 >> --- a/xen/arch/arm/gic-v3.c >> +++ b/xen/arch/arm/gic-v3.c >> @@ -1674,6 +1674,7 @@ static const struct gic_hw_operations gicv3_ops = { >> .make_hwdom_dt_node = gicv3_make_hwdom_dt_node, >> .make_hwdom_madt = gicv3_make_hwdom_madt, >> .iomem_deny_access = gicv3_iomem_deny_access, >> + .do_LPI = gicv3_do_LPI, >> }; >> >> static int __init gicv3_dt_preinit(struct dt_device_node *node, const >> void *data) >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 62ae3b8..d752352 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -735,7 +735,13 @@ void gic_interrupt(struct cpu_user_regs *regs, >> int is_fiq) >> do_IRQ(regs, irq, is_fiq); >> local_irq_disable(); >> } >> - else if (unlikely(irq < 16)) >> + else if ( is_lpi(irq) ) >> + { >> + local_irq_enable(); >> + gic_hw_ops->do_LPI(irq); >> + local_irq_disable(); >> + } >> + else if ( unlikely(irq < 16) ) >> { >> do_sgi(regs, irq); >> } >> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c >> index 0587569..df91940 100644 >> --- a/xen/arch/arm/vgic-v2.c >> +++ b/xen/arch/arm/vgic-v2.c >> @@ -709,11 +709,18 @@ static struct pending_irq >> *vgic_v2_lpi_to_pending(struct domain *d, >> BUG(); >> } >> >> +static int vgic_v2_lpi_get_priority(struct domain *d, unsigned int vlpi) >> +{ >> + /* Dummy function, no LPIs on a VGICv2. */ >> + BUG(); >> +} >> + >> static const struct vgic_ops vgic_v2_ops = { >> .vcpu_init = vgic_v2_vcpu_init, >> .domain_init = vgic_v2_domain_init, >> .domain_free = vgic_v2_domain_free, >> .lpi_to_pending = vgic_v2_lpi_to_pending, >> + .lpi_get_priority = vgic_v2_lpi_get_priority, >> .max_vcpus = 8, >> }; >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index f462610..c059dbd 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -1558,12 +1558,24 @@ static struct pending_irq >> *vgic_v3_lpi_to_pending(struct domain *d, >> return pirq; >> } >> >> +/* Retrieve the priority of an LPI from its struct pending_irq. */ >> +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi) >> +{ >> + struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi); >> + >> + if ( !p ) >> + return GIC_PRI_IRQ; > > Why the check here? And why returning GIC_PRI_IRQ? > > AFAICT, vgic_v3_lpi_to_pending should never return NULL when reading the > priority. Or else, you are in big trouble. Now (with one change I made after your comment on 02/27) we call vgic_get_virq_priority() as the very first thing in vgic_vcpu_inject_irq(). So lpi_to_pending() could very well returning NULL, which we would handle properly later. As we can't move the vgic_get_virq_priority() call due to the locking order rules, we just deal with this, returning *some* value here. Shall I make the return value zero or 0xff in this case? Cheers, Andre. >> + >> + return p->lpi_priority; >> +} >> + >> static const struct vgic_ops v3_ops = { >> .vcpu_init = vgic_v3_vcpu_init, >> .domain_init = vgic_v3_domain_init, >> .domain_free = vgic_v3_domain_free, >> .emulate_reg = vgic_v3_emulate_reg, >> .lpi_to_pending = vgic_v3_lpi_to_pending, >> + .lpi_get_priority = vgic_v3_lpi_get_priority, >> /* >> * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of >> CPU >> * that can be supported is up to 4096(==256*16) in theory. >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index c2bfdb1..b6fe34f 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -226,10 +226,15 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu >> *v, unsigned int virq) >> >> static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) >> { >> - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq); >> + struct vgic_irq_rank *rank; >> unsigned long flags; >> int priority; >> >> + /* LPIs don't have a rank, also store their priority separately. */ >> + if ( is_lpi(virq) ) >> + return >> v->domain->arch.vgic.handler->lpi_get_priority(v->domain, virq); >> + >> + rank = vgic_rank_irq(v, virq); >> vgic_lock_rank(v, rank, flags); >> priority = rank->priority[virq & INTERRUPT_RANK_MASK]; >> vgic_unlock_rank(v, rank, flags); >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 3d8e84c..ebaea35 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -260,7 +260,8 @@ struct arch_vcpu >> >> /* GICv3: redistributor base and flags for this vCPU */ >> paddr_t rdist_base; >> -#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */ >> +#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the >> rdist */ >> +#define VGIC_V3_LPIS_ENABLED (1 << 1) >> uint8_t flags; >> } vgic; >> >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >> index 836a103..42963c0 100644 >> --- a/xen/include/asm-arm/gic.h >> +++ b/xen/include/asm-arm/gic.h >> @@ -366,6 +366,8 @@ struct gic_hw_operations { >> int (*map_hwdom_extra_mappings)(struct domain *d); >> /* Deny access to GIC regions */ >> int (*iomem_deny_access)(const struct domain *d); >> + /* Handle LPIs, which require special handling */ >> + void (*do_LPI)(unsigned int lpi); >> }; >> >> void register_gic_ops(const struct gic_hw_operations *ops); >> diff --git a/xen/include/asm-arm/gic_v3_its.h >> b/xen/include/asm-arm/gic_v3_its.h >> index 29559a3..7470779 100644 >> --- a/xen/include/asm-arm/gic_v3_its.h >> +++ b/xen/include/asm-arm/gic_v3_its.h >> @@ -134,6 +134,8 @@ void gicv3_its_dt_init(const struct dt_device_node >> *node); >> >> bool gicv3_its_host_has_its(void); >> >> +void gicv3_do_LPI(unsigned int lpi); >> + >> int gicv3_lpi_init_rdist(void __iomem * rdist_base); >> >> /* Initialize the host structures for LPIs and the host ITSes. */ >> @@ -175,6 +177,12 @@ static inline bool gicv3_its_host_has_its(void) >> return false; >> } >> >> +static inline void gicv3_do_LPI(unsigned int lpi) >> +{ >> + /* We don't enable LPIs without an ITS. */ >> + BUG(); >> +} >> + >> static inline int gicv3_lpi_init_rdist(void __iomem * rdist_base) >> { >> return -ENODEV; >> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h >> index c9075a9..7efa164 100644 >> --- a/xen/include/asm-arm/vgic.h >> +++ b/xen/include/asm-arm/vgic.h >> @@ -72,6 +72,7 @@ struct pending_irq >> #define GIC_INVALID_LR (uint8_t)~0 >> uint8_t lr; >> uint8_t priority; >> + uint8_t lpi_priority; /* Caches the priority if this is an >> LPI. */ >> /* inflight is used to append instances of pending_irq to >> * vgic.inflight_irqs */ >> struct list_head inflight; >> @@ -136,6 +137,7 @@ struct vgic_ops { >> bool (*emulate_reg)(struct cpu_user_regs *regs, union hsr hsr); >> /* lookup the struct pending_irq for a given LPI interrupt */ >> struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned >> int vlpi); >> + int (*lpi_get_priority)(struct domain *d, uint32_t vlpi); >> /* Maximum number of vCPU supported */ >> const unsigned int max_vcpus; >> }; >> >
Hi, On 12/04/17 11:44, Julien Grall wrote: > Hi Andre, > > On 12/04/17 01:44, Andre Przywara wrote: >> Upon receiving an LPI on the host, 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. >> Also we enhance struct pending_irq to cache the pending bit and the >> priority information for LPIs. Reading the information from there is >> faster than accessing the property table from guest memory. Also it >> use some padding area, so does not require more memory. >> This introduces a do_LPI() as a hardware gic_ops and a function to >> retrieve the (cached) priority value of an LPI and a vgic_ops. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic-v2.c | 7 ++++ >> xen/arch/arm/gic-v3-lpi.c | 71 >> ++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/gic-v3.c | 1 + >> xen/arch/arm/gic.c | 8 ++++- >> xen/arch/arm/vgic-v2.c | 7 ++++ >> xen/arch/arm/vgic-v3.c | 12 +++++++ >> xen/arch/arm/vgic.c | 7 +++- >> xen/include/asm-arm/domain.h | 3 +- >> xen/include/asm-arm/gic.h | 2 ++ >> xen/include/asm-arm/gic_v3_its.h | 8 +++++ >> xen/include/asm-arm/vgic.h | 2 ++ >> 11 files changed, 125 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >> index 270a136..ffbe47c 100644 >> --- a/xen/arch/arm/gic-v2.c >> +++ b/xen/arch/arm/gic-v2.c >> @@ -1217,6 +1217,12 @@ static int __init gicv2_init(void) >> return 0; >> } >> >> +static void gicv2_do_LPI(unsigned int lpi) >> +{ >> + /* No LPIs in a GICv2 */ >> + BUG(); >> +} >> + >> const static struct gic_hw_operations gicv2_ops = { >> .info = &gicv2_info, >> .init = gicv2_init, >> @@ -1244,6 +1250,7 @@ const static struct gic_hw_operations gicv2_ops = { >> .make_hwdom_madt = gicv2_make_hwdom_madt, >> .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings, >> .iomem_deny_access = gicv2_iomem_deny_access, >> + .do_LPI = gicv2_do_LPI, >> }; >> >> /* Set up the GIC */ >> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c >> index 292f2d0..44f6315 100644 >> --- a/xen/arch/arm/gic-v3-lpi.c >> +++ b/xen/arch/arm/gic-v3-lpi.c >> @@ -136,6 +136,77 @@ 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. >> + * Please note that LPIs are edge-triggered only, also have no active >> state, >> + * so spurious interrupts on the host side are no issue (we can just >> ignore >> + * them). >> + * Also a guest cannot expect that firing interrupts that haven't been >> + * fully configured yet will reach the CPU, so we don't need to care >> about >> + * this special case. >> + */ >> +void gicv3_do_LPI(unsigned int lpi) >> +{ >> + struct domain *d; >> + union host_lpi *hlpip, hlpi; >> + struct vcpu *vcpu; >> + >> + irq_enter(); >> + >> + /* EOI the LPI already. */ >> + WRITE_SYSREG32(lpi, ICC_EOIR1_EL1); >> + >> + /* Find out if a guest mapped something to this physical LPI. */ >> + hlpip = gic_get_host_lpi(lpi); >> + if ( !hlpip ) >> + goto out; >> + >> + hlpi.data = read_u64_atomic(&hlpip->data); >> + >> + /* >> + * Unmapped events are marked with an invalid LPI ID. We can safely >> + * ignore them, as they have no further state and no-one can expect >> + * to see them if they have not been mapped. >> + */ >> + if ( hlpi.virt_lpi == INVALID_LPI ) >> + goto out; >> + >> + d = rcu_lock_domain_by_id(hlpi.dom_id); >> + if ( !d ) >> + goto out; >> + >> + /* Make sure we don't step beyond the vcpu array. */ >> + if ( hlpi.vcpu_id >= d->max_vcpus ) >> + { >> + rcu_unlock_domain(d); >> + goto out; >> + } >> + >> + vcpu = d->vcpu[hlpi.vcpu_id]; >> + >> + /* Check if the VCPU is ready to receive LPIs. */ >> + if ( vcpu->arch.vgic.flags & VGIC_V3_LPIS_ENABLED ) >> + /* >> + * TODO: Investigate what to do here for potential interrupt >> storms. >> + * As we keep all host LPIs enabled, for disabling LPIs we >> would need >> + * to queue a ITS host command, which we avoid so far during >> a guest's >> + * runtime. Also re-enabling would trigger a host command >> upon the >> + * guest sending a command, which could be an attack vector for >> + * hogging the host command queue. >> + * See the thread around here for some background: >> + * >> https://lists.xen.org/archives/html/xen-devel/2016-12/msg00003.html >> + */ > > This TODO should have been listed in the cover letter. OK. I extended the TODO/limitations section accordingly. >> + vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi); >> + >> + rcu_unlock_domain(d); >> + >> +out: >> + irq_exit(); >> +} >> + >> static int gicv3_lpi_allocate_pendtable(uint64_t *reg) >> { >> uint64_t val; >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c >> index 29c8964..8140c5f 100644 >> --- a/xen/arch/arm/gic-v3.c >> +++ b/xen/arch/arm/gic-v3.c >> @@ -1674,6 +1674,7 @@ static const struct gic_hw_operations gicv3_ops = { >> .make_hwdom_dt_node = gicv3_make_hwdom_dt_node, >> .make_hwdom_madt = gicv3_make_hwdom_madt, >> .iomem_deny_access = gicv3_iomem_deny_access, >> + .do_LPI = gicv3_do_LPI, >> }; >> >> static int __init gicv3_dt_preinit(struct dt_device_node *node, const >> void *data) >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 62ae3b8..d752352 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -735,7 +735,13 @@ void gic_interrupt(struct cpu_user_regs *regs, >> int is_fiq) >> do_IRQ(regs, irq, is_fiq); >> local_irq_disable(); >> } >> - else if (unlikely(irq < 16)) >> + else if ( is_lpi(irq) ) >> + { >> + local_irq_enable(); >> + gic_hw_ops->do_LPI(irq); >> + local_irq_disable(); >> + } >> + else if ( unlikely(irq < 16) ) >> { >> do_sgi(regs, irq); >> } >> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c >> index 0587569..df91940 100644 >> --- a/xen/arch/arm/vgic-v2.c >> +++ b/xen/arch/arm/vgic-v2.c >> @@ -709,11 +709,18 @@ static struct pending_irq >> *vgic_v2_lpi_to_pending(struct domain *d, >> BUG(); >> } >> >> +static int vgic_v2_lpi_get_priority(struct domain *d, unsigned int vlpi) >> +{ >> + /* Dummy function, no LPIs on a VGICv2. */ >> + BUG(); >> +} >> + >> static const struct vgic_ops vgic_v2_ops = { >> .vcpu_init = vgic_v2_vcpu_init, >> .domain_init = vgic_v2_domain_init, >> .domain_free = vgic_v2_domain_free, >> .lpi_to_pending = vgic_v2_lpi_to_pending, >> + .lpi_get_priority = vgic_v2_lpi_get_priority, >> .max_vcpus = 8, >> }; >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index f462610..c059dbd 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -1558,12 +1558,24 @@ static struct pending_irq >> *vgic_v3_lpi_to_pending(struct domain *d, >> return pirq; >> } >> >> +/* Retrieve the priority of an LPI from its struct pending_irq. */ >> +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi) >> +{ >> + struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi); >> + >> + if ( !p ) >> + return GIC_PRI_IRQ; > > Why the check here? And why returning GIC_PRI_IRQ? Because you surely want to avoid dereferencing NULL? I changed the return value to 0xff, which is the lowest priority. Frankly I think we could just return anything, as we will stop handling this LPI anyway a bit later in the code if p is NULL here. > AFAICT, vgic_v3_lpi_to_pending should never return NULL when reading the > priority. Or else, you are in big trouble. That depends on where and when you call it, which I don't want to make any assumptions about. In my latest version the vgic_get_virq_priority() call now stays at the very beginning of vgic_vcpu_inject_irq(), so at this point the LPI could have been unmapped meanwhile. Surely you will bail out handling this LPI later in the code if it returns NULL here, but for the sake of this function I think we need the check. To me it looks a bit like having this abstraction here is a bit pointless and complicates things, but well .... Cheers, Andre. > >> + >> + return p->lpi_priority; >> +} >> + >> static const struct vgic_ops v3_ops = { >> .vcpu_init = vgic_v3_vcpu_init, >> .domain_init = vgic_v3_domain_init, >> .domain_free = vgic_v3_domain_free, >> .emulate_reg = vgic_v3_emulate_reg, >> .lpi_to_pending = vgic_v3_lpi_to_pending, >> + .lpi_get_priority = vgic_v3_lpi_get_priority, >> /* >> * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of >> CPU >> * that can be supported is up to 4096(==256*16) in theory. >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index c2bfdb1..b6fe34f 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -226,10 +226,15 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu >> *v, unsigned int virq) >> >> static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) >> { >> - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq); >> + struct vgic_irq_rank *rank; >> unsigned long flags; >> int priority; >> >> + /* LPIs don't have a rank, also store their priority separately. */ >> + if ( is_lpi(virq) ) >> + return >> v->domain->arch.vgic.handler->lpi_get_priority(v->domain, virq); >> + >> + rank = vgic_rank_irq(v, virq); >> vgic_lock_rank(v, rank, flags); >> priority = rank->priority[virq & INTERRUPT_RANK_MASK]; >> vgic_unlock_rank(v, rank, flags); >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 3d8e84c..ebaea35 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -260,7 +260,8 @@ struct arch_vcpu >> >> /* GICv3: redistributor base and flags for this vCPU */ >> paddr_t rdist_base; >> -#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */ >> +#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the >> rdist */ >> +#define VGIC_V3_LPIS_ENABLED (1 << 1) >> uint8_t flags; >> } vgic; >> >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >> index 836a103..42963c0 100644 >> --- a/xen/include/asm-arm/gic.h >> +++ b/xen/include/asm-arm/gic.h >> @@ -366,6 +366,8 @@ struct gic_hw_operations { >> int (*map_hwdom_extra_mappings)(struct domain *d); >> /* Deny access to GIC regions */ >> int (*iomem_deny_access)(const struct domain *d); >> + /* Handle LPIs, which require special handling */ >> + void (*do_LPI)(unsigned int lpi); >> }; >> >> void register_gic_ops(const struct gic_hw_operations *ops); >> diff --git a/xen/include/asm-arm/gic_v3_its.h >> b/xen/include/asm-arm/gic_v3_its.h >> index 29559a3..7470779 100644 >> --- a/xen/include/asm-arm/gic_v3_its.h >> +++ b/xen/include/asm-arm/gic_v3_its.h >> @@ -134,6 +134,8 @@ void gicv3_its_dt_init(const struct dt_device_node >> *node); >> >> bool gicv3_its_host_has_its(void); >> >> +void gicv3_do_LPI(unsigned int lpi); >> + >> int gicv3_lpi_init_rdist(void __iomem * rdist_base); >> >> /* Initialize the host structures for LPIs and the host ITSes. */ >> @@ -175,6 +177,12 @@ static inline bool gicv3_its_host_has_its(void) >> return false; >> } >> >> +static inline void gicv3_do_LPI(unsigned int lpi) >> +{ >> + /* We don't enable LPIs without an ITS. */ >> + BUG(); >> +} >> + >> static inline int gicv3_lpi_init_rdist(void __iomem * rdist_base) >> { >> return -ENODEV; >> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h >> index c9075a9..7efa164 100644 >> --- a/xen/include/asm-arm/vgic.h >> +++ b/xen/include/asm-arm/vgic.h >> @@ -72,6 +72,7 @@ struct pending_irq >> #define GIC_INVALID_LR (uint8_t)~0 >> uint8_t lr; >> uint8_t priority; >> + uint8_t lpi_priority; /* Caches the priority if this is an >> LPI. */ >> /* inflight is used to append instances of pending_irq to >> * vgic.inflight_irqs */ >> struct list_head inflight; >> @@ -136,6 +137,7 @@ struct vgic_ops { >> bool (*emulate_reg)(struct cpu_user_regs *regs, union hsr hsr); >> /* lookup the struct pending_irq for a given LPI interrupt */ >> struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned >> int vlpi); >> + int (*lpi_get_priority)(struct domain *d, uint32_t vlpi); >> /* Maximum number of vCPU supported */ >> const unsigned int max_vcpus; >> }; >> >
On 05/10/2017 11:47 AM, Andre Przywara wrote: > Hi, Hi Andre, > On 12/04/17 11:44, Julien Grall wrote: >> On 12/04/17 01:44, Andre Przywara wrote: >>> +/* Retrieve the priority of an LPI from its struct pending_irq. */ >>> +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi) >>> +{ >>> + struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi); >>> + >>> + if ( !p ) >>> + return GIC_PRI_IRQ; >> >> Why the check here? And why returning GIC_PRI_IRQ? > > Because you surely want to avoid dereferencing NULL? > I changed the return value to 0xff, which is the lowest priority. > Frankly I think we could just return anything, as we will stop handling > this LPI anyway a bit later in the code if p is NULL here. I agree that you want to prevent NULL. But we also want to avoid return fake value because there was a caller that didn't bother to check whether the interrupt is valid at first hand. If you ever have NULL here then there is a latent BUG in your code somewhere else. Ignoring the NULL and return a fake value is likely not the right solution for development. I can see two solutions for this: - ASSERT(p) - if ( !p ) { ASSERT_UNREACHABLE(); return 0xff; } The later would still return a dumb value but at least we would catch programming mistake during development. > >> AFAICT, vgic_v3_lpi_to_pending should never return NULL when reading the >> priority. Or else, you are in big trouble. > > That depends on where and when you call it, which I don't want to make > any assumptions about. > In my latest version the vgic_get_virq_priority() call now stays at the > very beginning of vgic_vcpu_inject_irq(), so at this point the LPI could > have been unmapped meanwhile. > Surely you will bail out handling this LPI later in the code if it > returns NULL here, but for the sake of this function I think we need the > check. > To me it looks a bit like having this abstraction here is a bit > pointless and complicates things, but well .... Well, I am not against very defensive programming. I am more against returning a fake value that may impact the rest of the vGIC (not even mentioning the lack of comment explain why). After all, the priority is controlled by the guest and not the hypervisor. I suggested few way above to catch those errors. Cheers,
Hi, On 10/05/17 12:07, Julien Grall wrote: > > > On 05/10/2017 11:47 AM, Andre Przywara wrote: >> Hi, > > Hi Andre, > >> On 12/04/17 11:44, Julien Grall wrote: >>> On 12/04/17 01:44, Andre Przywara wrote: >>>> +/* Retrieve the priority of an LPI from its struct pending_irq. */ >>>> +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi) >>>> +{ >>>> + struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi); >>>> + >>>> + if ( !p ) >>>> + return GIC_PRI_IRQ; >>> >>> Why the check here? And why returning GIC_PRI_IRQ? >> >> Because you surely want to avoid dereferencing NULL? >> I changed the return value to 0xff, which is the lowest priority. >> Frankly I think we could just return anything, as we will stop handling >> this LPI anyway a bit later in the code if p is NULL here. > > I agree that you want to prevent NULL. But we also want to avoid return > fake value because there was a caller that didn't bother to check > whether the interrupt is valid at first hand. Well, I changed the sequence in vgic_vcpu_inject_irq() back to be: priority = vgic_get_virq_priority(v, virq); spin_lock_irqsave(&v->arch.vgic.lock, flags); n = irq_to_pending(v, virq); mostly to prevent the locking order (rank vs. VCPU lock) issue you mentioned. We read the latest priority value upfront, but only use it later if the pending_irq is valid. I don't see how this should create problems. Eventually this will be solved properly by the pending_irq lock. > If you ever have NULL here then there is a latent BUG in your code > somewhere else. Not in this case. > Ignoring the NULL and return a fake value is likely not > the right solution for development. > > I can see two solutions for this: > - ASSERT(p) > - if ( !p ) > { > ASSERT_UNREACHABLE(); > return 0xff; > } > > The later would still return a dumb value but at least we would catch > programming mistake during development. I think this solution asks for the ASSERT to trigger in corner cases: If the LPI fired on the host, but got unmapped shortly afterwards. In this case vgic_vcpu_inject_irq() can be reached with an invalid LPI number, and we handle this properly when irq_to_pending() returns NULL. But in this case get_priority() will be called with the same invalid LPI, so should be able to cope with that as well. Again this will eventually be solved properly with the per-IRQ lock. Cheers, Andre. >> >>> AFAICT, vgic_v3_lpi_to_pending should never return NULL when reading the >>> priority. Or else, you are in big trouble. >> >> That depends on where and when you call it, which I don't want to make >> any assumptions about. >> In my latest version the vgic_get_virq_priority() call now stays at the >> very beginning of vgic_vcpu_inject_irq(), so at this point the LPI could >> have been unmapped meanwhile. >> Surely you will bail out handling this LPI later in the code if it >> returns NULL here, but for the sake of this function I think we need the >> check. >> To me it looks a bit like having this abstraction here is a bit >> pointless and complicates things, but well .... > > Well, I am not against very defensive programming. I am more against > returning a fake value that may impact the rest of the vGIC (not even > mentioning the lack of comment explain why). After all, the priority is > controlled by the guest and not the hypervisor. > > I suggested few way above to catch those errors. > > Cheers, >
On 05/10/2017 06:14 PM, Andre Przywara wrote: > Hi, > > On 10/05/17 12:07, Julien Grall wrote: >> >> >> On 05/10/2017 11:47 AM, Andre Przywara wrote: >>> Hi, >> >> Hi Andre, >> >>> On 12/04/17 11:44, Julien Grall wrote: >>>> On 12/04/17 01:44, Andre Przywara wrote: >>>>> +/* Retrieve the priority of an LPI from its struct pending_irq. */ >>>>> +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi) >>>>> +{ >>>>> + struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi); >>>>> + >>>>> + if ( !p ) >>>>> + return GIC_PRI_IRQ; >>>> >>>> Why the check here? And why returning GIC_PRI_IRQ? >>> >>> Because you surely want to avoid dereferencing NULL? >>> I changed the return value to 0xff, which is the lowest priority. >>> Frankly I think we could just return anything, as we will stop handling >>> this LPI anyway a bit later in the code if p is NULL here. >> >> I agree that you want to prevent NULL. But we also want to avoid return >> fake value because there was a caller that didn't bother to check >> whether the interrupt is valid at first hand. > > Well, I changed the sequence in vgic_vcpu_inject_irq() back to be: > > priority = vgic_get_virq_priority(v, virq); > > spin_lock_irqsave(&v->arch.vgic.lock, flags); > n = irq_to_pending(v, virq); > > mostly to prevent the locking order (rank vs. VCPU lock) issue you > mentioned. We read the latest priority value upfront, but only use it > later if the pending_irq is valid. I don't see how this should create > problems. Eventually this will be solved properly by the pending_irq lock. > >> If you ever have NULL here then there is a latent BUG in your code >> somewhere else. > > Not in this case. Because of the locking issue? I know there are locking issue, but it does not mean we should introduce bad code just for workaround them for the time being... > >> Ignoring the NULL and return a fake value is likely not >> the right solution for development. >> >> I can see two solutions for this: >> - ASSERT(p) >> - if ( !p ) >> { >> ASSERT_UNREACHABLE(); >> return 0xff; >> } >> >> The later would still return a dumb value but at least we would catch >> programming mistake during development. > > I think this solution asks for the ASSERT to trigger in corner cases: If > the LPI fired on the host, but got unmapped shortly afterwards. In this > case vgic_vcpu_inject_irq() can be reached with an invalid LPI number, > and we handle this properly when irq_to_pending() returns NULL. > But in this case get_priority() will be called with the same invalid > LPI, so should be able to cope with that as well. > Again this will eventually be solved properly with the per-IRQ lock. See above. I still prefer to see the ASSERT firing time to time than bad code going in staging. Cheers,
Hi Julien, On 10/05/17 18:17, Julien Grall wrote: > > > On 05/10/2017 06:14 PM, Andre Przywara wrote: >> Hi, >> >> On 10/05/17 12:07, Julien Grall wrote: >>> >>> >>> On 05/10/2017 11:47 AM, Andre Przywara wrote: >>>> Hi, >>> >>> Hi Andre, >>> >>>> On 12/04/17 11:44, Julien Grall wrote: >>>>> On 12/04/17 01:44, Andre Przywara wrote: >>>>>> +/* Retrieve the priority of an LPI from its struct pending_irq. */ >>>>>> +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi) >>>>>> +{ >>>>>> + struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi); >>>>>> + >>>>>> + if ( !p ) >>>>>> + return GIC_PRI_IRQ; >>>>> >>>>> Why the check here? And why returning GIC_PRI_IRQ? >>>> >>>> Because you surely want to avoid dereferencing NULL? >>>> I changed the return value to 0xff, which is the lowest priority. >>>> Frankly I think we could just return anything, as we will stop handling >>>> this LPI anyway a bit later in the code if p is NULL here. >>> >>> I agree that you want to prevent NULL. But we also want to avoid return >>> fake value because there was a caller that didn't bother to check >>> whether the interrupt is valid at first hand. >> >> Well, I changed the sequence in vgic_vcpu_inject_irq() back to be: >> >> priority = vgic_get_virq_priority(v, virq); >> >> spin_lock_irqsave(&v->arch.vgic.lock, flags); >> n = irq_to_pending(v, virq); >> >> mostly to prevent the locking order (rank vs. VCPU lock) issue you >> mentioned. We read the latest priority value upfront, but only use it >> later if the pending_irq is valid. I don't see how this should create >> problems. Eventually this will be solved properly by the pending_irq >> lock. >> >>> If you ever have NULL here then there is a latent BUG in your code >>> somewhere else. >> >> Not in this case. > > Because of the locking issue? I know there are locking issue, but it > does not mean we should introduce bad code just for workaround them for > the time being... No, because we now (as before) call vgic_get_virq_priority() without any locks, so anything could happen. And in the spirit of checking *every* irq_to_pending() call I'd rather protect this case properly (without trading NULL pointer exceptions for ASSERTs). Please look at the new code and tell me if you still don't like it. Cheers, Andre. >> >>> Ignoring the NULL and return a fake value is likely not >>> the right solution for development. >>> >>> I can see two solutions for this: >>> - ASSERT(p) >>> - if ( !p ) >>> { >>> ASSERT_UNREACHABLE(); >>> return 0xff; >>> } >>> >>> The later would still return a dumb value but at least we would catch >>> programming mistake during development. >> >> I think this solution asks for the ASSERT to trigger in corner cases: If >> the LPI fired on the host, but got unmapped shortly afterwards. In this >> case vgic_vcpu_inject_irq() can be reached with an invalid LPI number, >> and we handle this properly when irq_to_pending() returns NULL. >> But in this case get_priority() will be called with the same invalid >> LPI, so should be able to cope with that as well. >> Again this will eventually be solved properly with the per-IRQ lock. > > See above. I still prefer to see the ASSERT firing time to time than bad > code going in staging. > > Cheers, >
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 270a136..ffbe47c 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -1217,6 +1217,12 @@ static int __init gicv2_init(void) return 0; } +static void gicv2_do_LPI(unsigned int lpi) +{ + /* No LPIs in a GICv2 */ + BUG(); +} + const static struct gic_hw_operations gicv2_ops = { .info = &gicv2_info, .init = gicv2_init, @@ -1244,6 +1250,7 @@ const static struct gic_hw_operations gicv2_ops = { .make_hwdom_madt = gicv2_make_hwdom_madt, .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings, .iomem_deny_access = gicv2_iomem_deny_access, + .do_LPI = gicv2_do_LPI, }; /* Set up the GIC */ diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c index 292f2d0..44f6315 100644 --- a/xen/arch/arm/gic-v3-lpi.c +++ b/xen/arch/arm/gic-v3-lpi.c @@ -136,6 +136,77 @@ 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. + * Please note that LPIs are edge-triggered only, also have no active state, + * so spurious interrupts on the host side are no issue (we can just ignore + * them). + * Also a guest cannot expect that firing interrupts that haven't been + * fully configured yet will reach the CPU, so we don't need to care about + * this special case. + */ +void gicv3_do_LPI(unsigned int lpi) +{ + struct domain *d; + union host_lpi *hlpip, hlpi; + struct vcpu *vcpu; + + irq_enter(); + + /* EOI the LPI already. */ + WRITE_SYSREG32(lpi, ICC_EOIR1_EL1); + + /* Find out if a guest mapped something to this physical LPI. */ + hlpip = gic_get_host_lpi(lpi); + if ( !hlpip ) + goto out; + + hlpi.data = read_u64_atomic(&hlpip->data); + + /* + * Unmapped events are marked with an invalid LPI ID. We can safely + * ignore them, as they have no further state and no-one can expect + * to see them if they have not been mapped. + */ + if ( hlpi.virt_lpi == INVALID_LPI ) + goto out; + + d = rcu_lock_domain_by_id(hlpi.dom_id); + if ( !d ) + goto out; + + /* Make sure we don't step beyond the vcpu array. */ + if ( hlpi.vcpu_id >= d->max_vcpus ) + { + rcu_unlock_domain(d); + goto out; + } + + vcpu = d->vcpu[hlpi.vcpu_id]; + + /* Check if the VCPU is ready to receive LPIs. */ + if ( vcpu->arch.vgic.flags & VGIC_V3_LPIS_ENABLED ) + /* + * TODO: Investigate what to do here for potential interrupt storms. + * As we keep all host LPIs enabled, for disabling LPIs we would need + * to queue a ITS host command, which we avoid so far during a guest's + * runtime. Also re-enabling would trigger a host command upon the + * guest sending a command, which could be an attack vector for + * hogging the host command queue. + * See the thread around here for some background: + * https://lists.xen.org/archives/html/xen-devel/2016-12/msg00003.html + */ + vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi); + + rcu_unlock_domain(d); + +out: + irq_exit(); +} + static int gicv3_lpi_allocate_pendtable(uint64_t *reg) { uint64_t val; diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 29c8964..8140c5f 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1674,6 +1674,7 @@ static const struct gic_hw_operations gicv3_ops = { .make_hwdom_dt_node = gicv3_make_hwdom_dt_node, .make_hwdom_madt = gicv3_make_hwdom_madt, .iomem_deny_access = gicv3_iomem_deny_access, + .do_LPI = gicv3_do_LPI, }; static int __init gicv3_dt_preinit(struct dt_device_node *node, const void *data) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 62ae3b8..d752352 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -735,7 +735,13 @@ void gic_interrupt(struct cpu_user_regs *regs, int is_fiq) do_IRQ(regs, irq, is_fiq); local_irq_disable(); } - else if (unlikely(irq < 16)) + else if ( is_lpi(irq) ) + { + local_irq_enable(); + gic_hw_ops->do_LPI(irq); + local_irq_disable(); + } + else if ( unlikely(irq < 16) ) { do_sgi(regs, irq); } diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index 0587569..df91940 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -709,11 +709,18 @@ static struct pending_irq *vgic_v2_lpi_to_pending(struct domain *d, BUG(); } +static int vgic_v2_lpi_get_priority(struct domain *d, unsigned int vlpi) +{ + /* Dummy function, no LPIs on a VGICv2. */ + BUG(); +} + static const struct vgic_ops vgic_v2_ops = { .vcpu_init = vgic_v2_vcpu_init, .domain_init = vgic_v2_domain_init, .domain_free = vgic_v2_domain_free, .lpi_to_pending = vgic_v2_lpi_to_pending, + .lpi_get_priority = vgic_v2_lpi_get_priority, .max_vcpus = 8, }; diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index f462610..c059dbd 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1558,12 +1558,24 @@ static struct pending_irq *vgic_v3_lpi_to_pending(struct domain *d, return pirq; } +/* Retrieve the priority of an LPI from its struct pending_irq. */ +static int vgic_v3_lpi_get_priority(struct domain *d, uint32_t vlpi) +{ + struct pending_irq *p = vgic_v3_lpi_to_pending(d, vlpi); + + if ( !p ) + return GIC_PRI_IRQ; + + return p->lpi_priority; +} + static const struct vgic_ops v3_ops = { .vcpu_init = vgic_v3_vcpu_init, .domain_init = vgic_v3_domain_init, .domain_free = vgic_v3_domain_free, .emulate_reg = vgic_v3_emulate_reg, .lpi_to_pending = vgic_v3_lpi_to_pending, + .lpi_get_priority = vgic_v3_lpi_get_priority, /* * We use both AFF1 and AFF0 in (v)MPIDR. Thus, the max number of CPU * that can be supported is up to 4096(==256*16) in theory. diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index c2bfdb1..b6fe34f 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -226,10 +226,15 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq) static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) { - struct vgic_irq_rank *rank = vgic_rank_irq(v, virq); + struct vgic_irq_rank *rank; unsigned long flags; int priority; + /* LPIs don't have a rank, also store their priority separately. */ + if ( is_lpi(virq) ) + return v->domain->arch.vgic.handler->lpi_get_priority(v->domain, virq); + + rank = vgic_rank_irq(v, virq); vgic_lock_rank(v, rank, flags); priority = rank->priority[virq & INTERRUPT_RANK_MASK]; vgic_unlock_rank(v, rank, flags); diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 3d8e84c..ebaea35 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -260,7 +260,8 @@ struct arch_vcpu /* GICv3: redistributor base and flags for this vCPU */ paddr_t rdist_base; -#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */ +#define VGIC_V3_RDIST_LAST (1 << 0) /* last vCPU of the rdist */ +#define VGIC_V3_LPIS_ENABLED (1 << 1) uint8_t flags; } vgic; diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 836a103..42963c0 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -366,6 +366,8 @@ struct gic_hw_operations { int (*map_hwdom_extra_mappings)(struct domain *d); /* Deny access to GIC regions */ int (*iomem_deny_access)(const struct domain *d); + /* Handle LPIs, which require special handling */ + void (*do_LPI)(unsigned int lpi); }; void register_gic_ops(const struct gic_hw_operations *ops); diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h index 29559a3..7470779 100644 --- a/xen/include/asm-arm/gic_v3_its.h +++ b/xen/include/asm-arm/gic_v3_its.h @@ -134,6 +134,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node); bool gicv3_its_host_has_its(void); +void gicv3_do_LPI(unsigned int lpi); + int gicv3_lpi_init_rdist(void __iomem * rdist_base); /* Initialize the host structures for LPIs and the host ITSes. */ @@ -175,6 +177,12 @@ static inline bool gicv3_its_host_has_its(void) return false; } +static inline void gicv3_do_LPI(unsigned int lpi) +{ + /* We don't enable LPIs without an ITS. */ + BUG(); +} + static inline int gicv3_lpi_init_rdist(void __iomem * rdist_base) { return -ENODEV; diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index c9075a9..7efa164 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -72,6 +72,7 @@ struct pending_irq #define GIC_INVALID_LR (uint8_t)~0 uint8_t lr; uint8_t priority; + uint8_t lpi_priority; /* Caches the priority if this is an LPI. */ /* inflight is used to append instances of pending_irq to * vgic.inflight_irqs */ struct list_head inflight; @@ -136,6 +137,7 @@ struct vgic_ops { bool (*emulate_reg)(struct cpu_user_regs *regs, union hsr hsr); /* lookup the struct pending_irq for a given LPI interrupt */ struct pending_irq *(*lpi_to_pending)(struct domain *d, unsigned int vlpi); + int (*lpi_get_priority)(struct domain *d, uint32_t vlpi); /* Maximum number of vCPU supported */ const unsigned int max_vcpus; };
Upon receiving an LPI on the host, 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. Also we enhance struct pending_irq to cache the pending bit and the priority information for LPIs. Reading the information from there is faster than accessing the property table from guest memory. Also it use some padding area, so does not require more memory. This introduces a do_LPI() as a hardware gic_ops and a function to retrieve the (cached) priority value of an LPI and a vgic_ops. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic-v2.c | 7 ++++ xen/arch/arm/gic-v3-lpi.c | 71 ++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/gic-v3.c | 1 + xen/arch/arm/gic.c | 8 ++++- xen/arch/arm/vgic-v2.c | 7 ++++ xen/arch/arm/vgic-v3.c | 12 +++++++ xen/arch/arm/vgic.c | 7 +++- xen/include/asm-arm/domain.h | 3 +- xen/include/asm-arm/gic.h | 2 ++ xen/include/asm-arm/gic_v3_its.h | 8 +++++ xen/include/asm-arm/vgic.h | 2 ++ 11 files changed, 125 insertions(+), 3 deletions(-)