Re: [PATCH 1/1] amd_iommu: fix wrong MMIO operations

[Michael S. Tsirkin]

On Mon, Apr 26, 2021 at 10:21:54AM +0200, Roman Kapl wrote:
> Address was swapped with value when writing MMIO registers, so the user
> saw garbage in lot of cases. The interrupt status was not correctly set.
> 
> Signed-off-by: Roman Kapl <rka@sysgo.com>

Ouch. This API is just inconsistent, everyone else
uses addr, value in this order. How about fixing the
function signature instead?


> ---
>  hw/i386/amd_iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 74a93a5d93..bb5ce8c04d 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -141,13 +141,13 @@ static bool amdvi_test_mask(AMDVIState *s, hwaddr addr, 
> uint64_t val)
>  /* OR a 64-bit register with a 64-bit value storing result in the register */
>  static void amdvi_assign_orq(AMDVIState *s, hwaddr addr, uint64_t val)
>  {
> -    amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) | val);
> +    amdvi_writeq_raw(s, amdvi_readq(s, addr) | val, addr);
>  }
>  
>  /* AND a 64-bit register with a 64-bit value storing result in the register 
> */
>  static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>  {
> -   amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) & val);
> +   amdvi_writeq_raw(s, amdvi_readq(s, addr) & val, addr);
>  }
>  
>  static void amdvi_generate_msi_interrupt(AMDVIState *s)
> @@ -382,7 +382,7 @@ static void amdvi_completion_wait(AMDVIState *s, uint64_t 
> *cmd)
>      }
>      /* set completion interrupt */
>      if (extract64(cmd[0], 1, 1)) {
> -        amdvi_test_mask(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_COMP_INT);
> +        amdvi_assign_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_COMP_INT);
>          /* generate interrupt */
>          amdvi_generate_msi_interrupt(s);
>      }
> -- 
> 2.20.1