Re: [Xen-devel] [PATCH] AMD IOMMU: only translate remapped IO-APIC RTEs

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Apr 23, 2015 at 05:51:05PM +0000, Suthikulpanit, Suravee wrote:
> 
> 
> On 4/23/15, 08:47, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
> >>>> On 23.04.15 at 15:31, <Suravee.Suthikulpanit@xxxxxxx> wrote:
> >
> >> 
> >> On 4/17/15, 10:27, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >> 
> >>>1aeb1156fa ("x86 don't change affinity with interrupt unmasked")
> >>>introducing RTE reads prior to the respective interrupt having got
> >>>enabled for the first time uncovered a bug in 2ca9fbd739 ("AMD IOMMU:
> >>>allocate IRTE entries instead of using a static mapping"): We obviously
> >>>shouldn't be translating RTEs for which remapping didn't get set up
> >>>yet.
> >>>
> >>>Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
> >>>Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>
> >>>--- a/xen/drivers/passthrough/amd/iommu_intr.c
> >>>+++ b/xen/drivers/passthrough/amd/iommu_intr.c
> >>>@@ -365,15 +365,17 @@ unsigned int amd_iommu_read_ioapic_from_
> >>>     unsigned int apic, unsigned int reg)
> >>> {
> >>>     unsigned int val = __io_apic_read(apic, reg);
> >>>+    unsigned int pin = (reg - 0x10) / 2;
> >>>+    unsigned int offset = ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx[pin];
> >>> 
> >>>-    if ( !(reg & 1) )
> >>>+    if ( !(reg & 1) && offset < INTREMAP_ENTRIES )
> >>>     {
> >>>-        unsigned int offset = val & (INTREMAP_ENTRIES - 1);
> >>>         u16 bdf = ioapic_sbdf[IO_APIC_ID(apic)].bdf;
> >>>         u16 seg = ioapic_sbdf[IO_APIC_ID(apic)].seg;
> >>>         u16 req_id = get_intremap_requestor_id(seg, bdf);
> >>>         const u32 *entry = get_intremap_entry(seg, req_id, offset);
> >>> 
> >>>+        ASSERT(offset == (val & (INTREMAP_ENTRIES - 1)));
> >> 
> >> Jan, could you please explain why the ASSERT is needed here?
> >
> >The previous value "offset" got assigned was calculated using
> >the right side expression. I.e. the assert makes sure that what
> >we used before and what we use now is the same.
> >
> >Jan
> 
> Jan,
> 
> I have tested this patch w/ staging branch booting Dom0, and this patch
> got rid of the following error from xl dmesg:
> (XEN) APIC error on CPU0: 00(40)
> (XEN) APIC error on CPU2: 00(40)
> 
> However, when I tried starting a guest w/ PCI device passthrough, the
> system crashed and reboot. Although, I don¹t think this is caused by the
> patch. 

Is it by any chance " BUG at iommu_map.c:455" ?

> 
> Acked-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> 
> I¹m looking into the issue. I also went back and tested it with 4.5 on the
> same setup and didn¹t see the issue.
> 
> 
> Thanks,
> Suravee
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel