Re: [Xen-devel] [PATCH 2/7] Nested VMX: Allow to ack irq even virtual intr delivery is enabled

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Andrew Cooper wrote on 2013-08-09:
> On 09/08/13 09:49, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>> 
>> In some special cases, we want to ack irq regardless of virtual
>> interrupt
> delivery.
> 
> Can you explain why and when we might want to force an ack?
After vritual_vmexit, if the interrupt already is already recorded in vmcs12. 
Then we should clear the irr and set isr regardless APIC-v.
> 
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>> ---
>>  xen/arch/x86/hvm/irq.c           |    2 +-
>>  xen/arch/x86/hvm/vlapic.c        |    4 ++--
>>  xen/include/asm-x86/hvm/vlapic.h |    2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>> diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index
>> 9eae5de..6a6fb68 100644
>> --- a/xen/arch/x86/hvm/irq.c
>> +++ b/xen/arch/x86/hvm/irq.c
>> @@ -437,7 +437,7 @@ struct hvm_intack hvm_vcpu_ack_pending_irq(
>>              intack.vector = (uint8_t)vector;
>>          break;
>>      case hvm_intsrc_lapic:
>> -        if ( !vlapic_ack_pending_irq(v, intack.vector) )
>> +        if ( !vlapic_ack_pending_irq(v, intack.vector, 0) )
>>              intack = hvm_intack_none;
>>          break;
>>      case hvm_intsrc_vector:
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 7a154f9..20a36a0 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -1048,11 +1048,11 @@ int vlapic_has_pending_irq(struct vcpu *v)
>>      return irr;
>>  }
>> -int vlapic_ack_pending_irq(struct vcpu *v, int vector)
>> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int
>> +force_ack)
>>  {
>>      struct vlapic *vlapic = vcpu_vlapic(v);
>> -    if ( vlapic_virtual_intr_delivery_enabled() )
>> +    if ( vlapic_virtual_intr_delivery_enabled() && !force_ack )
>>          return 1;
> 
> The logic in this entire function seems quite backwards.  It
> unconditionally returns 1 in all cases, and its sole callsite is in an if 
> statement.
> 
> Something like:
> 
> void vlapic_ack_pending_irq(struct vcpu *v, uint8_t vector, bool_t
> force_ack) {
>     struct vlapic *vlapic = vcpu_vlapic(v);
>     
>     if ( force_ack || !vlapic_virtual_intr_delivery_enabled() )
>     {
>         vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]);
>         vlapic_clear_irr(vector, vlapic);
>     }
> }
> 
> Seems substantially clearer.
> 
> ~Andrew
> 
>> 
>>      vlapic_set_vector(vector, &vlapic->regs->data[APIC_ISR]); diff
>> --git a/xen/include/asm-x86/hvm/vlapic.h
>> b/xen/include/asm-x86/hvm/vlapic.h
>> index 021a5f2..d8c9511 100644
>> --- a/xen/include/asm-x86/hvm/vlapic.h
>> +++ b/xen/include/asm-x86/hvm/vlapic.h
>> @@ -96,7 +96,7 @@ bool_t is_vlapic_lvtpc_enabled(struct vlapic
>> *vlapic);  void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec,
>> uint8_t trig);
>> 
>>  int vlapic_has_pending_irq(struct vcpu *v); -int
>> vlapic_ack_pending_irq(struct vcpu *v, int vector);
>> +int vlapic_ack_pending_irq(struct vcpu *v, int vector, int
>> +force_ack);
>> 
>>  int  vlapic_init(struct vcpu *v);
>>  void vlapic_destroy(struct vcpu *v);


Best regards,
Yang



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