1. Patchew
  2. QEMU
  3. [PATCH v2 0/6] hw/i386/amd_iommu: Cleanups and fixes
  4. View patch

[PATCH v2 4/6] hw/i386/amd_iommu: Fix amdvi_write*()

Sairaj Kodilkar posted 6 patches 3 months, 3 weeks ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
[PATCH v2 4/6] hw/i386/amd_iommu: Fix amdvi_write*()
Posted by Sairaj Kodilkar 3 months, 3 weeks ago 
amdvi_write*() function do not preserve the older values of W1C bits in
the MMIO register. This results in all W1C bits set to 0, when guest
tries to reset a single bit by writing 1 to it. Fix this by preserving
W1C bits in the old value of the MMIO register.

Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
Suggested-by: Ethan MILON <ethan.milon@eviden.com>
Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---
 hw/i386/amd_iommu.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 2183510d227c..bbffd07b4e48 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -124,7 +124,8 @@ static void amdvi_writew(AMDVIState *s, hwaddr addr, uint16_t val)
     uint16_t w1cmask = lduw_le_p(&s->w1cmask[addr]);
     uint16_t oldval = lduw_le_p(&s->mmior[addr]);
     stw_le_p(&s->mmior[addr],
-            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
+            ((oldval & (romask | w1cmask)) |
+             (val & ~romask)) & ~(val & w1cmask));
 }
 
 static void amdvi_writel(AMDVIState *s, hwaddr addr, uint32_t val)
@@ -133,7 +134,8 @@ static void amdvi_writel(AMDVIState *s, hwaddr addr, uint32_t val)
     uint32_t w1cmask = ldl_le_p(&s->w1cmask[addr]);
     uint32_t oldval = ldl_le_p(&s->mmior[addr]);
     stl_le_p(&s->mmior[addr],
-            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
+            ((oldval & (romask | w1cmask)) |
+             (val & ~romask)) & ~(val & w1cmask));
 }
 
 static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val)
@@ -142,7 +144,8 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val)
     uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
     uint64_t oldval = ldq_le_p(&s->mmior[addr]);
     stq_le_p(&s->mmior[addr],
-            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
+            ((oldval & (romask | w1cmask)) |
+             (val & ~romask)) & ~(val & w1cmask));
 }
 
 /* OR a 64-bit register with a 64-bit value */
-- 
2.34.1
Re: [PATCH v2 4/6] hw/i386/amd_iommu: Fix amdvi_write*()
Posted by Philippe Mathieu-Daudé 3 months, 3 weeks ago 
On 24/7/25 08:47, Sairaj Kodilkar wrote:
> amdvi_write*() function do not preserve the older values of W1C bits in
> the MMIO register. This results in all W1C bits set to 0, when guest
> tries to reset a single bit by writing 1 to it. Fix this by preserving
> W1C bits in the old value of the MMIO register.
> 
> Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
> Suggested-by: Ethan MILON <ethan.milon@eviden.com>
> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
> ---
>   hw/i386/amd_iommu.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 2183510d227c..bbffd07b4e48 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -124,7 +124,8 @@ static void amdvi_writew(AMDVIState *s, hwaddr addr, uint16_t val)
>       uint16_t w1cmask = lduw_le_p(&s->w1cmask[addr]);
>       uint16_t oldval = lduw_le_p(&s->mmior[addr]);
>       stw_le_p(&s->mmior[addr],
> -            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
> +            ((oldval & (romask | w1cmask)) |
> +             (val & ~romask)) & ~(val & w1cmask));

High code complexity, hard to review. Do you mind introducing
intermediate well-named variables for clarity?

>   }
>   
>   static void amdvi_writel(AMDVIState *s, hwaddr addr, uint32_t val)
> @@ -133,7 +134,8 @@ static void amdvi_writel(AMDVIState *s, hwaddr addr, uint32_t val)
>       uint32_t w1cmask = ldl_le_p(&s->w1cmask[addr]);
>       uint32_t oldval = ldl_le_p(&s->mmior[addr]);
>       stl_le_p(&s->mmior[addr],
> -            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
> +            ((oldval & (romask | w1cmask)) |
> +             (val & ~romask)) & ~(val & w1cmask));
>   }
>   
>   static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val)
> @@ -142,7 +144,8 @@ static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val)
>       uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
>       uint64_t oldval = ldq_le_p(&s->mmior[addr]);
>       stq_le_p(&s->mmior[addr],
> -            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
> +            ((oldval & (romask | w1cmask)) |
> +             (val & ~romask)) & ~(val & w1cmask));
>   }
>   
>   /* OR a 64-bit register with a 64-bit value */
Re: [PATCH v2 4/6] hw/i386/amd_iommu: Fix amdvi_write*()
Posted by Sairaj Kodilkar 3 months, 2 weeks ago 

On 7/24/2025 2:20 PM, Philippe Mathieu-Daudé wrote:
> On 24/7/25 08:47, Sairaj Kodilkar wrote:
>> amdvi_write*() function do not preserve the older values of W1C bits in
>> the MMIO register. This results in all W1C bits set to 0, when guest
>> tries to reset a single bit by writing 1 to it. Fix this by preserving
>> W1C bits in the old value of the MMIO register.
>>
>> Fixes: d29a09ca68428 ("hw/i386: Introduce AMD IOMMU")
>> Suggested-by: Ethan MILON <ethan.milon@eviden.com>
>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>> ---
>>   hw/i386/amd_iommu.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 2183510d227c..bbffd07b4e48 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -124,7 +124,8 @@ static void amdvi_writew(AMDVIState *s, hwaddr 
>> addr, uint16_t val)
>>       uint16_t w1cmask = lduw_le_p(&s->w1cmask[addr]);
>>       uint16_t oldval = lduw_le_p(&s->mmior[addr]);
>>       stw_le_p(&s->mmior[addr],
>> -            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>> +            ((oldval & (romask | w1cmask)) |
>> +             (val & ~romask)) & ~(val & w1cmask));
> 
> High code complexity, hard to review. Do you mind introducing
> intermediate well-named variables for clarity?
>

Sure !

>>   }
>>   static void amdvi_writel(AMDVIState *s, hwaddr addr, uint32_t val)
>> @@ -133,7 +134,8 @@ static void amdvi_writel(AMDVIState *s, hwaddr 
>> addr, uint32_t val)
>>       uint32_t w1cmask = ldl_le_p(&s->w1cmask[addr]);
>>       uint32_t oldval = ldl_le_p(&s->mmior[addr]);
>>       stl_le_p(&s->mmior[addr],
>> -            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>> +            ((oldval & (romask | w1cmask)) |
>> +             (val & ~romask)) & ~(val & w1cmask));
>>   }
>>   static void amdvi_writeq(AMDVIState *s, hwaddr addr, uint64_t val)
>> @@ -142,7 +144,8 @@ static void amdvi_writeq(AMDVIState *s, hwaddr 
>> addr, uint64_t val)
>>       uint64_t w1cmask = ldq_le_p(&s->w1cmask[addr]);
>>       uint64_t oldval = ldq_le_p(&s->mmior[addr]);
>>       stq_le_p(&s->mmior[addr],
>> -            ((oldval & romask) | (val & ~romask)) & ~(val & w1cmask));
>> +            ((oldval & (romask | w1cmask)) |
>> +             (val & ~romask)) & ~(val & w1cmask));
>>   }
>>   /* OR a 64-bit register with a 64-bit value */
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.