On 7/16/2025 7:57 PM, Ethan MILON wrote:
> Hi,
>
> On 7/16/25 09:31, Sairaj Kodilkar wrote:
>> Support the writes to the status register so that guest can reset the
>> EventOverflow, EventLogInt, ComWaitIntr, etc bits after servicing the
>> respective interrupt.
>>
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> hw/i386/amd_iommu.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 784be78f402d..e0f4220b8f25 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -1613,6 +1613,9 @@ static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>> amdvi_mmio_reg_write(s, size, val, addr);
>> amdvi_handle_pprtail_write(s);
>> break;
>> + case AMDVI_MMIO_STATUS:
>> + amdvi_mmio_reg_write(s, size, val, addr);
>> + break;
>
> This fixes the basic issue with interrupt reset, but there's still a
> subtle bug: any update to the status register clears the interrupt bits,
> regardless of the value.
>
> The current W1C logic looks like an incomplete copy of the Intel
> implementation, and only works properly if the W1C bits are also set in
> romask.
>
> We should update romask to include w1cmask, either in amdvi_write[wlq],
> amdvi_init, or directly in amdvi_set_quad:
Yep, verified it. The correct implementation of amdvi_set_quad should
have (oldval & (romask | w1cmask)).
Thanks for pointing this out.
-Sairaj
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 66d42f..48d991 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -213,7 +213,7 @@ static void amdvi_set_quad(AMDVIState *s, hwaddr addr, uint64_t val,
> uint64_t romask, uint64_t w1cmask)
> {
> stq_le_p(&s->mmior[addr], val);
> - stq_le_p(&s->romask[addr], romask);
> + stq_le_p(&s->romask[addr], romask | w1cmask);
> stq_le_p(&s->w1cmask[addr], w1cmask);
> }
>
> Thanks,
> Ethan
>
>> }
>> }
>>
>> --
>> 2.34.1
>>