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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.02.19 at 13:08, <igor.druzhinin@xxxxxxxxxx> wrote:
> On 19/02/2019 08:13, Jan Beulich wrote:
>>>>> On 18.02.19 at 17:21, <igor.druzhinin@xxxxxxxxxx> wrote:
>>> 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?

Some entity needs to decide whether to add the respective command
line option to the crash kernel's command line. It should be this same
entity to tell Xen whether to keep the IOMMU enabled while invoking
the crash kernel. I am merely guessing that this entity is the kexec
tool.

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

I'm simply asking what the bare metal equivalent behavior is here,
i.e. how command line option addition and IOMMU state are being
kept in sync in that case. Just for reference, in the hope that what
they do is sane, and hence we could follow that model.

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

I've explained how I read this initially. The main problem I see here
is that "shutdown" in this situation has two meanings - the fact
that we shut down (the entire system) in preparation of invoking
the crash kernel (that's what in my understanding the function's
name was derived from), and "shutting down" (i.e. disabling) of
the IOMMU in this case (which in my understanding is what you've
based the option and symbol names upon).

Commonly we don't use "shut down" for an action on the IOMMU.
We "enable" or "disable" it. Hence my alternative name suggestion.

Jan



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