Re: [Xen-devel] [PATCH v2 10/27] ARM: GICv3: forward pending LPIs to guests

[Andre Przywara <andre.przywara@xxxxxxx>]

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@xxxxxxx>
>> ---
>>  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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel