From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH 1/2] iommu/amd: Fix logic for clearing the IOMMU interrupt bits Date: Mon, 22 Apr 2013 19:53:00 -0500 Message-ID: <5175DB6C.7060101@amd.com> References: <1366310471-2569-1-git-send-email-suravee.suthikulpanit@amd.com> <517119E102000078000CEDB1@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <517119E102000078000CEDB1@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 4/19/2013 3:18 AM, Jan Beulich wrote: >>>> On 18.04.13 at 20:41, 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