Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue

[John Ernberg <john.ernberg@xxxxxxxx>]

Hi Julien,

On 2/9/24 14:14, John Ernberg wrote:
> Hi Julien,
> 
> Apologies for the delay, I was pulled away for a bit.
> 
> On 2/5/24 11:13, Julien Grall wrote:
>> Hi,
>>
>> On 04/02/2024 09:40, Peng Fan wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Julien Grall <julien@xxxxxxx>
>>>> Sent: 2024年2月2日 17:20
>>>> To: John Ernberg <john.ernberg@xxxxxxxx>; Stefano Stabellini
>>>> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <bertrand.marquis@xxxxxxx>;
>>>> Michal Orzel <michal.orzel@xxxxxxx>; Volodymyr Babchuk
>>>> <Volodymyr_Babchuk@xxxxxxxx>; Peng Fan <peng.fan@xxxxxxx>
>>>> Cc: Jonas Blixt <jonas.blixt@xxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue
>>>>
>>>> On 01/02/2024 16:17, John Ernberg wrote:
>>>>> On 2/1/24 13:20, Julien Grall wrote:
>>>>>>
>>>>>>
>>>>>> On 31/01/2024 15:32, John Ernberg wrote:
>>>>>>> Hi Julien,
>>>>>>
>>>>>> Hi John,
>>>>>>
>>>>>>> On 1/31/24 13:22, Julien Grall wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 31/01/2024 11:50, John Ernberg wrote:
> [ ... ]
>>>>>>
>>>>>>>> But even if we restrict to dom0, have you checked that none of the
>>>>>>>> SMCs use buffers?
>>>>>>> I haven't found any such instances in the Linux kernel where a
>>>>>>> buffer is used. Adding a call filtering like suggested below
>>>>>>> additions of such functions can be discovered and adapted for if 
>>>>>>> they
>>>> would show up later.
>>>>>>>>
>>>>>>>> Rather than providing a blanket forward, to me it sounds more like
>>>>>>>> you want to provide an allowlist of the SMCs. This is more
>>>>>>>> futureproof and avoid the risk to expose unsafe SMCs to any domain.
>>>>>>>>
>>>>>>>> For an example, you can have a look at the EEMI mediator for 
>>>>>>>> Xilinx.
>>>>>>>
>>>>>>> Ack. Do you prefer to see only on SMCCC service level or also on
>>>>>>> function level? (a1 register, per description earlier)
>>>>>>
>>>>>> I am not sure. It will depend on whether it is correct to expose
>>>>>> *all* the functions within a service level and they have the same 
>>>>>> format.
>>>>>>
>>>>>> If you can't guarantee that, then you will most likely need to
>>>>>> allowlist at the function level.
>>>>>>
>>>>>> Also, do you have a spec in hand that would help to understand which
>>>>>> service/function is implemented via those SMCs?
>>>>>
>>>>> I don't have the spec unfortunately, but I will add a filter on both
>>>>> service and function for V2 and we'll take it from there.
>>>>
>>>> @Peng, do you have any specification you could share? How stable is the
>>>> interface?
>>>
>>> No specification, the use is IMX_SIP_X in linux kernel source.
>>>
>>> Such as IMX_SIP_RTC, IMX_SIP_TIMER
>>>
>>> It is stable and no change, we only add new SIP macro if needed
>>> and no change the meaning or reuse old SIP.
>>
>> Thanks for the answer. It is a bit unfortunate there are no 
>> specification, but at least they are stable.
>>
>> I have searched IMX_SIP in Linux, there doesn't seem many so we could 
>> allowlist them (see more below). Do you know if there are more 
>> necessary that are not yet upstreamed in Linux?
> 
> I took a dive into both upstream kernel and the vendor tree and found 
> the following list and for which SoCs they are applicable (Please 
> correct me if you can Peng)
> 
> 0x82000006 IMX_SIP_BUSFREQ_CHANGE [unsure, probably not imx8]
> 0xC2000000 IMX_SIP_GPC [only imx8m series]
> 0xC2000001 IMX_SIP_CPUFREQ [only imx8q{x,m} series]
> 0xC2000002 IMX_SIP_SRTC / IMX_SIP_TIMER [only imx8q{x,m} series]
> 0xC2000004 IMX_SIP_DDR_DVFS [only imx8m series]
> 0xC2000005 IMX_SIP_RPROC / IMX_SIP_SRC [only imx8m series]
> 0xC2000006 IMX_SIP_GET_SOC_INFO [only imx8m series]
> 0xC2000008 IMX_SIP_NOC [only imx8m series]
> 0xC2000009 IMX_SIP_WAKEUP_SRC [only imx8q{x,m} series]
> 0xC200000B IMX_SIP_OTP_WRITE [only imx8q{x,m} series]
>>
>>

[ ... ]

> 
>>    * IMX_SIP_TIMER_*:  This seems to be related to the watchdog. 
>> Shouldn't dom0 rely on the watchdog provided by Xen instead? So those 
>> call will be used by Xen.
> 
> That is indeed a watchdog SIP, and also for setting the SoC internal RTC
> if it is being used.
> 
> I looked around if there was previous discussion and only really found [3].
> Is the xen/xen/include/watchdog.h header meant to be for this kind of 
> watchdog support or is that more for the VM watchdog? Looking at the x86 
> ACPI NMI watchdog it seems like the former, but I have never worked with 
> x86 nor ACPI.
> 
> Currently forwarding it to Dom0 has not caused any watchdog resets with 
> our watchdog timeout settings, our specific Dom0 setup and VM count.
> 
> For the remaining bits:
> 
> The wakeup src is not in the upstream kernel yet and related to system 
> resume from suspend which isn't supported in Xen on ARM yet - so this we 
> can block safely.
> 
> The OTP write is fuse programming in the SoC fuse banks, and should 
> probably be allowed but reserved for Dom0, as this can set fuses that
> affects the SoC boot.
> 
>>
>> Also, what happen if we don't expose those SMC to dom0?
>>
>> Cheers,
>>
> 
> [1]: 
> https://lore.kernel.org/xen-devel/1510247421-24094-1-git-send-email-olekstysh@xxxxxxxxx/
> [2]: 
> https://www.slideshare.net/xen_com_mgr/xpdds18-cpufreq-in-xen-on-arm-oleksandr-tyshchenko-epam-systems
> [3]: https://xen-users.narkive.com/ISXnlmB0/watchdog-support-in-xen
> 
> Thanks! // John Ernberg

Ping on the watchdog discussion bits.

Thanks! // John Ernberg