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
