[PATCH v1 0/2] Fix ARM Generic Timer interrupt parsing

[PATCH v1 0/2] Fix ARM Generic Timer interrupt parsing

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

This 2-patch series fixes the parsing of the ARM Generic Timer
interrupts from the device tree.

If the generic timer interrupts order in the DT was different than
the expected order in Xen code, these interrupts would no longer be
correctly parsed and registered by Xen, and would result in boot failure.

This method with using "interrupt-names" for the generic timer interrupts
instead of having them hardcoded in the DTB in a specific order is the newer
approach already implemented in Linux. Xen did not have the necessary code for
this approach, and it has been implemented by the means of this patch series.

Functionality should remain the same if "interrupt-names" is not present in the
Generic Timer DTB node of the platform, but the interrupts should then still be
present in the expected "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt" order.
If "interrupt-names" is present, now it is also correctly handled.

Andrei Cherechesu (2):
  arch/arm: irq: Add platform_get_irq_byname() implementation
  arch/arm: time: Add support for parsing interrupts by names

 xen/arch/arm/include/asm/irq.h        |  2 ++
 xen/arch/arm/include/asm/time.h       |  3 ++-
 xen/arch/arm/irq.c                    | 14 +++++++++++
 xen/arch/arm/time.c                   | 26 +++++++++++++++++---
 xen/drivers/passthrough/arm/smmu-v3.c | 35 +++++----------------------
 5 files changed, 46 insertions(+), 34 deletions(-)

-- 
2.35.1

[PATCH v1 1/2] arch/arm: irq: Add platform_get_irq_byname() implementation

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Moved implementation for the function which parses the IRQs of a DT
node by the "interrupt-names" property from the SMMU-v3 driver
to the IRQ core code and made it non-static to be used as helper.

Also changed it to receive a "struct dt_device_node*" as parameter,
like its counterpart, platform_get_irq(). Updated its usage inside
the SMMU-v3 driver accordingly.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 xen/arch/arm/include/asm/irq.h        |  2 ++
 xen/arch/arm/irq.c                    | 14 +++++++++++
 xen/drivers/passthrough/arm/smmu-v3.c | 35 +++++----------------------
 3 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
index 245f49dcba..af94f41994 100644
--- a/xen/arch/arm/include/asm/irq.h
+++ b/xen/arch/arm/include/asm/irq.h
@@ -89,6 +89,8 @@ int irq_set_type(unsigned int irq, unsigned int type);
 
 int platform_get_irq(const struct dt_device_node *device, int index);
 
+int platform_get_irq_byname(struct dt_device_node *np, const char *name);
+
 void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
 
 /*
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 79718f68e7..ded495792b 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -718,6 +718,20 @@ int platform_get_irq(const struct dt_device_node *device, int index)
     return irq;
 }
 
+int platform_get_irq_byname(struct dt_device_node *np, const char *name)
+{
+	int index;
+
+	if ( unlikely(!name) )
+		return -EINVAL;
+
+	index = dt_property_match_string(np, "interrupt-names", name);
+	if ( index < 0 )
+		return index;
+
+	return platform_get_irq(np, index);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index d58c5cd0bf..bfdb62b395 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -200,30 +200,6 @@ static inline void dev_iommu_priv_set(struct device *dev, void *priv)
 	fwspec->iommu_priv = priv;
 }
 
-static int platform_get_irq_byname_optional(struct device *dev,
-				const char *name)
-{
-	int index, ret;
-	struct dt_device_node *np  = dev_to_dt(dev);
-
-	if (unlikely(!name))
-		return -EINVAL;
-
-	index = dt_property_match_string(np, "interrupt-names", name);
-	if (index < 0) {
-		dev_info(dev, "IRQ %s not found\n", name);
-		return index;
-	}
-
-	ret = platform_get_irq(np, index);
-	if (ret < 0) {
-		dev_err(dev, "failed to get irq index %d\n", index);
-		return -ENODEV;
-	}
-
-	return ret;
-}
-
 /* Start of Linux SMMUv3 code */
 static bool disable_bypass = 1;
 
@@ -2434,6 +2410,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	int irq, ret;
 	paddr_t ioaddr, iosize;
 	struct arm_smmu_device *smmu;
+	struct dt_device_node *np = dev_to_dt(pdev);
 
 	smmu = xzalloc(struct arm_smmu_device);
 	if (!smmu)
@@ -2451,7 +2428,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 
 	/* Base address */
-	ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
+	ret = dt_device_get_address(np, 0, &ioaddr, &iosize);
 	if (ret)
 		goto out_free_smmu;
 
@@ -2484,19 +2461,19 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 
 	/* Interrupt lines */
 
-	irq = platform_get_irq_byname_optional(pdev, "combined");
+	irq = platform_get_irq_byname(np, "combined");
 	if (irq > 0)
 		smmu->combined_irq = irq;
 	else {
-		irq = platform_get_irq_byname_optional(pdev, "eventq");
+		irq = platform_get_irq_byname(np, "eventq");
 		if (irq > 0)
 			smmu->evtq.q.irq = irq;
 
-		irq = platform_get_irq_byname_optional(pdev, "priq");
+		irq = platform_get_irq_byname(np, "priq");
 		if (irq > 0)
 			smmu->priq.q.irq = irq;
 
-		irq = platform_get_irq_byname_optional(pdev, "gerror");
+		irq = platform_get_irq_byname(np, "gerror");
 		if (irq > 0)
 			smmu->gerr_irq = irq;
 	}
-- 
2.35.1

[PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Added support for parsing the ARM generic timer interrupts DT
node by the "interrupt-names" property, if it is available.

If not available, the usual parsing based on the expected
IRQ order is performed.

Also added the "hyp-virt" PPI to the timer PPI list, even
though it's currently not in use. If the "hyp-virt" PPI is
not found, the hypervisor won't panic.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 xen/arch/arm/include/asm/time.h |  3 ++-
 xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
index 4b401c1110..49ad8c1a6d 100644
--- a/xen/arch/arm/include/asm/time.h
+++ b/xen/arch/arm/include/asm/time.h
@@ -82,7 +82,8 @@ enum timer_ppi
     TIMER_PHYS_NONSECURE_PPI = 1,
     TIMER_VIRT_PPI = 2,
     TIMER_HYP_PPI = 3,
-    MAX_TIMER_PPI = 4,
+    TIMER_HYP_VIRT_PPI = 4,
+    MAX_TIMER_PPI = 5,
 };
 
 /*
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 433d7be909..794da646d6 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
 
 static unsigned int timer_irq[MAX_TIMER_PPI];
 
+static const char *timer_irq_names[MAX_TIMER_PPI] = {
+    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
+    [TIMER_PHYS_NONSECURE_PPI] = "phys",
+    [TIMER_VIRT_PPI] = "virt",
+    [TIMER_HYP_PPI] = "hyp-phys",
+    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
+};
+
 unsigned int timer_get_irq(enum timer_ppi ppi)
 {
     ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
@@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
 {
     int res;
     unsigned int i;
+    bool has_names;
+
+    has_names = dt_property_read_bool(timer, "interrupt-names");
 
     /* Retrieve all IRQs for the timer */
     for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
     {
-        res = platform_get_irq(timer, i);
-
-        if ( res < 0 )
+        if ( has_names )
+            res = platform_get_irq_byname(timer, timer_irq_names[i]);
+        else
+            res = platform_get_irq(timer, i);
+
+        if ( res > 0 )
+            timer_irq[i] = res;
+        /* Do not panic if "hyp-virt" PPI is not found, since it's not
+         * currently used.
+         */
+        else if ( i != TIMER_HYP_VIRT_PPI )
             panic("Timer: Unable to retrieve IRQ %u from the device tree\n", i);
-        timer_irq[i] = res;
     }
 }
 
-- 
2.35.1

Re: [PATCH v1 0/2] Fix ARM Generic Timer interrupt parsing

[Bertrand Marquis]

Hi Andrei,

When submitting patches, please use the add_maintainer.pl script so that maintainers of the code
modified are added in CC.

> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
> 
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> This 2-patch series fixes the parsing of the ARM Generic Timer
> interrupts from the device tree.
> 
> If the generic timer interrupts order in the DT was different than
> the expected order in Xen code, these interrupts would no longer be
> correctly parsed and registered by Xen, and would result in boot failure.
> 
> This method with using "interrupt-names" for the generic timer interrupts
> instead of having them hardcoded in the DTB in a specific order is the newer
> approach already implemented in Linux. Xen did not have the necessary code for
> this approach, and it has been implemented by the means of this patch series.

Would mind giving a link to an example or the Linux documentation if there is one ?

Cheers
Bertrand

> 
> Functionality should remain the same if "interrupt-names" is not present in the
> Generic Timer DTB node of the platform, but the interrupts should then still be
> present in the expected "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt" order.
> If "interrupt-names" is present, now it is also correctly handled.
> 
> Andrei Cherechesu (2):
>  arch/arm: irq: Add platform_get_irq_byname() implementation
>  arch/arm: time: Add support for parsing interrupts by names
> 
> xen/arch/arm/include/asm/irq.h        |  2 ++
> xen/arch/arm/include/asm/time.h       |  3 ++-
> xen/arch/arm/irq.c                    | 14 +++++++++++
> xen/arch/arm/time.c                   | 26 +++++++++++++++++---
> xen/drivers/passthrough/arm/smmu-v3.c | 35 +++++----------------------
> 5 files changed, 46 insertions(+), 34 deletions(-)
> 
> -- 
> 2.35.1
> 
> 

Re: [PATCH v1 1/2] arch/arm: irq: Add platform_get_irq_byname() implementation

[Bertrand Marquis]

Hi Andrei,

> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
> 
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Moved implementation for the function which parses the IRQs of a DT
> node by the "interrupt-names" property from the SMMU-v3 driver
> to the IRQ core code and made it non-static to be used as helper.
> 
> Also changed it to receive a "struct dt_device_node*" as parameter,
> like its counterpart, platform_get_irq(). Updated its usage inside
> the SMMU-v3 driver accordingly.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/include/asm/irq.h        |  2 ++
> xen/arch/arm/irq.c                    | 14 +++++++++++
> xen/drivers/passthrough/arm/smmu-v3.c | 35 +++++----------------------
> 3 files changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/irq.h b/xen/arch/arm/include/asm/irq.h
> index 245f49dcba..af94f41994 100644
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -89,6 +89,8 @@ int irq_set_type(unsigned int irq, unsigned int type);
> 
> int platform_get_irq(const struct dt_device_node *device, int index);
> 
> +int platform_get_irq_byname(struct dt_device_node *np, const char *name);
> +
> void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
> 
> /*
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 79718f68e7..ded495792b 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -718,6 +718,20 @@ int platform_get_irq(const struct dt_device_node *device, int index)
>     return irq;
> }
> 
> +int platform_get_irq_byname(struct dt_device_node *np, const char *name)
> +{
> + int index;
> +
> + if ( unlikely(!name) )
> + return -EINVAL;
> +
> + index = dt_property_match_string(np, "interrupt-names", name);
> + if ( index < 0 )
> + return index;
> +
> + return platform_get_irq(np, index);
> +}
> +
> /*
>  * Local variables:
>  * mode: C
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index d58c5cd0bf..bfdb62b395 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -200,30 +200,6 @@ static inline void dev_iommu_priv_set(struct device *dev, void *priv)
> fwspec->iommu_priv = priv;
> }
> 
> -static int platform_get_irq_byname_optional(struct device *dev,
> - const char *name)
> -{
> - int index, ret;
> - struct dt_device_node *np  = dev_to_dt(dev);
> -
> - if (unlikely(!name))
> - return -EINVAL;
> -
> - index = dt_property_match_string(np, "interrupt-names", name);
> - if (index < 0) {
> - dev_info(dev, "IRQ %s not found\n", name);
> - return index;
> - }
> -
> - ret = platform_get_irq(np, index);
> - if (ret < 0) {
> - dev_err(dev, "failed to get irq index %d\n", index);
> - return -ENODEV;
> - }
> -
> - return ret;
> -}
> -
> /* Start of Linux SMMUv3 code */
> static bool disable_bypass = 1;
> 
> @@ -2434,6 +2410,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> int irq, ret;
> paddr_t ioaddr, iosize;
> struct arm_smmu_device *smmu;
> + struct dt_device_node *np = dev_to_dt(pdev);
> 
> smmu = xzalloc(struct arm_smmu_device);
> if (!smmu)
> @@ -2451,7 +2428,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> }
> 
> /* Base address */
> - ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
> + ret = dt_device_get_address(np, 0, &ioaddr, &iosize);
> if (ret)
> goto out_free_smmu;
> 
> @@ -2484,19 +2461,19 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> 
> /* Interrupt lines */
> 
> - irq = platform_get_irq_byname_optional(pdev, "combined");
> + irq = platform_get_irq_byname(np, "combined");
> if (irq > 0)
> smmu->combined_irq = irq;
> else {
> - irq = platform_get_irq_byname_optional(pdev, "eventq");
> + irq = platform_get_irq_byname(np, "eventq");
> if (irq > 0)
> smmu->evtq.q.irq = irq;
> 
> - irq = platform_get_irq_byname_optional(pdev, "priq");
> + irq = platform_get_irq_byname(np, "priq");
> if (irq > 0)
> smmu->priq.q.irq = irq;
> 
> - irq = platform_get_irq_byname_optional(pdev, "gerror");
> + irq = platform_get_irq_byname(np, "gerror");
> if (irq > 0)
> smmu->gerr_irq = irq;
> }
> -- 
> 2.35.1
> 
> 

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Bertrand Marquis]

Hi Andrei,

> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
> 
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Added support for parsing the ARM generic timer interrupts DT
> node by the "interrupt-names" property, if it is available.
> 
> If not available, the usual parsing based on the expected
> IRQ order is performed.
> 
> Also added the "hyp-virt" PPI to the timer PPI list, even
> though it's currently not in use. If the "hyp-virt" PPI is
> not found, the hypervisor won't panic.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> ---
> xen/arch/arm/include/asm/time.h |  3 ++-
> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
> 2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
> index 4b401c1110..49ad8c1a6d 100644
> --- a/xen/arch/arm/include/asm/time.h
> +++ b/xen/arch/arm/include/asm/time.h
> @@ -82,7 +82,8 @@ enum timer_ppi
>     TIMER_PHYS_NONSECURE_PPI = 1,
>     TIMER_VIRT_PPI = 2,
>     TIMER_HYP_PPI = 3,
> -    MAX_TIMER_PPI = 4,
> +    TIMER_HYP_VIRT_PPI = 4,
> +    MAX_TIMER_PPI = 5,
> };
> 
> /*
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 433d7be909..794da646d6 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
> 
> static unsigned int timer_irq[MAX_TIMER_PPI];
> 
> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
> +    [TIMER_VIRT_PPI] = "virt",
> +    [TIMER_HYP_PPI] = "hyp-phys",
> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
> +};
> +

I would need some reference or a pointer to some doc to check those.

> unsigned int timer_get_irq(enum timer_ppi ppi)
> {
>     ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
> {
>     int res;
>     unsigned int i;
> +    bool has_names;
> +
> +    has_names = dt_property_read_bool(timer, "interrupt-names");
> 
>     /* Retrieve all IRQs for the timer */
>     for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>     {
> -        res = platform_get_irq(timer, i);
> -
> -        if ( res < 0 )
> +        if ( has_names )
> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
> +        else
> +            res = platform_get_irq(timer, i);
> +
> +        if ( res > 0 )

The behaviour of the code is changed here compared to the current
version as res = 0 will now generate a panic.

Some device tree might not specify an interrupt number and just put
0 and Xen will now panic on those systems.
As I have no idea if such systems exists and the behaviour is modified
you should justify this and mention it in the commit message or keep
the old behaviour and let 0 go through without a panic.

@stefano, julien any idea here ? should just keep the old behaviour ?

> +            timer_irq[i] = res;
> +        /* Do not panic if "hyp-virt" PPI is not found, since it's not
> +         * currently used.
> +         */

Please respect the standard for comments and keep the first line empty:
/*
 * comment
 */

> +        else if ( i != TIMER_HYP_VIRT_PPI )
>             panic("Timer: Unable to retrieve IRQ %u from the device tree\n", i);
> -        timer_irq[i] = res;
>     }
> }

Cheers
Bertrand

> 
> -- 
> 2.35.1
> 
> 

Re: [PATCH v1 0/2] Fix ARM Generic Timer interrupt parsing

[Andrei Cherechesu]

On 07/03/2023 17:27, Bertrand Marquis wrote:
> Hi Andrei,
> 
> When submitting patches, please use the add_maintainer.pl script so that maintainers of the code
> modified are added in CC.

Hi Bertrand,

Thank you for reviewing the patches. I apologize for not adding the
maintainers in CC. I added them now.

> 
>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>
>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>
>> This 2-patch series fixes the parsing of the ARM Generic Timer
>> interrupts from the device tree.
>>
>> If the generic timer interrupts order in the DT was different than
>> the expected order in Xen code, these interrupts would no longer be
>> correctly parsed and registered by Xen, and would result in boot failure.
>>
>> This method with using "interrupt-names" for the generic timer interrupts
>> instead of having them hardcoded in the DTB in a specific order is the newer
>> approach already implemented in Linux. Xen did not have the necessary code for
>> this approach, and it has been implemented by the means of this patch series.
> 
> Would mind giving a link to an example or the Linux documentation if there is one ?
> 

The bindings [0] for the ARM Generic Timer DT node were changed around
Linux 5.13, when the interrupt-names property was added, along with the
implementation for handling it [1].


[0]
https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml#L44
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clocksource/arm_arch_timer.c?id=86332e9e3477af8f31c9d5f3e81e57e0fd2118e7


Regards,
Andrei


> Cheers
> Bertrand
> 
>>
>> Functionality should remain the same if "interrupt-names" is not present in the
>> Generic Timer DTB node of the platform, but the interrupts should then still be
>> present in the expected "sec-phys", "phys", "virt", "hyp-phys", "hyp-virt" order.
>> If "interrupt-names" is present, now it is also correctly handled.
>>
>> Andrei Cherechesu (2):
>>  arch/arm: irq: Add platform_get_irq_byname() implementation
>>  arch/arm: time: Add support for parsing interrupts by names
>>
>> xen/arch/arm/include/asm/irq.h        |  2 ++
>> xen/arch/arm/include/asm/time.h       |  3 ++-
>> xen/arch/arm/irq.c                    | 14 +++++++++++
>> xen/arch/arm/time.c                   | 26 +++++++++++++++++---
>> xen/drivers/passthrough/arm/smmu-v3.c | 35 +++++----------------------
>> 5 files changed, 46 insertions(+), 34 deletions(-)
>>
>> -- 
>> 2.35.1
>>
>>
> 

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Andrei Cherechesu]

On 07/03/2023 17:38, Bertrand Marquis wrote:
> Hi Andrei,
> 
>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>
>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>
>> Added support for parsing the ARM generic timer interrupts DT
>> node by the "interrupt-names" property, if it is available.
>>
>> If not available, the usual parsing based on the expected
>> IRQ order is performed.
>>
>> Also added the "hyp-virt" PPI to the timer PPI list, even
>> though it's currently not in use. If the "hyp-virt" PPI is
>> not found, the hypervisor won't panic.
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> ---
>> xen/arch/arm/include/asm/time.h |  3 ++-
>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>> index 4b401c1110..49ad8c1a6d 100644
>> --- a/xen/arch/arm/include/asm/time.h
>> +++ b/xen/arch/arm/include/asm/time.h
>> @@ -82,7 +82,8 @@ enum timer_ppi
>>     TIMER_PHYS_NONSECURE_PPI = 1,
>>     TIMER_VIRT_PPI = 2,
>>     TIMER_HYP_PPI = 3,
>> -    MAX_TIMER_PPI = 4,
>> +    TIMER_HYP_VIRT_PPI = 4,
>> +    MAX_TIMER_PPI = 5,
>> };
>>
>> /*
>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>> index 433d7be909..794da646d6 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>
>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>
>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>> +    [TIMER_VIRT_PPI] = "virt",
>> +    [TIMER_HYP_PPI] = "hyp-phys",
>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>> +};
>> +
> 
> I would need some reference or a pointer to some doc to check those.

Hi Bertrand,

This implementation follows the one in Linux [0]. The parsing order for
the IRQs remains the same whether or not the "interrupt-names" property
is available, since the driver in both Linux and Xen expects them in a
specific order (defined by enum arch_timer_ppi_nr in Linux, for example)
which, most of the time, does not correspond to how they are mapped onto
the SoC. But now it can discover them correctly regardless of their
order in the "interrupts" property in the DT node.

Only the "hyp-virt" IRQ is not required to be present, which is also the
last one parsed.

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/clocksource/arm_arch_timer.c?id=86332e9e3477af8f31c9d5f3e81e57e0fd2118e7

> 
>> unsigned int timer_get_irq(enum timer_ppi ppi)
>> {
>>     ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>> {
>>     int res;
>>     unsigned int i;
>> +    bool has_names;
>> +
>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>
>>     /* Retrieve all IRQs for the timer */
>>     for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>     {
>> -        res = platform_get_irq(timer, i);
>> -
>> -        if ( res < 0 )
>> +        if ( has_names )
>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>> +        else
>> +            res = platform_get_irq(timer, i);
>> +
>> +        if ( res > 0 )
> 
> The behaviour of the code is changed here compared to the current
> version as res = 0 will now generate a panic.
> 
> Some device tree might not specify an interrupt number and just put
> 0 and Xen will now panic on those systems.
> As I have no idea if such systems exists and the behaviour is modified
> you should justify this and mention it in the commit message or keep
> the old behaviour and let 0 go through without a panic.
> 
> @stefano, julien any idea here ? should just keep the old behaviour ?
> 

You're right, I didn't take the dummy fake interrupts case into
consideration. I also think we should keep the old behaviour then, and
let 0 go through too, as you mentioned.


>> +            timer_irq[i] = res;
>> +        /* Do not panic if "hyp-virt" PPI is not found, since it's not
>> +         * currently used.
>> +         */
> 
> Please respect the standard for comments and keep the first line empty:
> /*
>  * comment
>  */
> 

Will update in v2.

>> +        else if ( i != TIMER_HYP_VIRT_PPI )
>>             panic("Timer: Unable to retrieve IRQ %u from the device tree\n", i);
>> -        timer_irq[i] = res;
>>     }
>> }
> 
> Cheers
> Bertrand
> 

Thanks for the review.

Regards,
Andrei

>>
>> -- 
>> 2.35.1
>>
>>
> 

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Stefano Stabellini]

On Tue, 7 Mar 2023, Bertrand Marquis wrote:
> > On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
> > 
> > From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> > 
> > Added support for parsing the ARM generic timer interrupts DT
> > node by the "interrupt-names" property, if it is available.
> > 
> > If not available, the usual parsing based on the expected
> > IRQ order is performed.
> > 
> > Also added the "hyp-virt" PPI to the timer PPI list, even
> > though it's currently not in use. If the "hyp-virt" PPI is
> > not found, the hypervisor won't panic.
> > 
> > Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> > ---
> > xen/arch/arm/include/asm/time.h |  3 ++-
> > xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
> > 2 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
> > index 4b401c1110..49ad8c1a6d 100644
> > --- a/xen/arch/arm/include/asm/time.h
> > +++ b/xen/arch/arm/include/asm/time.h
> > @@ -82,7 +82,8 @@ enum timer_ppi
> >     TIMER_PHYS_NONSECURE_PPI = 1,
> >     TIMER_VIRT_PPI = 2,
> >     TIMER_HYP_PPI = 3,
> > -    MAX_TIMER_PPI = 4,
> > +    TIMER_HYP_VIRT_PPI = 4,
> > +    MAX_TIMER_PPI = 5,
> > };
> > 
> > /*
> > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> > index 433d7be909..794da646d6 100644
> > --- a/xen/arch/arm/time.c
> > +++ b/xen/arch/arm/time.c
> > @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
> > 
> > static unsigned int timer_irq[MAX_TIMER_PPI];
> > 
> > +static const char *timer_irq_names[MAX_TIMER_PPI] = {
> > +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
> > +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
> > +    [TIMER_VIRT_PPI] = "virt",
> > +    [TIMER_HYP_PPI] = "hyp-phys",
> > +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
> > +};
> > +
> 
> I would need some reference or a pointer to some doc to check those.
> 
> > unsigned int timer_get_irq(enum timer_ppi ppi)
> > {
> >     ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
> > @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
> > {
> >     int res;
> >     unsigned int i;
> > +    bool has_names;
> > +
> > +    has_names = dt_property_read_bool(timer, "interrupt-names");
> > 
> >     /* Retrieve all IRQs for the timer */
> >     for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
> >     {
> > -        res = platform_get_irq(timer, i);
> > -
> > -        if ( res < 0 )
> > +        if ( has_names )
> > +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
> > +        else
> > +            res = platform_get_irq(timer, i);
> > +
> > +        if ( res > 0 )
> 
> The behaviour of the code is changed here compared to the current
> version as res = 0 will now generate a panic.
> 
> Some device tree might not specify an interrupt number and just put
> 0 and Xen will now panic on those systems.
> As I have no idea if such systems exists and the behaviour is modified
> you should justify this and mention it in the commit message or keep
> the old behaviour and let 0 go through without a panic.
> 
> @stefano, julien any idea here ? should just keep the old behaviour ?

platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
error.

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Bertrand Marquis]

Hi Stefano,

> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>> 
>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>> 
>>> Added support for parsing the ARM generic timer interrupts DT
>>> node by the "interrupt-names" property, if it is available.
>>> 
>>> If not available, the usual parsing based on the expected
>>> IRQ order is performed.
>>> 
>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>> though it's currently not in use. If the "hyp-virt" PPI is
>>> not found, the hypervisor won't panic.
>>> 
>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>> ---
>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>> index 4b401c1110..49ad8c1a6d 100644
>>> --- a/xen/arch/arm/include/asm/time.h
>>> +++ b/xen/arch/arm/include/asm/time.h
>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>    TIMER_PHYS_NONSECURE_PPI = 1,
>>>    TIMER_VIRT_PPI = 2,
>>>    TIMER_HYP_PPI = 3,
>>> -    MAX_TIMER_PPI = 4,
>>> +    TIMER_HYP_VIRT_PPI = 4,
>>> +    MAX_TIMER_PPI = 5,
>>> };
>>> 
>>> /*
>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>> index 433d7be909..794da646d6 100644
>>> --- a/xen/arch/arm/time.c
>>> +++ b/xen/arch/arm/time.c
>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>> 
>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>> 
>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>> +    [TIMER_VIRT_PPI] = "virt",
>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>> +};
>>> +
>> 
>> I would need some reference or a pointer to some doc to check those.
>> 
>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>> {
>>>    ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>> {
>>>    int res;
>>>    unsigned int i;
>>> +    bool has_names;
>>> +
>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>> 
>>>    /* Retrieve all IRQs for the timer */
>>>    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>    {
>>> -        res = platform_get_irq(timer, i);
>>> -
>>> -        if ( res < 0 )
>>> +        if ( has_names )
>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>> +        else
>>> +            res = platform_get_irq(timer, i);
>>> +
>>> +        if ( res > 0 )
>> 
>> The behaviour of the code is changed here compared to the current
>> version as res = 0 will now generate a panic.
>> 
>> Some device tree might not specify an interrupt number and just put
>> 0 and Xen will now panic on those systems.
>> As I have no idea if such systems exists and the behaviour is modified
>> you should justify this and mention it in the commit message or keep
>> the old behaviour and let 0 go through without a panic.
>> 
>> @stefano, julien any idea here ? should just keep the old behaviour ?
> 
> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
> error.

Problem here is that a DTB might not specify all interrupts and just put
0 for the one not used (or not available for example if you have no secure
world).

So I think we need to keep the current behaviour, might be ok to put a 
debug print.
What I would think is feasible would be to panic for interrupt numbers we
need only.

Cheers
Bertrand

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Michal Orzel]

On 09/03/2023 09:02, Bertrand Marquis wrote:
> 
> 
> Hi Stefano,
> 
>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>
>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>
>>>> Added support for parsing the ARM generic timer interrupts DT
>>>> node by the "interrupt-names" property, if it is available.
>>>>
>>>> If not available, the usual parsing based on the expected
>>>> IRQ order is performed.
>>>>
>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>> not found, the hypervisor won't panic.
>>>>
>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>> ---
>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>> index 4b401c1110..49ad8c1a6d 100644
>>>> --- a/xen/arch/arm/include/asm/time.h
>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>    TIMER_PHYS_NONSECURE_PPI = 1,
>>>>    TIMER_VIRT_PPI = 2,
>>>>    TIMER_HYP_PPI = 3,
>>>> -    MAX_TIMER_PPI = 4,
>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>> +    MAX_TIMER_PPI = 5,
>>>> };
>>>>
>>>> /*
>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>> index 433d7be909..794da646d6 100644
>>>> --- a/xen/arch/arm/time.c
>>>> +++ b/xen/arch/arm/time.c
>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>
>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>
>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>> +};
>>>> +
>>>
>>> I would need some reference or a pointer to some doc to check those.
>>>
>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>> {
>>>>    ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>> {
>>>>    int res;
>>>>    unsigned int i;
>>>> +    bool has_names;
>>>> +
>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>
>>>>    /* Retrieve all IRQs for the timer */
>>>>    for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>    {
>>>> -        res = platform_get_irq(timer, i);
>>>> -
>>>> -        if ( res < 0 )
>>>> +        if ( has_names )
>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>> +        else
>>>> +            res = platform_get_irq(timer, i);
>>>> +
>>>> +        if ( res > 0 )
>>>
>>> The behaviour of the code is changed here compared to the current
>>> version as res = 0 will now generate a panic.
>>>
>>> Some device tree might not specify an interrupt number and just put
>>> 0 and Xen will now panic on those systems.
>>> As I have no idea if such systems exists and the behaviour is modified
>>> you should justify this and mention it in the commit message or keep
>>> the old behaviour and let 0 go through without a panic.
>>>
>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>
>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>> error.
> 
> Problem here is that a DTB might not specify all interrupts and just put
> 0 for the one not used (or not available for example if you have no secure
> world).
Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
according to Arm ARM:
- EL3 phys (if EL3 is there)
- EL1 phys (always)
- EL1 virt (always)
- NS EL2 phys (if EL2 is there)

therefore, if we get 0 for one of those, it means that something went wrong and we shall consider
it as an error.

~Michal

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Bertrand Marquis]

Hi Michal,

> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 09/03/2023 09:02, Bertrand Marquis wrote:
>> 
>> 
>> Hi Stefano,
>> 
>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>> 
>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>> 
>>>>> Added support for parsing the ARM generic timer interrupts DT
>>>>> node by the "interrupt-names" property, if it is available.
>>>>> 
>>>>> If not available, the usual parsing based on the expected
>>>>> IRQ order is performed.
>>>>> 
>>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>>> not found, the hypervisor won't panic.
>>>>> 
>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>> ---
>>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>> 
>>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>>> index 4b401c1110..49ad8c1a6d 100644
>>>>> --- a/xen/arch/arm/include/asm/time.h
>>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>>   TIMER_PHYS_NONSECURE_PPI = 1,
>>>>>   TIMER_VIRT_PPI = 2,
>>>>>   TIMER_HYP_PPI = 3,
>>>>> -    MAX_TIMER_PPI = 4,
>>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>>> +    MAX_TIMER_PPI = 5,
>>>>> };
>>>>> 
>>>>> /*
>>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>>> index 433d7be909..794da646d6 100644
>>>>> --- a/xen/arch/arm/time.c
>>>>> +++ b/xen/arch/arm/time.c
>>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>> 
>>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>> 
>>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>>> +};
>>>>> +
>>>> 
>>>> I would need some reference or a pointer to some doc to check those.
>>>> 
>>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>>> {
>>>>>   ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>>> {
>>>>>   int res;
>>>>>   unsigned int i;
>>>>> +    bool has_names;
>>>>> +
>>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>> 
>>>>>   /* Retrieve all IRQs for the timer */
>>>>>   for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>>   {
>>>>> -        res = platform_get_irq(timer, i);
>>>>> -
>>>>> -        if ( res < 0 )
>>>>> +        if ( has_names )
>>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>>> +        else
>>>>> +            res = platform_get_irq(timer, i);
>>>>> +
>>>>> +        if ( res > 0 )
>>>> 
>>>> The behaviour of the code is changed here compared to the current
>>>> version as res = 0 will now generate a panic.
>>>> 
>>>> Some device tree might not specify an interrupt number and just put
>>>> 0 and Xen will now panic on those systems.
>>>> As I have no idea if such systems exists and the behaviour is modified
>>>> you should justify this and mention it in the commit message or keep
>>>> the old behaviour and let 0 go through without a panic.
>>>> 
>>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>> 
>>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>>> error.
>> 
>> Problem here is that a DTB might not specify all interrupts and just put
>> 0 for the one not used (or not available for example if you have no secure
>> world).
> Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
> according to Arm ARM:
> - EL3 phys (if EL3 is there)

This might be needed by EL3 but not by Xen.

> - EL1 phys (always)
> - EL1 virt (always)
> - NS EL2 phys (if EL2 is there)

Agree

> 
> therefore, if we get 0 for one of those, it means that something went wrong and we shall consider
> it as an error.

Agree except for the EL3 one :-)

Cheers
Bertrand

> 
> ~Michal

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Michal Orzel]

On 09/03/2023 11:39, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>
>>>
>>> Hi Stefano,
>>>
>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>
>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>
>>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>
>>>>>> Added support for parsing the ARM generic timer interrupts DT
>>>>>> node by the "interrupt-names" property, if it is available.
>>>>>>
>>>>>> If not available, the usual parsing based on the expected
>>>>>> IRQ order is performed.
>>>>>>
>>>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>>>> not found, the hypervisor won't panic.
>>>>>>
>>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>> ---
>>>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>>>> index 4b401c1110..49ad8c1a6d 100644
>>>>>> --- a/xen/arch/arm/include/asm/time.h
>>>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>>>   TIMER_PHYS_NONSECURE_PPI = 1,
>>>>>>   TIMER_VIRT_PPI = 2,
>>>>>>   TIMER_HYP_PPI = 3,
>>>>>> -    MAX_TIMER_PPI = 4,
>>>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>>>> +    MAX_TIMER_PPI = 5,
>>>>>> };
>>>>>>
>>>>>> /*
>>>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>>>> index 433d7be909..794da646d6 100644
>>>>>> --- a/xen/arch/arm/time.c
>>>>>> +++ b/xen/arch/arm/time.c
>>>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>>>
>>>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>>>
>>>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>>>> +};
>>>>>> +
>>>>>
>>>>> I would need some reference or a pointer to some doc to check those.
>>>>>
>>>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>>>> {
>>>>>>   ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>>>> {
>>>>>>   int res;
>>>>>>   unsigned int i;
>>>>>> +    bool has_names;
>>>>>> +
>>>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>>>
>>>>>>   /* Retrieve all IRQs for the timer */
>>>>>>   for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>>>   {
>>>>>> -        res = platform_get_irq(timer, i);
>>>>>> -
>>>>>> -        if ( res < 0 )
>>>>>> +        if ( has_names )
>>>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>>>> +        else
>>>>>> +            res = platform_get_irq(timer, i);
>>>>>> +
>>>>>> +        if ( res > 0 )
>>>>>
>>>>> The behaviour of the code is changed here compared to the current
>>>>> version as res = 0 will now generate a panic.
>>>>>
>>>>> Some device tree might not specify an interrupt number and just put
>>>>> 0 and Xen will now panic on those systems.
>>>>> As I have no idea if such systems exists and the behaviour is modified
>>>>> you should justify this and mention it in the commit message or keep
>>>>> the old behaviour and let 0 go through without a panic.
>>>>>
>>>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>>>
>>>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>>>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>>>> error.
>>>
>>> Problem here is that a DTB might not specify all interrupts and just put
>>> 0 for the one not used (or not available for example if you have no secure
>>> world).
>> Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
>> according to Arm ARM:
>> - EL3 phys (if EL3 is there)
> 
> This might be needed by EL3 but not by Xen.
Xen requires system with EL3 and if there is EL3, both Arm spec and dt bindings requires sec-phys timer to be there.
So it would be very strange to see a fake interrupt with IRQ being 0. But if we relly want to only care about
what Xen needs, then we could live with that (although it is difficult for me to find justification for 0 there).
Device trees are created per system and if system has EL3, then why forcing 0 to be listed for sec-phys timer?

~Michal

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Bertrand Marquis]

Hi Michal,

> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> 
> 
> On 09/03/2023 11:39, Bertrand Marquis wrote:
>> 
>> 
>> Hi Michal,
>> 
>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>> 
>>>> 
>>>> Hi Stefano,
>>>> 
>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>> 
>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>> 
>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>> 
>>>>>>> Added support for parsing the ARM generic timer interrupts DT
>>>>>>> node by the "interrupt-names" property, if it is available.
>>>>>>> 
>>>>>>> If not available, the usual parsing based on the expected
>>>>>>> IRQ order is performed.
>>>>>>> 
>>>>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>>>>> not found, the hypervisor won't panic.
>>>>>>> 
>>>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>> ---
>>>>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>>>>> index 4b401c1110..49ad8c1a6d 100644
>>>>>>> --- a/xen/arch/arm/include/asm/time.h
>>>>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>>>>  TIMER_PHYS_NONSECURE_PPI = 1,
>>>>>>>  TIMER_VIRT_PPI = 2,
>>>>>>>  TIMER_HYP_PPI = 3,
>>>>>>> -    MAX_TIMER_PPI = 4,
>>>>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>>>>> +    MAX_TIMER_PPI = 5,
>>>>>>> };
>>>>>>> 
>>>>>>> /*
>>>>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>>>>> index 433d7be909..794da646d6 100644
>>>>>>> --- a/xen/arch/arm/time.c
>>>>>>> +++ b/xen/arch/arm/time.c
>>>>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>>>> 
>>>>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>>>> 
>>>>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>>>>> +};
>>>>>>> +
>>>>>> 
>>>>>> I would need some reference or a pointer to some doc to check those.
>>>>>> 
>>>>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>>>>> {
>>>>>>>  ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>>>>> {
>>>>>>>  int res;
>>>>>>>  unsigned int i;
>>>>>>> +    bool has_names;
>>>>>>> +
>>>>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>>>> 
>>>>>>>  /* Retrieve all IRQs for the timer */
>>>>>>>  for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>>>>  {
>>>>>>> -        res = platform_get_irq(timer, i);
>>>>>>> -
>>>>>>> -        if ( res < 0 )
>>>>>>> +        if ( has_names )
>>>>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>>>>> +        else
>>>>>>> +            res = platform_get_irq(timer, i);
>>>>>>> +
>>>>>>> +        if ( res > 0 )
>>>>>> 
>>>>>> The behaviour of the code is changed here compared to the current
>>>>>> version as res = 0 will now generate a panic.
>>>>>> 
>>>>>> Some device tree might not specify an interrupt number and just put
>>>>>> 0 and Xen will now panic on those systems.
>>>>>> As I have no idea if such systems exists and the behaviour is modified
>>>>>> you should justify this and mention it in the commit message or keep
>>>>>> the old behaviour and let 0 go through without a panic.
>>>>>> 
>>>>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>>>> 
>>>>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>>>>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>>>>> error.
>>>> 
>>>> Problem here is that a DTB might not specify all interrupts and just put
>>>> 0 for the one not used (or not available for example if you have no secure
>>>> world).
>>> Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
>>> according to Arm ARM:
>>> - EL3 phys (if EL3 is there)
>> 
>> This might be needed by EL3 but not by Xen.
> Xen requires system with EL3 and if there is EL3, both Arm spec and dt bindings requires sec-phys timer to be there.
> So it would be very strange to see a fake interrupt with IRQ being 0. But if we relly want to only care about
> what Xen needs, then we could live with that (although it is difficult for me to find justification for 0 there).
> Device trees are created per system and if system has EL3, then why forcing 0 to be listed for sec-phys timer?
> 

Let's see that on the other angle: why should Xen check stuff that it does not need ?

Bertrand

> ~Michal

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Michal Orzel]

Hi Bertrand,

On 09/03/2023 13:19, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>
>>>>>
>>>>> Hi Stefano,
>>>>>
>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>
>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>
>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>
>>>>>>>> Added support for parsing the ARM generic timer interrupts DT
>>>>>>>> node by the "interrupt-names" property, if it is available.
>>>>>>>>
>>>>>>>> If not available, the usual parsing based on the expected
>>>>>>>> IRQ order is performed.
>>>>>>>>
>>>>>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>>>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>>>>>> not found, the hypervisor won't panic.
>>>>>>>>
>>>>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>> ---
>>>>>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>>>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>>>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>>>>>> index 4b401c1110..49ad8c1a6d 100644
>>>>>>>> --- a/xen/arch/arm/include/asm/time.h
>>>>>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>>>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>>>>>  TIMER_PHYS_NONSECURE_PPI = 1,
>>>>>>>>  TIMER_VIRT_PPI = 2,
>>>>>>>>  TIMER_HYP_PPI = 3,
>>>>>>>> -    MAX_TIMER_PPI = 4,
>>>>>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>>>>>> +    MAX_TIMER_PPI = 5,
>>>>>>>> };
>>>>>>>>
>>>>>>>> /*
>>>>>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>>>>>> index 433d7be909..794da646d6 100644
>>>>>>>> --- a/xen/arch/arm/time.c
>>>>>>>> +++ b/xen/arch/arm/time.c
>>>>>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>>>>>
>>>>>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>>>>>
>>>>>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>>>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>>>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>>>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>>>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>>>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>>>>>> +};
>>>>>>>> +
>>>>>>>
>>>>>>> I would need some reference or a pointer to some doc to check those.
>>>>>>>
>>>>>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>>>>>> {
>>>>>>>>  ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>>>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>>>>>> {
>>>>>>>>  int res;
>>>>>>>>  unsigned int i;
>>>>>>>> +    bool has_names;
>>>>>>>> +
>>>>>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>>>>>
>>>>>>>>  /* Retrieve all IRQs for the timer */
>>>>>>>>  for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>>>>>  {
>>>>>>>> -        res = platform_get_irq(timer, i);
>>>>>>>> -
>>>>>>>> -        if ( res < 0 )
>>>>>>>> +        if ( has_names )
>>>>>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>>>>>> +        else
>>>>>>>> +            res = platform_get_irq(timer, i);
>>>>>>>> +
>>>>>>>> +        if ( res > 0 )
>>>>>>>
>>>>>>> The behaviour of the code is changed here compared to the current
>>>>>>> version as res = 0 will now generate a panic.
>>>>>>>
>>>>>>> Some device tree might not specify an interrupt number and just put
>>>>>>> 0 and Xen will now panic on those systems.
>>>>>>> As I have no idea if such systems exists and the behaviour is modified
>>>>>>> you should justify this and mention it in the commit message or keep
>>>>>>> the old behaviour and let 0 go through without a panic.
>>>>>>>
>>>>>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>>>>>
>>>>>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>>>>>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>>>>>> error.
>>>>>
>>>>> Problem here is that a DTB might not specify all interrupts and just put
>>>>> 0 for the one not used (or not available for example if you have no secure
>>>>> world).
>>>> Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
>>>> according to Arm ARM:
>>>> - EL3 phys (if EL3 is there)
>>>
>>> This might be needed by EL3 but not by Xen.
>> Xen requires system with EL3 and if there is EL3, both Arm spec and dt bindings requires sec-phys timer to be there.
>> So it would be very strange to see a fake interrupt with IRQ being 0. But if we relly want to only care about
>> what Xen needs, then we could live with that (although it is difficult for me to find justification for 0 there).
>> Device trees are created per system and if system has EL3, then why forcing 0 to be listed for sec-phys timer?
>>
> 
> Let's see that on the other angle: why should Xen check stuff that it does not need ?
Because apart from what it needs or not, there is a matter of a failure in Xen.
Xen exposes timer IRQs to dom0 as they were taken from dtb and allowing 0 for any of the timer IRQ would result
in a Xen failure when reserving such IRQ. Xen presets SGI IRQs, meaning bits 0:15 in allocated_irqs bitmap are set.
This is why when calling vgic_reserve_virq and passing SGI always results in calling a BUG().

So we have two options:
- panic earlier with a meaningful message when IRQ is 0
- let Xen continue and reach BUG which would be not that obvious for people using but not knowing Xen

I think first option is always better.

~Michal

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Bertrand Marquis]

Hi Michal,

> On 9 Mar 2023, at 13:42, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Hi Bertrand,
> 
> On 09/03/2023 13:19, Bertrand Marquis wrote:
>> 
>> 
>> Hi Michal,
>> 
>>> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>> 
>>>> 
>>>> Hi Michal,
>>>> 
>>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>> 
>>>>>> 
>>>>>> Hi Stefano,
>>>>>> 
>>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>> 
>>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>> 
>>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>> 
>>>>>>>>> Added support for parsing the ARM generic timer interrupts DT
>>>>>>>>> node by the "interrupt-names" property, if it is available.
>>>>>>>>> 
>>>>>>>>> If not available, the usual parsing based on the expected
>>>>>>>>> IRQ order is performed.
>>>>>>>>> 
>>>>>>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>>>>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>>>>>>> not found, the hypervisor won't panic.
>>>>>>>>> 
>>>>>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>> ---
>>>>>>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>>>>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>>>>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>>>>>> 
>>>>>>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>>>>>>> index 4b401c1110..49ad8c1a6d 100644
>>>>>>>>> --- a/xen/arch/arm/include/asm/time.h
>>>>>>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>>>>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>>>>>> TIMER_PHYS_NONSECURE_PPI = 1,
>>>>>>>>> TIMER_VIRT_PPI = 2,
>>>>>>>>> TIMER_HYP_PPI = 3,
>>>>>>>>> -    MAX_TIMER_PPI = 4,
>>>>>>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>>>>>>> +    MAX_TIMER_PPI = 5,
>>>>>>>>> };
>>>>>>>>> 
>>>>>>>>> /*
>>>>>>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>>>>>>> index 433d7be909..794da646d6 100644
>>>>>>>>> --- a/xen/arch/arm/time.c
>>>>>>>>> +++ b/xen/arch/arm/time.c
>>>>>>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>>>>>> 
>>>>>>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>>>>>> 
>>>>>>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>>>>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>>>>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>>>>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>>>>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>>>>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>> 
>>>>>>>> I would need some reference or a pointer to some doc to check those.
>>>>>>>> 
>>>>>>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>>>>>>> {
>>>>>>>>> ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>>>>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>>>>>>> {
>>>>>>>>> int res;
>>>>>>>>> unsigned int i;
>>>>>>>>> +    bool has_names;
>>>>>>>>> +
>>>>>>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>>>>>> 
>>>>>>>>> /* Retrieve all IRQs for the timer */
>>>>>>>>> for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>>>>>> {
>>>>>>>>> -        res = platform_get_irq(timer, i);
>>>>>>>>> -
>>>>>>>>> -        if ( res < 0 )
>>>>>>>>> +        if ( has_names )
>>>>>>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>>>>>>> +        else
>>>>>>>>> +            res = platform_get_irq(timer, i);
>>>>>>>>> +
>>>>>>>>> +        if ( res > 0 )
>>>>>>>> 
>>>>>>>> The behaviour of the code is changed here compared to the current
>>>>>>>> version as res = 0 will now generate a panic.
>>>>>>>> 
>>>>>>>> Some device tree might not specify an interrupt number and just put
>>>>>>>> 0 and Xen will now panic on those systems.
>>>>>>>> As I have no idea if such systems exists and the behaviour is modified
>>>>>>>> you should justify this and mention it in the commit message or keep
>>>>>>>> the old behaviour and let 0 go through without a panic.
>>>>>>>> 
>>>>>>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>>>>>> 
>>>>>>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>>>>>>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>>>>>>> error.
>>>>>> 
>>>>>> Problem here is that a DTB might not specify all interrupts and just put
>>>>>> 0 for the one not used (or not available for example if you have no secure
>>>>>> world).
>>>>> Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
>>>>> according to Arm ARM:
>>>>> - EL3 phys (if EL3 is there)
>>>> 
>>>> This might be needed by EL3 but not by Xen.
>>> Xen requires system with EL3 and if there is EL3, both Arm spec and dt bindings requires sec-phys timer to be there.
>>> So it would be very strange to see a fake interrupt with IRQ being 0. But if we relly want to only care about
>>> what Xen needs, then we could live with that (although it is difficult for me to find justification for 0 there).
>>> Device trees are created per system and if system has EL3, then why forcing 0 to be listed for sec-phys timer?
>>> 
>> 
>> Let's see that on the other angle: why should Xen check stuff that it does not need ?
> Because apart from what it needs or not, there is a matter of a failure in Xen.
> Xen exposes timer IRQs to dom0 as they were taken from dtb and allowing 0 for any of the timer IRQ would result
> in a Xen failure when reserving such IRQ. Xen presets SGI IRQs, meaning bits 0:15 in allocated_irqs bitmap are set.
> This is why when calling vgic_reserve_virq and passing SGI always results in calling a BUG().
> 
> So we have two options:
> - panic earlier with a meaningful message when IRQ is 0
> - let Xen continue and reach BUG which would be not that obvious for people using but not knowing Xen

So you are saying that in the current state 0 would be ignored during scan and create a bug later.

If this is the case than definitely we should panic earlier with a proper message I agree.

Regards
Bertrand

> 
> I think first option is always better.
> 
> ~Michal

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Michal Orzel]

On 09/03/2023 13:45, Bertrand Marquis wrote:
> 
> 
> Hi Michal,
> 
>> On 9 Mar 2023, at 13:42, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> Hi Bertrand,
>>
>> On 09/03/2023 13:19, Bertrand Marquis wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>>> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>>>
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi Stefano,
>>>>>>>
>>>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>>>
>>>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>>>
>>>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>>>
>>>>>>>>>> Added support for parsing the ARM generic timer interrupts DT
>>>>>>>>>> node by the "interrupt-names" property, if it is available.
>>>>>>>>>>
>>>>>>>>>> If not available, the usual parsing based on the expected
>>>>>>>>>> IRQ order is performed.
>>>>>>>>>>
>>>>>>>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>>>>>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>>>>>>>> not found, the hypervisor won't panic.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>>> ---
>>>>>>>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>>>>>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>>>>>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>>>>>>>> index 4b401c1110..49ad8c1a6d 100644
>>>>>>>>>> --- a/xen/arch/arm/include/asm/time.h
>>>>>>>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>>>>>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>>>>>>> TIMER_PHYS_NONSECURE_PPI = 1,
>>>>>>>>>> TIMER_VIRT_PPI = 2,
>>>>>>>>>> TIMER_HYP_PPI = 3,
>>>>>>>>>> -    MAX_TIMER_PPI = 4,
>>>>>>>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>>>>>>>> +    MAX_TIMER_PPI = 5,
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> /*
>>>>>>>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>>>>>>>> index 433d7be909..794da646d6 100644
>>>>>>>>>> --- a/xen/arch/arm/time.c
>>>>>>>>>> +++ b/xen/arch/arm/time.c
>>>>>>>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>>>>>>>
>>>>>>>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>>>>>>>
>>>>>>>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>>>>>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>>>>>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>>>>>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>>>>>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>>>>>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> I would need some reference or a pointer to some doc to check those.
>>>>>>>>>
>>>>>>>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>>>>>>>> {
>>>>>>>>>> ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>>>>>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>>>>>>>> {
>>>>>>>>>> int res;
>>>>>>>>>> unsigned int i;
>>>>>>>>>> +    bool has_names;
>>>>>>>>>> +
>>>>>>>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>>>>>>>
>>>>>>>>>> /* Retrieve all IRQs for the timer */
>>>>>>>>>> for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>>>>>>> {
>>>>>>>>>> -        res = platform_get_irq(timer, i);
>>>>>>>>>> -
>>>>>>>>>> -        if ( res < 0 )
>>>>>>>>>> +        if ( has_names )
>>>>>>>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>>>>>>>> +        else
>>>>>>>>>> +            res = platform_get_irq(timer, i);
>>>>>>>>>> +
>>>>>>>>>> +        if ( res > 0 )
>>>>>>>>>
>>>>>>>>> The behaviour of the code is changed here compared to the current
>>>>>>>>> version as res = 0 will now generate a panic.
>>>>>>>>>
>>>>>>>>> Some device tree might not specify an interrupt number and just put
>>>>>>>>> 0 and Xen will now panic on those systems.
>>>>>>>>> As I have no idea if such systems exists and the behaviour is modified
>>>>>>>>> you should justify this and mention it in the commit message or keep
>>>>>>>>> the old behaviour and let 0 go through without a panic.
>>>>>>>>>
>>>>>>>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>>>>>>>
>>>>>>>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>>>>>>>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>>>>>>>> error.
>>>>>>>
>>>>>>> Problem here is that a DTB might not specify all interrupts and just put
>>>>>>> 0 for the one not used (or not available for example if you have no secure
>>>>>>> world).
>>>>>> Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
>>>>>> according to Arm ARM:
>>>>>> - EL3 phys (if EL3 is there)
>>>>>
>>>>> This might be needed by EL3 but not by Xen.
>>>> Xen requires system with EL3 and if there is EL3, both Arm spec and dt bindings requires sec-phys timer to be there.
>>>> So it would be very strange to see a fake interrupt with IRQ being 0. But if we relly want to only care about
>>>> what Xen needs, then we could live with that (although it is difficult for me to find justification for 0 there).
>>>> Device trees are created per system and if system has EL3, then why forcing 0 to be listed for sec-phys timer?
>>>>
>>>
>>> Let's see that on the other angle: why should Xen check stuff that it does not need ?
>> Because apart from what it needs or not, there is a matter of a failure in Xen.
>> Xen exposes timer IRQs to dom0 as they were taken from dtb and allowing 0 for any of the timer IRQ would result
>> in a Xen failure when reserving such IRQ. Xen presets SGI IRQs, meaning bits 0:15 in allocated_irqs bitmap are set.
>> This is why when calling vgic_reserve_virq and passing SGI always results in calling a BUG().
>>
>> So we have two options:
>> - panic earlier with a meaningful message when IRQ is 0
>> - let Xen continue and reach BUG which would be not that obvious for people using but not knowing Xen
> 
> So you are saying that in the current state 0 would be ignored during scan and create a bug later.
Yes, provided platform_get_irq() returns 0. This is however only theory because IMO it is impossible at the moment.
Both GICs, do not allow specifying SGIs in dt bindings and require at least 3 cells where 1st one specifies type.
So if we have a fake IRQ 0 as PPI, platform_get_irq will return 16 and for SPI - 32.
Therefore I cannot see how it would return 0.

~Michal

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Andrei Cherechesu]

On 09/03/2023 15:46, Michal Orzel wrote:
> 
> 
> On 09/03/2023 13:45, Bertrand Marquis wrote:
>>
>>
>> Hi Michal,
>>
>>> On 9 Mar 2023, at 13:42, Michal Orzel <michal.orzel@amd.com> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On 09/03/2023 13:19, Bertrand Marquis wrote:
>>>>
>>>>
>>>> Hi Michal,
>>>>
>>>>> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>>>>
>>>>>>
>>>>>> Hi Michal,
>>>>>>
>>>>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Stefano,
>>>>>>>>
>>>>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>>>>
>>>>>>>>>>> Added support for parsing the ARM generic timer interrupts DT
>>>>>>>>>>> node by the "interrupt-names" property, if it is available.
>>>>>>>>>>>
>>>>>>>>>>> If not available, the usual parsing based on the expected
>>>>>>>>>>> IRQ order is performed.
>>>>>>>>>>>
>>>>>>>>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>>>>>>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>>>>>>>>> not found, the hypervisor won't panic.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>>>> ---
>>>>>>>>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>>>>>>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>>>>>>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>>>>>>>>> index 4b401c1110..49ad8c1a6d 100644
>>>>>>>>>>> --- a/xen/arch/arm/include/asm/time.h
>>>>>>>>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>>>>>>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>>>>>>>> TIMER_PHYS_NONSECURE_PPI = 1,
>>>>>>>>>>> TIMER_VIRT_PPI = 2,
>>>>>>>>>>> TIMER_HYP_PPI = 3,
>>>>>>>>>>> -    MAX_TIMER_PPI = 4,
>>>>>>>>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>>>>>>>>> +    MAX_TIMER_PPI = 5,
>>>>>>>>>>> };
>>>>>>>>>>>
>>>>>>>>>>> /*
>>>>>>>>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>>>>>>>>> index 433d7be909..794da646d6 100644
>>>>>>>>>>> --- a/xen/arch/arm/time.c
>>>>>>>>>>> +++ b/xen/arch/arm/time.c
>>>>>>>>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>>>>>>>>
>>>>>>>>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>>>>>>>>
>>>>>>>>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>>>>>>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>>>>>>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>>>>>>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>>>>>>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>>>>>>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> I would need some reference or a pointer to some doc to check those.
>>>>>>>>>>
>>>>>>>>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>>>>>>>>> {
>>>>>>>>>>> ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>>>>>>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>>>>>>>>> {
>>>>>>>>>>> int res;
>>>>>>>>>>> unsigned int i;
>>>>>>>>>>> +    bool has_names;
>>>>>>>>>>> +
>>>>>>>>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>>>>>>>>
>>>>>>>>>>> /* Retrieve all IRQs for the timer */
>>>>>>>>>>> for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>>>>>>>> {
>>>>>>>>>>> -        res = platform_get_irq(timer, i);
>>>>>>>>>>> -
>>>>>>>>>>> -        if ( res < 0 )
>>>>>>>>>>> +        if ( has_names )
>>>>>>>>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>>>>>>>>> +        else
>>>>>>>>>>> +            res = platform_get_irq(timer, i);
>>>>>>>>>>> +
>>>>>>>>>>> +        if ( res > 0 )
>>>>>>>>>>
>>>>>>>>>> The behaviour of the code is changed here compared to the current
>>>>>>>>>> version as res = 0 will now generate a panic.
>>>>>>>>>>
>>>>>>>>>> Some device tree might not specify an interrupt number and just put
>>>>>>>>>> 0 and Xen will now panic on those systems.
>>>>>>>>>> As I have no idea if such systems exists and the behaviour is modified
>>>>>>>>>> you should justify this and mention it in the commit message or keep
>>>>>>>>>> the old behaviour and let 0 go through without a panic.
>>>>>>>>>>
>>>>>>>>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>>>>>>>>
>>>>>>>>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>>>>>>>>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>>>>>>>>> error.
>>>>>>>>
>>>>>>>> Problem here is that a DTB might not specify all interrupts and just put
>>>>>>>> 0 for the one not used (or not available for example if you have no secure
>>>>>>>> world).
>>>>>>> Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
>>>>>>> according to Arm ARM:
>>>>>>> - EL3 phys (if EL3 is there)
>>>>>>
>>>>>> This might be needed by EL3 but not by Xen.
>>>>> Xen requires system with EL3 and if there is EL3, both Arm spec and dt bindings requires sec-phys timer to be there.
>>>>> So it would be very strange to see a fake interrupt with IRQ being 0. But if we relly want to only care about
>>>>> what Xen needs, then we could live with that (although it is difficult for me to find justification for 0 there).
>>>>> Device trees are created per system and if system has EL3, then why forcing 0 to be listed for sec-phys timer?
>>>>>
>>>>
>>>> Let's see that on the other angle: why should Xen check stuff that it does not need ?
>>> Because apart from what it needs or not, there is a matter of a failure in Xen.
>>> Xen exposes timer IRQs to dom0 as they were taken from dtb and allowing 0 for any of the timer IRQ would result
>>> in a Xen failure when reserving such IRQ. Xen presets SGI IRQs, meaning bits 0:15 in allocated_irqs bitmap are set.
>>> This is why when calling vgic_reserve_virq and passing SGI always results in calling a BUG().
>>>
>>> So we have two options:
>>> - panic earlier with a meaningful message when IRQ is 0
>>> - let Xen continue and reach BUG which would be not that obvious for people using but not knowing Xen
>>
>> So you are saying that in the current state 0 would be ignored during scan and create a bug later.
> Yes, provided platform_get_irq() returns 0. This is however only theory because IMO it is impossible at the moment.
> Both GICs, do not allow specifying SGIs in dt bindings and require at least 3 cells where 1st one specifies type.
> So if we have a fake IRQ 0 as PPI, platform_get_irq will return 16 and for SPI - 32.
> Therefore I cannot see how it would return 0.
> 
> ~Michal
> 


This was my original thought process as well when I initially
implemented this fix. Thus, I figured 0 should also be an error case.

Extending this, all SGI possible return values (0 to 15) should be
errors here, right? But I'm not sure if we should also treat those extra
cases here (1 to 15).

So, to make sure we all aligned, the only change to be made in a v2 for
this patch is the coding style for the added comment? Or do you also
think a more specific check for valid PPI IDs returned (16 <= id <= 31)
would be beneficial?

Regards,
Andrei

Re: [PATCH v1 2/2] arch/arm: time: Add support for parsing interrupts by names

[Bertrand Marquis]

Hi,

> On 9 Mar 2023, at 15:36, Andrei Cherechesu <andrei.cherechesu@oss.nxp.com> wrote:
> 
> 
> 
> On 09/03/2023 15:46, Michal Orzel wrote:
>> 
>> 
>> On 09/03/2023 13:45, Bertrand Marquis wrote:
>>> 
>>> 
>>> Hi Michal,
>>> 
>>>> On 9 Mar 2023, at 13:42, Michal Orzel <michal.orzel@amd.com> wrote:
>>>> 
>>>> Hi Bertrand,
>>>> 
>>>> On 09/03/2023 13:19, Bertrand Marquis wrote:
>>>>> 
>>>>> 
>>>>> Hi Michal,
>>>>> 
>>>>>> On 9 Mar 2023, at 12:35, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 09/03/2023 11:39, Bertrand Marquis wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> Hi Michal,
>>>>>>> 
>>>>>>>> On 9 Mar 2023, at 11:05, Michal Orzel <michal.orzel@amd.com> wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 09/03/2023 09:02, Bertrand Marquis wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Hi Stefano,
>>>>>>>>> 
>>>>>>>>>> On 7 Mar 2023, at 22:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>>>>>> 
>>>>>>>>>> On Tue, 7 Mar 2023, Bertrand Marquis wrote:
>>>>>>>>>>>> On 7 Mar 2023, at 11:09, Andrei Cherechesu (OSS) <andrei.cherechesu@oss.nxp.com> wrote:
>>>>>>>>>>>> 
>>>>>>>>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>>>>> 
>>>>>>>>>>>> Added support for parsing the ARM generic timer interrupts DT
>>>>>>>>>>>> node by the "interrupt-names" property, if it is available.
>>>>>>>>>>>> 
>>>>>>>>>>>> If not available, the usual parsing based on the expected
>>>>>>>>>>>> IRQ order is performed.
>>>>>>>>>>>> 
>>>>>>>>>>>> Also added the "hyp-virt" PPI to the timer PPI list, even
>>>>>>>>>>>> though it's currently not in use. If the "hyp-virt" PPI is
>>>>>>>>>>>> not found, the hypervisor won't panic.
>>>>>>>>>>>> 
>>>>>>>>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> xen/arch/arm/include/asm/time.h |  3 ++-
>>>>>>>>>>>> xen/arch/arm/time.c             | 26 ++++++++++++++++++++++----
>>>>>>>>>>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>>>>>>>>>> 
>>>>>>>>>>>> diff --git a/xen/arch/arm/include/asm/time.h b/xen/arch/arm/include/asm/time.h
>>>>>>>>>>>> index 4b401c1110..49ad8c1a6d 100644
>>>>>>>>>>>> --- a/xen/arch/arm/include/asm/time.h
>>>>>>>>>>>> +++ b/xen/arch/arm/include/asm/time.h
>>>>>>>>>>>> @@ -82,7 +82,8 @@ enum timer_ppi
>>>>>>>>>>>> TIMER_PHYS_NONSECURE_PPI = 1,
>>>>>>>>>>>> TIMER_VIRT_PPI = 2,
>>>>>>>>>>>> TIMER_HYP_PPI = 3,
>>>>>>>>>>>> -    MAX_TIMER_PPI = 4,
>>>>>>>>>>>> +    TIMER_HYP_VIRT_PPI = 4,
>>>>>>>>>>>> +    MAX_TIMER_PPI = 5,
>>>>>>>>>>>> };
>>>>>>>>>>>> 
>>>>>>>>>>>> /*
>>>>>>>>>>>> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
>>>>>>>>>>>> index 433d7be909..794da646d6 100644
>>>>>>>>>>>> --- a/xen/arch/arm/time.c
>>>>>>>>>>>> +++ b/xen/arch/arm/time.c
>>>>>>>>>>>> @@ -38,6 +38,14 @@ uint32_t __read_mostly timer_dt_clock_frequency;
>>>>>>>>>>>> 
>>>>>>>>>>>> static unsigned int timer_irq[MAX_TIMER_PPI];
>>>>>>>>>>>> 
>>>>>>>>>>>> +static const char *timer_irq_names[MAX_TIMER_PPI] = {
>>>>>>>>>>>> +    [TIMER_PHYS_SECURE_PPI] = "sec-phys",
>>>>>>>>>>>> +    [TIMER_PHYS_NONSECURE_PPI] = "phys",
>>>>>>>>>>>> +    [TIMER_VIRT_PPI] = "virt",
>>>>>>>>>>>> +    [TIMER_HYP_PPI] = "hyp-phys",
>>>>>>>>>>>> +    [TIMER_HYP_VIRT_PPI] = "hyp-virt",
>>>>>>>>>>>> +};
>>>>>>>>>>>> +
>>>>>>>>>>> 
>>>>>>>>>>> I would need some reference or a pointer to some doc to check those.
>>>>>>>>>>> 
>>>>>>>>>>>> unsigned int timer_get_irq(enum timer_ppi ppi)
>>>>>>>>>>>> {
>>>>>>>>>>>> ASSERT(ppi >= TIMER_PHYS_SECURE_PPI && ppi < MAX_TIMER_PPI);
>>>>>>>>>>>> @@ -149,15 +157,25 @@ static void __init init_dt_xen_time(void)
>>>>>>>>>>>> {
>>>>>>>>>>>> int res;
>>>>>>>>>>>> unsigned int i;
>>>>>>>>>>>> +    bool has_names;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    has_names = dt_property_read_bool(timer, "interrupt-names");
>>>>>>>>>>>> 
>>>>>>>>>>>> /* Retrieve all IRQs for the timer */
>>>>>>>>>>>> for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
>>>>>>>>>>>> {
>>>>>>>>>>>> -        res = platform_get_irq(timer, i);
>>>>>>>>>>>> -
>>>>>>>>>>>> -        if ( res < 0 )
>>>>>>>>>>>> +        if ( has_names )
>>>>>>>>>>>> +            res = platform_get_irq_byname(timer, timer_irq_names[i]);
>>>>>>>>>>>> +        else
>>>>>>>>>>>> +            res = platform_get_irq(timer, i);
>>>>>>>>>>>> +
>>>>>>>>>>>> +        if ( res > 0 )
>>>>>>>>>>> 
>>>>>>>>>>> The behaviour of the code is changed here compared to the current
>>>>>>>>>>> version as res = 0 will now generate a panic.
>>>>>>>>>>> 
>>>>>>>>>>> Some device tree might not specify an interrupt number and just put
>>>>>>>>>>> 0 and Xen will now panic on those systems.
>>>>>>>>>>> As I have no idea if such systems exists and the behaviour is modified
>>>>>>>>>>> you should justify this and mention it in the commit message or keep
>>>>>>>>>>> the old behaviour and let 0 go through without a panic.
>>>>>>>>>>> 
>>>>>>>>>>> @stefano, julien any idea here ? should just keep the old behaviour ?
>>>>>>>>>> 
>>>>>>>>>> platform_get_irq returns 0 if the irq is 0. The irq cannot be 0 because
>>>>>>>>>> 0 is reserved for SGIs, not PPIs. So I think it is OK to consider 0 an
>>>>>>>>>> error.
>>>>>>>>> 
>>>>>>>>> Problem here is that a DTB might not specify all interrupts and just put
>>>>>>>>> 0 for the one not used (or not available for example if you have no secure
>>>>>>>>> world).
>>>>>>>> Xen requires presence of EL3,EL2 and on such system, at least the following timers needs to be there
>>>>>>>> according to Arm ARM:
>>>>>>>> - EL3 phys (if EL3 is there)
>>>>>>> 
>>>>>>> This might be needed by EL3 but not by Xen.
>>>>>> Xen requires system with EL3 and if there is EL3, both Arm spec and dt bindings requires sec-phys timer to be there.
>>>>>> So it would be very strange to see a fake interrupt with IRQ being 0. But if we relly want to only care about
>>>>>> what Xen needs, then we could live with that (although it is difficult for me to find justification for 0 there).
>>>>>> Device trees are created per system and if system has EL3, then why forcing 0 to be listed for sec-phys timer?
>>>>>> 
>>>>> 
>>>>> Let's see that on the other angle: why should Xen check stuff that it does not need ?
>>>> Because apart from what it needs or not, there is a matter of a failure in Xen.
>>>> Xen exposes timer IRQs to dom0 as they were taken from dtb and allowing 0 for any of the timer IRQ would result
>>>> in a Xen failure when reserving such IRQ. Xen presets SGI IRQs, meaning bits 0:15 in allocated_irqs bitmap are set.
>>>> This is why when calling vgic_reserve_virq and passing SGI always results in calling a BUG().
>>>> 
>>>> So we have two options:
>>>> - panic earlier with a meaningful message when IRQ is 0
>>>> - let Xen continue and reach BUG which would be not that obvious for people using but not knowing Xen
>>> 
>>> So you are saying that in the current state 0 would be ignored during scan and create a bug later.
>> Yes, provided platform_get_irq() returns 0. This is however only theory because IMO it is impossible at the moment.
>> Both GICs, do not allow specifying SGIs in dt bindings and require at least 3 cells where 1st one specifies type.
>> So if we have a fake IRQ 0 as PPI, platform_get_irq will return 16 and for SPI - 32.
>> Therefore I cannot see how it would return 0.
>> 
>> ~Michal
>> 
> 
> 
> This was my original thought process as well when I initially
> implemented this fix. Thus, I figured 0 should also be an error case.
> 
> Extending this, all SGI possible return values (0 to 15) should be
> errors here, right? But I'm not sure if we should also treat those extra
> cases here (1 to 15).
> 
> So, to make sure we all aligned, the only change to be made in a v2 for
> this patch is the coding style for the added comment? Or do you also
> think a more specific check for valid PPI IDs returned (16 <= id <= 31)
> would be beneficial?
> 

No i think we should not check more.
Just add something in the commit message to mention this change and why it is ok.


Cheers
Bertrand

> Regards,
> Andrei