Re: [Xen-devel] [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs()

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

sorry for the delay on this, I just found this in preparation for a repost.

On 08/12/17 21:40, Stefano Stabellini wrote:
> On Thu, 7 Dec 2017, Andre Przywara wrote:
>> In gic_restore_pending_irqs() we push our pending virtual IRQs into the
>> list registers. This function is called once from a GIC context and once
>> from a VGIC context. Refactor the calls so that we have only one callsite
>> from the VGIC context. This will help separating the two worlds later.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>> ---
>>  xen/arch/arm/domain.c     |  1 +
>>  xen/arch/arm/gic.c        | 11 +++++------
>>  xen/arch/arm/traps.c      |  2 +-
>>  xen/include/asm-arm/gic.h |  2 +-
>>  4 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index a74ff1c07c..73f4d4b2b2 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -185,6 +185,7 @@ static void ctxt_switch_to(struct vcpu *n)
>>  
>>      /* VGIC */
>>      gic_restore_state(n);
>> +    gic_inject(n);
>>  
>>      /* VFP */
>>      vfp_restore_state(n);
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index bac8ada2bb..1f00654ef5 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -36,8 +36,6 @@
>>  #include <asm/vgic.h>
>>  #include <asm/acpi.h>
>>  
>> -static void gic_restore_pending_irqs(struct vcpu *v);
>> -
>>  static DEFINE_PER_CPU(uint64_t, lr_mask);
>>  
>>  #define lr_all_full() (this_cpu(lr_mask) == ((1 << 
>> gic_hw_ops->info->nr_lrs) - 1))
>> @@ -91,8 +89,6 @@ void gic_restore_state(struct vcpu *v)
>>      gic_hw_ops->restore_state(v);
>>  
>>      isb();
>> -
>> -    gic_restore_pending_irqs(v);
>>  }
>>  
>>  /* desc->irq needs to be disabled before calling this function */
>> @@ -715,11 +711,14 @@ out:
>>      return rc;
>>  }
>>  
>> -void gic_inject(void)
>> +void gic_inject(struct vcpu *v)
>>  {
>>      ASSERT(!local_irq_is_enabled());
>>  
>> -    gic_restore_pending_irqs(current);
>> +    gic_restore_pending_irqs(v);
> 
> This looks suspicious to me: gic_restore_pending_irqs calls gic_set_lr.
> It doesn't look like gic_restore_pending_irqs can actually be called for
> v != current.

Good point!

> However, I think that we could remove the call to
> gic_restore_pending_irqs from gic_restore_state safely, because we can
> still rely on gic_inject being called before entering the guest.

Yes, I believe so as well, actually found this when debugging the new
VGIC. Debug printks showed LR injections happening twice, which is
actually harmful with the new VGIC (because it sometimes changes the
state of the virtual IRQ).

> There is no need to add a call to gic_inject in ctxt_switch_to, I think.

Yes, I agree.

So this simplifies this patch quite a lot, actually to just removing the
call of gic_restore_pending_irqs(), and the forward declaration.
But in contrast to this v2 patch here (which was really just
refactoring) this changes behaviour now, so needs some testing (works
for me (tm), though)

Cheers,
Andre.

>> +    if ( v != current )
>> +        return;
>>  
>>      if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
>>          gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index ff3d6ff2aa..7fd676ed9d 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2298,7 +2298,7 @@ void leave_hypervisor_tail(void)
>>      {
>>          local_irq_disable();
>>          if (!softirq_pending(smp_processor_id())) {
>> -            gic_inject();
>> +            gic_inject(current);
>>  
>>              /*
>>               * If the SErrors handle option is "DIVERSE", we have to prevent
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index 587a14f8b9..28cf16654a 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -235,7 +235,7 @@ extern int gic_route_irq_to_guest(struct domain *, 
>> unsigned int virq,
>>  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>>                                struct irq_desc *desc);
>>  
>> -extern void gic_inject(void);
>> +extern void gic_inject(struct vcpu *v);
>>  extern void gic_clear_pending_irqs(struct vcpu *v);
>>  extern int gic_events_need_delivery(void);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel