Re: [Xen-devel] [RFC] Add the panic info when disable VT-d

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.03.15 at 03:32, <liang.z.li@xxxxxxxxx> wrote:
>> >>> On 19.01.15 at 10:00, <liang.z.li@xxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/apic.c
>> > +++ b/xen/arch/x86/apic.c
>> > @@ -915,6 +915,11 @@ void __init x2apic_bsp_setup(void)
>> >              return;
>> >          }
>> >          printk("x2APIC: Already enabled by BIOS: Ignoring cmdline
>> > disable.\n");
>> > +    } else {
>> > +        if ( !iommu_enable) {
>> > +            panic("x2APIC should be disabled while IOMMU is disabled,"
>> > +             "try to set x2apic=0 in cmdline and disable x2apic in BIOS");
>> > +        }
>> 
>> Putting aside the coding style violations (figure braces on their own lines, 
> no
>> hard tabs), you tie this to the wrong thing: You need interrupt remapping to
>> be enabled, whereas iommu_enable may only mean DMA remapping. And
>> I'm afraid you'd run into an ordering problem (iommu_intremap getting set
>> to its final value vs. the code above being run) if you tried to correct 
> this.
> 
> I don't understand the ordering problem you referred to, the patch is just 
> change the panic info, 
> and tell the user what they should do to avoid the panic, I didn't change 
> the logic of the current code.
> Without my patch, the current code will also print the panic info like this:
> 
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Couldn't enable IOMMU and iommu=required/force
> (XEN) ****************************************
>  
> It's odd because the user have set ' iommu=0', anyway, it should be fixed.

Not sure what you consider odd here - x2apic_bsp_setup() is
where force_iommu gets set to 1, i.e. the user specifying
"iommu=0" gets intentionally overridden in this case.

> How about change the panic info to this.
>  +    } else {
>  +        if ( !iommu_enable) {
>  +            panic("x2APIC should be disabled while iommu=0 is set,"
>  +                  "try to set x2apic=0 option and disable x2apic in BIOS to 
> avoid this");
>  +    }
> 
> or could you give some suggestion?

You only altered the text, not the condition. As said before, I think
this really should be keyed off of iommu_intremap. I can't exclude
that the existing logic isn't really correct either, but if that's the
case the solution is not to add another random panic() invocation,
but to fix that logic.

Jan


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