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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2024 12:01, Andrew Cooper wrote:
> 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.

That's what we do, yes, but is that correct? I agree with Roger's reading
of that sentence.

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

..., while at the same time preventing LVTERR delivery when there was
another error already pending.

Jan