Re: [PATCH 4/7] hw/i386/amd_iommu: Support MMIO writes to the status register

[Ethan MILON]

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 <sarun...@amd.com>
> Reviewed-by: Vasant Hegde <vasant.he...@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:

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