Re: [Xen-devel] [PATCH] iommu: leave IOMMU enabled by default during kexec crash transition

[Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>]

On 19/02/2019 08:13, Jan Beulich wrote:
>>>> On 18.02.19 at 17:21, <igor.druzhinin@xxxxxxxxxx> wrote:
> 
> First of all - please follow patch submission rules: They get sent _to_
> the list, with maintainers and others _cc_-ed.
> 
>> It's unsafe to disable IOMMU on a live system which is the case
>> if we're crashing since remapping hardware doesn't usually know what
>> to do with ongoing bus transactions and frequently raises NMI/MCE/SMI,
>> etc. (depends on the firmware configuration) to signal these abnormalities.
>> This, in turn, doesn't play well with kexec transition process as there is
>> no any handling available at the moment for this kind of events resulting
>> in failures to enter the kernel.
>>
>> Modern Linux kernels taught to copy all the necessary DMAR/IR tables
>> following kexec from the previous kernel (Xen in our case) - so it's
>> currently normal to keep IOMMU enabled. It would only require to change
>> crash kernel command line by enabling IOMMU drivers from the existing users.
>>
>> An option is left for compatibility with ancient crash kernels which
>> didn't like to have IOMMU active under their feet on boot.
>>
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
>> ---
>>
>> Jan, Andrew, should we have this option here and, if so, what is the default
>> value for it should be?
> 
> I think the option should definitely be here (I can't even see what
> the alternative would be, as then the patch would be effectively
> be deletion of the iommu_crash_shutdown() invocation, just for a
> patch adding the option to re-instate it.
> 
> The default is more difficult to choose: Keeping the IOMMU on for
> an unaware crash kernel is perhaps about as bad as turning it off
> when the crash kernel would know how to deal with it.
> 
> Wouldn't it be possible to allow the kexec tool to specify the
> intended behavior via a (flag to a) hypercall? How is adding of the
> respective command line option controlled in the bare metal Linux
> case?

We certainly don't want to change the existing ABI but as to extending:
Could you give a usecase for kexec tool having such an additional
hypercall? What additional value to the command line option it would add?

I'm not following your last question. Could you elaborate?

>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1235,6 +1235,11 @@ boolean (e.g. `iommu=no`) can override this and leave 
>> the IOMMUs disabled.
>>      This option depends on `intremap`, and is disabled by default due to 
>> some
>>      corner cases in the implementation which have yet to be resolved.
>>  
>> +*   The `crash-shutdown` boolean controls shutting down IOMMU before 
>> switching
>> +    to a crash kernel through kexec. This option is inactive by default and
>> +    is for compatibility with older kexec kernels only as modern kernels 
>> copy
>> +    all the necessary tables from the previous kernel following kexec 
>> transition.
> 
> You also want to append to the "List of" at the top of this option's
> description.
> 

Will do.

>> --- a/xen/arch/x86/crash.c
>> +++ b/xen/arch/x86/crash.c
>> @@ -162,8 +162,9 @@ static void nmi_shootdown_cpus(void)
>>          printk("Failed to shoot down CPUs {%*pbl}\n",
>>                 nr_cpu_ids, cpumask_bits(&waiting_to_crash));
>>  
>> -    /* Crash shutdown any IOMMU functionality as the crashdump kernel is not
>> -     * happy when booting if interrupt/dma remapping is still enabled */
>> +    /* Try to crash shutdown IOMMU functionality as some old crashdump
>> +     * kernels are not happy when booting if interrupt/dma remapping
>> +     * is still enabled */
>>      iommu_crash_shutdown();
> 
> Please can you correct comment style here seeing the you
> re-write it anyway?
> 

Ok.

>> @@ -88,6 +89,8 @@ static int __init parse_iommu_param(const char *s)
>>              iommu_intremap = val;
>>          else if ( (val = parse_boolean("intpost", s, ss)) >= 0 )
>>              iommu_intpost = val;
>> +        else if ( (val = parse_boolean("crash-shutdown", s, ss)) >= 0 )
>> +            iommu_crash_shutdown_enable = val;
> 
> #ifdef CONFIG_KEXEC ?
> 
>> @@ -579,6 +582,9 @@ void iommu_share_p2m_table(struct domain* d)
>>  
>>  void iommu_crash_shutdown(void)
>>  {
>> +    if ( !iommu_crash_shutdown_enable )
>> +        return;
> 
> How to read this is very ambiguous - the way I've been reading it
> first ("enable IOMMU on crash shutdown") the condition seemed
> outright wrong. I think the command line option wants to be
> something like "iommu=crash-disable" and the variable then
> iommu_crash_disable (subject to further improvement if still
> considered potentially ambiguous).
> 

I can't see how "crash-shutdown" is ambiguous but if you prefer
"crash-disable" I'll rename it.

Igor

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel