Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
1 2  View All
[PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue
john.ernberg at actia
Jan 31, 2024, 3:50 AM
Post #1 of 27 (103 views)
Permalink
When using Linux for dom0 there are a bunch of drivers that need to do SMC
SIP calls into the PSCI provider to enable certain hardware bits like the
watchdog.

Provide a basic platform glue that implements the needed SMC forwarding.

Signed-off-by: John Ernberg <john.ernberg@actia.se>
---
NOTE: This is based on code found in NXP Xen tree located here:
https://github.com/nxp-imx/imx-xen/blob/lf-5.10.y_4.13/xen/arch/arm/platforms/imx8qm.c

xen/arch/arm/platforms/Makefile | 1 +
xen/arch/arm/platforms/imx8qm.c | 65 +++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
create mode 100644 xen/arch/arm/platforms/imx8qm.c

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 8632f4115f..bec6e55d1f 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT) += sunxi.o
obj-$(CONFIG_ALL64_PLAT) += thunderx.o
obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
+obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp.o
obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp-eemi.o
diff --git a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c
new file mode 100644
index 0000000000..a9cd9c3615
--- /dev/null
+++ b/xen/arch/arm/platforms/imx8qm.c
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * xen/arch/arm/platforms/imx8qm.c
+ *
+ * i.MX 8QM setup
+ *
+ * Copyright (c) 2016 Freescale Inc.
+ * Copyright 2018-2019 NXP
+ *
+ *
+ * Peng Fan <peng.fan@nxp.com>
+ */
+
+#include <asm/platform.h>
+#include <asm/smccc.h>
+
+static const char * const imx8qm_dt_compat[] __initconst =
+{
+ "fsl,imx8qm",
+ "fsl,imx8qxp",
+ NULL
+};
+
+static bool imx8qm_smc(struct cpu_user_regs *regs)
+{
+ struct arm_smccc_res res;
+
+ if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )
+ {
+ printk_once(XENLOG_WARNING "no SMCCC 1.1 support. Disabling firmware calls\n");
+
+ return false;
+ }
+
+ arm_smccc_1_1_smc(get_user_reg(regs, 0),
+ get_user_reg(regs, 1),
+ get_user_reg(regs, 2),
+ get_user_reg(regs, 3),
+ get_user_reg(regs, 4),
+ get_user_reg(regs, 5),
+ get_user_reg(regs, 6),
+ get_user_reg(regs, 7),
+ &res);
+
+ set_user_reg(regs, 0, res.a0);
+ set_user_reg(regs, 1, res.a1);
+ set_user_reg(regs, 2, res.a2);
+ set_user_reg(regs, 3, res.a3);
+
+ return true;
+}
+
+PLATFORM_START(imx8qm, "i.MX 8")
+ .compatible = imx8qm_dt_compat,
+ .smc = imx8qm_smc,
+PLATFORM_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.43.0
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
julien at xen
Jan 31, 2024, 4:22 AM
Post #2 of 27 (103 views)
Permalink
Hi,

On 31/01/2024 11:50, John Ernberg wrote:
> When using Linux for dom0 there are a bunch of drivers that need to do SMC
> SIP calls into the PSCI provider to enable certain hardware bits like the
> watchdog.

Do you know which protocol this is under the hood. Is this SCMI?

>
> Provide a basic platform glue that implements the needed SMC forwarding.
>
> Signed-off-by: John Ernberg <john.ernberg@actia.se>
> ---
> NOTE: This is based on code found in NXP Xen tree located here:
> https://github.com/nxp-imx/imx-xen/blob/lf-5.10.y_4.13/xen/arch/arm/platforms/imx8qm.c

Anything after --- will be removed while applied to the three. I think
this NOTE should be written down in the commit message.

You also possibly want a signed-off-by from Peng as this is his code.

>
> xen/arch/arm/platforms/Makefile | 1 +
> xen/arch/arm/platforms/imx8qm.c | 65 +++++++++++++++++++++++++++++++++
> 2 files changed, 66 insertions(+)
> create mode 100644 xen/arch/arm/platforms/imx8qm.c
>
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 8632f4115f..bec6e55d1f 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT) += sunxi.o
> obj-$(CONFIG_ALL64_PLAT) += thunderx.o
> obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
> obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
> +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
> obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp.o
> obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c
> new file mode 100644
> index 0000000000..a9cd9c3615
> --- /dev/null
> +++ b/xen/arch/arm/platforms/imx8qm.c
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/platforms/imx8qm.c
> + *
> + * i.MX 8QM setup
> + *
> + * Copyright (c) 2016 Freescale Inc.
> + * Copyright 2018-2019 NXP
> + *
> + *
> + * Peng Fan <peng.fan@nxp.com>
> + */
> +
> +#include <asm/platform.h>
> +#include <asm/smccc.h>
> +
> +static const char * const imx8qm_dt_compat[] __initconst =
> +{
> + "fsl,imx8qm",
> + "fsl,imx8qxp",
> + NULL
> +};
> +
> +static bool imx8qm_smc(struct cpu_user_regs *regs)
> +{

Your implementation below will not only forward SMC for dom0 but also
for any non-trusted domains. Have you investigated that all the SIP
calls are safe to be called by anyone?

But even if we restrict to dom0, have you checked that none of the SMCs
use buffers?

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.

Cheers,

--
Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Jan 31, 2024, 7:32 AM
Post #3 of 27 (103 views)
Permalink
Hi Julien,

On 1/31/24 13:22, Julien Grall wrote:
> Hi,
>
> On 31/01/2024 11:50, John Ernberg wrote:
>> When using Linux for dom0 there are a bunch of drivers that need to do
>> SMC
>> SIP calls into the PSCI provider to enable certain hardware bits like the
>> watchdog.
>
> Do you know which protocol this is under the hood. Is this SCMI?

I think I confused myself here when I wrote the commit log.

The EL3 code in our case is ATF, and it does not appear to be SCMI, nor
PSCI. The register usage of these SMC SIP calls are as follows:
a0 - service
a1 - function
a2-a7 - args

In ATF the handler is declared as a runtime service.

Would the appropriate commmit message here be something along the lines
of below?
"""
When using Linux for dom0 there are a bunch of drivers that need to do SMC
SIP calls into the firmware to enable certain hardware bits like the
watchdog.
"""
>
>>
>> Provide a basic platform glue that implements the needed SMC forwarding.
>>
>> Signed-off-by: John Ernberg <john.ernberg@actia.se>
>> ---
>> NOTE: This is based on code found in NXP Xen tree located here:
>> https://github.com/nxp-imx/imx-xen/blob/lf-5.10.y_4.13/xen/arch/arm/platforms/imx8qm.c
>
> Anything after --- will be removed while applied to the three. I think
> this NOTE should be written down in the commit message.

Ack.
>
> You also possibly want a signed-off-by from Peng as this is his code.

@Peng: May I add a sign-off from you?
>
>>
>>   xen/arch/arm/platforms/Makefile |  1 +
>>   xen/arch/arm/platforms/imx8qm.c | 65 +++++++++++++++++++++++++++++++++
>>   2 files changed, 66 insertions(+)
>>   create mode 100644 xen/arch/arm/platforms/imx8qm.c
>>
>> diff --git a/xen/arch/arm/platforms/Makefile
>> b/xen/arch/arm/platforms/Makefile
>> index 8632f4115f..bec6e55d1f 100644
>> --- a/xen/arch/arm/platforms/Makefile
>> +++ b/xen/arch/arm/platforms/Makefile
>> @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT)   += sunxi.o
>>   obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>>   obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
>>   obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
>> +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
>>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
>> diff --git a/xen/arch/arm/platforms/imx8qm.c
>> b/xen/arch/arm/platforms/imx8qm.c
>> new file mode 100644
>> index 0000000000..a9cd9c3615
>> --- /dev/null
>> +++ b/xen/arch/arm/platforms/imx8qm.c
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * xen/arch/arm/platforms/imx8qm.c
>> + *
>> + * i.MX 8QM setup
>> + *
>> + * Copyright (c) 2016 Freescale Inc.
>> + * Copyright 2018-2019 NXP
>> + *
>> + *
>> + * Peng Fan <peng.fan@nxp.com>
>> + */
>> +
>> +#include <asm/platform.h>
>> +#include <asm/smccc.h>
>> +
>> +static const char * const imx8qm_dt_compat[] __initconst =
>> +{
>> +    "fsl,imx8qm",
>> +    "fsl,imx8qxp",
>> +    NULL
>> +};
>> +
>> +static bool imx8qm_smc(struct cpu_user_regs *regs)
>> +{
>
> Your implementation below will not only forward SMC for dom0 but also
> for any non-trusted domains. Have you investigated that all the SIP
> calls are safe to be called by anyone?

We use pure virtualized domUs, so we do not expect any calls to this SMC
interface from the guest. I'll limit it to dom0.
>
> 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)
>
> Cheers,
>

Thanks! // John Ernberg
RE: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
peng.fan at nxp
Jan 31, 2024, 8:10 PM
Post #4 of 27 (102 views)
Permalink
> Cc: Jonas Blixt <jonas.blixt@actia.se>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue
>
> Hi Julien,
>
> On 1/31/24 13:22, Julien Grall wrote:
> > Hi,
> >
> > On 31/01/2024 11:50, John Ernberg wrote:
> >> When using Linux for dom0 there are a bunch of drivers that need to
> >> do SMC SIP calls into the PSCI provider to enable certain hardware
> >> bits like the watchdog.
> >
> > Do you know which protocol this is under the hood. Is this SCMI?
>
> I think I confused myself here when I wrote the commit log.
>
> The EL3 code in our case is ATF, and it does not appear to be SCMI, nor PSCI.
> The register usage of these SMC SIP calls are as follows:
> a0 - service
> a1 - function
> a2-a7 - args
>
> In ATF the handler is declared as a runtime service.
>
> Would the appropriate commmit message here be something along the lines
> of below?
> """
> When using Linux for dom0 there are a bunch of drivers that need to do
> SMC
> SIP calls into the firmware to enable certain hardware bits like the watchdog.
> """
> >
> >>
> >> Provide a basic platform glue that implements the needed SMC forwarding.
> >>
> >> Signed-off-by: John Ernberg <john.ernberg@actia.se>
> >> ---
> >> NOTE: This is based on code found in NXP Xen tree located here:
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> >> hub.com%2Fnxp-imx%2Fimx-xen%2Fblob%2Flf-
> 5.10.y_4.13%2Fxen%2Farch%2Far
> >>
> m%2Fplatforms%2Fimx8qm.c&data=05%7C02%7Cpeng.fan%40nxp.com%7C
> 573b599a
> >>
> 4b4143ceca1d08dc2271e5be%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7
> >>
> C638423119777601548%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQI
> >>
> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=ZO
> 0TXjL6
> >> g0W7TIZo8x8lTNBXEZW%2BDNcLPndWlEf5D2A%3D&reserved=0
> >
> > Anything after --- will be removed while applied to the three. I think
> > this NOTE should be written down in the commit message.
>
> Ack.
> >
> > You also possibly want a signed-off-by from Peng as this is his code.
>
> @Peng: May I add a sign-off from you?

Yeah. You could add my sign off.

> >
> >>
> >> ? xen/arch/arm/platforms/Makefile |? 1 +
> >> ? xen/arch/arm/platforms/imx8qm.c | 65
> >> +++++++++++++++++++++++++++++++++
> >> ? 2 files changed, 66 insertions(+)
> >> ? create mode 100644 xen/arch/arm/platforms/imx8qm.c
> >>
> >> diff --git a/xen/arch/arm/platforms/Makefile
> >> b/xen/arch/arm/platforms/Makefile index 8632f4115f..bec6e55d1f
> 100644
> >> --- a/xen/arch/arm/platforms/Makefile
> >> +++ b/xen/arch/arm/platforms/Makefile
> >> @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT)?? += sunxi.o
> >> ? obj-$(CONFIG_ALL64_PLAT) += thunderx.o
> >> ? obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
> >> ? obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
> >> +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
> >> ? obj-$(CONFIG_MPSOC_PLATFORM)? += xilinx-zynqmp.o
> >> ? obj-$(CONFIG_MPSOC_PLATFORM)? += xilinx-zynqmp-eemi.o diff --git
> >> a/xen/arch/arm/platforms/imx8qm.c
> b/xen/arch/arm/platforms/imx8qm.c
> >> new file mode 100644 index 0000000000..a9cd9c3615
> >> --- /dev/null
> >> +++ b/xen/arch/arm/platforms/imx8qm.c
> >> @@ -0,0 +1,65 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * xen/arch/arm/platforms/imx8qm.c
> >> + *
> >> + * i.MX 8QM setup
> >> + *
> >> + * Copyright (c) 2016 Freescale Inc.
> >> + * Copyright 2018-2019 NXP
> >> + *
> >> + *
> >> + * Peng Fan <peng.fan@nxp.com>
> >> + */
> >> +
> >> +#include <asm/platform.h>
> >> +#include <asm/smccc.h>
> >> +
> >> +static const char * const imx8qm_dt_compat[] __initconst = {
> >> +??? "fsl,imx8qm",
> >> +??? "fsl,imx8qxp",
> >> +??? NULL
> >> +};
> >> +
> >> +static bool imx8qm_smc(struct cpu_user_regs *regs) {
> >
> > Your implementation below will not only forward SMC for dom0 but also
> > for any non-trusted domains. Have you investigated that all the SIP
> > calls are safe to be called by anyone?
>
> We use pure virtualized domUs, so we do not expect any calls to this SMC
> interface from the guest. I'll limit it to dom0.

Would you mind to share what features are supported in your DomU?

Pure virtualized, you using xen pv or virtio?

Thanks,
Peng.

> >
> > 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)
> >
> > Cheers,
> >
>
> Thanks! // John Ernberg
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
julien at xen
Feb 1, 2024, 4:20 AM
Post #5 of 27 (101 views)
Permalink
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:
>>> When using Linux for dom0 there are a bunch of drivers that need to do
>>> SMC
>>> SIP calls into the PSCI provider to enable certain hardware bits like the
>>> watchdog.
>>
>> Do you know which protocol this is under the hood. Is this SCMI?
>
> I think I confused myself here when I wrote the commit log.
>
> The EL3 code in our case is ATF, and it does not appear to be SCMI, nor
> PSCI. The register usage of these SMC SIP calls are as follows:
> a0 - service
> a1 - function
> a2-a7 - args
>
> In ATF the handler is declared as a runtime service.
>
> Would the appropriate commmit message here be something along the lines
> of below?
> """
> When using Linux for dom0 there are a bunch of drivers that need to do SMC
> SIP calls into the firmware to enable certain hardware bits like the
> watchdog.
> """

It reads better thanks.

[...]

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

Cheers,

--
Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Feb 1, 2024, 8:15 AM
Post #6 of 27 (100 views)
Permalink
Hi Peng,

On 2/1/24 05:10, Peng Fan wrote:
>> Cc: Jonas Blixt <jonas.blixt@actia.se>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue
>>
>> Hi Julien,
>>
>> On 1/31/24 13:22, Julien Grall wrote:
>>> Hi,
>>>
>>> On 31/01/2024 11:50, John Ernberg wrote:
>>>> When using Linux for dom0 there are a bunch of drivers that need to
>>>> do SMC SIP calls into the PSCI provider to enable certain hardware
>>>> bits like the watchdog.
>>>
>>> Do you know which protocol this is under the hood. Is this SCMI?
>>
>> I think I confused myself here when I wrote the commit log.
>>
>> The EL3 code in our case is ATF, and it does not appear to be SCMI, nor PSCI.
>> The register usage of these SMC SIP calls are as follows:
>> a0 - service
>> a1 - function
>> a2-a7 - args
>>
>> In ATF the handler is declared as a runtime service.
>>
>> Would the appropriate commmit message here be something along the lines
>> of below?
>> """
>> When using Linux for dom0 there are a bunch of drivers that need to do
>> SMC
>> SIP calls into the firmware to enable certain hardware bits like the watchdog.
>> """
>>>
>>>>
>>>> Provide a basic platform glue that implements the needed SMC forwarding.
>>>>
>>>> Signed-off-by: John Ernberg <john.ernberg@actia.se>
>>>> ---
>>>> NOTE: This is based on code found in NXP Xen tree located here:
>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
>>>> hub.com%2Fnxp-imx%2Fimx-xen%2Fblob%2Flf-
>> 5.10.y_4.13%2Fxen%2Farch%2Far
>>>>
>> m%2Fplatforms%2Fimx8qm.c&data=05%7C02%7Cpeng.fan%40nxp.com%7C
>> 573b599a
>>>>
>> 4b4143ceca1d08dc2271e5be%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
>> 0%7C0%7
>>>>
>> C638423119777601548%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
>> wMDAiLCJQI
>>>>
>> joiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=ZO
>> 0TXjL6
>>>> g0W7TIZo8x8lTNBXEZW%2BDNcLPndWlEf5D2A%3D&reserved=0
>>>
>>> Anything after --- will be removed while applied to the three. I think
>>> this NOTE should be written down in the commit message.
>>
>> Ack.
>>>
>>> You also possibly want a signed-off-by from Peng as this is his code.
>>
>> @Peng: May I add a sign-off from you?
>
> Yeah. You could add my sign off.

Great, thanks!
>
>>>
>>>>
>>>>   xen/arch/arm/platforms/Makefile |  1 +
>>>>   xen/arch/arm/platforms/imx8qm.c | 65
>>>> +++++++++++++++++++++++++++++++++
>>>>   2 files changed, 66 insertions(+)
>>>>   create mode 100644 xen/arch/arm/platforms/imx8qm.c
>>>>
>>>> diff --git a/xen/arch/arm/platforms/Makefile
>>>> b/xen/arch/arm/platforms/Makefile index 8632f4115f..bec6e55d1f
>> 100644
>>>> --- a/xen/arch/arm/platforms/Makefile
>>>> +++ b/xen/arch/arm/platforms/Makefile
>>>> @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT)   += sunxi.o
>>>>   obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>>>>   obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
>>>>   obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
>>>> +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
>>>>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>>>>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o diff --git
>>>> a/xen/arch/arm/platforms/imx8qm.c
>> b/xen/arch/arm/platforms/imx8qm.c
>>>> new file mode 100644 index 0000000000..a9cd9c3615
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/platforms/imx8qm.c
>>>> @@ -0,0 +1,65 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * xen/arch/arm/platforms/imx8qm.c
>>>> + *
>>>> + * i.MX 8QM setup
>>>> + *
>>>> + * Copyright (c) 2016 Freescale Inc.
>>>> + * Copyright 2018-2019 NXP
>>>> + *
>>>> + *
>>>> + * Peng Fan <peng.fan@nxp.com>
>>>> + */
>>>> +
>>>> +#include <asm/platform.h>
>>>> +#include <asm/smccc.h>
>>>> +
>>>> +static const char * const imx8qm_dt_compat[] __initconst = {
>>>> +    "fsl,imx8qm",
>>>> +    "fsl,imx8qxp",
>>>> +    NULL
>>>> +};
>>>> +
>>>> +static bool imx8qm_smc(struct cpu_user_regs *regs) {
>>>
>>> Your implementation below will not only forward SMC for dom0 but also
>>> for any non-trusted domains. Have you investigated that all the SIP
>>> calls are safe to be called by anyone?
>>
>> We use pure virtualized domUs, so we do not expect any calls to this SMC
>> interface from the guest. I'll limit it to dom0.
>
> Would you mind to share what features are supported in your DomU?
>
> Pure virtualized, you using xen pv or virtio?

Not at all. We're forwarding block and networking (and additionally
usbip over networking for USB sharing), via the Xen PV drivers.
>
> Thanks,
> Peng.
>

Thanks! // John Ernberg
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Feb 1, 2024, 8:17 AM
Post #7 of 27 (100 views)
Permalink
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:
>>>> When using Linux for dom0 there are a bunch of drivers that need to do
>>>> SMC
>>>> SIP calls into the PSCI provider to enable certain hardware bits
>>>> like the
>>>> watchdog.
>>>
>>> Do you know which protocol this is under the hood. Is this SCMI?
>>
>> I think I confused myself here when I wrote the commit log.
>>
>> The EL3 code in our case is ATF, and it does not appear to be SCMI, nor
>> PSCI. The register usage of these SMC SIP calls are as follows:
>> a0 - service
>> a1 - function
>> a2-a7 - args
>>
>> In ATF the handler is declared as a runtime service.
>>
>> Would the appropriate commmit message here be something along the lines
>> of below?
>> """
>> When using Linux for dom0 there are a bunch of drivers that need to
>> do   SMC
>> SIP calls into the firmware to enable certain hardware bits like the
>> watchdog.
>> """
>
> It reads better thanks.
>
> [...]
>
>>> 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.
>
> Cheers,
>

Thanks! // John Ernberg
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
julien at xen
Feb 2, 2024, 1:19 AM
Post #8 of 27 (98 views)
Permalink
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:
>>>>> When using Linux for dom0 there are a bunch of drivers that need to do
>>>>> SMC
>>>>> SIP calls into the PSCI provider to enable certain hardware bits
>>>>> like the
>>>>> watchdog.
>>>>
>>>> Do you know which protocol this is under the hood. Is this SCMI?
>>>
>>> I think I confused myself here when I wrote the commit log.
>>>
>>> The EL3 code in our case is ATF, and it does not appear to be SCMI, nor
>>> PSCI. The register usage of these SMC SIP calls are as follows:
>>> a0 - service
>>> a1 - function
>>> a2-a7 - args
>>>
>>> In ATF the handler is declared as a runtime service.
>>>
>>> Would the appropriate commmit message here be something along the lines
>>> of below?
>>> """
>>> When using Linux for dom0 there are a bunch of drivers that need to
>>> do   SMC
>>> SIP calls into the firmware to enable certain hardware bits like the
>>> watchdog.
>>> """
>>
>> It reads better thanks.
>>
>> [...]
>>
>>>> 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?

Cheers,

--
Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
sstabellini at kernel
Feb 2, 2024, 2:09 PM
Post #9 of 27 (91 views)
Permalink
On Fri, 2 Feb 2024, Julien Grall wrote:
> 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:
> > > > > > When using Linux for dom0 there are a bunch of drivers that need to
> > > > > > do
> > > > > > SMC
> > > > > > SIP calls into the PSCI provider to enable certain hardware bits
> > > > > > like the
> > > > > > watchdog.
> > > > >
> > > > > Do you know which protocol this is under the hood. Is this SCMI?
> > > >
> > > > I think I confused myself here when I wrote the commit log.
> > > >
> > > > The EL3 code in our case is ATF, and it does not appear to be SCMI, nor
> > > > PSCI. The register usage of these SMC SIP calls are as follows:
> > > > a0 - service
> > > > a1 - function
> > > > a2-a7 - args
> > > >
> > > > In ATF the handler is declared as a runtime service.
> > > >
> > > > Would the appropriate commmit message here be something along the lines
> > > > of below?
> > > > """
> > > > When using Linux for dom0 there are a bunch of drivers that need to
> > > > do   SMC
> > > > SIP calls into the firmware to enable certain hardware bits like the
> > > > watchdog.
> > > > """
> > >
> > > It reads better thanks.
> > >
> > > [...]
> > >
> > > > > 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?

Just to add some context to make the reason for the question clearer, if
we have a specification we could check the patch for correctness.
Without it, it is difficult to know if it is doing the right thing.

The other aspect is about expectation of forward and backward
compatibility. Can we guarantee that the next version of Xen and the one
after it will still work against this interface? If not, can we check
for the version of the interface before continuing? If not, can we at
least document that the interface is only known-to-work with specific
firmware versions?

This is basically just to provide the right expectations to users and
ideally to prevent a future version of Xen to break on boot silently
without information.

If we don't have a spec and we don't know if the interface is stable, I
think we should try to detect the version of the interface and print a
warning in Xen if it not a known version.
RE: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
peng.fan at nxp
Feb 4, 2024, 1:40 AM
Post #10 of 27 (79 views)
Permalink
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 2024?2?2? 17:20
> To: John Ernberg <john.ernberg@actia.se>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <bertrand.marquis@arm.com>;
> Michal Orzel <michal.orzel@amd.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Peng Fan <peng.fan@nxp.com>
> Cc: Jonas Blixt <jonas.blixt@actia.se>; xen-devel@lists.xenproject.org
> 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:
> >>>>> When using Linux for dom0 there are a bunch of drivers that need
> >>>>> to do SMC SIP calls into the PSCI provider to enable certain
> >>>>> hardware bits like the watchdog.
> >>>>
> >>>> Do you know which protocol this is under the hood. Is this SCMI?
> >>>
> >>> I think I confused myself here when I wrote the commit log.
> >>>
> >>> The EL3 code in our case is ATF, and it does not appear to be SCMI,
> >>> nor PSCI. The register usage of these SMC SIP calls are as follows:
> >>> a0 - service
> >>> a1 - function
> >>> a2-a7 - args
> >>>
> >>> In ATF the handler is declared as a runtime service.
> >>>
> >>> Would the appropriate commmit message here be something along the
> >>> lines of below?
> >>> """
> >>> When using Linux for dom0 there are a bunch of drivers that need to
> >>> do   SMC SIP calls into the firmware to enable certain hardware bits
> >>> like the watchdog.
> >>> """
> >>
> >> It reads better thanks.
> >>
> >> [...]
> >>
> >>>> 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.

Regards,
Peng.

>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
julien at xen
Feb 5, 2024, 2:13 AM
Post #11 of 27 (76 views)
Permalink
Hi,

On 04/02/2024 09:40, Peng Fan wrote:
>
>
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 2024?2?2? 17:20
>> To: John Ernberg <john.ernberg@actia.se>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <bertrand.marquis@arm.com>;
>> Michal Orzel <michal.orzel@amd.com>; Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com>; Peng Fan <peng.fan@nxp.com>
>> Cc: Jonas Blixt <jonas.blixt@actia.se>; xen-devel@lists.xenproject.org
>> 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:
>>>>>>> When using Linux for dom0 there are a bunch of drivers that need
>>>>>>> to do SMC SIP calls into the PSCI provider to enable certain
>>>>>>> hardware bits like the watchdog.
>>>>>>
>>>>>> Do you know which protocol this is under the hood. Is this SCMI?
>>>>>
>>>>> I think I confused myself here when I wrote the commit log.
>>>>>
>>>>> The EL3 code in our case is ATF, and it does not appear to be SCMI,
>>>>> nor PSCI. The register usage of these SMC SIP calls are as follows:
>>>>> a0 - service
>>>>> a1 - function
>>>>> a2-a7 - args
>>>>>
>>>>> In ATF the handler is declared as a runtime service.
>>>>>
>>>>> Would the appropriate commmit message here be something along the
>>>>> lines of below?
>>>>> """
>>>>> When using Linux for dom0 there are a bunch of drivers that need to
>>>>> do   SMC SIP calls into the firmware to enable certain hardware bits
>>>>> like the watchdog.
>>>>> """
>>>>
>>>> It reads better thanks.
>>>>
>>>> [...]
>>>>
>>>>>> 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?


Looking through the list, there are some that probably want a bit more
discussion on whether we want to expose them:
* IMX_SIP_CPUFREQ: Right now, dom0 is not aware of the full system.
So it may take wrong decision.
* IMX_SIP_DDR_DVFS: Some operation seems to take the number of online
CPUs. Dom0 doesn't know that
* 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.

Also, what happen if we don't expose those SMC to dom0?

Cheers,

--
Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Feb 9, 2024, 5:14 AM
Post #12 of 27 (64 views)
Permalink
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@xen.org>
>>> Sent: 2024?2?2? 17:20
>>> To: John Ernberg <john.ernberg@actia.se>; Stefano Stabellini
>>> <sstabellini@kernel.org>; Bertrand Marquis <bertrand.marquis@arm.com>;
>>> Michal Orzel <michal.orzel@amd.com>; Volodymyr Babchuk
>>> <Volodymyr_Babchuk@epam.com>; Peng Fan <peng.fan@nxp.com>
>>> Cc: Jonas Blixt <jonas.blixt@actia.se>; xen-devel@lists.xenproject.org
>>> 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]
>
>
> Looking through the list, there are some that probably want a bit more
> discussion on whether we want to expose them:
>   * IMX_SIP_CPUFREQ: Right now, dom0 is not aware of the full system.
> So it may take wrong decision.
The cpufreq function operates on the cluster level, it performs no error
checking so blocking it will be invisible to Linux. I don't know yet
what kind of impact that could have. Looking for hints about cpufreq in
Xen for ARM I found [1] and [2].

In our current setup we do not use cpufreq and it is stable enough for
our usecase.

From my point of view we can block it for now, and when cpufreq
functionality gets picked up again, we can revisit.

>   * IMX_SIP_DDR_DVFS: Some operation seems to take the number of online
> CPUs. Dom0 doesn't know that

The iMX8Q{X,M} series comes with a system controller called the SCU,
running in a small M-core. This controller takes care of all the DDR
bits, so we do not need to care for these SoCs.

>   * 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@gmail.com/
[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
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Mar 6, 2024, 5:13 AM
Post #13 of 27 (54 views)
Permalink
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@xen.org>
>>>> Sent: 2024?2?2? 17:20
>>>> To: John Ernberg <john.ernberg@actia.se>; Stefano Stabellini
>>>> <sstabellini@kernel.org>; Bertrand Marquis <bertrand.marquis@arm.com>;
>>>> Michal Orzel <michal.orzel@amd.com>; Volodymyr Babchuk
>>>> <Volodymyr_Babchuk@epam.com>; Peng Fan <peng.fan@nxp.com>
>>>> Cc: Jonas Blixt <jonas.blixt@actia.se>; xen-devel@lists.xenproject.org
>>>> 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@gmail.com/
> [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
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
julien at xen
Mar 6, 2024, 3:07 PM
Post #14 of 27 (54 views)
Permalink
Hi John,

> Ping on the watchdog discussion bits.

Sorry for the late reply.

On 06/03/2024 13:13, John Ernberg wrote:
> On 2/9/24 14:14, John Ernberg wrote:
>>
>>>    * 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.

include/watchdog.h contains helper to configure the watchdog for Xen. We
also have per-VM watchdog and this is configured by the hypercall
SCHEDOP_watchdog.

>>
>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>> our watchdog timeout settings, our specific Dom0 setup and VM count.

IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
watchdog. So I think it would make more sense if this is not exposed to
dom0 (even if Xen is doing nothing with it).

Can you try to hide the SMCs and check if dom0 still behave properly?

Cheers,

--
Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Mar 8, 2024, 5:40 AM
Post #15 of 27 (54 views)
Permalink
Hi Julien,

On 3/7/24 00:07, Julien Grall wrote:
> Hi John,
>
> > Ping on the watchdog discussion bits.
>
> Sorry for the late reply.
>
> On 06/03/2024 13:13, John Ernberg wrote:
>> On 2/9/24 14:14, John Ernberg wrote:
>>>
>>>>     * 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.
>
> include/watchdog.h contains helper to configure the watchdog for Xen. We
> also have per-VM watchdog and this is configured by the hypercall
> SCHEDOP_watchdog.
>
>>>
>>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>
> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
> watchdog. So I think it would make more sense if this is not exposed to
> dom0 (even if Xen is doing nothing with it).
>
> Can you try to hide the SMCs and check if dom0 still behave properly?
>
> Cheers,
>

This SMC manages a hardware watchdog, if it's not pinged within a
specific interval the entire board resets.

If I block the SMCs the watchdog driver in Dom0 will fail to ping the
watchdog, triggering a board reset because the system looks to have
become unresponsive. The reason this patch set started is because we
couldn't ping the watchdog when running with Xen.

In our specific system the bootloader enables the watchdog as early as
possible so that we can get watchdog protection for as much of the boot
as we possibly can.

So, if we are to block the SMC from Dom0, then Xen needs to take over
the pinging. It could be implemented similarly to the NMI watchdog,
except that the system will reset if the ping is missed rather than
backtrace.
It would also mean that Xen will get a whole watchdog driver-category
due to the watchdog being vendor and sometimes even SoC specific when it
comes to Arm.

My understanding of the domain watchdog code is that today the domain
needs to call SCHEDOP_watchdog at least once to start the watchdog
timer. Since watchdog protection through the whole boot process is
desirable we'd need some core changes, such as an option to start the
domain watchdog on init.

It's quite a big change to make, while I am not against doing it if it
makes sense, I now wonder if Xen should manage hardware watchdogs.
Looking at the domain watchdog code it looks like if a domain does not
get enough execution time, the watchdog will not be pinged enough and
the guest will be reset. So either watchdog approach requires Dom0 to
get execution time. Dom0 also needs to service all the PV backends it's
responsible for. I'm not sure it's valuable to add another layer of
watchdog for this scenario as the end result (checking that the entire
system works) is achieved without it as well.

So, before I try to find the time to make a proposal for moving the
hardware watchdog bit to Xen, do we really want it?

Thanks! // John Ernberg
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
julien at xen
Mar 8, 2024, 6:04 AM
Post #16 of 27 (54 views)
Permalink
Hi John,

Thank you for the reply.

On 08/03/2024 13:40, John Ernberg wrote:
> On 3/7/24 00:07, Julien Grall wrote:
>> > Ping on the watchdog discussion bits.
>>
>> Sorry for the late reply.
>>
>> On 06/03/2024 13:13, John Ernberg wrote:
>>> On 2/9/24 14:14, John Ernberg wrote:
>>>>
>>>>>     * 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.
>>
>> include/watchdog.h contains helper to configure the watchdog for Xen. We
>> also have per-VM watchdog and this is configured by the hypercall
>> SCHEDOP_watchdog.
>>
>>>>
>>>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>>
>> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
>> watchdog. So I think it would make more sense if this is not exposed to
>> dom0 (even if Xen is doing nothing with it).
>>
>> Can you try to hide the SMCs and check if dom0 still behave properly?
>>
>> Cheers,
>>
>
> This SMC manages a hardware watchdog, if it's not pinged within a
> specific interval the entire board resets.

Do you know what's the default interval? Is it large enough so Xen +
dom0 can boot (at least up to when the watchdog driver is initialized)?

>
> If I block the SMCs the watchdog driver in Dom0 will fail to ping the
> watchdog, triggering a board reset because the system looks to have
> become unresponsive. The reason this patch set started is because we
> couldn't ping the watchdog when running with Xen.
>
> In our specific system the bootloader enables the watchdog as early as
> possible so that we can get watchdog protection for as much of the boot
> as we possibly can.
>
> So, if we are to block the SMC from Dom0, then Xen needs to take over
> the pinging. It could be implemented similarly to the NMI watchdog,
> except that the system will reset if the ping is missed rather than
> backtrace.
> It would also mean that Xen will get a whole watchdog driver-category
> due to the watchdog being vendor and sometimes even SoC specific when it
> comes to Arm.
>
> My understanding of the domain watchdog code is that today the domain
> needs to call SCHEDOP_watchdog at least once to start the watchdog
> timer. Since watchdog protection through the whole boot process is
> desirable we'd need some core changes, such as an option to start the
> domain watchdog on init. >
> It's quite a big change to make

For clarification, above you seem to mention two changes:

1) Allow Xen to use the HW watchdog
2) Allow the domain to use the watchdog early

I am assuming by big change, you are referring to 2?

, while I am not against doing it if it
> makes sense, I now wonder if Xen should manage hardware watchdogs.
> Looking at the domain watchdog code it looks like if a domain does not
> get enough execution time, the watchdog will not be pinged enough and
> the guest will be reset. So either watchdog approach requires Dom0 to
> get execution time. Dom0 also needs to service all the PV backends it's
> responsible for. I'm not sure it's valuable to add another layer of
> watchdog for this scenario as the end result (checking that the entire
> system works) is achieved without it as well.
>
> So, before I try to find the time to make a proposal for moving the
> hardware watchdog bit to Xen, do we really want it?

Thanks for the details. Given that the watchdog is enabled by the
bootloader, I think we want Xen to drive the watchdog for two reasons:
1) In true dom0less environment, dom0 would not exist
2) You are relying on Xen + Dom0 to boot (or at least enough to get
the watchdog working) within the watchdog interval.

Let see what the other Arm maintainer thinks.

--
Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Mar 11, 2024, 1:39 AM
Post #17 of 27 (51 views)
Permalink
Hi Julien,

On 3/8/24 15:04, Julien Grall wrote:
> Hi John,
>
> Thank you for the reply.
>
> On 08/03/2024 13:40, John Ernberg wrote:
>> On 3/7/24 00:07, Julien Grall wrote:
>>>   > Ping on the watchdog discussion bits.
>>>
>>> Sorry for the late reply.
>>>
>>> On 06/03/2024 13:13, John Ernberg wrote:
>>>> On 2/9/24 14:14, John Ernberg wrote:
>>>>>
>>>>>>      * 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.
>>>
>>> include/watchdog.h contains helper to configure the watchdog for Xen. We
>>> also have per-VM watchdog and this is configured by the hypercall
>>> SCHEDOP_watchdog.
>>>
>>>>>
>>>>> Currently forwarding it to Dom0 has not caused any watchdog resets
>>>>> with
>>>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>>>
>>> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
>>> watchdog. So I think it would make more sense if this is not exposed to
>>> dom0 (even if Xen is doing nothing with it).
>>>
>>> Can you try to hide the SMCs and check if dom0 still behave properly?
>>>
>>> Cheers,
>>>
>>
>> This SMC manages a hardware watchdog, if it's not pinged within a
>> specific interval the entire board resets.
>
> Do you know what's the default interval? Is it large enough so Xen +
> dom0 can boot (at least up to when the watchdog driver is initialized)?
>
>>
>> If I block the SMCs the watchdog driver in Dom0 will fail to ping the
>> watchdog, triggering a board reset because the system looks to have
>> become unresponsive. The reason this patch set started is because we
>> couldn't ping the watchdog when running with Xen.
>>
>> In our specific system the bootloader enables the watchdog as early as
>> possible so that we can get watchdog protection for as much of the boot
>> as we possibly can.
>>
>> So, if we are to block the SMC from Dom0, then Xen needs to take over
>> the pinging. It could be implemented similarly to the NMI watchdog,
>> except that the system will reset if the ping is missed rather than
>> backtrace.
>> It would also mean that Xen will get a whole watchdog driver-category
>> due to the watchdog being vendor and sometimes even SoC specific when it
>> comes to Arm.
>>
>> My understanding of the domain watchdog code is that today the domain
>> needs to call SCHEDOP_watchdog at least once to start the watchdog
>> timer. Since watchdog protection through the whole boot process is
>> desirable we'd need some core changes, such as an option to start the
>> domain watchdog on init. >
>> It's quite a big change to make
>
> For clarification, above you seem to mention two changes:
>
>  1) Allow Xen to use the HW watchdog
>  2) Allow the domain to use the watchdog early
>
> I am assuming by big change, you are referring to 2?

Both of them. I'm expecting the addition of a new driver category
(hardware watchdog) to be a decent amount of work as well.
>
> , while I am not against doing it if it
>> makes sense, I now wonder if Xen should manage hardware watchdogs.
>> Looking at the domain watchdog code it looks like if a domain does not
>> get enough execution time, the watchdog will not be pinged enough and
>> the guest will be reset. So either watchdog approach requires Dom0 to
>> get execution time. Dom0 also needs to service all the PV backends it's
>> responsible for. I'm not sure it's valuable to add another layer of
>> watchdog for this scenario as the end result (checking that the entire
>> system works) is achieved without it as well.
>>
>> So, before I try to find the time to make a proposal for moving the
>> hardware watchdog bit to Xen, do we really want it?
>
> Thanks for the details. Given that the watchdog is enabled by the
> bootloader, I think we want Xen to drive the watchdog for two reasons:
>  1) In true dom0less environment, dom0 would not exist
>  2) You are relying on Xen + Dom0 to boot (or at least enough to get
> the watchdog working) within the watchdog interval.
>
> Let see what the other Arm maintainer thinks.
>

Regards // John Ernberg
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
Bertrand.Marquis at arm
Mar 13, 2024, 3:07 AM
Post #18 of 27 (50 views)
Permalink
Hi,

> On 8 Mar 2024, at 15:04, Julien Grall <julien@xen.org> wrote:
>
> Hi John,
>
> Thank you for the reply.
>
> On 08/03/2024 13:40, John Ernberg wrote:
>> On 3/7/24 00:07, Julien Grall wrote:
>>> > Ping on the watchdog discussion bits.
>>>
>>> Sorry for the late reply.
>>>
>>> On 06/03/2024 13:13, John Ernberg wrote:
>>>> On 2/9/24 14:14, John Ernberg wrote:
>>>>>
>>>>>> * 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.
>>>
>>> include/watchdog.h contains helper to configure the watchdog for Xen. We
>>> also have per-VM watchdog and this is configured by the hypercall
>>> SCHEDOP_watchdog.
>>>
>>>>>
>>>>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>>>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>>>
>>> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
>>> watchdog. So I think it would make more sense if this is not exposed to
>>> dom0 (even if Xen is doing nothing with it).
>>>
>>> Can you try to hide the SMCs and check if dom0 still behave properly?
>>>
>>> Cheers,
>>>
>> This SMC manages a hardware watchdog, if it's not pinged within a
>> specific interval the entire board resets.
>
> Do you know what's the default interval? Is it large enough so Xen + dom0 can boot (at least up to when the watchdog driver is initialized)?
>
>> If I block the SMCs the watchdog driver in Dom0 will fail to ping the
>> watchdog, triggering a board reset because the system looks to have
>> become unresponsive. The reason this patch set started is because we
>> couldn't ping the watchdog when running with Xen.
>> In our specific system the bootloader enables the watchdog as early as
>> possible so that we can get watchdog protection for as much of the boot
>> as we possibly can.
>> So, if we are to block the SMC from Dom0, then Xen needs to take over
>> the pinging. It could be implemented similarly to the NMI watchdog,
>> except that the system will reset if the ping is missed rather than
>> backtrace.
>> It would also mean that Xen will get a whole watchdog driver-category
>> due to the watchdog being vendor and sometimes even SoC specific when it
>> comes to Arm.
>> My understanding of the domain watchdog code is that today the domain
>> needs to call SCHEDOP_watchdog at least once to start the watchdog
>> timer. Since watchdog protection through the whole boot process is
>> desirable we'd need some core changes, such as an option to start the
>> domain watchdog on init. >
>> It's quite a big change to make
>
> For clarification, above you seem to mention two changes:
>
> 1) Allow Xen to use the HW watchdog
> 2) Allow the domain to use the watchdog early
>
> I am assuming by big change, you are referring to 2?
>
> , while I am not against doing it if it
>> makes sense, I now wonder if Xen should manage hardware watchdogs.
>> Looking at the domain watchdog code it looks like if a domain does not
>> get enough execution time, the watchdog will not be pinged enough and
>> the guest will be reset. So either watchdog approach requires Dom0 to
>> get execution time. Dom0 also needs to service all the PV backends it's
>> responsible for. I'm not sure it's valuable to add another layer of
>> watchdog for this scenario as the end result (checking that the entire
>> system works) is achieved without it as well.
>> So, before I try to find the time to make a proposal for moving the
>> hardware watchdog bit to Xen, do we really want it?
>
> Thanks for the details. Given that the watchdog is enabled by the bootloader, I think we want Xen to drive the watchdog for two reasons:
> 1) In true dom0less environment, dom0 would not exist
> 2) You are relying on Xen + Dom0 to boot (or at least enough to get the watchdog working) within the watchdog interval.

Definitely we need to consider the case where there is no Dom0.

I think there are in fact 3 different use cases here:
- watchdog fully driven in a domain (dom0 or another): would work if it is expected
that Xen + Domain boot time is under the watchdog initial refresh rate. I think this
could make sense on some applications where your system depends on a specific
domain to be properly booted to work. This would require an initial refresh time
configurable in the boot loader probably.
- watchdog fully driven by Xen. One might want to just make sure the hypervisor is alive.
- hybrid model where the watchdog is driven by Xen until a domain comes up to drive it.
This could make sense to relax the stress on boot time but would raise the question of
what should be done if the domain dies. This is also kind of complex as Xen should stop
refreshing the watchdog when a domain starts doing it (might require a trap and emulate
initially that is then mapped directly to a domain). I am not completely sure this makes sense.

Regards
Bertrand


>
> Let see what the other Arm maintainer thinks.
>
> --
> Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Mar 20, 2024, 9:24 AM
Post #19 of 27 (43 views)
Permalink
Hi Bertrand,

On 3/13/24 11:07, Bertrand Marquis wrote:
> Hi,
>
>> On 8 Mar 2024, at 15:04, Julien Grall <julien@xen.org> wrote:
>>
>> Hi John,
>>
>> Thank you for the reply.
>>
>> On 08/03/2024 13:40, John Ernberg wrote:
>>> On 3/7/24 00:07, Julien Grall wrote:
>>>> > Ping on the watchdog discussion bits.
>>>>
>>>> Sorry for the late reply.
>>>>
>>>> On 06/03/2024 13:13, John Ernberg wrote:
>>>>> On 2/9/24 14:14, John Ernberg wrote:
>>>>>>
>>>>>>> * 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.
>>>>
>>>> include/watchdog.h contains helper to configure the watchdog for Xen. We
>>>> also have per-VM watchdog and this is configured by the hypercall
>>>> SCHEDOP_watchdog.
>>>>
>>>>>>
>>>>>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>>>>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>>>>
>>>> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
>>>> watchdog. So I think it would make more sense if this is not exposed to
>>>> dom0 (even if Xen is doing nothing with it).
>>>>
>>>> Can you try to hide the SMCs and check if dom0 still behave properly?
>>>>
>>>> Cheers,
>>>>
>>> This SMC manages a hardware watchdog, if it's not pinged within a
>>> specific interval the entire board resets.
>>
>> Do you know what's the default interval? Is it large enough so Xen + dom0 can boot (at least up to when the watchdog driver is initialized)?
>>
>>> If I block the SMCs the watchdog driver in Dom0 will fail to ping the
>>> watchdog, triggering a board reset because the system looks to have
>>> become unresponsive. The reason this patch set started is because we
>>> couldn't ping the watchdog when running with Xen.
>>> In our specific system the bootloader enables the watchdog as early as
>>> possible so that we can get watchdog protection for as much of the boot
>>> as we possibly can.
>>> So, if we are to block the SMC from Dom0, then Xen needs to take over
>>> the pinging. It could be implemented similarly to the NMI watchdog,
>>> except that the system will reset if the ping is missed rather than
>>> backtrace.
>>> It would also mean that Xen will get a whole watchdog driver-category
>>> due to the watchdog being vendor and sometimes even SoC specific when it
>>> comes to Arm.
>>> My understanding of the domain watchdog code is that today the domain
>>> needs to call SCHEDOP_watchdog at least once to start the watchdog
>>> timer. Since watchdog protection through the whole boot process is
>>> desirable we'd need some core changes, such as an option to start the
>>> domain watchdog on init. >
>>> It's quite a big change to make
>>
>> For clarification, above you seem to mention two changes:
>>
>> 1) Allow Xen to use the HW watchdog
>> 2) Allow the domain to use the watchdog early
>>
>> I am assuming by big change, you are referring to 2?
>>
>> , while I am not against doing it if it
>>> makes sense, I now wonder if Xen should manage hardware watchdogs.
>>> Looking at the domain watchdog code it looks like if a domain does not
>>> get enough execution time, the watchdog will not be pinged enough and
>>> the guest will be reset. So either watchdog approach requires Dom0 to
>>> get execution time. Dom0 also needs to service all the PV backends it's
>>> responsible for. I'm not sure it's valuable to add another layer of
>>> watchdog for this scenario as the end result (checking that the entire
>>> system works) is achieved without it as well.
>>> So, before I try to find the time to make a proposal for moving the
>>> hardware watchdog bit to Xen, do we really want it?
>>
>> Thanks for the details. Given that the watchdog is enabled by the bootloader, I think we want Xen to drive the watchdog for two reasons:
>> 1) In true dom0less environment, dom0 would not exist
>> 2) You are relying on Xen + Dom0 to boot (or at least enough to get the watchdog working) within the watchdog interval.
>
> Definitely we need to consider the case where there is no Dom0.
>
> I think there are in fact 3 different use cases here:
> - watchdog fully driven in a domain (dom0 or another): would work if it is expected
> that Xen + Domain boot time is under the watchdog initial refresh rate. I think this
> could make sense on some applications where your system depends on a specific
> domain to be properly booted to work. This would require an initial refresh time
> configurable in the boot loader probably.

This is our use-case. ^

Our dom0 is monitoring and managing the other domains in our system.
Without dom0 working the system isn't really working as a whole.

@Julien: Would you be ok with the patch set continuing in the direction
of the
original proposal, letting another party (or me at a later time) implement
the fully driven by Xen option?

> - watchdog fully driven by Xen. One might want to just make sure the hypervisor is alive.
> - hybrid model where the watchdog is driven by Xen until a domain comes up to drive it.
> This could make sense to relax the stress on boot time but would raise the question of
> what should be done if the domain dies. This is also kind of complex as Xen should stop
> refreshing the watchdog when a domain starts doing it (might require a trap and emulate
> initially that is then mapped directly to a domain). I am not completely sure this makes sense.
>
> Regards
> Bertrand
>

Thanks and best regards // John Ernberg
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
julien at xen
Mar 20, 2024, 10:40 AM
Post #20 of 27 (43 views)
Permalink
Hi John,

On 20/03/2024 16:24, John Ernberg wrote:
> Hi Bertrand,
>
> On 3/13/24 11:07, Bertrand Marquis wrote:
>> Hi,
>>
>>> On 8 Mar 2024, at 15:04, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi John,
>>>
>>> Thank you for the reply.
>>>
>>> On 08/03/2024 13:40, John Ernberg wrote:
>>>> On 3/7/24 00:07, Julien Grall wrote:
>>>>> > Ping on the watchdog discussion bits.
>>>>>
>>>>> Sorry for the late reply.
>>>>>
>>>>> On 06/03/2024 13:13, John Ernberg wrote:
>>>>>> On 2/9/24 14:14, John Ernberg wrote:
>>>>>>>
>>>>>>>> * 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.
>>>>>
>>>>> include/watchdog.h contains helper to configure the watchdog for Xen. We
>>>>> also have per-VM watchdog and this is configured by the hypercall
>>>>> SCHEDOP_watchdog.
>>>>>
>>>>>>>
>>>>>>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>>>>>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>>>>>
>>>>> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
>>>>> watchdog. So I think it would make more sense if this is not exposed to
>>>>> dom0 (even if Xen is doing nothing with it).
>>>>>
>>>>> Can you try to hide the SMCs and check if dom0 still behave properly?
>>>>>
>>>>> Cheers,
>>>>>
>>>> This SMC manages a hardware watchdog, if it's not pinged within a
>>>> specific interval the entire board resets.
>>>
>>> Do you know what's the default interval? Is it large enough so Xen + dom0 can boot (at least up to when the watchdog driver is initialized)?
>>>
>>>> If I block the SMCs the watchdog driver in Dom0 will fail to ping the
>>>> watchdog, triggering a board reset because the system looks to have
>>>> become unresponsive. The reason this patch set started is because we
>>>> couldn't ping the watchdog when running with Xen.
>>>> In our specific system the bootloader enables the watchdog as early as
>>>> possible so that we can get watchdog protection for as much of the boot
>>>> as we possibly can.
>>>> So, if we are to block the SMC from Dom0, then Xen needs to take over
>>>> the pinging. It could be implemented similarly to the NMI watchdog,
>>>> except that the system will reset if the ping is missed rather than
>>>> backtrace.
>>>> It would also mean that Xen will get a whole watchdog driver-category
>>>> due to the watchdog being vendor and sometimes even SoC specific when it
>>>> comes to Arm.
>>>> My understanding of the domain watchdog code is that today the domain
>>>> needs to call SCHEDOP_watchdog at least once to start the watchdog
>>>> timer. Since watchdog protection through the whole boot process is
>>>> desirable we'd need some core changes, such as an option to start the
>>>> domain watchdog on init. >
>>>> It's quite a big change to make
>>>
>>> For clarification, above you seem to mention two changes:
>>>
>>> 1) Allow Xen to use the HW watchdog
>>> 2) Allow the domain to use the watchdog early
>>>
>>> I am assuming by big change, you are referring to 2?
>>>
>>> , while I am not against doing it if it
>>>> makes sense, I now wonder if Xen should manage hardware watchdogs.
>>>> Looking at the domain watchdog code it looks like if a domain does not
>>>> get enough execution time, the watchdog will not be pinged enough and
>>>> the guest will be reset. So either watchdog approach requires Dom0 to
>>>> get execution time. Dom0 also needs to service all the PV backends it's
>>>> responsible for. I'm not sure it's valuable to add another layer of
>>>> watchdog for this scenario as the end result (checking that the entire
>>>> system works) is achieved without it as well.
>>>> So, before I try to find the time to make a proposal for moving the
>>>> hardware watchdog bit to Xen, do we really want it?
>>>
>>> Thanks for the details. Given that the watchdog is enabled by the bootloader, I think we want Xen to drive the watchdog for two reasons:
>>> 1) In true dom0less environment, dom0 would not exist
>>> 2) You are relying on Xen + Dom0 to boot (or at least enough to get the watchdog working) within the watchdog interval.
>>
>> Definitely we need to consider the case where there is no Dom0.
>>
>> I think there are in fact 3 different use cases here:
>> - watchdog fully driven in a domain (dom0 or another): would work if it is expected
>> that Xen + Domain boot time is under the watchdog initial refresh rate. I think this
>> could make sense on some applications where your system depends on a specific
>> domain to be properly booted to work. This would require an initial refresh time
>> configurable in the boot loader probably.
>
> This is our use-case. ^
>
> Our dom0 is monitoring and managing the other domains in our system.
> Without dom0 working the system isn't really working as a whole.
>
> @Julien: Would you be ok with the patch set continuing in the direction
> of the
> original proposal, letting another party (or me at a later time) implement
> the fully driven by Xen option?
I am concerned about this particular point from Bertrand:

"would work if it is expected that Xen + Domain boot time is under the
watchdog initial refresh rate."

How will a user be able to figure out how to initially configure the
watchdog? Is this something you can easily configure in the bootloader
at runtime?


Overall, I am not for this approach at least in the current status. I
would be more inclined if there are some documentations explaining how
this is supposed to be configured on NXP, so others can use the code.

Anyway, this is why we have multiple Arm maintainers for this kind of
situation. If they other agrees with you, then they can ack the patch
and this can be merged.

Cheers,

--
Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
sstabellini at kernel
Mar 20, 2024, 4:53 PM
Post #21 of 27 (43 views)
Permalink
On Wed, 20 Mar 2024, Julien Grall wrote:
> Hi John,
>
> On 20/03/2024 16:24, John Ernberg wrote:
> > Hi Bertrand,
> >
> > On 3/13/24 11:07, Bertrand Marquis wrote:
> > > Hi,
> > >
> > > > On 8 Mar 2024, at 15:04, Julien Grall <julien@xen.org> wrote:
> > > >
> > > > Hi John,
> > > >
> > > > Thank you for the reply.
> > > >
> > > > On 08/03/2024 13:40, John Ernberg wrote:
> > > > > On 3/7/24 00:07, Julien Grall wrote:
> > > > > > > Ping on the watchdog discussion bits.
> > > > > >
> > > > > > Sorry for the late reply.
> > > > > >
> > > > > > On 06/03/2024 13:13, John Ernberg wrote:
> > > > > > > On 2/9/24 14:14, John Ernberg wrote:
> > > > > > > >
> > > > > > > > > * 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.
> > > > > >
> > > > > > include/watchdog.h contains helper to configure the watchdog for
> > > > > > Xen. We
> > > > > > also have per-VM watchdog and this is configured by the hypercall
> > > > > > SCHEDOP_watchdog.
> > > > > >
> > > > > > > >
> > > > > > > > Currently forwarding it to Dom0 has not caused any watchdog
> > > > > > > > resets with
> > > > > > > > our watchdog timeout settings, our specific Dom0 setup and VM
> > > > > > > > count.
> > > > > >
> > > > > > IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
> > > > > > watchdog. So I think it would make more sense if this is not exposed
> > > > > > to
> > > > > > dom0 (even if Xen is doing nothing with it).
> > > > > >
> > > > > > Can you try to hide the SMCs and check if dom0 still behave
> > > > > > properly?
> > > > > >
> > > > > > Cheers,
> > > > > >
> > > > > This SMC manages a hardware watchdog, if it's not pinged within a
> > > > > specific interval the entire board resets.
> > > >
> > > > Do you know what's the default interval? Is it large enough so Xen +
> > > > dom0 can boot (at least up to when the watchdog driver is initialized)?
> > > >
> > > > > If I block the SMCs the watchdog driver in Dom0 will fail to ping the
> > > > > watchdog, triggering a board reset because the system looks to have
> > > > > become unresponsive. The reason this patch set started is because we
> > > > > couldn't ping the watchdog when running with Xen.
> > > > > In our specific system the bootloader enables the watchdog as early as
> > > > > possible so that we can get watchdog protection for as much of the
> > > > > boot
> > > > > as we possibly can.
> > > > > So, if we are to block the SMC from Dom0, then Xen needs to take over
> > > > > the pinging. It could be implemented similarly to the NMI watchdog,
> > > > > except that the system will reset if the ping is missed rather than
> > > > > backtrace.
> > > > > It would also mean that Xen will get a whole watchdog driver-category
> > > > > due to the watchdog being vendor and sometimes even SoC specific when
> > > > > it
> > > > > comes to Arm.
> > > > > My understanding of the domain watchdog code is that today the domain
> > > > > needs to call SCHEDOP_watchdog at least once to start the watchdog
> > > > > timer. Since watchdog protection through the whole boot process is
> > > > > desirable we'd need some core changes, such as an option to start the
> > > > > domain watchdog on init. >
> > > > > It's quite a big change to make
> > > >
> > > > For clarification, above you seem to mention two changes:
> > > >
> > > > 1) Allow Xen to use the HW watchdog
> > > > 2) Allow the domain to use the watchdog early
> > > >
> > > > I am assuming by big change, you are referring to 2?
> > > >
> > > > , while I am not against doing it if it
> > > > > makes sense, I now wonder if Xen should manage hardware watchdogs.
> > > > > Looking at the domain watchdog code it looks like if a domain does not
> > > > > get enough execution time, the watchdog will not be pinged enough and
> > > > > the guest will be reset. So either watchdog approach requires Dom0 to
> > > > > get execution time. Dom0 also needs to service all the PV backends
> > > > > it's
> > > > > responsible for. I'm not sure it's valuable to add another layer of
> > > > > watchdog for this scenario as the end result (checking that the entire
> > > > > system works) is achieved without it as well.
> > > > > So, before I try to find the time to make a proposal for moving the
> > > > > hardware watchdog bit to Xen, do we really want it?
> > > >
> > > > Thanks for the details. Given that the watchdog is enabled by the
> > > > bootloader, I think we want Xen to drive the watchdog for two reasons:
> > > > 1) In true dom0less environment, dom0 would not exist
> > > > 2) You are relying on Xen + Dom0 to boot (or at least enough to get the
> > > > watchdog working) within the watchdog interval.
> > >
> > > Definitely we need to consider the case where there is no Dom0.
> > >
> > > I think there are in fact 3 different use cases here:
> > > - watchdog fully driven in a domain (dom0 or another): would work if it is
> > > expected
> > > that Xen + Domain boot time is under the watchdog initial refresh
> > > rate. I think this
> > > could make sense on some applications where your system depends on a
> > > specific
> > > domain to be properly booted to work. This would require an initial
> > > refresh time
> > > configurable in the boot loader probably.
> >
> > This is our use-case. ^
> >
> > Our dom0 is monitoring and managing the other domains in our system.
> > Without dom0 working the system isn't really working as a whole.
> >
> > @Julien: Would you be ok with the patch set continuing in the direction
> > of the
> > original proposal, letting another party (or me at a later time) implement
> > the fully driven by Xen option?
> I am concerned about this particular point from Bertrand:
>
> "would work if it is expected that Xen + Domain boot time is under the
> watchdog initial refresh rate."
>
> How will a user be able to figure out how to initially configure the watchdog?
> Is this something you can easily configure in the bootloader at runtime?
>
>
> Overall, I am not for this approach at least in the current status. I would be
> more inclined if there are some documentations explaining how this is supposed
> to be configured on NXP, so others can use the code.
>
> Anyway, this is why we have multiple Arm maintainers for this kind of
> situation. If they other agrees with you, then they can ack the patch and this
> can be merged.


The approach here would not be my choice either. However, I think it
would be nice to have better support for NXP imx8 boards in upstream
Xen. To that end, I would ack these patches but I would ask to add a
document under xen.git/docs/ explaining the approach, limitations, and
requirements, so that someone else can use the code effectively.
RE: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
peng.fan at nxp
Mar 20, 2024, 6:09 PM
Post #22 of 27 (43 views)
Permalink
> Subject: Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue
>
> On Wed, 20 Mar 2024, Julien Grall wrote:
> > Hi John,
> >
> > On 20/03/2024 16:24, John Ernberg wrote:
> > > Hi Bertrand,
> > >
> > > On 3/13/24 11:07, Bertrand Marquis wrote:
> > > > Hi,
> > > >
> > > > > On 8 Mar 2024, at 15:04, Julien Grall <julien@xen.org> wrote:
> > > > >
> > > > > Hi John,
> > > > >
> > > > > Thank you for the reply.
> > > > >
> > > > > On 08/03/2024 13:40, John Ernberg wrote:
> > > > > > On 3/7/24 00:07, Julien Grall wrote:
> > > > > > > > Ping on the watchdog discussion bits.
> > > > > > >
> > > > > > > Sorry for the late reply.
> > > > > > >
> > > > > > > On 06/03/2024 13:13, John Ernberg wrote:
> > > > > > > > On 2/9/24 14:14, John Ernberg wrote:
> > > > > > > > >
> > > > > > > > > > * 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.
> > > > > > >
> > > > > > > include/watchdog.h contains helper to configure the watchdog
> > > > > > > for Xen. We also have per-VM watchdog and this is configured
> > > > > > > by the hypercall SCHEDOP_watchdog.
> > > > > > >
> > > > > > > > >
> > > > > > > > > Currently forwarding it to Dom0 has not caused any
> > > > > > > > > watchdog resets with our watchdog timeout settings, our
> > > > > > > > > specific Dom0 setup and VM count.
> > > > > > >
> > > > > > > IIUC, the SMC API for the watchdog would be similar to the
> > > > > > > ACPI NMI watchdog. So I think it would make more sense if
> > > > > > > this is not exposed to
> > > > > > > dom0 (even if Xen is doing nothing with it).
> > > > > > >
> > > > > > > Can you try to hide the SMCs and check if dom0 still behave
> > > > > > > properly?
> > > > > > >
> > > > > > > Cheers,
> > > > > > >
> > > > > > This SMC manages a hardware watchdog, if it's not pinged
> > > > > > within a specific interval the entire board resets.
> > > > >
> > > > > Do you know what's the default interval? Is it large enough so
> > > > > Xen +
> > > > > dom0 can boot (at least up to when the watchdog driver is initialized)?
> > > > >
> > > > > > If I block the SMCs the watchdog driver in Dom0 will fail to
> > > > > > ping the watchdog, triggering a board reset because the system
> > > > > > looks to have become unresponsive. The reason this patch set
> > > > > > started is because we couldn't ping the watchdog when running with
> Xen.
> > > > > > In our specific system the bootloader enables the watchdog as
> > > > > > early as possible so that we can get watchdog protection for
> > > > > > as much of the boot as we possibly can.
> > > > > > So, if we are to block the SMC from Dom0, then Xen needs to
> > > > > > take over the pinging. It could be implemented similarly to
> > > > > > the NMI watchdog, except that the system will reset if the
> > > > > > ping is missed rather than backtrace.
> > > > > > It would also mean that Xen will get a whole watchdog
> > > > > > driver-category due to the watchdog being vendor and sometimes
> > > > > > even SoC specific when it comes to Arm.
> > > > > > My understanding of the domain watchdog code is that today the
> > > > > > domain needs to call SCHEDOP_watchdog at least once to start
> > > > > > the watchdog timer. Since watchdog protection through the
> > > > > > whole boot process is desirable we'd need some core changes,
> > > > > > such as an option to start the domain watchdog on init. > It's
> > > > > > quite a big change to make
> > > > >
> > > > > For clarification, above you seem to mention two changes:
> > > > >
> > > > > 1) Allow Xen to use the HW watchdog
> > > > > 2) Allow the domain to use the watchdog early
> > > > >
> > > > > I am assuming by big change, you are referring to 2?
> > > > >
> > > > > , while I am not against doing it if it
> > > > > > makes sense, I now wonder if Xen should manage hardware
> watchdogs.
> > > > > > Looking at the domain watchdog code it looks like if a domain
> > > > > > does not get enough execution time, the watchdog will not be
> > > > > > pinged enough and the guest will be reset. So either watchdog
> > > > > > approach requires Dom0 to get execution time. Dom0 also needs
> > > > > > to service all the PV backends it's responsible for. I'm not
> > > > > > sure it's valuable to add another layer of watchdog for this
> > > > > > scenario as the end result (checking that the entire system
> > > > > > works) is achieved without it as well.
> > > > > > So, before I try to find the time to make a proposal for
> > > > > > moving the hardware watchdog bit to Xen, do we really want it?
> > > > >
> > > > > Thanks for the details. Given that the watchdog is enabled by
> > > > > the bootloader, I think we want Xen to drive the watchdog for two
> reasons:
> > > > > 1) In true dom0less environment, dom0 would not exist
> > > > > 2) You are relying on Xen + Dom0 to boot (or at least enough to
> > > > > get the watchdog working) within the watchdog interval.
> > > >
> > > > Definitely we need to consider the case where there is no Dom0.
> > > >
> > > > I think there are in fact 3 different use cases here:
> > > > - watchdog fully driven in a domain (dom0 or another): would work
> > > > if it is expected
> > > > that Xen + Domain boot time is under the watchdog initial
> > > > refresh rate. I think this
> > > > could make sense on some applications where your system
> > > > depends on a specific
> > > > domain to be properly booted to work. This would require an
> > > > initial refresh time
> > > > configurable in the boot loader probably.
> > >
> > > This is our use-case. ^
> > >
> > > Our dom0 is monitoring and managing the other domains in our system.
> > > Without dom0 working the system isn't really working as a whole.
> > >
> > > @Julien: Would you be ok with the patch set continuing in the
> > > direction of the original proposal, letting another party (or me at
> > > a later time) implement the fully driven by Xen option?
> > I am concerned about this particular point from Bertrand:
> >
> > "would work if it is expected that Xen + Domain boot time is under the
> > watchdog initial refresh rate."
> >
> > How will a user be able to figure out how to initially configure the watchdog?
> > Is this something you can easily configure in the bootloader at runtime?
> >
> >
> > Overall, I am not for this approach at least in the current status. I
> > would be more inclined if there are some documentations explaining how
> > this is supposed to be configured on NXP, so others can use the code.

I will try to push inside NXP to release a documentation on SIP usage.
But not expect it will be released soon.
The SIP number is quite stable, and we not change the meaning.

Regards,
Peng.

> >
> > Anyway, this is why we have multiple Arm maintainers for this kind of
> > situation. If they other agrees with you, then they can ack the patch
> > and this can be merged.
>
>
> The approach here would not be my choice either. However, I think it would
> be nice to have better support for NXP imx8 boards in upstream Xen. To that
> end, I would ack these patches but I would ask to add a document under
> xen.git/docs/ explaining the approach, limitations, and requirements, so that
> someone else can use the code effectively.
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
Bertrand.Marquis at arm
Mar 21, 2024, 12:41 AM
Post #23 of 27 (43 views)
Permalink
Hi,

> On 20 Mar 2024, at 18:40, Julien Grall <julien@xen.org> wrote:
>
> Hi John,
>
> On 20/03/2024 16:24, John Ernberg wrote:
>> Hi Bertrand,
>> On 3/13/24 11:07, Bertrand Marquis wrote:
>>> Hi,
>>>
>>>> On 8 Mar 2024, at 15:04, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi John,
>>>>
>>>> Thank you for the reply.
>>>>
>>>> On 08/03/2024 13:40, John Ernberg wrote:
>>>>> On 3/7/24 00:07, Julien Grall wrote:
>>>>>> > Ping on the watchdog discussion bits.
>>>>>>
>>>>>> Sorry for the late reply.
>>>>>>
>>>>>> On 06/03/2024 13:13, John Ernberg wrote:
>>>>>>> On 2/9/24 14:14, John Ernberg wrote:
>>>>>>>>
>>>>>>>>> * 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.
>>>>>>
>>>>>> include/watchdog.h contains helper to configure the watchdog for Xen. We
>>>>>> also have per-VM watchdog and this is configured by the hypercall
>>>>>> SCHEDOP_watchdog.
>>>>>>
>>>>>>>>
>>>>>>>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>>>>>>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>>>>>>
>>>>>> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
>>>>>> watchdog. So I think it would make more sense if this is not exposed to
>>>>>> dom0 (even if Xen is doing nothing with it).
>>>>>>
>>>>>> Can you try to hide the SMCs and check if dom0 still behave properly?
>>>>>>
>>>>>> Cheers,
>>>>>>
>>>>> This SMC manages a hardware watchdog, if it's not pinged within a
>>>>> specific interval the entire board resets.
>>>>
>>>> Do you know what's the default interval? Is it large enough so Xen + dom0 can boot (at least up to when the watchdog driver is initialized)?
>>>>
>>>>> If I block the SMCs the watchdog driver in Dom0 will fail to ping the
>>>>> watchdog, triggering a board reset because the system looks to have
>>>>> become unresponsive. The reason this patch set started is because we
>>>>> couldn't ping the watchdog when running with Xen.
>>>>> In our specific system the bootloader enables the watchdog as early as
>>>>> possible so that we can get watchdog protection for as much of the boot
>>>>> as we possibly can.
>>>>> So, if we are to block the SMC from Dom0, then Xen needs to take over
>>>>> the pinging. It could be implemented similarly to the NMI watchdog,
>>>>> except that the system will reset if the ping is missed rather than
>>>>> backtrace.
>>>>> It would also mean that Xen will get a whole watchdog driver-category
>>>>> due to the watchdog being vendor and sometimes even SoC specific when it
>>>>> comes to Arm.
>>>>> My understanding of the domain watchdog code is that today the domain
>>>>> needs to call SCHEDOP_watchdog at least once to start the watchdog
>>>>> timer. Since watchdog protection through the whole boot process is
>>>>> desirable we'd need some core changes, such as an option to start the
>>>>> domain watchdog on init. >
>>>>> It's quite a big change to make
>>>>
>>>> For clarification, above you seem to mention two changes:
>>>>
>>>> 1) Allow Xen to use the HW watchdog
>>>> 2) Allow the domain to use the watchdog early
>>>>
>>>> I am assuming by big change, you are referring to 2?
>>>>
>>>> , while I am not against doing it if it
>>>>> makes sense, I now wonder if Xen should manage hardware watchdogs.
>>>>> Looking at the domain watchdog code it looks like if a domain does not
>>>>> get enough execution time, the watchdog will not be pinged enough and
>>>>> the guest will be reset. So either watchdog approach requires Dom0 to
>>>>> get execution time. Dom0 also needs to service all the PV backends it's
>>>>> responsible for. I'm not sure it's valuable to add another layer of
>>>>> watchdog for this scenario as the end result (checking that the entire
>>>>> system works) is achieved without it as well.
>>>>> So, before I try to find the time to make a proposal for moving the
>>>>> hardware watchdog bit to Xen, do we really want it?
>>>>
>>>> Thanks for the details. Given that the watchdog is enabled by the bootloader, I think we want Xen to drive the watchdog for two reasons:
>>>> 1) In true dom0less environment, dom0 would not exist
>>>> 2) You are relying on Xen + Dom0 to boot (or at least enough to get the watchdog working) within the watchdog interval.
>>>
>>> Definitely we need to consider the case where there is no Dom0.
>>>
>>> I think there are in fact 3 different use cases here:
>>> - watchdog fully driven in a domain (dom0 or another): would work if it is expected
>>> that Xen + Domain boot time is under the watchdog initial refresh rate. I think this
>>> could make sense on some applications where your system depends on a specific
>>> domain to be properly booted to work. This would require an initial refresh time
>>> configurable in the boot loader probably.
>> This is our use-case. ^
>> Our dom0 is monitoring and managing the other domains in our system.
>> Without dom0 working the system isn't really working as a whole.
>> @Julien: Would you be ok with the patch set continuing in the direction
>> of the
>> original proposal, letting another party (or me at a later time) implement
>> the fully driven by Xen option?
> I am concerned about this particular point from Bertrand:
>
> "would work if it is expected that Xen + Domain boot time is under the watchdog initial refresh rate."
>
> How will a user be able to figure out how to initially configure the watchdog? Is this something you can easily configure in the bootloader at runtime?

Definitely here it would be better to have the watchdog turned off by default and document how to enable it in the firmware.

Even if a long timeout is configured by default, a user could run into trouble if using a linux without watchdog or not running linux or using dom0less without a linux having access to it.
I agree with Julien here that the concern could be that users would come to us instead of NXP if there is system is doing a reset without reasons after some seconds or minutes.

>
>
> Overall, I am not for this approach at least in the current status. I would be more inclined if there are some documentations explaining how this is supposed to be configured on NXP, so others can use the code.
>
> Anyway, this is why we have multiple Arm maintainers for this kind of situation. If they other agrees with you, then they can ack the patch and this can be merged.

I agree with Stefano that it would be good to have those board supported.

One thing i could suggest until there is a watchdog driver inside Xen is to have a clear Warning at Xen boot on those boards in the console so that we could at least identify the reason easily if a reset occurs for someone.

Cheers
Bertrand

>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
john.ernberg at actia
Mar 21, 2024, 9:05 AM
Post #24 of 27 (43 views)
Permalink
Hi Bertrand,

On 3/21/24 08:41, Bertrand Marquis wrote:
> Hi,
>
>> On 20 Mar 2024, at 18:40, Julien Grall <julien@xen.org> wrote:
>>
>> Hi John,
>>
>> On 20/03/2024 16:24, John Ernberg wrote:
>>> Hi Bertrand,
>>> On 3/13/24 11:07, Bertrand Marquis wrote:
>>>> Hi,
>>>>
>>>>> On 8 Mar 2024, at 15:04, Julien Grall <julien@xen.org> wrote:
>>>>>
>>>>> Hi John,
>>>>>
>>>>> Thank you for the reply.
>>>>>
>>>>> On 08/03/2024 13:40, John Ernberg wrote:
>>>>>> On 3/7/24 00:07, Julien Grall wrote:
>>>>>>> > Ping on the watchdog discussion bits.
>>>>>>>
>>>>>>> Sorry for the late reply.
>>>>>>>
>>>>>>> On 06/03/2024 13:13, John Ernberg wrote:
>>>>>>>> On 2/9/24 14:14, John Ernberg wrote:
>>>>>>>>>
>>>>>>>>>> * 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.
>>>>>>>
>>>>>>> include/watchdog.h contains helper to configure the watchdog for Xen. We
>>>>>>> also have per-VM watchdog and this is configured by the hypercall
>>>>>>> SCHEDOP_watchdog.
>>>>>>>
>>>>>>>>>
>>>>>>>>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>>>>>>>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>>>>>>>
>>>>>>> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
>>>>>>> watchdog. So I think it would make more sense if this is not exposed to
>>>>>>> dom0 (even if Xen is doing nothing with it).
>>>>>>>
>>>>>>> Can you try to hide the SMCs and check if dom0 still behave properly?
>>>>>>>
>>>>>>> Cheers,
>>>>>>>
>>>>>> This SMC manages a hardware watchdog, if it's not pinged within a
>>>>>> specific interval the entire board resets.
>>>>>
>>>>> Do you know what's the default interval? Is it large enough so Xen + dom0 can boot (at least up to when the watchdog driver is initialized)?
>>>>>
>>>>>> If I block the SMCs the watchdog driver in Dom0 will fail to ping the
>>>>>> watchdog, triggering a board reset because the system looks to have
>>>>>> become unresponsive. The reason this patch set started is because we
>>>>>> couldn't ping the watchdog when running with Xen.
>>>>>> In our specific system the bootloader enables the watchdog as early as
>>>>>> possible so that we can get watchdog protection for as much of the boot
>>>>>> as we possibly can.
>>>>>> So, if we are to block the SMC from Dom0, then Xen needs to take over
>>>>>> the pinging. It could be implemented similarly to the NMI watchdog,
>>>>>> except that the system will reset if the ping is missed rather than
>>>>>> backtrace.
>>>>>> It would also mean that Xen will get a whole watchdog driver-category
>>>>>> due to the watchdog being vendor and sometimes even SoC specific when it
>>>>>> comes to Arm.
>>>>>> My understanding of the domain watchdog code is that today the domain
>>>>>> needs to call SCHEDOP_watchdog at least once to start the watchdog
>>>>>> timer. Since watchdog protection through the whole boot process is
>>>>>> desirable we'd need some core changes, such as an option to start the
>>>>>> domain watchdog on init. >
>>>>>> It's quite a big change to make
>>>>>
>>>>> For clarification, above you seem to mention two changes:
>>>>>
>>>>> 1) Allow Xen to use the HW watchdog
>>>>> 2) Allow the domain to use the watchdog early
>>>>>
>>>>> I am assuming by big change, you are referring to 2?
>>>>>
>>>>> , while I am not against doing it if it
>>>>>> makes sense, I now wonder if Xen should manage hardware watchdogs.
>>>>>> Looking at the domain watchdog code it looks like if a domain does not
>>>>>> get enough execution time, the watchdog will not be pinged enough and
>>>>>> the guest will be reset. So either watchdog approach requires Dom0 to
>>>>>> get execution time. Dom0 also needs to service all the PV backends it's
>>>>>> responsible for. I'm not sure it's valuable to add another layer of
>>>>>> watchdog for this scenario as the end result (checking that the entire
>>>>>> system works) is achieved without it as well.
>>>>>> So, before I try to find the time to make a proposal for moving the
>>>>>> hardware watchdog bit to Xen, do we really want it?
>>>>>
>>>>> Thanks for the details. Given that the watchdog is enabled by the bootloader, I think we want Xen to drive the watchdog for two reasons:
>>>>> 1) In true dom0less environment, dom0 would not exist
>>>>> 2) You are relying on Xen + Dom0 to boot (or at least enough to get the watchdog working) within the watchdog interval.
>>>>
>>>> Definitely we need to consider the case where there is no Dom0.
>>>>
>>>> I think there are in fact 3 different use cases here:
>>>> - watchdog fully driven in a domain (dom0 or another): would work if it is expected
>>>> that Xen + Domain boot time is under the watchdog initial refresh rate. I think this
>>>> could make sense on some applications where your system depends on a specific
>>>> domain to be properly booted to work. This would require an initial refresh time
>>>> configurable in the boot loader probably.
>>> This is our use-case. ^
>>> Our dom0 is monitoring and managing the other domains in our system.
>>> Without dom0 working the system isn't really working as a whole.
>>> @Julien: Would you be ok with the patch set continuing in the direction
>>> of the
>>> original proposal, letting another party (or me at a later time) implement
>>> the fully driven by Xen option?
>> I am concerned about this particular point from Bertrand:
>>
>> "would work if it is expected that Xen + Domain boot time is under the watchdog initial refresh rate."
>>
>> How will a user be able to figure out how to initially configure the watchdog? Is this something you can easily configure in the bootloader at runtime?

If you go through the trouble of enabling the watchdog in the bootloader you
usually know what you're doing and set the timeout yourself.

Since our systems can be mounted in really annoying locations (both in
installations and world-wise) we need as much auto-recovery as possible to
avoid people having to travel to collect a unit that just needed a reset due
to some unexpected obscure issue at startup.
>
> Definitely here it would be better to have the watchdog turned off by default and document how to enable it in the firmware.
>
> Even if a long timeout is configured by default, a user could run into trouble if using a linux without watchdog or not running linux or using dom0less without a linux having access to it.
> I agree with Julien here that the concern could be that users would come to us instead of NXP if there is system is doing a reset without reasons after some seconds or minutes.

I could add myself as a reviewer for the iMX parts if that helps routing
such
questions (and future patches) also to me. We have experience with the QXP,
and the QM (for the supported parts by this patch set) is identical.

Would that help with the concerns?

>
>>
>>
>> Overall, I am not for this approach at least in the current status. I would be more inclined if there are some documentations explaining how this is supposed to be configured on NXP, so others can use the code.
>>
>> Anyway, this is why we have multiple Arm maintainers for this kind of situation. If they other agrees with you, then they can ack the patch and this can be merged.
>
> I agree with Stefano that it would be good to have those board supported.
>
> One thing i could suggest until there is a watchdog driver inside Xen is to have a clear Warning at Xen boot on those boards in the console so that we could at least identify the reason easily if a reset occurs for someone.

How do other SoCs deal with this currently? The iMX SoCs aren't the only
ones
with a watchdog, just the first one added to Xen that pings the watchdog
over
an SMC. What about the OMAPs, the R-Cars, Xilinx's, Exynos' and so on?

Thanks! // John Ernberg
Re: [PATCH 1/2] xen/arm: Add imx8q{m,x} platform glue [ In reply to ]
Bertrand.Marquis at arm
Mar 21, 2024, 9:15 AM
Post #25 of 27 (43 views)
Permalink
Hi John,

> On 21 Mar 2024, at 17:05, John Ernberg <john.ernberg@actia.se> wrote:
>
> Hi Bertrand,
>
> On 3/21/24 08:41, Bertrand Marquis wrote:
>> Hi,
>>
>>> On 20 Mar 2024, at 18:40, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi John,
>>>
>>> On 20/03/2024 16:24, John Ernberg wrote:
>>>> Hi Bertrand,
>>>> On 3/13/24 11:07, Bertrand Marquis wrote:
>>>>> Hi,
>>>>>
>>>>>> On 8 Mar 2024, at 15:04, Julien Grall <julien@xen.org> wrote:
>>>>>>
>>>>>> Hi John,
>>>>>>
>>>>>> Thank you for the reply.
>>>>>>
>>>>>> On 08/03/2024 13:40, John Ernberg wrote:
>>>>>>> On 3/7/24 00:07, Julien Grall wrote:
>>>>>>>>> Ping on the watchdog discussion bits.
>>>>>>>>
>>>>>>>> Sorry for the late reply.
>>>>>>>>
>>>>>>>> On 06/03/2024 13:13, John Ernberg wrote:
>>>>>>>>> On 2/9/24 14:14, John Ernberg wrote:
>>>>>>>>>>
>>>>>>>>>>> * 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.
>>>>>>>>
>>>>>>>> include/watchdog.h contains helper to configure the watchdog for Xen. We
>>>>>>>> also have per-VM watchdog and this is configured by the hypercall
>>>>>>>> SCHEDOP_watchdog.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Currently forwarding it to Dom0 has not caused any watchdog resets with
>>>>>>>>>> our watchdog timeout settings, our specific Dom0 setup and VM count.
>>>>>>>>
>>>>>>>> IIUC, the SMC API for the watchdog would be similar to the ACPI NMI
>>>>>>>> watchdog. So I think it would make more sense if this is not exposed to
>>>>>>>> dom0 (even if Xen is doing nothing with it).
>>>>>>>>
>>>>>>>> Can you try to hide the SMCs and check if dom0 still behave properly?
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>>
>>>>>>> This SMC manages a hardware watchdog, if it's not pinged within a
>>>>>>> specific interval the entire board resets.
>>>>>>
>>>>>> Do you know what's the default interval? Is it large enough so Xen + dom0 can boot (at least up to when the watchdog driver is initialized)?
>>>>>>
>>>>>>> If I block the SMCs the watchdog driver in Dom0 will fail to ping the
>>>>>>> watchdog, triggering a board reset because the system looks to have
>>>>>>> become unresponsive. The reason this patch set started is because we
>>>>>>> couldn't ping the watchdog when running with Xen.
>>>>>>> In our specific system the bootloader enables the watchdog as early as
>>>>>>> possible so that we can get watchdog protection for as much of the boot
>>>>>>> as we possibly can.
>>>>>>> So, if we are to block the SMC from Dom0, then Xen needs to take over
>>>>>>> the pinging. It could be implemented similarly to the NMI watchdog,
>>>>>>> except that the system will reset if the ping is missed rather than
>>>>>>> backtrace.
>>>>>>> It would also mean that Xen will get a whole watchdog driver-category
>>>>>>> due to the watchdog being vendor and sometimes even SoC specific when it
>>>>>>> comes to Arm.
>>>>>>> My understanding of the domain watchdog code is that today the domain
>>>>>>> needs to call SCHEDOP_watchdog at least once to start the watchdog
>>>>>>> timer. Since watchdog protection through the whole boot process is
>>>>>>> desirable we'd need some core changes, such as an option to start the
>>>>>>> domain watchdog on init. >
>>>>>>> It's quite a big change to make
>>>>>>
>>>>>> For clarification, above you seem to mention two changes:
>>>>>>
>>>>>> 1) Allow Xen to use the HW watchdog
>>>>>> 2) Allow the domain to use the watchdog early
>>>>>>
>>>>>> I am assuming by big change, you are referring to 2?
>>>>>>
>>>>>> , while I am not against doing it if it
>>>>>>> makes sense, I now wonder if Xen should manage hardware watchdogs.
>>>>>>> Looking at the domain watchdog code it looks like if a domain does not
>>>>>>> get enough execution time, the watchdog will not be pinged enough and
>>>>>>> the guest will be reset. So either watchdog approach requires Dom0 to
>>>>>>> get execution time. Dom0 also needs to service all the PV backends it's
>>>>>>> responsible for. I'm not sure it's valuable to add another layer of
>>>>>>> watchdog for this scenario as the end result (checking that the entire
>>>>>>> system works) is achieved without it as well.
>>>>>>> So, before I try to find the time to make a proposal for moving the
>>>>>>> hardware watchdog bit to Xen, do we really want it?
>>>>>>
>>>>>> Thanks for the details. Given that the watchdog is enabled by the bootloader, I think we want Xen to drive the watchdog for two reasons:
>>>>>> 1) In true dom0less environment, dom0 would not exist
>>>>>> 2) You are relying on Xen + Dom0 to boot (or at least enough to get the watchdog working) within the watchdog interval.
>>>>>
>>>>> Definitely we need to consider the case where there is no Dom0.
>>>>>
>>>>> I think there are in fact 3 different use cases here:
>>>>> - watchdog fully driven in a domain (dom0 or another): would work if it is expected
>>>>> that Xen + Domain boot time is under the watchdog initial refresh rate. I think this
>>>>> could make sense on some applications where your system depends on a specific
>>>>> domain to be properly booted to work. This would require an initial refresh time
>>>>> configurable in the boot loader probably.
>>>> This is our use-case. ^
>>>> Our dom0 is monitoring and managing the other domains in our system.
>>>> Without dom0 working the system isn't really working as a whole.
>>>> @Julien: Would you be ok with the patch set continuing in the direction
>>>> of the
>>>> original proposal, letting another party (or me at a later time) implement
>>>> the fully driven by Xen option?
>>> I am concerned about this particular point from Bertrand:
>>>
>>> "would work if it is expected that Xen + Domain boot time is under the watchdog initial refresh rate."
>>>
>>> How will a user be able to figure out how to initially configure the watchdog? Is this something you can easily configure in the bootloader at runtime?
>
> If you go through the trouble of enabling the watchdog in the bootloader you
> usually know what you're doing and set the timeout yourself.
>
> Since our systems can be mounted in really annoying locations (both in
> installations and world-wise) we need as much auto-recovery as possible to
> avoid people having to travel to collect a unit that just needed a reset due
> to some unexpected obscure issue at startup.

I definitely get the need do not get me wrong.
I am just concerned by potential users having target restarting when using Xen because of that and not knowing why.

>>
>> Definitely here it would be better to have the watchdog turned off by default and document how to enable it in the firmware.
>>
>> Even if a long timeout is configured by default, a user could run into trouble if using a linux without watchdog or not running linux or using dom0less without a linux having access to it.
>> I agree with Julien here that the concern could be that users would come to us instead of NXP if there is system is doing a reset without reasons after some seconds or minutes.
>
> I could add myself as a reviewer for the iMX parts if that helps routing
> such
> questions (and future patches) also to me. We have experience with the QXP,
> and the QM (for the supported parts by this patch set) is identical.
>
> Would that help with the concerns?

Definitely it could help.

>
>>
>>>
>>>
>>> Overall, I am not for this approach at least in the current status. I would be more inclined if there are some documentations explaining how this is supposed to be configured on NXP, so others can use the code.
>>>
>>> Anyway, this is why we have multiple Arm maintainers for this kind of situation. If they other agrees with you, then they can ack the patch and this can be merged.
>>
>> I agree with Stefano that it would be good to have those board supported.
>>
>> One thing i could suggest until there is a watchdog driver inside Xen is to have a clear Warning at Xen boot on those boards in the console so that we could at least identify the reason easily if a reset occurs for someone.
>
> How do other SoCs deal with this currently? The iMX SoCs aren't the only
> ones
> with a watchdog, just the first one added to Xen that pings the watchdog
> over
> an SMC. What about the OMAPs, the R-Cars, Xilinx's, Exynos' and so on?

As far as I know the watchdog is usually not active until a driver activates it.
Which means that by default it will not fire.
Having it activated by the bootloader by default is not usual.
Now definitely on a lot of use cases the users are activating it in the booloader
but their systems are design for it.

Do I understand that the default boot loader configuration is not activating it on your side ?

Regards
Bertrand

>
> Thanks! // John Ernberg
1 2  View All

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal