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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> --- 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.

> --- 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?

> @@ -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).

Since it looks as if we leave the IOMMU entirely untouched in the
non-crash-kexec case already, I also wonder whether an inverse
control for that case wouldn't have been desirable already in the
past.

Jan


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