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

[Sander Eikelenboom <linux@xxxxxxxxxxxxxx>]

Friday, April 24, 2015, 12:12:32 AM, you wrote:



> On 4/23/15, 12:59, "Sander Eikelenboom" <linux@xxxxxxxxxxxxxx> 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. 
>>
>>> 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
>>
>>If it crashes with:
>>
>>
>>That problem is likely solved by another patch:
>>
>>http://lists.xen.org/archives/html/xen-devel/2015-04/msg02253.html
>>

> Yes, this has fixed the crash.

> Thanks,

> Suravee


Hi Jan,

I see you commited "AMD IOMMU: only translate remapped IO-APIC RTEs" to staging,
any reason why your patch in 
http://lists.xen.org/archives/html/xen-devel/2015-04/msg02253.html
isn't commited yet ? 

(still waiting for an formal ack from someone ? as Suravee also seems to have 
tested it)

--
Sander


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