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

[Roman Kapl]

On 4/27/21 1:24 AM, Michael S. Tsirkin wrote:
> 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?
True, even the code in the same module. I will send v2.
>
>> ---
>>  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
>