AMD IOMMU: Fix an interrupt remapping issue
PATCH 1/1 AMD IOMMU: Fix an interrupt remapping issue
(sorry, Forget the patch)
Some device could generate bogus interrupts if an IO-APIC RTE and an iommu interrupt remapping entry are not consistent during 2 adjacent 64bits IO-APIC RTE updates. For example, if the 2nd operation updates destination bits in RTE for SATA device and unmask it, in some case, SATA device will assert ioapic pin to generate interrupt immediately using new destination but iommu could still translate it into the old destination, then dom0 would be confused. To fix that, we sync up interrupt remapping entry with IO-APIC IRE on every 32 bits operation and foward IOAPIC RTE updates after interrupt remapping table has been changed.
Jan, This patch fixes SATA device issue we observed (Bug #680824), please review it. Thanks!
Wei
--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim,
Landkreis München Registergericht München,
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd
# HG changeset patch
# User Wei Wang <[email protected]>
# Node ID ab2944070ca99790546b34fa04a80103d3e7464f
# Parent e5a750d1bf9bb021713c6721000e655a4054ebea
Some device could generate bogus interrupts if an IO-APIC RTE and an iommu interrupt remapping entry are not consistent during 2 adjacent 64bits IO-APIC RTE updates. For example, if the 2nd operation updates destination bits in RTE for SATA device and unmask it, in some case, SATA device will assert ioapic pin to generate interrupt immediately using new destination but iommu could still translate it into the old destination, then dom0 would be confused. To fix that, we sync up interrupt remapping entry with IO-APIC IRE on every 32 bits operation and foward IOAPIC RTE updates after interrupt remapping table has been changed.
Signed-off-by Wei Wang <[email protected]>
xen/drivers/passthrough/amd/iommu_intr.c +39 -37
@@ -118,7 +118,7 @@ static void update_intremap_entry_from_iint bdf, struct amd_iommu *iommu, struct IO_APIC_route_entry *ioapic_rte, - unsigned int rte_upper, unsigned int value) + unsigned int value) { unsigned long flags;u32 * entry;
@@ -130,28 +130,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 +197,8 @@ int __init amd_iommu_setup_ioapic_remappspin_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 )
@@ -218,15 +217,12 @@ 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 ; +unsigned int rte_lo;int saved_mask, bdf;struct amd_iommu *iommu; - *IO_APIC_BASE (apic) = reg; - *(IO_APIC_BASE (apic)+4 ) = value; -if ( !iommu_intremap ) -return ; + goto done;/* get device id of ioapic devices */ bdf = ioapic_bdf[IO_APIC_ID (apic)];
@@ -237,28 +233,34 @@ void amd_iommu_ioapic_update_ire( bdf);return ; } -if ( rte_upper ) -return ; + +/* get lower 32 bits IO-APIC ire index */ + rte_lo = (reg &1 ) ? reg -1 : reg;/* read both lower and upper 32-bits of rte entry */ - *IO_APIC_BASE (apic) = reg; + *IO_APIC_BASE (apic) = rte_lo; *(((u32 *)&ioapic_rte) +0 ) = *(IO_APIC_BASE (apic)+4 ); - *IO_APIC_BASE (apic) = reg +1 ; + *IO_APIC_BASE (apic) = rte_lo +1 ; *(((u32 *)&ioapic_rte) +1 ) = *(IO_APIC_BASE (apic)+4 );/* 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) = rte_lo; *(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); +/* Update interrupt remapping entry */ +update_intremap_entry_from_ioapic (bdf, iommu, &ioapic_rte, value);/* unmask the interrupt after we have updated the intremap table */ + *IO_APIC_BASE (apic) = rte_lo; + *(IO_APIC_BASE (apic)+4 ) = *(((u32 *)&ioapic_rte)+0 ); + +done: +/* Forward write access to IO-APIC */ *IO_APIC_BASE (apic) = reg; - *(IO_APIC_BASE (apic)+4 ) = *(((u32 *)&ioapic_rte)+0 ); + *(IO_APIC_BASE (apic)+4 ) = value; } static void update_intremap_entry_from_msi_msg(
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 10 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 13:35, Wei Wang2 <[email protected]> wrote:
Some device could generate bogus interrupts if an IO-APIC RTE and an iommu interrupt remapping entry are not consistent during 2 adjacent 64bits IO-APIC RTE updates. For example, if the 2nd operation updates destination bits in RTE for SATA device and unmask it, in some case, SATA device will assert ioapic pin to generate interrupt immediately using new destination but iommu could still translate it into the old destination, then dom0 would be confused. To fix that, we sync up interrupt remapping entry with IO-APIC IRE on every 32 bits operation and foward IOAPIC RTE updates after interrupt remapping table has been changed.
I don't think this is correct: Without the patch, the filling of ioapic_rte takes into account the value already written. Now that you only write the value at the end of the function, you should overwrite the affected half with "value" immediately before calling update_intremap_entry_from_ioapic(). (Without knowing which half gets written, passing "value" to update_intremap_entry_from_ioapic() is pointless, and indeed the function doesn't use that parameter.)
Eliminating the double write if reg == rte_lo would also seem desirable (and in no case should you write back the old value after having called update_intremap_entry_from_ioapic()).
Jan
Quoted 12 lines · On Friday 08 April 2011 15:43:57 Jan Beulich wrote:
Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 13:35, Wei Wang2 <[email protected]> wrote:
Some device could generate bogus interrupts if an IO-APIC RTE and an iommu interrupt remapping entry are not consistent during 2 adjacent 64bits IO-APIC RTE updates. For example, if the 2nd operation updates destination bits in RTE for SATA device and unmask it, in some case, SATA device will assert ioapic pin to generate interrupt immediately using new destination but iommu could still translate it into the old destination, then dom0 would be confused. To fix that, we sync up interrupt remapping entry with IO-APIC IRE on every 32 bits operation and foward IOAPIC RTE updates after interrupt remapping table has been changed.
Quoted 5 lines
I don't think this is correct: Without the patch, the filling of
ioapic_rtetakes into account the value already written. Now that you only write the value at the end of the function, you should overwrite the affected half with "value" immediately before callingupdate_intremap_entry_from_ioapic().
Sorry, not quite understand your point. My thought is, no matter dom0 tried to updates lower half or upper half of RTE, we always updates interrupt table from the lower half. This will keep iommu table strictly identically to RTE. The old code has an assumption that both lower half and upper of RTE should be updated together. But this might not be always true. If by incident, dom0 only updates the upper half and we don't sync iommu with it, then the destination in RTE and iommu table will be different.
Quoted 3 lines
(Without knowing which half
gets written, passing "value" toupdate_intremap_entry_from_ioapic()is pointless, and indeed the function doesn't use that parameter.)
True, I will remove this parameter in update_intremap_entry_from_ioapic(). ioapic_rte should have all information.
Quoted 3 lines
Eliminating the double write if reg ==
rte_lowould also seem desirable (and in no case should you write back the old value after having calledupdate_intremap_entry_from_ioapic()).
It not a write back, It just finishes IO-APIC RTE writes. After updating interrupt remapping table we still have to update RTE. It is just a copy of __io_apic_write (maybe I should just call it). Old code updates ioapic earlier than interrupt remapping table and sata device might generate interrupt right after this, which is not expected.
Thanks,
Wei
Quoted 1 line
Jan
Quoted 20 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 16:26, Wei Wang2 <[email protected]> wrote:
Quoted 12 lines · On Friday 08 April 2011 15:43:57 Jan Beulich wrote:
Quoted 11 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 13:35, Wei Wang2 <[email protected]> wrote:
Some device could generate bogus interrupts if an IO-APIC RTE and an iommu interrupt remapping entry are not consistent during 2 adjacent 64bits IO-APIC RTE updates. For example, if the 2nd operation updates destination bits in RTE for SATA device and unmask it, in some case, SATA device will assert ioapic pin to generate interrupt immediately using new destination but iommu could still translate it into the old destination, then dom0 would be confused. To fix that, we sync up interrupt remapping entry with IO-APIC IRE on every 32 bits operation and foward IOAPIC RTE updates after interrupt remapping table has been changed.
Quoted 5 lines
I don't think this is correct: Without the patch, the filling of
ioapic_rtetakes into account the value already written. Now that you only write the value at the end of the function, you should overwrite the affected half with "value" immediately before callingupdate_intremap_entry_from_ioapic().
Quoted 8 lines
Sorry, not quite understand your point. My thought is, no matter dom0 tried to
updates lower half or upper half of RTE, we always updates interrupt table from the lower half. This will keep iommu table strictly identically to RTE. The old code has an assumption that both lower half and upper of RTE should be updated together. But this might not be always true. If by incident, dom0 only updates the upper half and we don't sync iommu with it, then the destination in RTE and iommu table will be different.
No, that's not my point. The problem I'm seeing is that you pass the old value (as read from the IO-APIC) to update_intremap_entry_from_ioapic(), but the function certainly should use the to-be-written one. Previously this was implicit because the IO-APIC register write happened first.
Quoted 9 lines
Quoted 3 lines
Eliminating the double write if reg ==
rte_lowould also seem desirable (and in no case should you write back the old value after having calledupdate_intremap_entry_from_ioapic()).It not a write back, It just finishes IO-APIC RTE writes. After updating interrupt remapping table we still have to update RTE. It is just a copy of
__io_apic_write(maybe I should just call it). Old code updates ioapic earlier than interrupt remapping table and sata device might generate interrupt right after this, which is not expected.
No. If reg == ret_lo, you write that IO-APIC register twice, which is pointless. With the other problem unaddressed, you actually first write back the old value (with the mask bit restored), which gets IO-APIC and remapping tables out of sync for a brief period of time (which is a problem by itself), then write the new value. With the other problem addressed, you would simply write the new value twice, which is wasteful given that these writes are uncached.
Jan
Quoted 33 lines · On Friday 08 April 2011 16:39:13 Jan Beulich wrote:
Quoted 23 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 16:26, Wei Wang2 <[email protected]> wrote:
Quoted 13 lines · On Friday 08 April 2011 15:43:57 Jan Beulich wrote:
Quoted 12 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 13:35, Wei Wang2 <[email protected]> wrote:
Some device could generate bogus interrupts if an IO-APIC RTE and an iommu interrupt remapping entry are not consistent during 2 adjacent 64bits IO-APIC RTE updates. For example, if the 2nd operation updates destination bits in RTE for SATA device and unmask it, in some case, SATA device will assert ioapic pin to generate interrupt immediately using new destination but iommu could still translate it into the old destination, then dom0 would be confused. To fix that, we sync up interrupt remapping entry with IO-APIC IRE on every 32 bits operation and foward IOAPIC RTE updates after interrupt remapping table has been changed.
Quoted 6 lines
I don't think this is correct: Without the patch, the filling of
ioapic_rtetakes into account the value already written. Now that you only write the value at the end of the function, you should overwrite the
affected half with "value" immediately before callingupdate_intremap_entry_from_ioapic().Quoted 9 lines
Sorry, not quite understand your point. My thought is, no matter dom0 tried to
updates lower half or upper half of RTE, we always updates interrupt table from the lower half. This will keep iommu table strictly identically to RTE. The old code has an assumption that both lower half and upper of RTE should be updated together. But this might not be always true. If by incident, dom0 only updates the upper half and we don't sync iommu with it, then the destination in RTE and iommu table will be different.
Quoted 5 lines
No, that's not my point. The problem I'm seeing is that you pass the old value (as read from the IO-APIC) to
update_intremap_entry_from_ioapic(), but the function certainly should use the to-be-written one. Previously this was implicit because the IO-APIC register write happened first.
OK, got it. That is definitely problematic. will fix it.
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Eliminating the double write if reg ==
rte_lowould also seem desirable (and in no case should you write back the old value after having calledupdate_intremap_entry_from_ioapic()).It not a write back, It just finishes IO-APIC RTE writes. After updating interrupt remapping table we still have to update RTE. It is just a copy of
__io_apic_write(maybe I should just call it). Old code updates ioapic earlier than interrupt remapping table and sata device might generate interrupt right after this, which is not expected.
Quoted 7 lines
No. If reg ==
ret_lo, you write that IO-APIC register twice, which is pointless. With the other problem unaddressed, you actually first write back the old value (with the mask bit restored), which gets IO-APIC and remapping tables out of sync for a brief period of time (which is a problem by itself), then write the new value. With the other problem addressed, you would simply write the new value twice, which is wasteful given that these writes are uncached.
True. I will rework the patch try to eliminate this. Thanks
Wei
Quoted 1 line
Jan
PATCH 1/1 AMD IOMMU: Fix an interrupt remapping issue (v2)
Jan, How dose this one look like to you?
Thanks,
Wei
Signed-off-by Wei Wang <[email protected]>
--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim,
Landkreis München Registergericht München,
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd
Quoted 64 lines · On Friday 08 April 2011 17:06:16 Wei Wang2 wrote:
Quoted 33 lines · On Friday 08 April 2011 16:39:13 Jan Beulich wrote:
Quoted 23 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 16:26, Wei Wang2 <[email protected]> wrote:
Quoted 13 lines · On Friday 08 April 2011 15:43:57 Jan Beulich wrote:
Quoted 12 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 13:35, Wei Wang2 <[email protected]> wrote:
Some device could generate bogus interrupts if an IO-APIC RTE and an iommu interrupt remapping entry are not consistent during 2 adjacent 64bits IO-APIC RTE updates. For example, if the 2nd operation updates destination bits in RTE for SATA device and unmask it, in some case, SATA device will assert ioapic pin to generate interrupt immediately using new destination but iommu could still translate it into the old destination, then dom0 would be confused. To fix that, we sync up interrupt remapping entry with IO-APIC IRE on every 32 bits operation and foward IOAPIC RTE updates after interrupt remapping table has been changed.
Quoted 6 lines
I don't think this is correct: Without the patch, the filling of
ioapic_rtetakes into account the value already written. Now that you only write the value at the end of the function, you should overwrite the
affected half with "value" immediately before callingupdate_intremap_entry_from_ioapic().Quoted 9 lines
Sorry, not quite understand your point. My thought is, no matter dom0 tried to
updates lower half or upper half of RTE, we always updates interrupt table from the lower half. This will keep iommu table strictly identically to RTE. The old code has an assumption that both lower half and upper of RTE should be updated together. But this might not be always true. If by incident, dom0 only updates the upper half and we don't sync iommu with it, then the destination in RTE and iommu table will be different.Quoted 5 lines
No, that's not my point. The problem I'm seeing is that you pass the old value (as read from the IO-APIC) to
update_intremap_entry_from_ioapic(), but the function certainly should use the to-be-written one. Previously this was implicit because the IO-APIC register write happened first.OK, got it. That is definitely problematic. will fix it.
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Eliminating the double write if reg ==
rte_lowould also seem desirable (and in no case should you write back the old value after having calledupdate_intremap_entry_from_ioapic()).It not a write back, It just finishes IO-APIC RTE writes. After updating interrupt remapping table we still have to update RTE. It is just a copy of
__io_apic_write(maybe I should just call it). Old code updates ioapic earlier than interrupt remapping table and sata device might generate interrupt right after this, which is not expected.Quoted 7 lines
No. If reg ==
ret_lo, you write that IO-APIC register twice, which is pointless. With the other problem unaddressed, you actually first write back the old value (with the mask bit restored), which gets IO-APIC and remapping tables out of sync for a brief period of time (which is a problem by itself), then write the new value. With the other problem addressed, you would simply write the new value twice, which is wasteful given that these writes are uncached.True. I will rework the patch try to eliminate this. Thanks
Wei
Quoted 6 lines
Quoted 1 line
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
xen/drivers/passthrough/amd/iommu_intr.c +64 -51
@@ -117,8 +117,7 @@ static void update_intremap_entry_from_istatic 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_remappspin_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,14 @@ 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 ; + goto done;/* get device id of ioapic devices */ bdf = ioapic_bdf[IO_APIC_ID (apic)];
@@ -235,30 +231,47 @@ 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 ); + goto done; + } + +/* Save io-apic rte lower 32 bits */ + *IO_APIC_BASE (apic) = rte_lo; + *((u32 *)&old_rte) = *(IO_APIC_BASE (apic) +4 ); + saved_mask = old_rte.mask; + +if ( reg == rte_lo ) + { + *((u32 *)&new_rte) = value; +/* read upper 32 bits from io-apic rte */ + *IO_APIC_BASE (apic) = reg +1 ; + *(((u32 *)&new_rte) +1 ) = *(IO_APIC_BASE (apic) +4 ); + } +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); + old_rte.mask =1 ; + *IO_APIC_BASE (apic) = rte_lo; + *(IO_APIC_BASE (apic) +4 ) = *((u32 *)&old_rte); + +/* Update interrupt remapping entry */ +update_intremap_entry_from_ioapic (bdf, iommu, &new_rte); + +/* Update IO-APIC directly to avoid double writes */ +if ( reg == rte_lo ) + goto done;/* unmask the interrupt after we have updated the intremap table */ - *IO_APIC_BASE (apic) = reg; - *(IO_APIC_BASE (apic)+4 ) = *(((u32 *)&ioapic_rte)+0 ); + old_rte.mask = saved_mask; + *IO_APIC_BASE (apic) = rte_lo; + *(IO_APIC_BASE (apic) +4 ) = *((u32 *)&old_rte); + +done: +/* Forward write access to IO-APIC */ +__io_apic_write (apic, reg, value); } static void update_intremap_entry_from_msi_msg(
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Re: [PATCH] AMD IOMMU: Fix an interrupt remapping issue (v2)
Quoted 2 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 18:52, Wei Wang2 <[email protected]> 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
Re: [PATCH] AMD IOMMU: Fix an interrupt remapping issue (v2)
Quoted 7 lines · On Monday 11 April 2011 09:23:38 Jan Beulich wrote:
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 18:52, Wei Wang2 <[email protected]> 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).
Quoted 7 lines
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
Good points. I will send a new version addressing the remaining issues. Thanks,
Wei
PATCH v3 1/1 AMD IOMMU: Fix an interrupt remapping issue
In reply to Jan BeulichJan,
This patch is the latest. Should have addressed all of your concerns. Please take a look.
Thanks,
Wei
Quoted 7 lines · On Monday 11 April 2011 09:23:38 Jan Beulich wrote:
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 18:52, Wei Wang2 <[email protected]> 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).
Quoted 7 lines
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
xen/drivers/passthrough/amd/iommu_intr.c +69 -51
@@ -117,8 +117,7 @@ static void update_intremap_entry_from_istatic 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_remappspin_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(
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 11.04.11 at 12:31, Wei Wang2 <[email protected]> wrote:
This patch is the latest. Should have addressed all of your concerns. Please take a look.
Looks good to me now. Assuming it also still addresses the problem at hand:
Thanks, Jan
Quoted 20 lines
Thanks,
WeiSigned-off-by Wei Wang <[email protected]>Quoted 14 lines · On Monday 11 April 2011 09:23:38 Jan Beulich wrote:
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 18:52, Wei Wang2 <[email protected]> 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
Is this a candidate for backporting to 4.1?
-George
Quoted 39 lines · Jan Beulich
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 11.04.11 at 12:31, Wei Wang2 <[email protected]> wrote:
This patch is the latest. Should have addressed all of your concerns. Please take a look.
Looks good to me now. Assuming it also still addresses the problem at hand:
Acked-by Jan Beulich <[email protected]>Thanks, Jan
Quoted 20 lines
Thanks,
WeiSigned-off-by Wei Wang <[email protected]>Quoted 14 lines · On Monday 11 April 2011 09:23:38 Jan Beulich wrote:
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 18:52, Wei Wang2 <[email protected]> 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
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Oops, n/m -- already in. :-)
-G
Quoted 45 lines · George Dunlap
Is this a candidate for backporting to 4.1?
-George
Quoted 39 lines · Jan Beulich
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 11.04.11 at 12:31, Wei Wang2 <[email protected]> wrote:
This patch is the latest. Should have addressed all of your concerns. Please take a look.
Looks good to me now. Assuming it also still addresses the problem at hand:
Acked-by Jan Beulich <[email protected]>Thanks, Jan
Quoted 20 lines
Thanks,
WeiSigned-off-by Wei Wang <[email protected]>Quoted 14 lines · On Monday 11 April 2011 09:23:38 Jan Beulich wrote:
Quoted 3 lines
Quoted 1 line
Quoted 1 line · On 08.04.11 at 18:52, Wei Wang2 <[email protected]> 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
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel