Re: [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support

[Julien Grall <julien.grall@citrix.com>]

On 22/09/15 11:44, Ian Campbell wrote:
> On Tue, 2015-09-22 at 11:29 +0100, Julien Grall wrote:
>>
>> On 22/09/2015 11:05, Ian Campbell wrote:
>>> On Tue, 2015-09-22 at 10:45 +0100, Julien Grall wrote:
>>>>
>>>> On 22/09/2015 10:17, Vijay Kilari wrote:
>>>>>> Why do you need a command line option to enable/disable the
>>>>>> physical
>>>>>> ITS?
>>>>>
>>>>> I have added this to remove ITS support?.
>>>>> Did I misunderstood it. May be I have to avoid generating its dt
>>>>> node
>>>>> to disable its for dom0?
>>>>
>>>> My question is why do you want to let the user disabling the ITS
>>>> using
>>>> the command line? What's the use case?
>>>
>>> It's always useful to be able to nobble this sort of thing, either for
>>> debugging/development purposes or to allow users to workaround issues.
>>
>> This is can be done via the firmware table (see the property "status" in 
>> the DT).
> 
> This is not always trivial to override (think DTB provided by the UEFI
> firmware).

Hmmm, right. I always have in mind the U-boot case and forgot the UEFI one.

>>  I see no advantage to use the command line for a such purpose, 
>> mainly for the debugging/development point.
>>
>> Also what kind of workaround do you expect to be fixed by disabling ITS? 
> 
> Any bug relating to running on an ITS which a user trips over in the field.

TBH, I asked this question because it has been introduced in this
version without any explanation. Maybe I'm too picky but I think it's
important to know what the usage of a new option.

Aside that, just reading the code, I can tell that this parameter can't
even work on Thunder-X.

With its_enable=false, the platform code (see patch #28) will fail to
add the PCI and therefore return an error which will make Xen to fail
building DOM0.

> This isn't really any different from the myriad of options which already
> exist to disable particular hardware features lists in 
> http://xenbits.xen.org/docs/unstable/misc/xen-command-line.html (many of
> which are x86 specific, including I notice now msi=<boolean>)
>> I know that it will disable all the PCI MSI in general. But should not 
>> it be done with a PCI related parameter?
> 
> Maybe there should be a PCI option as well, but disabling PCI MSI should
> inherit from the disabling of the ITS. Just as it should be disabled if the
> ITS isn't present at all.

While today we support MSI only with ITS, we will have at some point
GICv2m and maybe new interrupt controllers.

I would much prefer having a common parameter (such a "msi=...") to
disable MSI on the platform rather adding a new parameter for each new
interrupt controller using MSI.

It much easier to tell the user "please disable msi" rather than "please
check what interrupt controller you are using" and after another round
of mail "please use this option".

> 
>>> Assuming the switch is simple an reasonably self contained (and it really
>>> should be, since it should just be checked at start of day and then arrange
>>> to behave like no ITS is present) then I can't see why not to have it.
>>
>> I think that disabling the ITS is more complex than not using in Xen. 
>> This will be tied with PCI passthrough very soon and we will have to 
>> spread this decision everywhere.
>>
>> We would also have to remove everything related to ITS (i.e 
>> msi-parent...) to the DT to avoid to pass an half-valid DT to DOM0. 
>> Otherwise we may just not be able to boot or using PCI (even without
>> MSI).
> 
> So this might fall into the "assuming the switch is simple" caveat which I
> mentioned, although I'm not convinced we need to go this far. We could
> equally well just mark the ITS node disabled and leave everything else
> alone.

Today the ITS driver in Linux doesn't check if the ITS node is disabled
or not.

If we don't take into account this fact, the ITS node are creating from
scratch for DOM0. If the vITS is not present there will no region to
create it. So adding fake node maybe more complex than just having a DT
partially valid and hoping the guest doing the right thing.

The behavior chosen should be documented in the
docs/misc/xen-command-line.markdown to make clear what is expected with
this option turned on.

Regards,

-- 
Julien Grall