Re: [PATCH v6 5/7] xen/arm: do not map IRQs and memory for disabled devices

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Julien!

On 16.11.21 21:22, Julien Grall wrote:
> Hi Oleksandr,
>
> On 05/11/2021 06:33, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> Currently Xen maps all IRQs and memory ranges for all devices except
>> those marked for passthrough, e.g. it doesn't pay attention to the
>> "status" property of the node.
>>
>> According to the device tree specification [1]:
>>   - "okay"     Indicates the device is operational.
>>   - "disabled" Indicates that the device is not presently operational,
>>                but it might become operational in the future (for example,
>>           something is not plugged in, or switched off).
>>           Refer to the device binding for details on what disabled means
>>           for a given device.
>>
>> So, "disabled" status is device dependent and mapping should be taken by
>> case-by-case approach with that respect. Although in general Xen should map
>> IRQs and memory ranges as the disabled devices might become operational
>
> Right, this change effectively prevent dom0 to use such device if it becomes 
> operational in the future.
Or doesn't, see below
> So this sounds like a big regression.
>
> How do you plan to handle it?
It depends if this is really a regression or is ok and "by design"
>
>> it
>> makes it impossible for the other devices, which are not operational in
>> any case, to skip the mappings.
>
> You wrote "makes it impossible for the other devices", but it is not clear to 
> me what's would go wrong when we map a disabled device (Dom0 should not touch 
> it). Do you have an example?
Ok, here we go. In the SoC I am working with the PCIe controller may run in two 
modes:
Root or Endpoint. Not configurable at run-time, so you configure it in the 
device tree.
The are two device tree entries for that: for the root [1] and for the endpoint 
[2].
So, when you want the controller to be a Root Complex then you put status = 
"disabled"
for the Endpoint entry and vise versa.

If you take a look at the nodes they both define the same "reg" and 
"interrupts",
effectively making it impossible for me in the patch [3] to actually trap MMIOs:
we skip the mappings for [1] and then, because we assume "disabled" is
still valid for mappings, we add those for [2].

So, this is probably why device tree documentation says about the disabled 
status
to be device specific.

Hope this describes a very valid use-case. At the same time you may argue that
I just need to remove the offending entry from the device tree which may also be
valid.
>
> Cheers,
>
Thank you,
Oleksandr

[1] 
https://github.com/renesas-rcar/linux-bsp/blob/v5.10.41/rcar-5.1.3.rc6/arch/arm64/boot/dts/renesas/r8a779f0.dtsi#L843
[2] 
https://github.com/renesas-rcar/linux-bsp/blob/v5.10.41/rcar-5.1.3.rc6/arch/arm64/boot/dts/renesas/r8a779f0.dtsi#L896
[3] 
https://patchwork.kernel.org/project/xen-devel/patch/20211105063326.939843-5-andr2000@xxxxxxxxx/