From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Wang2 Subject: [PATCH V3] AMD IOMMU: Fix an interrupt remapping issue Date: Mon, 11 Apr 2011 12:31:23 +0200 Message-ID: <201104111231.24422.wei.wang2@amd.com> References: <201104081335.36718.wei.wang2@amd.com> <201104081852.20738.wei.wang2@amd.com> <4DA2C89A020000780003AD92@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="Boundary-00=_8htoNIQ3DoZQ3gG" Return-path: In-Reply-To: <4DA2C89A020000780003AD92@vpn.id2.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: "Ostrovsky, Boris" , "Huang2, Wei" , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org --Boundary-00=_8htoNIQ3DoZQ3gG Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Jan, This patch is the latest. Should have addressed all of your concerns. Please take a look. Thanks, Wei Signed-off-by: Wei Wang On Monday 11 April 2011 09:23:38 Jan Beulich wrote: > >>> On 08.04.11 at 18:52, Wei Wang2 wrote: > > > > Jan, How dose this one look like to you? > > Much better, but still not quite there: The unmasking must happen > *after* the writing of the upper half (if that's what is being modified). > > You could also skip the unmasking altogether if saved_mask == 1. > > And if you start using __io_apic_write() (which I find very desirable) > is there a reason not to use it (and __io_apic_read()) in all of the > other places you touch anyway, too? > > Jan --Boundary-00=_8htoNIQ3DoZQ3gG Content-Type: text/x-diff; charset="iso-8859-1"; name="fix_intremap_v3.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="fix_intremap_v3.patch" Content-Description: fix_intremap_v3.patch diff -r e5a750d1bf9b xen/drivers/passthrough/amd/iommu_intr.c --- a/xen/drivers/passthrough/amd/iommu_intr.c Thu Apr 07 11:12:55 2011 +0100 +++ b/xen/drivers/passthrough/amd/iommu_intr.c Mon Apr 11 12:09:42 2011 +0200 @@ -117,8 +117,7 @@ static void update_intremap_entry_from_i static void update_intremap_entry_from_ioapic( int bdf, struct amd_iommu *iommu, - struct IO_APIC_route_entry *ioapic_rte, - unsigned int rte_upper, unsigned int value) + struct IO_APIC_route_entry *ioapic_rte) { unsigned long flags; u32* entry; @@ -130,28 +129,26 @@ static void update_intremap_entry_from_i req_id = get_intremap_requestor_id(bdf); lock = get_intremap_lock(req_id); - /* only remap interrupt vector when lower 32 bits in ioapic ire changed */ - if ( likely(!rte_upper) ) - { - delivery_mode = rte->delivery_mode; - vector = rte->vector; - dest_mode = rte->dest_mode; - dest = rte->dest.logical.logical_dest; - - spin_lock_irqsave(lock, flags); - offset = get_intremap_offset(vector, delivery_mode); - entry = (u32*)get_intremap_entry(req_id, offset); - - update_intremap_entry(entry, vector, delivery_mode, dest_mode, dest); - spin_unlock_irqrestore(lock, flags); - - if ( iommu->enabled ) - { - spin_lock_irqsave(&iommu->lock, flags); - invalidate_interrupt_table(iommu, req_id); - flush_command_buffer(iommu); - spin_unlock_irqrestore(&iommu->lock, flags); - } + + delivery_mode = rte->delivery_mode; + vector = rte->vector; + dest_mode = rte->dest_mode; + dest = rte->dest.logical.logical_dest; + + spin_lock_irqsave(lock, flags); + + offset = get_intremap_offset(vector, delivery_mode); + entry = (u32*)get_intremap_entry(req_id, offset); + update_intremap_entry(entry, vector, delivery_mode, dest_mode, dest); + + spin_unlock_irqrestore(lock, flags); + + if ( iommu->enabled ) + { + spin_lock_irqsave(&iommu->lock, flags); + invalidate_interrupt_table(iommu, req_id); + flush_command_buffer(iommu); + spin_unlock_irqrestore(&iommu->lock, flags); } } @@ -199,7 +196,8 @@ int __init amd_iommu_setup_ioapic_remapp spin_lock_irqsave(lock, flags); offset = get_intremap_offset(vector, delivery_mode); entry = (u32*)get_intremap_entry(req_id, offset); - update_intremap_entry(entry, vector, delivery_mode, dest_mode, dest); + update_intremap_entry(entry, vector, + delivery_mode, dest_mode, dest); spin_unlock_irqrestore(lock, flags); if ( iommu->enabled ) @@ -217,16 +215,17 @@ void amd_iommu_ioapic_update_ire( void amd_iommu_ioapic_update_ire( unsigned int apic, unsigned int reg, unsigned int value) { - struct IO_APIC_route_entry ioapic_rte = { 0 }; - unsigned int rte_upper = (reg & 1) ? 1 : 0; + struct IO_APIC_route_entry old_rte = { 0 }; + struct IO_APIC_route_entry new_rte = { 0 }; + unsigned int rte_lo = (reg & 1) ? reg - 1 : reg; int saved_mask, bdf; struct amd_iommu *iommu; - *IO_APIC_BASE(apic) = reg; - *(IO_APIC_BASE(apic)+4) = value; - if ( !iommu_intremap ) - return; + { + __io_apic_write(apic, reg, value); + return; + } /* get device id of ioapic devices */ bdf = ioapic_bdf[IO_APIC_ID(apic)]; @@ -235,30 +234,49 @@ void amd_iommu_ioapic_update_ire( { AMD_IOMMU_DEBUG("Fail to find iommu for ioapic device id = 0x%x\n", bdf); - return; - } - if ( rte_upper ) - return; - - /* read both lower and upper 32-bits of rte entry */ - *IO_APIC_BASE(apic) = reg; - *(((u32 *)&ioapic_rte) + 0) = *(IO_APIC_BASE(apic)+4); - *IO_APIC_BASE(apic) = reg + 1; - *(((u32 *)&ioapic_rte) + 1) = *(IO_APIC_BASE(apic)+4); + __io_apic_write(apic, reg, value); + return; + } + + /* save io-apic rte lower 32 bits */ + *((u32 *)&old_rte) = __io_apic_read(apic, rte_lo); + saved_mask = old_rte.mask; + + if ( reg == rte_lo ) + { + *((u32 *)&new_rte) = value; + /* read upper 32 bits from io-apic rte */ + *(((u32 *)&new_rte) + 1) = __io_apic_read(apic, reg + 1); + } + else + { + *((u32 *)&new_rte) = *((u32 *)&old_rte); + *(((u32 *)&new_rte) + 1) = value; + } /* mask the interrupt while we change the intremap table */ - saved_mask = ioapic_rte.mask; - ioapic_rte.mask = 1; - *IO_APIC_BASE(apic) = reg; - *(IO_APIC_BASE(apic)+4) = *(((int *)&ioapic_rte)+0); - ioapic_rte.mask = saved_mask; - - update_intremap_entry_from_ioapic( - bdf, iommu, &ioapic_rte, rte_upper, value); + if ( !saved_mask ) + { + old_rte.mask = 1; + __io_apic_write(apic, rte_lo, *((u32 *)&old_rte)); + } + + /* Update interrupt remapping entry */ + update_intremap_entry_from_ioapic(bdf, iommu, &new_rte); + + /* Forward write access to IO-APIC RTE */ + __io_apic_write(apic, reg, value); + + /* For lower bits access, return directly to avoid double writes */ + if ( reg == rte_lo ) + return; /* unmask the interrupt after we have updated the intremap table */ - *IO_APIC_BASE(apic) = reg; - *(IO_APIC_BASE(apic)+4) = *(((u32 *)&ioapic_rte)+0); + if ( !saved_mask ) + { + old_rte.mask = saved_mask; + __io_apic_write(apic, rte_lo, *((u32 *)&old_rte)); + } } static void update_intremap_entry_from_msi_msg( --Boundary-00=_8htoNIQ3DoZQ3gG Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --Boundary-00=_8htoNIQ3DoZQ3gG--