| Message ID | 20170526173540.10066-12-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 26/05/17 18:35, 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 the domain ID and the virtual > LPI number quickly when the host takes an LPI. We then look up the > right VCPU in the struct pending_irq. > We use the existing injection function to let the GIC emulation deal > with this interrupt. > This introduces a do_LPI() as a hardware gic_ops. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-v2.c | 7 ++++ > xen/arch/arm/gic-v3-lpi.c | 76 ++++++++++++++++++++++++++++++++++++++-- > xen/arch/arm/gic-v3.c | 1 + > xen/arch/arm/gic.c | 8 ++++- > xen/include/asm-arm/domain.h | 3 +- > xen/include/asm-arm/gic.h | 2 ++ > xen/include/asm-arm/gic_v3_its.h | 8 +++++ > 7 files changed, 101 insertions(+), 4 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..438bbfe 100644 > --- a/xen/arch/arm/gic-v3-lpi.c > +++ b/xen/arch/arm/gic-v3-lpi.c > @@ -47,7 +47,6 @@ union host_lpi { > struct { > uint32_t virt_lpi; > uint16_t dom_id; > - uint16_t vcpu_id; You don't explain why you remove vcpu_id from host_lpi. This likely require a separate patch anyway. Also, I would prefer if you make the padding in the structure explicit (i.e using pad0). > }; > }; > > @@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, bool use_pta) > return per_cpu(lpi_redist, cpu).redist_id << 16; > } > > +static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq) > +{ > + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq); > + struct vcpu *v = NULL; > + > + if ( !p ) > + return; > + > + if ( p->lpi_vcpu_id < d->max_vcpus ) > + v = d->vcpu[read_atomic(&p->lpi_vcpu_id)]; Hmmm, what does prevent lpi_vcpu_id to change between the check and the read? > + > + if ( v ) v will always be valid if you read d->vcpu[....] and the way you wrote the code is very confusing. It would be clearer if you do: if ( p->lpi_vcpu_id >= d->max_vcpus ) return; v = .... vgic_vcpu_inject_irq(v, irq); Cheers,
On Tue, 30 May 2017, Julien Grall wrote: > Hi Andre, > > On 26/05/17 18:35, 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 the domain ID and the virtual > > LPI number quickly when the host takes an LPI. We then look up the > > right VCPU in the struct pending_irq. > > We use the existing injection function to let the GIC emulation deal > > with this interrupt. > > This introduces a do_LPI() as a hardware gic_ops. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > xen/arch/arm/gic-v2.c | 7 ++++ > > xen/arch/arm/gic-v3-lpi.c | 76 > > ++++++++++++++++++++++++++++++++++++++-- > > xen/arch/arm/gic-v3.c | 1 + > > xen/arch/arm/gic.c | 8 ++++- > > xen/include/asm-arm/domain.h | 3 +- > > xen/include/asm-arm/gic.h | 2 ++ > > xen/include/asm-arm/gic_v3_its.h | 8 +++++ > > 7 files changed, 101 insertions(+), 4 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..438bbfe 100644 > > --- a/xen/arch/arm/gic-v3-lpi.c > > +++ b/xen/arch/arm/gic-v3-lpi.c > > @@ -47,7 +47,6 @@ union host_lpi { > > struct { > > uint32_t virt_lpi; > > uint16_t dom_id; > > - uint16_t vcpu_id; > > You don't explain why you remove vcpu_id from host_lpi. This likely require a > separate patch anyway. > > Also, I would prefer if you make the padding in the structure explicit (i.e > using pad0). > > > }; > > }; > > > > @@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, > > bool use_pta) > > return per_cpu(lpi_redist, cpu).redist_id << 16; > > } > > > > +static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq) > > +{ > > + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq); > > + struct vcpu *v = NULL; > > + > > + if ( !p ) > > + return; > > + > > + if ( p->lpi_vcpu_id < d->max_vcpus ) > > + v = d->vcpu[read_atomic(&p->lpi_vcpu_id)]; > > Hmmm, what does prevent lpi_vcpu_id to change between the check and the read? Supposedly we are going to set lpi_vcpu_id only to good values? Meaning that we are going to do the lpi_vcpu_id checks at the time of setting lpi_vcpu_id. Thus, even if lpi_vcpu_id changes, it is not a problem. In fact, if that is true, can we even drop the if ( p->lpi_vcpu_id < d->max_vcpus ) test here? > > + > > + if ( v ) > > v will always be valid if you read d->vcpu[....] and the way you wrote the > code is very confusing. > > It would be clearer if you do: > > if ( p->lpi_vcpu_id >= d->max_vcpus ) > return; > > v = .... > vgic_vcpu_inject_irq(v, irq);
Hi Stefano, On 30/05/17 23:07, Stefano Stabellini wrote: > On Tue, 30 May 2017, Julien Grall wrote: >>> }; >>> }; >>> >>> @@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, >>> bool use_pta) >>> return per_cpu(lpi_redist, cpu).redist_id << 16; >>> } >>> >>> +static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq) >>> +{ >>> + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq); >>> + struct vcpu *v = NULL; >>> + >>> + if ( !p ) >>> + return; >>> + >>> + if ( p->lpi_vcpu_id < d->max_vcpus ) >>> + v = d->vcpu[read_atomic(&p->lpi_vcpu_id)]; >> >> Hmmm, what does prevent lpi_vcpu_id to change between the check and the read? > > Supposedly we are going to set lpi_vcpu_id only to good values? Meaning > that we are going to do the lpi_vcpu_id checks at the time of setting > lpi_vcpu_id. Thus, even if lpi_vcpu_id changes, it is not a problem. In > fact, if that is true, can we even drop the if ( p->lpi_vcpu_id < > d->max_vcpus ) test here? I am not too confident to say lpi_vcpu_id will always be valid with the current locking in the vGIC. There is a potential race between its_discard_event and this function. The former may reset pending_irq whilst reading lpi_vcpu_id as we cannot take the vCPU lock yet. But all of this is racy anyway because of the locking. This will get solved by the vGIC rework after the merge. So For the time being I would keep the check. We can revisit it later if necessary. Cheers,
On Wed, 31 May 2017, Julien Grall wrote: > Hi Stefano, > > On 30/05/17 23:07, Stefano Stabellini wrote: > > On Tue, 30 May 2017, Julien Grall wrote: > > > > }; > > > > }; > > > > > > > > @@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, > > > > bool use_pta) > > > > return per_cpu(lpi_redist, cpu).redist_id << 16; > > > > } > > > > > > > > +static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq) > > > > +{ > > > > + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq); > > > > + struct vcpu *v = NULL; > > > > + > > > > + if ( !p ) > > > > + return; > > > > + > > > > + if ( p->lpi_vcpu_id < d->max_vcpus ) > > > > + v = d->vcpu[read_atomic(&p->lpi_vcpu_id)]; > > > > > > Hmmm, what does prevent lpi_vcpu_id to change between the check and the > > > read? > > > > Supposedly we are going to set lpi_vcpu_id only to good values? Meaning > > that we are going to do the lpi_vcpu_id checks at the time of setting > > lpi_vcpu_id. Thus, even if lpi_vcpu_id changes, it is not a problem. In > > fact, if that is true, can we even drop the if ( p->lpi_vcpu_id < > > d->max_vcpus ) test here? > > I am not too confident to say lpi_vcpu_id will always be valid with the > current locking in the vGIC. Fair enough, but I wouldn't solve a bug with unnecessary code soon to be removed. The problem lies elsewhere and should be fixed elsewhere. We could even add an ASSERT(p->lpi_vcpu_id < d->max_vcpus) here, would that work for you? > There is a potential race between its_discard_event and this function. The > former may reset pending_irq whilst reading lpi_vcpu_id as we cannot take the > vCPU lock yet. > > But all of this is racy anyway because of the locking. This will get solved by > the vGIC rework after the merge. > > So For the time being I would keep the check. We can revisit it later if > necessary. This is one of those things that are unimportant because they work either way.
Hi Stefano, On 31/05/2017 18:56, Stefano Stabellini wrote: > On Wed, 31 May 2017, Julien Grall wrote: >> Hi Stefano, >> >> On 30/05/17 23:07, Stefano Stabellini wrote: >>> On Tue, 30 May 2017, Julien Grall wrote: >>>>> }; >>>>> }; >>>>> >>>>> @@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, >>>>> bool use_pta) >>>>> return per_cpu(lpi_redist, cpu).redist_id << 16; >>>>> } >>>>> >>>>> +static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq) >>>>> +{ >>>>> + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq); >>>>> + struct vcpu *v = NULL; >>>>> + >>>>> + if ( !p ) >>>>> + return; >>>>> + >>>>> + if ( p->lpi_vcpu_id < d->max_vcpus ) >>>>> + v = d->vcpu[read_atomic(&p->lpi_vcpu_id)]; >>>> >>>> Hmmm, what does prevent lpi_vcpu_id to change between the check and the >>>> read? >>> >>> Supposedly we are going to set lpi_vcpu_id only to good values? Meaning >>> that we are going to do the lpi_vcpu_id checks at the time of setting >>> lpi_vcpu_id. Thus, even if lpi_vcpu_id changes, it is not a problem. In >>> fact, if that is true, can we even drop the if ( p->lpi_vcpu_id < >>> d->max_vcpus ) test here? >> >> I am not too confident to say lpi_vcpu_id will always be valid with the >> current locking in the vGIC. > > Fair enough, but I wouldn't solve a bug with unnecessary code soon to be > removed. The problem lies elsewhere and should be fixed elsewhere. We > could even add an ASSERT(p->lpi_vcpu_id < d->max_vcpus) here, would > that work for you? The problem lies in this code because we don't have per-IRQ locking. The ASSERT is not a solution to make sure a race does not happen (it could easily happen if the LPI is pending whilst the guest is discarding it). If you add an ASSERT you will potentially get your platform crashing time to time on discard. So I don't think the ASSERT is acceptable here. > > >> There is a potential race between its_discard_event and this function. The >> former may reset pending_irq whilst reading lpi_vcpu_id as we cannot take the >> vCPU lock yet. >> >> But all of this is racy anyway because of the locking. This will get solved by >> the vGIC rework after the merge. >> >> So For the time being I would keep the check. We can revisit it later if >> necessary. > > This is one of those things that are unimportant because they work > either way. I am sorry, but it matters when you can easily prevent crash and limit the racy condition. 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..438bbfe 100644 --- a/xen/arch/arm/gic-v3-lpi.c +++ b/xen/arch/arm/gic-v3-lpi.c @@ -47,7 +47,6 @@ union host_lpi { struct { uint32_t virt_lpi; uint16_t dom_id; - uint16_t vcpu_id; }; }; @@ -136,6 +135,80 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, bool use_pta) return per_cpu(lpi_redist, cpu).redist_id << 16; } +static void vgic_vcpu_inject_lpi(struct domain *d, unsigned int virq) +{ + struct pending_irq *p = irq_to_pending(d->vcpu[0], virq); + struct vcpu *v = NULL; + + if ( !p ) + return; + + if ( p->lpi_vcpu_id < d->max_vcpus ) + v = d->vcpu[read_atomic(&p->lpi_vcpu_id)]; + + if ( v ) + vgic_vcpu_inject_irq(v, virq); +} + +/* + * 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; + + 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; + + /* + * 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_lpi(d, hlpi.virt_lpi); + + rcu_unlock_domain(d); + +out: + irq_exit(); +} + static int gicv3_lpi_allocate_pendtable(uint64_t *reg) { uint64_t val; @@ -417,7 +490,6 @@ int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t *first_lpi) */ hlpi.virt_lpi = INVALID_LPI; hlpi.dom_id = d->domain_id; - hlpi.vcpu_id = INVALID_VCPU_ID; write_u64_atomic(&lpi_data.host_lpis[chunk][lpi_idx + i].data, hlpi.data); 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 957d516..9e32103 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -733,7 +733,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/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 5d5b4cc..783937b 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -367,6 +367,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;
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 the domain ID and the virtual LPI number quickly when the host takes an LPI. We then look up the right VCPU in the struct pending_irq. We use the existing injection function to let the GIC emulation deal with this interrupt. This introduces a do_LPI() as a hardware gic_ops. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic-v2.c | 7 ++++ xen/arch/arm/gic-v3-lpi.c | 76 ++++++++++++++++++++++++++++++++++++++-- xen/arch/arm/gic-v3.c | 1 + xen/arch/arm/gic.c | 8 ++++- xen/include/asm-arm/domain.h | 3 +- xen/include/asm-arm/gic.h | 2 ++ xen/include/asm-arm/gic_v3_its.h | 8 +++++ 7 files changed, 101 insertions(+), 4 deletions(-)