[PATCH 0/2] Xen: ARM: Improved NXP iMX8 platform support

[PATCH 0/2] Xen: ARM: Improved NXP iMX8 platform support

[John Ernberg]

The iMX lpuart driver added at 44e17aa60d47 ("xen/arm: Add i.MX lpuart driver")
is not enough to boot a Linux based dom0 when certain drivers, such as the
watchdog driver, are enabled.

We're also adding iMX8QXP compatibles to allow Xen to use the UART on the
QXP variant as well.

John Ernberg (2):
  xen/arm: Add imx8q{m,x} platform glue
  xen/drivers: imx-lpuart: Add iMX8QXP compatible

 xen/arch/arm/platforms/Makefile |  1 +
 xen/arch/arm/platforms/imx8qm.c | 65 +++++++++++++++++++++++++++++++++
 xen/drivers/char/imx-lpuart.c   |  1 +
 3 files changed, 67 insertions(+)
 create mode 100644 xen/arch/arm/platforms/imx8qm.c

-- 
2.43.0

[PATCH 2/2] xen/drivers: imx-lpuart: Add iMX8QXP compatible

[John Ernberg]

Allow the uart to probe also with iMX8QXP. The ip-block is the same as in the QM,
only the compatible is needed.

Signed-off-by: John Ernberg <john.ernberg@actia.se>
---
 xen/drivers/char/imx-lpuart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c
index 77f70c2719..c85e81109a 100644
--- a/xen/drivers/char/imx-lpuart.c
+++ b/xen/drivers/char/imx-lpuart.c
@@ -257,6 +257,7 @@ static int __init imx_lpuart_init(struct dt_device_node *dev,
 static const struct dt_device_match imx_lpuart_dt_compat[] __initconst =
 {
     DT_MATCH_COMPATIBLE("fsl,imx8qm-lpuart"),
+    DT_MATCH_COMPATIBLE("fsl,imx8qxp-lpuart"),
     { /* sentinel */ },
 };
 
-- 
2.43.0

Re: [PATCH 2/2] xen/drivers: imx-lpuart: Add iMX8QXP compatible

[Julien Grall]

Hi John,

On 31/01/2024 11:50, John Ernberg wrote:
> Allow the uart to probe also with iMX8QXP. The ip-block is the same as in the QM,
> only the compatible is needed.
> 
> Signed-off-by: John Ernberg <john.ernberg@actia.se>

With one remark below:

Acked-by: Julien Grall <jgrall@amazon.com>

> ---
>   xen/drivers/char/imx-lpuart.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c
> index 77f70c2719..c85e81109a 100644
> --- a/xen/drivers/char/imx-lpuart.c
> +++ b/xen/drivers/char/imx-lpuart.c
> @@ -257,6 +257,7 @@ static int __init imx_lpuart_init(struct dt_device_node *dev,
>   static const struct dt_device_match imx_lpuart_dt_compat[] __initconst =
>   {
>       DT_MATCH_COMPATIBLE("fsl,imx8qm-lpuart"),
> +    DT_MATCH_COMPATIBLE("fsl,imx8qxp-lpuart"),

IIUC the binding, the Device-Tree node compatible should have both 
fsl,imx8qm-lpuart and fsl,imx8qxp-lpuart. In fact, the Linux driver 
doesn't recognize the first compatible.

So maybe we can remove the first one.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/2] xen/drivers: imx-lpuart: Add iMX8QXP compatible

[John Ernberg]

Hi Julien,

On 1/31/24 13:29, Julien Grall wrote:
> Hi John,
> 
> On 31/01/2024 11:50, John Ernberg wrote:
>> Allow the uart to probe also with iMX8QXP. The ip-block is the same as 
>> in the QM,
>> only the compatible is needed.
>>
>> Signed-off-by: John Ernberg <john.ernberg@actia.se>
> 
> With one remark below:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
>> ---
>>   xen/drivers/char/imx-lpuart.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/drivers/char/imx-lpuart.c 
>> b/xen/drivers/char/imx-lpuart.c
>> index 77f70c2719..c85e81109a 100644
>> --- a/xen/drivers/char/imx-lpuart.c
>> +++ b/xen/drivers/char/imx-lpuart.c
>> @@ -257,6 +257,7 @@ static int __init imx_lpuart_init(struct 
>> dt_device_node *dev,
>>   static const struct dt_device_match imx_lpuart_dt_compat[] 
>> __initconst =
>>   {
>>       DT_MATCH_COMPATIBLE("fsl,imx8qm-lpuart"),
>> +    DT_MATCH_COMPATIBLE("fsl,imx8qxp-lpuart"),
> 
> IIUC the binding, the Device-Tree node compatible should have both 
> fsl,imx8qm-lpuart and fsl,imx8qxp-lpuart. In fact, the Linux driver 
> doesn't recognize the first compatible.
> 
> So maybe we can remove the first one.

It's listed in the bindings but the Linux driver indeed never looks for 
qm, it should be safe to drop. I'll drop this in V2.

> 
> Cheers,
> 

Thanks! // John Ernberg

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

[John Ernberg]

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

[Julien Grall]

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

[John Ernberg]

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

[Peng Fan]

> 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

[John Ernberg]

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

[Julien Grall]

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

[John Ernberg]

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

[Julien Grall]

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

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

[Peng Fan]

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

[Julien Grall]

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

[John Ernberg]

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

[John Ernberg]

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

[Julien Grall]

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

[John Ernberg]

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

[Julien Grall]

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

[John Ernberg]

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

[Bertrand Marquis]

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

[John Ernberg]

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

[Julien Grall]

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

[Stefano Stabellini]

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

[Peng Fan]

> 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

[Bertrand Marquis]

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

[John Ernberg]

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

[Bertrand Marquis]

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

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

[John Ernberg]

Hi Bertrand,

On 3/21/24 17:15, Bertrand Marquis wrote:
> 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.

I'll figure out how to include myself in the MAINTAINERS file for the 
next spin.

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

We are using a bootloader called Punchboot [1] developed by one of our 
employees. With Punchboot you have to explicitly write the code to 
enable the watchdog yourself. In u-boot you need to enable the watchdog 
drivers and configure the watchdog first before it is started. I don't 
know how the situation is in other bootloaders such as BareBox.

Best regards // John Ernberg

[1]: https://github.com/jonasblixt/punchboot

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

[Bertrand Marquis]

Hi John,

> On 25 Mar 2024, at 13:18, John Ernberg <john.ernberg@actia.se> wrote:
> 
> Hi Bertrand,
> 
> On 3/21/24 17:15, Bertrand Marquis wrote:
>> 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.
> 
> I'll figure out how to include myself in the MAINTAINERS file for the 
> next spin.
> 

Thanks.

>> 
>>> 
>>>> 
>>>>> 
>>>>> 
>>>>> 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 ?
> 
> We are using a bootloader called Punchboot [1] developed by one of our 
> employees. With Punchboot you have to explicitly write the code to 
> enable the watchdog yourself. In u-boot you need to enable the watchdog 
> drivers and configure the watchdog first before it is started. I don't 
> know how the situation is in other bootloaders such as BareBox.

Then a user not knowing will not have it activated and will not encounter issues.
So all good from my point of view :-)

Regards
Bertrand


> 
> Best regards // John Ernberg
> 
> [1]: https://github.com/jonasblixt/punchboot