Re: [PATCH 1/2] iommu/amd: Fix logic for clearing the IOMMU interrupt bits

[Suravee Suthikulanit <suravee.suthikulpanit@amd.com>]

On 4/19/2013 3:18 AM, Jan Beulich wrote:
>>>> On 18.04.13 at 20:41, <suravee.suthikulpanit@amd.com> wrote:
>> The IOMMU interrupt bits in the IOMMU status registers are
>> cleared when writing 1.  Therefore, the existing logic which reads
>> the register, set the bit, and then writing back the values
>> could accidentally clear certain bits if it has been set.
>>
>> The correct logic would just be writing only the value which only
>> set the interrupt bits, and leave the rest to zeros.
> So looking through the status register accesses, I would assume
> that guest_iommu_mmio_write64() is broken too? In that it
> clearly doesn't implement the RW1C behavior for the interrupt
> and overflow bits?
Actually, I'm not quite sure why hvm guest would be writing to IOMMU 
mmio.  I don't actually see AMD IOMMU is exposed to the hvm guest. 
However, isn't this function is supposed to be passing on the value 
intended to be written from guest?  In the sense, if the guest is trying 
to set the bit to 1, the function will just do what it's been told to 
write to the register?

>
> Further (as indicated above), the overflow bits being RW1C too,
> doesn't iommu_reset_log() need a similar fix
> (iommu_set_bit(&entry, of_bit) instead of
> iommu_clear_bit(&entry, of_bit))?
>
> And, perhaps even more importantly, iommu_interrupt_handler()
> also ought to use iommu_set_bit()? Or wait - isn't this touching
> the wrong bits altogether? Logically I would expect interrupt
> enable bits to be cleared here, i.e. IOMMU_CONTROL_* to be
> used throughout this function instead of IOMMU_STATUS_*.
>
> Jan
>
Thanks for catching all these several places where RW1C is not properly 
implemented.  I'll submit a new patch.

Suravee