Re: [PATCH 1/2] x86/vlapic: Fix handling of writes to APIC_ESR

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/11/2024 9:03 am, Roger Pau Monné wrote:
> On Thu, Nov 28, 2024 at 12:47:36AM +0000, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>> index 3363926b487b..98394ed26a52 100644
>> --- a/xen/arch/x86/hvm/vlapic.c
>> +++ b/xen/arch/x86/hvm/vlapic.c
>> @@ -108,7 +108,7 @@ static void vlapic_error(struct vlapic *vlapic, unsigned 
>> int errmask)
>>      uint32_t esr;
>>  
>>      spin_lock_irqsave(&vlapic->esr_lock, flags);
>> -    esr = vlapic_get_reg(vlapic, APIC_ESR);
>> +    esr = vlapic->hw.pending_esr;
>>      if ( (esr & errmask) != errmask )
>>      {
>>          uint32_t lvterr = vlapic_get_reg(vlapic, APIC_LVTERR);
>> @@ -127,7 +127,7 @@ static void vlapic_error(struct vlapic *vlapic, unsigned 
>> int errmask)
>>                   errmask |= APIC_ESR_RECVILL;
>>          }
>>  
>> -        vlapic_set_reg(vlapic, APIC_ESR, esr | errmask);
>> +        vlapic->hw.pending_esr |= errmask;
>>  
>>          if ( inj )
>>              vlapic_set_irq(vlapic, lvterr & APIC_VECTOR_MASK, 0);
> The SDM also contains:
>
> "This write also rearms the APIC error interrupt triggering
> mechanism."
>
> Where "this write" is a write to the ESR register.

Correct.

> My understanding
> is that the error vector will only be injected for the first reported
> error. I think the logic regarding whether to inject the lvterr vector
> needs to additionally be gated on whether vlapic->hw.pending_esr ==
> 0.

I think it's clumsy wording.

Bits being set mask subsequent LVTERR's of the same type.  That's what
the "if ( (esr & errmask) != errmask )" guard is doing above.

What I think it's referring to is that writing APIC_ESR will zero
pending_esr and thus any subsequent error will cause LVTERR to deliver.


Having said all that, I can't find anything in the current SDM/APM which
states this.  I think I need to go back to the older manuals.

~Andrew