[XEN][RFC PATCH v4 07/16] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[XEN][RFC PATCH v4 07/16] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Vikram Garhwal]

Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_lock().

Moving spin_lock to caller was done to prevent the concurrent access to
iommu_dt_device_is_assigned while doing add/remove/assign/deassign.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/drivers/passthrough/device_tree.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 1c32d7b50c..bb4cf7784d 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -83,16 +83,15 @@ fail:
     return rc;
 }
 
-static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
+static bool_t
+    iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
 {
     bool_t assigned = 0;
 
     if ( !dt_device_is_protected(dev) )
         return 0;
 
-    spin_lock(&dtdevs_lock);
     assigned = !list_empty(&dev->domain_list);
-    spin_unlock(&dtdevs_lock);
 
     return assigned;
 }
@@ -213,27 +212,43 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
         if ( (d && d->is_dying) || domctl->u.assign_device.flags )
             break;
 
+        spin_lock(&dtdevs_lock);
+
         ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
                                     domctl->u.assign_device.u.dt.size,
                                     &dev);
         if ( ret )
+        {
+            spin_unlock(&dtdevs_lock);
+
             break;
+        }
 
         ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
         if ( ret )
+        {
+            spin_unlock(&dtdevs_lock);
+
             break;
+        }
 
         if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
         {
-            if ( iommu_dt_device_is_assigned(dev) )
+
+            if ( iommu_dt_device_is_assigned_locked(dev) )
             {
                 printk(XENLOG_G_ERR "%s already assigned.\n",
                        dt_node_full_name(dev));
                 ret = -EINVAL;
             }
+
+            spin_unlock(&dtdevs_lock);
+
             break;
         }
 
+        spin_unlock(&dtdevs_lock);
+
         if ( d == dom_io )
             return -EINVAL;
 
-- 
2.17.1

[XEN][RFC PATCH v4 08/16] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock

[Vikram Garhwal]

Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access add.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
---
 xen/drivers/passthrough/device_tree.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index bb4cf7784d..457df333a0 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -146,6 +146,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
     if ( dev_iommu_fwspec_get(dev) )
         return 0;
 
+    spin_lock(&dtdevs_lock);
+
     /*
      * According to the Documentation/devicetree/bindings/iommu/iommu.txt
      * from Linux.
@@ -158,7 +160,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
          * these callback implemented.
          */
         if ( !ops->add_device || !ops->dt_xlate )
-            return -EINVAL;
+        {
+            rc = -EINVAL;
+            goto fail;
+        }
 
         if ( !dt_device_is_available(iommu_spec.np) )
             break;
@@ -189,6 +194,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
     if ( rc < 0 )
         iommu_fwspec_free(dev);
 
+fail:
+    spin_unlock(&dtdevs_lock);
     return rc;
 }
 
-- 
2.17.1

[XEN][RFC PATCH v4 09/16] xen/iommu: Introduce iommu_remove_dt_device()

[Vikram Garhwal]

Remove master device from the IOMMU.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++
 xen/include/xen/iommu.h               |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 457df333a0..a8ba0b0d17 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -126,6 +126,44 @@ int iommu_release_dt_devices(struct domain *d)
     return 0;
 }
 
+int iommu_remove_dt_device(struct dt_device_node *np)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    struct device *dev = dt_to_dev(np);
+    int rc;
+
+    if ( !ops )
+        return -EOPNOTSUPP;
+
+    spin_lock(&dtdevs_lock);
+
+    if ( iommu_dt_device_is_assigned_locked(np) ) {
+        rc = -EBUSY;
+        goto fail;
+    }
+
+    /*
+     * The driver which supports generic IOMMU DT bindings must have
+     * these callback implemented.
+     */
+    if ( !ops->remove_device ) {
+        rc = -EOPNOTSUPP;
+        goto fail;
+    }
+
+    /*
+     * Remove master device from the IOMMU if latter is present and available.
+     */
+    rc = ops->remove_device(0, dev);
+
+    if ( rc == 0 )
+        iommu_fwspec_free(dev);
+
+fail:
+    spin_unlock(&dtdevs_lock);
+    return rc;
+}
+
 int iommu_add_dt_device(struct dt_device_node *np)
 {
     const struct iommu_ops *ops = iommu_get_ops();
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 4f22fc1bed..1b36c0419d 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -225,6 +225,8 @@ int iommu_release_dt_devices(struct domain *d);
  */
 int iommu_add_dt_device(struct dt_device_node *np);
 
+int iommu_remove_dt_device(struct dt_device_node *np);
+
 int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
 
-- 
2.17.1

Re: [XEN][RFC PATCH v4 09/16] xen/iommu: Introduce iommu_remove_dt_device()

[Michal Orzel]

Hi Vikram,

On 07/12/2022 07:18, Vikram Garhwal wrote:
> 
> 
> Remove master device from the IOMMU.
Adding some description on the purpose would be beneficial.

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> ---
>  xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++
>  xen/include/xen/iommu.h               |  2 ++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 457df333a0..a8ba0b0d17 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -126,6 +126,44 @@ int iommu_release_dt_devices(struct domain *d)
>      return 0;
>  }
> 
> +int iommu_remove_dt_device(struct dt_device_node *np)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    struct device *dev = dt_to_dev(np);
> +    int rc;
> +
Aren't we missing a check if iommu is enabled?

> +    if ( !ops )
> +        return -EOPNOTSUPP;
-EINVAL to match the return values returned by other functions?

> +
> +    spin_lock(&dtdevs_lock);
> +
> +    if ( iommu_dt_device_is_assigned_locked(np) ) {
Incorrect coding style. The closing brace should be placed on the next line.

> +        rc = -EBUSY;
> +        goto fail;
> +    }
> +
> +    /*
> +     * The driver which supports generic IOMMU DT bindings must have
> +     * these callback implemented.
> +     */
> +    if ( !ops->remove_device ) {
Incorrect coding style. The closing brace should be placed on the next line.

> +        rc = -EOPNOTSUPP;
-EINVAL to match the return values returned by other functions?

> +        goto fail;
> +    }
> +
> +    /*
> +     * Remove master device from the IOMMU if latter is present and available.
> +     */
No need for a multi-line comment style.

> +    rc = ops->remove_device(0, dev);
> +
> +    if ( rc == 0 )
!rc is preffered.

> +        iommu_fwspec_free(dev);
> +
> +fail:
> +    spin_unlock(&dtdevs_lock);
> +    return rc;
> +}
> +
>  int iommu_add_dt_device(struct dt_device_node *np)
>  {
>      const struct iommu_ops *ops = iommu_get_ops();
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 4f22fc1bed..1b36c0419d 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -225,6 +225,8 @@ int iommu_release_dt_devices(struct domain *d);
>   */
>  int iommu_add_dt_device(struct dt_device_node *np);
> 
> +int iommu_remove_dt_device(struct dt_device_node *np);
These prototypes look to be placed in order. So your function should be
placed before add function.

> +
>  int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
> 
> --
> 2.17.1
> 
> 

~Michal

Re: [XEN][RFC PATCH v4 09/16] xen/iommu: Introduce iommu_remove_dt_device()

[Julien Grall]

Hi,

On 23/01/2023 10:00, Michal Orzel wrote:
>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>> ---
>>   xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++
>>   xen/include/xen/iommu.h               |  2 ++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>> index 457df333a0..a8ba0b0d17 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -126,6 +126,44 @@ int iommu_release_dt_devices(struct domain *d)
>>       return 0;
>>   }
>>
>> +int iommu_remove_dt_device(struct dt_device_node *np)
>> +{
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +    struct device *dev = dt_to_dev(np);
>> +    int rc;
>> +
> Aren't we missing a check if iommu is enabled?
> 
>> +    if ( !ops )
>> +        return -EOPNOTSUPP;
> -EINVAL to match the return values returned by other functions?

The meaning of -EINVAL is quite overloaded. So it would be better to use 
a mix of errno to help differentiating the error paths.

In this case, '!ops' means there are no possibility (read "support") to 
remove the device. So I think -EOPNOTUSUPP is suitable.

> 
>> +
>> +    spin_lock(&dtdevs_lock);
>> +
>> +    if ( iommu_dt_device_is_assigned_locked(np) ) {
> Incorrect coding style. The closing brace should be placed on the next line.
> 
>> +        rc = -EBUSY;
>> +        goto fail;
>> +    }
>> +
>> +    /*
>> +     * The driver which supports generic IOMMU DT bindings must have
>> +     * these callback implemented.
>> +     */
>> +    if ( !ops->remove_device ) {
> Incorrect coding style. The closing brace should be placed on the next line.
> 
>> +        rc = -EOPNOTSUPP;
> -EINVAL to match the return values returned by other functions?

Ditto.

Cheers,

-- 
Julien Grall

Re: [XEN][RFC PATCH v4 09/16] xen/iommu: Introduce iommu_remove_dt_device()

[Vikram Garhwal]

Hi Michal,

On 1/23/23 2:00 AM, Michal Orzel wrote:
> Hi Vikram,
>
> On 07/12/2022 07:18, Vikram Garhwal wrote:
>>
>> Remove master device from the IOMMU.
> Adding some description on the purpose would be beneficial.
will do.
>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>> ---
>>   xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++
>>   xen/include/xen/iommu.h               |  2 ++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
>> index 457df333a0..a8ba0b0d17 100644
>> --- a/xen/drivers/passthrough/device_tree.c
>> +++ b/xen/drivers/passthrough/device_tree.c
>> @@ -126,6 +126,44 @@ int iommu_release_dt_devices(struct domain *d)
>>       return 0;
>>   }
>>
>> +int iommu_remove_dt_device(struct dt_device_node *np)
>> +{
>> +    const struct iommu_ops *ops = iommu_get_ops();
>> +    struct device *dev = dt_to_dev(np);
>> +    int rc;
>> +
> Aren't we missing a check if iommu is enabled?
IIUC your question: There is only one caller which is in dynamic 
programming part handle_remove_irq_iommu(). The call only happen if the 
dt_node has iommu property.
>> +    if ( !ops )
>> +        return -EOPNOTSUPP;
> -EINVAL to match the return values returned by other functions?
>
>> +
>> +    spin_lock(&dtdevs_lock);
>> +
>> +    if ( iommu_dt_device_is_assigned_locked(np) ) {
> Incorrect coding style. The closing brace should be placed on the next line.
Fixed this for v5.
>
>> +        rc = -EBUSY;
>> +        goto fail;
>> +    }
>> +
>> +    /*
>> +     * The driver which supports generic IOMMU DT bindings must have
>> +     * these callback implemented.
>> +     */
>> +    if ( !ops->remove_device ) {
> Incorrect coding style. The closing brace should be placed on the next line.
Fixed this for v5.
>
>> +        rc = -EOPNOTSUPP;
> -EINVAL to match the return values returned by other functions?
>
>> +        goto fail;
>> +    }
>> +
>> +    /*
>> +     * Remove master device from the IOMMU if latter is present and available.
>> +     */
> No need for a multi-line comment style.
Fixed this for v5.
>
>> +    rc = ops->remove_device(0, dev);
>> +
>> +    if ( rc == 0 )
> !rc is preffered.
Fixed this for v5.
>
>> +        iommu_fwspec_free(dev);
>> +
>> +fail:
>> +    spin_unlock(&dtdevs_lock);
>> +    return rc;
>> +}
>> +
>>   int iommu_add_dt_device(struct dt_device_node *np)
>>   {
>>       const struct iommu_ops *ops = iommu_get_ops();
>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>> index 4f22fc1bed..1b36c0419d 100644
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -225,6 +225,8 @@ int iommu_release_dt_devices(struct domain *d);
>>    */
>>   int iommu_add_dt_device(struct dt_device_node *np);
>>
>> +int iommu_remove_dt_device(struct dt_device_node *np);
> These prototypes look to be placed in order. So your function should be
> placed before add function.
Fixed this for v5.
>
>> +
>>   int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>>
>> --
>> 2.17.1
>>
>>
> ~Michal

[XEN][RFC PATCH v4 10/16] asm/smp.h: move cpu related function to asm/cpu.h

[Vikram Garhwal]

Dynamic programming ops will modify the dt_host and there might be other
function which are browsing the dt_host at the same time. To avoid the race
conditions, adding rwlock for browsing the dt_host. But adding rwlock in
device_tree.h causes following circular dependency:
    device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h

Inside arch/arm/include/asm/smp.h, there is one function which needs
device_tree.h, moved the cpu related function to a new file:
arch/arm/include/asm/cpu.h

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 xen/arch/arm/efi/efi-boot.h       |  1 +
 xen/arch/arm/include/asm/cpu.h    | 35 +++++++++++++++++++++++++++++++
 xen/arch/arm/include/asm/domain.h |  1 +
 xen/arch/arm/include/asm/psci.h   |  1 +
 xen/arch/arm/include/asm/smp.h    | 24 ---------------------
 xen/include/xen/cpu.h             |  4 ++++
 xen/include/xen/softirq.h         |  4 ++++
 7 files changed, 46 insertions(+), 24 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/cpu.h

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 43a836c3a7..ca40c6f73f 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -5,6 +5,7 @@
  */
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
+#include <asm/cpu.h>
 #include <asm/setup.h>
 #include <asm/smp.h>
 
diff --git a/xen/arch/arm/include/asm/cpu.h b/xen/arch/arm/include/asm/cpu.h
new file mode 100644
index 0000000000..4df80ca1b5
--- /dev/null
+++ b/xen/arch/arm/include/asm/cpu.h
@@ -0,0 +1,35 @@
+#ifndef __ASM_CPU_H
+#define __ASM_CPU_H
+
+#ifndef __ASSEMBLY__
+#include <xen/cpumask.h>
+#include <xen/device_tree.h>
+#include <asm/current.h>
+#endif
+
+DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
+DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
+
+#define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
+
+/*
+ * Do we, for platform reasons, need to actually keep CPUs online when we
+ * would otherwise prefer them to be off?
+ */
+#define park_offline_cpus false
+
+extern void noreturn stop_cpu(void);
+
+extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
+extern int arch_cpu_up(int cpu);
+
+int cpu_up_send_sgi(int cpu);
+
+/* Secondary CPU entry point */
+extern void init_secondary(void);
+
+#define cpu_physical_id(cpu) cpu_logical_map(cpu)
+
+#endif
+
+
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 2ce6764322..f9440e5c7e 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -3,6 +3,7 @@
 
 #include <xen/cache.h>
 #include <xen/timer.h>
+#include <asm/cpu.h>
 #include <asm/page.h>
 #include <asm/p2m.h>
 #include <asm/vfp.h>
diff --git a/xen/arch/arm/include/asm/psci.h b/xen/arch/arm/include/asm/psci.h
index 832f77afff..74c1bc6368 100644
--- a/xen/arch/arm/include/asm/psci.h
+++ b/xen/arch/arm/include/asm/psci.h
@@ -1,6 +1,7 @@
 #ifndef __ASM_PSCI_H__
 #define __ASM_PSCI_H__
 
+#include <asm/cpu.h>
 #include <asm/smccc.h>
 
 /* PSCI return values (inclusive of all PSCI versions) */
diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h
index 8133d5c295..76944b07f7 100644
--- a/xen/arch/arm/include/asm/smp.h
+++ b/xen/arch/arm/include/asm/smp.h
@@ -3,40 +3,16 @@
 
 #ifndef __ASSEMBLY__
 #include <xen/cpumask.h>
-#include <xen/device_tree.h>
 #include <asm/current.h>
 #endif
 
-DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask);
-DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
-
-#define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))
-
 #define smp_processor_id() get_processor_id()
 
-/*
- * Do we, for platform reasons, need to actually keep CPUs online when we
- * would otherwise prefer them to be off?
- */
-#define park_offline_cpus false
-
-extern void noreturn stop_cpu(void);
-
 extern int arch_smp_init(void);
-extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
-extern int arch_cpu_up(int cpu);
-
-int cpu_up_send_sgi(int cpu);
-
-/* Secondary CPU entry point */
-extern void init_secondary(void);
 
 extern void smp_init_cpus(void);
 extern void smp_clear_cpu_maps (void);
 extern unsigned int smp_get_max_cpus(void);
-
-#define cpu_physical_id(cpu) cpu_logical_map(cpu)
-
 #endif
 
 /*
diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
index e8eeb217a0..ce93eb0003 100644
--- a/xen/include/xen/cpu.h
+++ b/xen/include/xen/cpu.h
@@ -5,6 +5,10 @@
 #include <xen/spinlock.h>
 #include <xen/notifier.h>
 
+#ifdef CONFIG_ARM
+#include <asm/cpu.h>
+#endif
+
 /* Safely access cpu_online_map, cpu_present_map, etc. */
 bool get_cpu_maps(void);
 void put_cpu_maps(void);
diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
index 1f6c4783da..cc98a65287 100644
--- a/xen/include/xen/softirq.h
+++ b/xen/include/xen/softirq.h
@@ -19,6 +19,10 @@ enum {
 #include <asm/hardirq.h>
 #include <asm/softirq.h>
 
+#ifdef CONFIG_ARM
+#include <asm/cpu.h>
+#endif
+
 #define NR_SOFTIRQS (NR_COMMON_SOFTIRQS + NR_ARCH_SOFTIRQS)
 
 typedef void (*softirq_handler)(void);
-- 
2.17.1

Re: [XEN][RFC PATCH v4 10/16] asm/smp.h: move cpu related function to asm/cpu.h

[Jan Beulich]

On 07.12.2022 07:18, Vikram Garhwal wrote:
> --- a/xen/include/xen/cpu.h
> +++ b/xen/include/xen/cpu.h
> @@ -5,6 +5,10 @@
>  #include <xen/spinlock.h>
>  #include <xen/notifier.h>
>  
> +#ifdef CONFIG_ARM
> +#include <asm/cpu.h>
> +#endif
> +
>  /* Safely access cpu_online_map, cpu_present_map, etc. */
>  bool get_cpu_maps(void);
>  void put_cpu_maps(void);
> --- a/xen/include/xen/softirq.h
> +++ b/xen/include/xen/softirq.h
> @@ -19,6 +19,10 @@ enum {
>  #include <asm/hardirq.h>
>  #include <asm/softirq.h>
>  
> +#ifdef CONFIG_ARM
> +#include <asm/cpu.h>
> +#endif

We generally try to avoid such #ifdef-ary whenever possible, so the
description wants to justify these if they cannot be avoided.

Jan

Re: [XEN][RFC PATCH v4 10/16] asm/smp.h: move cpu related function to asm/cpu.h

[Julien Grall]

Hi Vikram,

On 07/12/2022 06:18, Vikram Garhwal wrote:
> Dynamic programming ops will modify the dt_host and there might be other
> function which are browsing the dt_host at the same time. To avoid the race
> conditions, adding rwlock for browsing the dt_host. But adding rwlock in
> device_tree.h causes following circular dependency:
>      device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h
> 
> Inside arch/arm/include/asm/smp.h, there is one function which needs
> device_tree.h, moved the cpu related function to a new file:
> arch/arm/include/asm/cpu.h

Given there is only one function, I don't really see the benefits of 
splitting smp.h and then adding #ifdef CONFIG_ARM in the common code.

Instead, it would be better if we don't include device_tree.h in the 
header but in the c files that need to call arch_cpu_init() and forward 
declare dt_device_node.

Another potential approach is to move out the percpu_rwlock helpers in a 
separate header. The advantage with this approach is we would reduce the 
number of definition included everywhere (there are not many use of the 
percpu rwlock).

Cheers,

-- 
Julien Grall

Re: [XEN][RFC PATCH v4 10/16] asm/smp.h: move cpu related function to asm/cpu.h

[Vikram Garhwal]

Hi Julien

On 12/7/22 8:28 AM, Julien Grall wrote:
> Hi Vikram,
>
> On 07/12/2022 06:18, Vikram Garhwal wrote:
>> Dynamic programming ops will modify the dt_host and there might be other
>> function which are browsing the dt_host at the same time. To avoid 
>> the race
>> conditions, adding rwlock for browsing the dt_host. But adding rwlock in
>> device_tree.h causes following circular dependency:
>> device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h
>>
>> Inside arch/arm/include/asm/smp.h, there is one function which needs
>> device_tree.h, moved the cpu related function to a new file:
>> arch/arm/include/asm/cpu.h
>
> Given there is only one function, I don't really see the benefits of 
> splitting smp.h and then adding #ifdef CONFIG_ARM in the common code.
>
> Instead, it would be better if we don't include device_tree.h in the 
> header but in the c files that need to call arch_cpu_init() and 
> forward declare dt_device_node.
>
This was my initial approach also and there were less changes(compare to 
my v4) but then though someone might have issues with forward 
declaration of dt_device_node in smp.h.

> Another potential approach is to move out the percpu_rwlock helpers in 
> a separate header. The advantage with this approach is we would reduce 
> the number of definition included everywhere (there are not many use 
> of the percpu rwlock).

Will check this option.

Thank you suggestions!

>
> Cheers,
>

Re: [XEN][RFC PATCH v4 10/16] asm/smp.h: move cpu related function to asm/cpu.h

[Jan Beulich]

On 07.12.2022 20:39, Vikram Garhwal wrote:
> Hi Julien
> 
> On 12/7/22 8:28 AM, Julien Grall wrote:
>> Hi Vikram,
>>
>> On 07/12/2022 06:18, Vikram Garhwal wrote:
>>> Dynamic programming ops will modify the dt_host and there might be other
>>> function which are browsing the dt_host at the same time. To avoid 
>>> the race
>>> conditions, adding rwlock for browsing the dt_host. But adding rwlock in
>>> device_tree.h causes following circular dependency:
>>> device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h
>>>
>>> Inside arch/arm/include/asm/smp.h, there is one function which needs
>>> device_tree.h, moved the cpu related function to a new file:
>>> arch/arm/include/asm/cpu.h
>>
>> Given there is only one function, I don't really see the benefits of 
>> splitting smp.h and then adding #ifdef CONFIG_ARM in the common code.
>>
>> Instead, it would be better if we don't include device_tree.h in the 
>> header but in the c files that need to call arch_cpu_init() and 
>> forward declare dt_device_node.
>>
> This was my initial approach also and there were less changes(compare to 
> my v4) but then though someone might have issues with forward 
> declaration of dt_device_node in smp.h.

We use forward declarations of struct/union is many places, precisely to
limit dependencies among headers.

Jan

[XEN][RFC PATCH v4 11/16] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal]

 Dynamic programming ops will modify the dt_host and there might be other
 function which are browsing the dt_host at the same time. To avoid the race
 conditions, adding rwlock for browsing the dt_host.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 xen/common/device_tree.c      | 27 +++++++++++++++++++++++++++
 xen/include/xen/device_tree.h |  6 ++++++
 2 files changed, 33 insertions(+)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index acf26a411d..51ee2a5edf 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np,
     if ( !np )
         return NULL;
 
+    read_lock(&dt_host->lock);
+
     for ( pp = np->properties; pp; pp = pp->next )
     {
         if ( dt_prop_cmp(pp->name, name) == 0 )
@@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np,
         }
     }
 
+    read_unlock(&dt_host->lock);
     return pp;
 }
 
@@ -336,11 +339,14 @@ struct dt_device_node *dt_find_node_by_name(struct dt_device_node *from,
     struct dt_device_node *np;
     struct dt_device_node *dt;
 
+    read_lock(&dt_host->lock);
+
     dt = from ? from->allnext : dt_host;
     dt_for_each_device_node(dt, np)
         if ( np->name && (dt_node_cmp(np->name, name) == 0) )
             break;
 
+    read_unlock(&dt_host->lock);
     return np;
 }
 
@@ -350,11 +356,14 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
     struct dt_device_node *np;
     struct dt_device_node *dt;
 
+    read_lock(&dt_host->lock);
+
     dt = from ? from->allnext : dt_host;
     dt_for_each_device_node(dt, np)
         if ( np->type && (dt_node_cmp(np->type, type) == 0) )
             break;
 
+    read_unlock(&dt_host->lock);
     return np;
 }
 
@@ -363,10 +372,13 @@ struct dt_device_node *device_tree_find_node_by_path(struct dt_device_node *dt,
 {
     struct dt_device_node *np;
 
+    read_lock(&dt_host->lock);
+
     dt_for_each_device_node(dt, np)
         if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
             break;
 
+    read_unlock(&dt_host->lock);
     return np;
 }
 
@@ -450,6 +462,8 @@ dt_find_compatible_node(struct dt_device_node *from,
     struct dt_device_node *np;
     struct dt_device_node *dt;
 
+    read_lock(&dt_host->lock);
+
     dt = from ? from->allnext : dt_host;
     dt_for_each_device_node(dt, np)
     {
@@ -460,6 +474,7 @@ dt_find_compatible_node(struct dt_device_node *from,
             break;
     }
 
+    read_unlock(&dt_host->lock);
     return np;
 }
 
@@ -470,13 +485,19 @@ dt_find_matching_node(struct dt_device_node *from,
     struct dt_device_node *np;
     struct dt_device_node *dt;
 
+    read_lock(&dt_host->lock);
+
     dt = from ? from->allnext : dt_host;
     dt_for_each_device_node(dt, np)
     {
         if ( dt_match_node(matches, np) )
+        {
+            read_unlock(&dt_host->lock);
             return np;
+        }
     }
 
+    read_unlock(&dt_host->lock);
     return NULL;
 }
 
@@ -1052,10 +1073,13 @@ struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle)
 {
     struct dt_device_node *np;
 
+    read_lock(&dt_host->lock);
+
     dt_for_each_device_node(dt_host, np)
         if ( np->phandle == handle )
             break;
 
+    read_unlock(&dt_host->lock);
     return np;
 }
 
@@ -2102,6 +2126,9 @@ int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
 
     dt_dprintk(" <- unflatten_device_tree()\n");
 
+    /* Init r/w lock for host device tree. */
+    rwlock_init(&dt_host->lock);
+
     return 0;
 }
 
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 51e251b0b4..bafc898f1c 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -18,6 +18,7 @@
 #include <xen/string.h>
 #include <xen/types.h>
 #include <xen/list.h>
+#include <xen/rwlock.h>
 
 #define DEVICE_TREE_MAX_DEPTH 16
 
@@ -106,6 +107,11 @@ struct dt_device_node {
     struct list_head domain_list;
 
     struct device dev;
+
+    /*
+     * Lock that protects r/w updates to unflattened device tree i.e. dt_host.
+     */
+    rwlock_t lock;
 };
 
 #define dt_to_dev(dt_node)  (&(dt_node)->dev)
-- 
2.17.1

Re: [XEN][RFC PATCH v4 11/16] common/device_tree: Add rwlock for dt_host

[Julien Grall]

Hi Vikram,

On 07/12/2022 06:18, Vikram Garhwal wrote:
>   Dynamic programming ops will modify the dt_host and there might be other
>   function which are browsing the dt_host at the same time. To avoid the race
>   conditions, adding rwlock for browsing the dt_host.

Looking at the user below, it is not entirely clear what the lock is 
actually protecting. For instance...

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> ---
>   xen/common/device_tree.c      | 27 +++++++++++++++++++++++++++
>   xen/include/xen/device_tree.h |  6 ++++++
>   2 files changed, 33 insertions(+)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index acf26a411d..51ee2a5edf 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np,
>       if ( !np )
>           return NULL;
>   
> +    read_lock(&dt_host->lock);
> +
>       for ( pp = np->properties; pp; pp = pp->next )
>       {
>           if ( dt_prop_cmp(pp->name, name) == 0 )
> @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const struct dt_device_node *np,
>           }
>       }
>   
> +    read_unlock(&dt_host->lock);
>       return pp;
>   }
>   
> @@ -336,11 +339,14 @@ struct dt_device_node *dt_find_node_by_name(struct dt_device_node *from,
>       struct dt_device_node *np;
>       struct dt_device_node *dt;
>   
> +    read_lock(&dt_host->lock);
> +
>       dt = from ? from->allnext : dt_host;
>       dt_for_each_device_node(dt, np)
>           if ( np->name && (dt_node_cmp(np->name, name) == 0) )
>               break;
>   
> +    read_unlock(&dt_host->lock);
>       return np;

... I was expecting the read lock to also protect the value returned 
from being freed. But this is not the case.

Cheers,

-- 
Julien Grall

Re: [XEN][RFC PATCH v4 11/16] common/device_tree: Add rwlock for dt_host

[Vikram Garhwal]

Hi Julien,

On 12/7/22 8:31 AM, Julien Grall wrote:
> Hi Vikram,
>
> On 07/12/2022 06:18, Vikram Garhwal wrote:
>>   Dynamic programming ops will modify the dt_host and there might be 
>> other
>>   function which are browsing the dt_host at the same time. To avoid 
>> the race
>>   conditions, adding rwlock for browsing the dt_host.
>
> Looking at the user below, it is not entirely clear what the lock is 
> actually protecting. For instance...

Purpose of the lock was to protect the read/scanning of dt_host while we 
remove the add/nodes. This lock is also used when nodes are 
added/removed in "[XEN][RFC PATCH v4 12/16]: static int 
remove_nodes(const struct overlay_track *tracker)".


>
>>
>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>> ---
>>   xen/common/device_tree.c      | 27 +++++++++++++++++++++++++++
>>   xen/include/xen/device_tree.h |  6 ++++++
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index acf26a411d..51ee2a5edf 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const 
>> struct dt_device_node *np,
>>       if ( !np )
>>           return NULL;
>>   +    read_lock(&dt_host->lock);
>> +
>>       for ( pp = np->properties; pp; pp = pp->next )
>>       {
>>           if ( dt_prop_cmp(pp->name, name) == 0 )
>> @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const 
>> struct dt_device_node *np,
>>           }
>>       }
>>   +    read_unlock(&dt_host->lock);
>>       return pp;
>>   }
>>   @@ -336,11 +339,14 @@ struct dt_device_node 
>> *dt_find_node_by_name(struct dt_device_node *from,
>>       struct dt_device_node *np;
>>       struct dt_device_node *dt;
>>   +    read_lock(&dt_host->lock);
>> +
>>       dt = from ? from->allnext : dt_host;
>>       dt_for_each_device_node(dt, np)
>>           if ( np->name && (dt_node_cmp(np->name, name) == 0) )
>>               break;
>>   +    read_unlock(&dt_host->lock);
>>       return np;
>
> ... I was expecting the read lock to also protect the value returned 
> from being freed. But this is not the case.
>
Okay. Shall I remove the lock from here and perhaps add it when 
dt_find_node_by_name() and other related functions are called?
> Cheers,
>

Re: [XEN][RFC PATCH v4 11/16] common/device_tree: Add rwlock for dt_host

[Julien Grall]

Hi,

On 01/02/2023 17:05, Vikram Garhwal wrote:
> On 12/7/22 8:31 AM, Julien Grall wrote:
>>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>>> ---
>>>   xen/common/device_tree.c      | 27 +++++++++++++++++++++++++++
>>>   xen/include/xen/device_tree.h |  6 ++++++
>>>   2 files changed, 33 insertions(+)
>>>
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index acf26a411d..51ee2a5edf 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -140,6 +140,8 @@ const struct dt_property *dt_find_property(const 
>>> struct dt_device_node *np,
>>>       if ( !np )
>>>           return NULL;
>>>   +    read_lock(&dt_host->lock);
>>> +
>>>       for ( pp = np->properties; pp; pp = pp->next )
>>>       {
>>>           if ( dt_prop_cmp(pp->name, name) == 0 )
>>> @@ -150,6 +152,7 @@ const struct dt_property *dt_find_property(const 
>>> struct dt_device_node *np,
>>>           }
>>>       }
>>>   +    read_unlock(&dt_host->lock);
>>>       return pp;
>>>   }
>>>   @@ -336,11 +339,14 @@ struct dt_device_node 
>>> *dt_find_node_by_name(struct dt_device_node *from,
>>>       struct dt_device_node *np;
>>>       struct dt_device_node *dt;
>>>   +    read_lock(&dt_host->lock);
>>> +
>>>       dt = from ? from->allnext : dt_host;
>>>       dt_for_each_device_node(dt, np)
>>>           if ( np->name && (dt_node_cmp(np->name, name) == 0) )
>>>               break;
>>>   +    read_unlock(&dt_host->lock);
>>>       return np;
>>
>> ... I was expecting the read lock to also protect the value returned 
>> from being freed. But this is not the case.
>>
> Okay. Shall I remove the lock from here and perhaps add it when 
> dt_find_node_by_name() and other related functions are called?

It is a possibility. I would need to see the end result to be sure a 
lock is actually suitable.

Maybe we will end up to need a refcounting instead if we keep the DT 
node for longer.

Cheers,

-- 
Julien Grall

[XEN][RFC PATCH v4 12/16] xen/arm: Implement device tree node removal functionalities

[Vikram Garhwal]

Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes added using
device tree overlay.

xl dt_overlay remove file.dtbo:
    Removes all the nodes in a given dtbo.
    First, removes IRQ permissions and MMIO accesses. Next, it finds the nodes
    in dt_host and delete the device node entries from dt_host.

    The nodes get removed only if it is not used by any of dom0 or domio.

Also, added overlay_track struct to keep the track of added node through device
tree overlay. overlay_track has dt_host_new which is unflattened form of updated
fdt and name of overlay nodes. When a node is removed, we also free the memory
used by overlay_track for the particular overlay node.

Nested overlay removal is supported in sequential manner only i.e. if
overlay_child nests under overlay_parent, it is assumed that user first removes
overlay_child and then removes overlay_parent.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 xen/common/Makefile          |   1 +
 xen/common/dt_overlay.c      | 411 +++++++++++++++++++++++++++++++++++
 xen/common/sysctl.c          |   5 +
 xen/include/public/sysctl.h  |  19 ++
 xen/include/xen/dt_overlay.h |  55 +++++
 5 files changed, 491 insertions(+)
 create mode 100644 xen/common/dt_overlay.c
 create mode 100644 xen/include/xen/dt_overlay.h

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 3baf83d527..58a35f55b2 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
 obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
 obj-$(CONFIG_IOREQ_SERVER) += dm.o
 obj-y += domain.o
+obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o
 obj-y += event_2l.o
 obj-y += event_channel.o
 obj-y += event_fifo.o
diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
new file mode 100644
index 0000000000..477341f0aa
--- /dev/null
+++ b/xen/common/dt_overlay.c
@@ -0,0 +1,411 @@
+/*
+ * xen/common/dt_overlay.c
+ *
+ * Device tree overlay support in Xen.
+ *
+ * Copyright (c) 2022 AMD Inc.
+ * Written by Vikram Garhwal <vikram.garhwal@amd.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <xen/iocap.h>
+#include <xen/xmalloc.h>
+#include <asm/domain_build.h>
+#include <xen/dt_overlay.h>
+#include <xen/guest_access.h>
+
+static LIST_HEAD(overlay_tracker);
+static DEFINE_SPINLOCK(overlay_lock);
+
+/* Find last descendants of the device_node. */
+static struct dt_device_node *find_last_descendants_node(
+                                            struct dt_device_node *device_node)
+{
+    struct dt_device_node *child_node;
+
+    for ( child_node = device_node->child; child_node->sibling != NULL;
+          child_node = child_node->sibling )
+    {
+    }
+
+    /* If last child_node also have children. */
+    if ( child_node->child )
+        child_node = find_last_descendants_node(child_node);
+    return child_node;
+}
+
+static int dt_overlay_remove_node(struct dt_device_node *device_node)
+{
+    struct dt_device_node *np;
+    struct dt_device_node *parent_node;
+    struct dt_device_node *device_node_last_descendant = device_node->child;
+
+    parent_node = device_node->parent;
+
+    if ( parent_node == NULL )
+    {
+        dt_dprintk("%s's parent node not found\n", device_node->name);
+        return -EFAULT;
+    }
+
+    np = parent_node->child;
+
+    if ( np == NULL )
+    {
+        dt_dprintk("parent node %s's not found\n", parent_node->name);
+        return -EFAULT;
+    }
+
+    /* If node to be removed is only child node or first child. */
+    if ( !dt_node_cmp(np->full_name, device_node->full_name) )
+    {
+        parent_node->child = np->sibling;
+
+        /*
+         * Iterate over all child nodes of device_node. Given that we are
+         * removing parent node, we need to remove all it's descendants too.
+         */
+        if ( device_node_last_descendant )
+        {
+            device_node_last_descendant =
+                                        find_last_descendants_node(device_node);
+            parent_node->allnext = device_node_last_descendant->allnext;
+        }
+        else
+            parent_node->allnext = np->allnext;
+
+        return 0;
+    }
+
+    for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
+    {
+        if ( !dt_node_cmp(np->sibling->full_name, device_node->full_name) )
+        {
+            /* Found the node. Now we remove it. */
+            np->sibling = np->sibling->sibling;
+
+            if ( np->child )
+                np = find_last_descendants_node(np);
+
+            /*
+             * Iterate over all child nodes of device_node. Given that we are
+             * removing parent node, we need to remove all it's descendants too.
+             */
+            if ( device_node_last_descendant )
+                device_node_last_descendant =
+                                        find_last_descendants_node(device_node);
+
+            if ( device_node_last_descendant )
+                np->allnext = device_node_last_descendant->allnext;
+            else
+                np->allnext = np->allnext->allnext;
+
+            break;
+        }
+    }
+
+    return 0;
+}
+
+/* Basic sanity check for the dtbo tool stack provided to Xen. */
+static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
+{
+    if ( (fdt_totalsize(overlay_fdt) != overlay_fdt_size) ||
+          fdt_check_header(overlay_fdt) )
+    {
+        printk(XENLOG_ERR "The overlay FDT is not a valid Flat Device Tree\n");
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+/* Count number of nodes till one level of __overlay__ tag. */
+static unsigned int overlay_node_count(void *fdto)
+{
+    unsigned int num_overlay_nodes = 0;
+    int fragment;
+
+    fdt_for_each_subnode(fragment, fdto, 0)
+    {
+        int subnode;
+        int overlay;
+
+        overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+
+        /*
+         * overlay value can be < 0. But fdt_for_each_subnode() loop checks for
+         * overlay >= 0. So, no need for a overlay>=0 check here.
+         */
+        fdt_for_each_subnode(subnode, fdto, overlay)
+        {
+            num_overlay_nodes++;
+        }
+    }
+
+    return num_overlay_nodes;
+}
+
+static int handle_remove_irq_iommu(struct dt_device_node *device_node)
+{
+    int rc = 0;
+    struct domain *d = hardware_domain;
+    domid_t domid = 0;
+    unsigned int naddr, len;
+    unsigned int i, nirq;
+    u64 addr, size;
+
+    domid = dt_device_used_by(device_node);
+
+    dt_dprintk("Checking if node %s is used by any domain\n",
+               device_node->full_name);
+
+    /* Remove the node iff it's assigned to domain 0 or domain io. */
+    if ( domid != 0 && domid != DOMID_IO )
+    {
+        printk(XENLOG_ERR "Device %s as it is being used by domain %d. Removing nodes failed\n",
+               device_node->full_name, domid);
+        return -EINVAL;
+    }
+
+    dt_dprintk("Removing node: %s\n", device_node->full_name);
+
+    nirq = dt_number_of_irq(device_node);
+
+    /* Remove IRQ permission */
+    for ( i = 0; i < nirq; i++ )
+    {
+        rc = platform_get_irq(device_node, i);;
+
+        if ( irq_access_permitted(d, rc) == false )
+        {
+            printk(XENLOG_ERR "IRQ %d is not routed to domain %d\n", rc,
+                   domid);
+            return -EINVAL;
+        }
+        /*
+         * TODO: We don't handle shared IRQs for now. So, it is assumed that
+         * the IRQs was not shared with another devices.
+         */
+        rc = irq_deny_access(d, rc);
+        if ( rc )
+        {
+            printk(XENLOG_ERR "unable to revoke access for irq %u for %s\n",
+                   i, device_node->full_name);
+            return rc;
+        }
+    }
+
+    /* Check if iommu property exists. */
+    if ( dt_get_property(device_node, "iommus", &len) )
+    {
+
+        rc = iommu_remove_dt_device(device_node);
+        if ( rc != 0 && rc != -ENXIO )
+            return rc;
+    }
+
+    naddr = dt_number_of_address(device_node);
+
+    /* Remove mmio access. */
+    for ( i = 0; i < naddr; i++ )
+    {
+        rc = dt_device_get_address(device_node, i, &addr, &size);
+        if ( rc )
+        {
+            printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
+                   i, dt_node_full_name(device_node));
+            return rc;
+        }
+
+        rc = iomem_deny_access(d, paddr_to_pfn(addr),
+                               paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+        if ( rc )
+        {
+            printk(XENLOG_ERR "Unable to remove dom%d access to"
+                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
+                   d->domain_id,
+                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
+            return rc;
+        }
+
+    }
+
+    return rc;
+}
+
+/* Removes all descendants of the given node. */
+static int remove_all_descendant_nodes(struct dt_device_node *device_node)
+{
+    int rc = 0;
+    struct dt_device_node *child_node;
+
+    for ( child_node = device_node->child; child_node != NULL;
+         child_node = child_node->sibling )
+    {
+        if ( child_node->child )
+            remove_all_descendant_nodes(child_node);
+
+        rc = handle_remove_irq_iommu(child_node);
+        if ( rc )
+            return rc;
+    }
+
+    return rc;
+}
+
+/* Remove nodes from dt_host. */
+static int remove_nodes(const struct overlay_track *tracker)
+{
+    int rc = 0;
+    struct dt_device_node *overlay_node;
+    unsigned int j;
+
+    for ( j = 0; j < tracker->num_nodes; j++ )
+    {
+        overlay_node = (struct dt_device_node *)tracker->nodes_address[j];
+        if ( overlay_node == NULL )
+        {
+            printk(XENLOG_ERR "Device %s is not present in the tree. Removing nodes failed\n",
+                   overlay_node->full_name);
+            return -EINVAL;
+        }
+
+        rc = remove_all_descendant_nodes(overlay_node);
+
+        /* All children nodes are unmapped. Now remove the node itself. */
+        rc = handle_remove_irq_iommu(overlay_node);
+        if ( rc )
+            return rc;
+
+        read_lock(&dt_host->lock);
+
+        rc = dt_overlay_remove_node(overlay_node);
+        if ( rc )
+        {
+            read_unlock(&dt_host->lock);
+
+            return rc;
+        }
+
+        read_unlock(&dt_host->lock);
+    }
+
+    return rc;
+}
+
+/*
+ * First finds the device node to remove. Check if the device is being used by
+ * any dom and finally remove it from dt_host. IOMMU is already being taken care
+ * while destroying the domain.
+ */
+static long handle_remove_overlay_nodes(void *overlay_fdt,
+                                        uint32_t overlay_fdt_size)
+{
+    int rc = 0;
+    struct overlay_track *entry, *temp, *track;
+    bool found_entry = false;
+
+    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+    if ( rc )
+        return rc;
+
+    if ( overlay_node_count(overlay_fdt) == 0 )
+        return -ENOMEM;
+
+    spin_lock(&overlay_lock);
+
+    /*
+     * First check if dtbo is correct i.e. it should one of the dtbo which was
+     * used when dynamically adding the node.
+     * Limitation: Cases with same node names but different property are not
+     * supported currently. We are relying on user to provide the same dtbo
+     * as it was used when adding the nodes.
+     */
+    list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
+    {
+        if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
+        {
+            track = entry;
+            found_entry = true;
+            break;
+        }
+    }
+
+    if ( found_entry == false )
+    {
+        rc = -EINVAL;
+
+        printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
+               " Removing nodes is supported for only prior added dtbo. Please"
+               " provide a valid dtbo which was used to add the nodes.\n");
+        goto out;
+
+    }
+
+    rc = remove_nodes(entry);
+
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Removing node failed\n");
+        goto out;
+    }
+
+    list_del(&entry->entry);
+
+    xfree(entry->dt_host_new);
+    xfree(entry->fdt);
+    xfree(entry->overlay_fdt);
+
+    xfree(entry->nodes_address);
+
+    xfree(entry);
+
+out:
+    spin_unlock(&overlay_lock);
+    return rc;
+}
+
+long dt_sysctl(struct xen_sysctl_dt_overlay *op)
+{
+    long ret = 0;
+    void *overlay_fdt;
+
+    if ( op->overlay_fdt_size <= 0 || op->overlay_fdt_size > 500000 )
+        return -EINVAL;
+
+    overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
+
+    if ( overlay_fdt == NULL )
+        return -ENOMEM;
+
+    ret = copy_from_guest(overlay_fdt, op->overlay_fdt, op->overlay_fdt_size);
+    if ( ret )
+    {
+        gprintk(XENLOG_ERR, "copy from guest failed\n");
+        xfree(overlay_fdt);
+
+        return -EFAULT;
+    }
+
+    switch ( op->overlay_op )
+    {
+    case XEN_SYSCTL_DT_OVERLAY_REMOVE:
+        ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
+        xfree(overlay_fdt);
+
+        break;
+
+    default:
+        break;
+    }
+
+    return ret;
+}
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 02505ab044..bb338b7c27 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -28,6 +28,7 @@
 #include <xen/pmstat.h>
 #include <xen/livepatch.h>
 #include <xen/coverage.h>
+#include <xen/dt_overlay.h>
 
 long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 {
@@ -482,6 +483,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
             copyback = 1;
         break;
 
+    case XEN_SYSCTL_dt_overlay:
+        ret = dt_sysctl(&op->u.dt_overlay);
+        break;
+
     default:
         ret = arch_do_sysctl(op, u_sysctl);
         copyback = 0;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 5672906729..4bc76bbe27 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1079,6 +1079,23 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
 #endif
 
+#define XEN_SYSCTL_DT_OVERLAY_ADD                   1
+#define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
+
+/*
+ * XEN_SYSCTL_dt_overlay
+ * Performs addition/removal of device tree nodes under parent node using dtbo.
+ * This does in three steps:
+ *  - Adds/Removes the nodes from dt_host.
+ *  - Adds/Removes IRQ permission for the nodes.
+ *  - Adds/Removes MMIO accesses.
+ */
+struct xen_sysctl_dt_overlay {
+    XEN_GUEST_HANDLE_64(void) overlay_fdt;
+    uint32_t overlay_fdt_size;  /* Overlay dtb size. */
+    uint8_t overlay_op; /* Add or remove. */
+};
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -1109,6 +1126,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_livepatch_op                  27
 /* #define XEN_SYSCTL_set_parameter              28 */
 #define XEN_SYSCTL_get_cpu_policy                29
+#define XEN_SYSCTL_dt_overlay                    30
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -1139,6 +1157,7 @@ struct xen_sysctl {
 #if defined(__i386__) || defined(__x86_64__)
         struct xen_sysctl_cpu_policy        cpu_policy;
 #endif
+        struct xen_sysctl_dt_overlay        dt_overlay;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/xen/dt_overlay.h b/xen/include/xen/dt_overlay.h
new file mode 100644
index 0000000000..30f4b86586
--- /dev/null
+++ b/xen/include/xen/dt_overlay.h
@@ -0,0 +1,55 @@
+/*
+ * xen/common/dt_overlay.h
+ *
+ * Device tree overlay support in Xen.
+ *
+ * Copyright (c) 2022 AMD Inc.
+ * Written by Vikram Garhwal <vikram.garhwal@amd.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#ifndef __XEN_DT_SYSCTL_H__
+#define __XEN_DT_SYSCTL_H__
+
+#include <xen/list.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/device_tree.h>
+#include <xen/rangeset.h>
+
+/*
+ * overlay_node_track describes information about added nodes through dtbo.
+ * @entry: List pointer.
+ * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
+ * @fdt: Stores the fdt.
+ * @nodes_fullname: Stores the full name of nodes.
+ * @nodes_irq: Stores the IRQ added from overlay dtb.
+ * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
+ * @num_nodes: Stores total number of nodes in overlay dtb.
+ */
+struct overlay_track {
+    struct list_head entry;
+    struct dt_device_node *dt_host_new;
+    void *fdt;
+    void *overlay_fdt;
+    unsigned long *nodes_address;
+    unsigned int num_nodes;
+};
+
+struct xen_sysctl_dt_overlay;
+
+#ifdef CONFIG_OVERLAY_DTB
+long dt_sysctl(struct xen_sysctl_dt_overlay *op);
+#else
+static inline long dt_sysctl(struct xen_sysctl_dt_overlay *op)
+{
+    return -ENOSYS;
+}
+#endif
+#endif
-- 
2.17.1

Re: [XEN][RFC PATCH v4 12/16] xen/arm: Implement device tree node removal functionalities

[Jan Beulich]

On 07.12.2022 07:18, Vikram Garhwal wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1079,6 +1079,23 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
>  #endif
>  
> +#define XEN_SYSCTL_DT_OVERLAY_ADD                   1
> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
> +
> +/*
> + * XEN_SYSCTL_dt_overlay
> + * Performs addition/removal of device tree nodes under parent node using dtbo.
> + * This does in three steps:
> + *  - Adds/Removes the nodes from dt_host.
> + *  - Adds/Removes IRQ permission for the nodes.
> + *  - Adds/Removes MMIO accesses.
> + */
> +struct xen_sysctl_dt_overlay {
> +    XEN_GUEST_HANDLE_64(void) overlay_fdt;
> +    uint32_t overlay_fdt_size;  /* Overlay dtb size. */
> +    uint8_t overlay_op; /* Add or remove. */
> +};
> +
>  struct xen_sysctl {
>      uint32_t cmd;
>  #define XEN_SYSCTL_readconsole                    1
> @@ -1109,6 +1126,7 @@ struct xen_sysctl {
>  #define XEN_SYSCTL_livepatch_op                  27
>  /* #define XEN_SYSCTL_set_parameter              28 */
>  #define XEN_SYSCTL_get_cpu_policy                29
> +#define XEN_SYSCTL_dt_overlay                    30
>      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>      union {
>          struct xen_sysctl_readconsole       readconsole;
> @@ -1139,6 +1157,7 @@ struct xen_sysctl {
>  #if defined(__i386__) || defined(__x86_64__)
>          struct xen_sysctl_cpu_policy        cpu_policy;
>  #endif
> +        struct xen_sysctl_dt_overlay        dt_overlay;

For now your additions are Arm-only, aren't they? You want to use
#ifdef-ary similar to what you see in context in this last hunk then,
to avoid undue exposure.

Jan

Re: [XEN][RFC PATCH v4 12/16] xen/arm: Implement device tree node removal functionalities

[Vikram Garhwal]

Hi Jan,

On 12/7/22 12:41 AM, Jan Beulich wrote:
> On 07.12.2022 07:18, Vikram Garhwal wrote:
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1079,6 +1079,23 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
>>   DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
>>   #endif
>>   
>> +#define XEN_SYSCTL_DT_OVERLAY_ADD                   1
>> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>> +
>> +/*
>> + * XEN_SYSCTL_dt_overlay
>> + * Performs addition/removal of device tree nodes under parent node using dtbo.
>> + * This does in three steps:
>> + *  - Adds/Removes the nodes from dt_host.
>> + *  - Adds/Removes IRQ permission for the nodes.
>> + *  - Adds/Removes MMIO accesses.
>> + */
>> +struct xen_sysctl_dt_overlay {
>> +    XEN_GUEST_HANDLE_64(void) overlay_fdt;
>> +    uint32_t overlay_fdt_size;  /* Overlay dtb size. */
>> +    uint8_t overlay_op; /* Add or remove. */
>> +};
>> +
>>   struct xen_sysctl {
>>       uint32_t cmd;
>>   #define XEN_SYSCTL_readconsole                    1
>> @@ -1109,6 +1126,7 @@ struct xen_sysctl {
>>   #define XEN_SYSCTL_livepatch_op                  27
>>   /* #define XEN_SYSCTL_set_parameter              28 */
>>   #define XEN_SYSCTL_get_cpu_policy                29
>> +#define XEN_SYSCTL_dt_overlay                    30
>>       uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>>       union {
>>           struct xen_sysctl_readconsole       readconsole;
>> @@ -1139,6 +1157,7 @@ struct xen_sysctl {
>>   #if defined(__i386__) || defined(__x86_64__)
>>           struct xen_sysctl_cpu_policy        cpu_policy;
>>   #endif
>> +        struct xen_sysctl_dt_overlay        dt_overlay;
> For now your additions are Arm-only, aren't they? You want to use
> #ifdef-ary similar to what you see in context in this last hunk then,
> to avoid undue exposure.
In v2 there was a comment regarding "No CONFIG_* dependencies in public 
headers". So, i remove the ifdef.
Will add "if defined(__arm__) || defined (__aarch64__)"
>
> Jan

Re: [XEN][RFC PATCH v4 12/16] xen/arm: Implement device tree node removal functionalities

[Michal Orzel]

Hi Vikram,

You received extensive feedback for v3 from others, so I will limit my review to just coding
and general comments.

On 07/12/2022 07:18, Vikram Garhwal wrote:
> 
> 
> Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes added using
> device tree overlay.
> 
> xl dt_overlay remove file.dtbo:
>     Removes all the nodes in a given dtbo.
>     First, removes IRQ permissions and MMIO accesses. Next, it finds the nodes
>     in dt_host and delete the device node entries from dt_host.
> 
>     The nodes get removed only if it is not used by any of dom0 or domio.
> 
> Also, added overlay_track struct to keep the track of added node through device
> tree overlay. overlay_track has dt_host_new which is unflattened form of updated
> fdt and name of overlay nodes. When a node is removed, we also free the memory
> used by overlay_track for the particular overlay node.
> 
> Nested overlay removal is supported in sequential manner only i.e. if
> overlay_child nests under overlay_parent, it is assumed that user first removes
> overlay_child and then removes overlay_parent.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> ---
>  xen/common/Makefile          |   1 +
>  xen/common/dt_overlay.c      | 411 +++++++++++++++++++++++++++++++++++
>  xen/common/sysctl.c          |   5 +
>  xen/include/public/sysctl.h  |  19 ++
>  xen/include/xen/dt_overlay.h |  55 +++++
>  5 files changed, 491 insertions(+)
>  create mode 100644 xen/common/dt_overlay.c
>  create mode 100644 xen/include/xen/dt_overlay.h
> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 3baf83d527..58a35f55b2 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
>  obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
>  obj-$(CONFIG_IOREQ_SERVER) += dm.o
>  obj-y += domain.o
> +obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o
>  obj-y += event_2l.o
>  obj-y += event_channel.o
>  obj-y += event_fifo.o
> diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
> new file mode 100644
> index 0000000000..477341f0aa
> --- /dev/null
> +++ b/xen/common/dt_overlay.c
> @@ -0,0 +1,411 @@
> +/*
> + * xen/common/dt_overlay.c
New files should start with SPDX comment expressing license.

> + *
> + * Device tree overlay support in Xen.
> + *
> + * Copyright (c) 2022 AMD Inc.
> + * Written by Vikram Garhwal <vikram.garhwal@amd.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <xen/iocap.h>
> +#include <xen/xmalloc.h>
> +#include <asm/domain_build.h>
> +#include <xen/dt_overlay.h>
> +#include <xen/guest_access.h>
> +
> +static LIST_HEAD(overlay_tracker);
> +static DEFINE_SPINLOCK(overlay_lock);
> +
> +/* Find last descendants of the device_node. */
> +static struct dt_device_node *find_last_descendants_node(
> +                                            struct dt_device_node *device_node)
> +{
> +    struct dt_device_node *child_node;
> +
> +    for ( child_node = device_node->child; child_node->sibling != NULL;
> +          child_node = child_node->sibling )
> +    {
> +    }
> +
> +    /* If last child_node also have children. */
> +    if ( child_node->child )
> +        child_node = find_last_descendants_node(child_node);
Please add a blank line here.

> +    return child_node;
> +}
> +
> +static int dt_overlay_remove_node(struct dt_device_node *device_node)
> +{
> +    struct dt_device_node *np;
> +    struct dt_device_node *parent_node;
> +    struct dt_device_node *device_node_last_descendant = device_node->child;
> +
> +    parent_node = device_node->parent;
> +
> +    if ( parent_node == NULL )
> +    {
> +        dt_dprintk("%s's parent node not found\n", device_node->name);
> +        return -EFAULT;
> +    }
> +
> +    np = parent_node->child;
> +
> +    if ( np == NULL )
> +    {
> +        dt_dprintk("parent node %s's not found\n", parent_node->name);
> +        return -EFAULT;
> +    }
> +
> +    /* If node to be removed is only child node or first child. */
> +    if ( !dt_node_cmp(np->full_name, device_node->full_name) )
> +    {
> +        parent_node->child = np->sibling;
> +
> +        /*
> +         * Iterate over all child nodes of device_node. Given that we are
> +         * removing parent node, we need to remove all it's descendants too.
> +         */
> +        if ( device_node_last_descendant )
> +        {
> +            device_node_last_descendant =
> +                                        find_last_descendants_node(device_node);
> +            parent_node->allnext = device_node_last_descendant->allnext;
> +        }
> +        else
> +            parent_node->allnext = np->allnext;
> +
> +        return 0;
> +    }
> +
> +    for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
> +    {
> +        if ( !dt_node_cmp(np->sibling->full_name, device_node->full_name) )
> +        {
> +            /* Found the node. Now we remove it. */
> +            np->sibling = np->sibling->sibling;
> +
> +            if ( np->child )
> +                np = find_last_descendants_node(np);
> +
> +            /*
> +             * Iterate over all child nodes of device_node. Given that we are
> +             * removing parent node, we need to remove all it's descendants too.
> +             */
> +            if ( device_node_last_descendant )
> +                device_node_last_descendant =
> +                                        find_last_descendants_node(device_node);
> +
> +            if ( device_node_last_descendant )
> +                np->allnext = device_node_last_descendant->allnext;
> +            else
> +                np->allnext = np->allnext->allnext;
> +
> +            break;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/* Basic sanity check for the dtbo tool stack provided to Xen. */
> +static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
> +{
> +    if ( (fdt_totalsize(overlay_fdt) != overlay_fdt_size) ||
> +          fdt_check_header(overlay_fdt) )
> +    {
> +        printk(XENLOG_ERR "The overlay FDT is not a valid Flat Device Tree\n");
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +/* Count number of nodes till one level of __overlay__ tag. */
> +static unsigned int overlay_node_count(void *fdto)
> +{
> +    unsigned int num_overlay_nodes = 0;
> +    int fragment;
> +
> +    fdt_for_each_subnode(fragment, fdto, 0)
> +    {
> +        int subnode;
> +        int overlay;
> +
> +        overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +
> +        /*
> +         * overlay value can be < 0. But fdt_for_each_subnode() loop checks for
> +         * overlay >= 0. So, no need for a overlay>=0 check here.
> +         */
> +        fdt_for_each_subnode(subnode, fdto, overlay)
> +        {
> +            num_overlay_nodes++;
> +        }
> +    }
> +
> +    return num_overlay_nodes;
> +}
> +
> +static int handle_remove_irq_iommu(struct dt_device_node *device_node)
> +{
> +    int rc = 0;
> +    struct domain *d = hardware_domain;
> +    domid_t domid = 0;
No need for this assignment.

> +    unsigned int naddr, len;
> +    unsigned int i, nirq;
> +    u64 addr, size;
We should not be using types like these anymore. Use uint64_t.

> +
> +    domid = dt_device_used_by(device_node);
> +
> +    dt_dprintk("Checking if node %s is used by any domain\n",
> +               device_node->full_name);
> +
> +    /* Remove the node iff it's assigned to domain 0 or domain io. */
> +    if ( domid != 0 && domid != DOMID_IO )
> +    {
> +        printk(XENLOG_ERR "Device %s as it is being used by domain %d. Removing nodes failed\n",
> +               device_node->full_name, domid);
> +        return -EINVAL;
> +    }
> +
> +    dt_dprintk("Removing node: %s\n", device_node->full_name);
> +
> +    nirq = dt_number_of_irq(device_node);
> +
> +    /* Remove IRQ permission */
> +    for ( i = 0; i < nirq; i++ )
> +    {
> +        rc = platform_get_irq(device_node, i);;
> +
> +        if ( irq_access_permitted(d, rc) == false )
> +        {
> +            printk(XENLOG_ERR "IRQ %d is not routed to domain %d\n", rc,
> +                   domid);
> +            return -EINVAL;
> +        }
> +        /*
> +         * TODO: We don't handle shared IRQs for now. So, it is assumed that
> +         * the IRQs was not shared with another devices.
> +         */
> +        rc = irq_deny_access(d, rc);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR "unable to revoke access for irq %u for %s\n",
> +                   i, device_node->full_name);
> +            return rc;
> +        }
> +    }
> +
> +    /* Check if iommu property exists. */
> +    if ( dt_get_property(device_node, "iommus", &len) )
> +    {
> +
Remove extra line.

> +        rc = iommu_remove_dt_device(device_node);
> +        if ( rc != 0 && rc != -ENXIO )
> +            return rc;
> +    }
> +
> +    naddr = dt_number_of_address(device_node);
> +
> +    /* Remove mmio access. */
> +    for ( i = 0; i < naddr; i++ )
> +    {
> +        rc = dt_device_get_address(device_node, i, &addr, &size);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> +                   i, dt_node_full_name(device_node));
> +            return rc;
> +        }
> +
> +        rc = iomem_deny_access(d, paddr_to_pfn(addr),
> +                               paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR "Unable to remove dom%d access to"
> +                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
> +                   d->domain_id,
> +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> +            return rc;
> +        }
What about removing p2m mappings (comment from Julien on v3)?

> +
> +    }
> +
> +    return rc;
> +}
> +
> +/* Removes all descendants of the given node. */
> +static int remove_all_descendant_nodes(struct dt_device_node *device_node)
> +{
> +    int rc = 0;
> +    struct dt_device_node *child_node;
> +
> +    for ( child_node = device_node->child; child_node != NULL;
> +         child_node = child_node->sibling )
> +    {
> +        if ( child_node->child )
> +            remove_all_descendant_nodes(child_node);
> +
> +        rc = handle_remove_irq_iommu(child_node);
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    return rc;
> +}
> +
> +/* Remove nodes from dt_host. */
> +static int remove_nodes(const struct overlay_track *tracker)
> +{
> +    int rc = 0;
> +    struct dt_device_node *overlay_node;
> +    unsigned int j;
> +
> +    for ( j = 0; j < tracker->num_nodes; j++ )
> +    {
> +        overlay_node = (struct dt_device_node *)tracker->nodes_address[j];
> +        if ( overlay_node == NULL )
> +        {
> +            printk(XENLOG_ERR "Device %s is not present in the tree. Removing nodes failed\n",
> +                   overlay_node->full_name);
> +            return -EINVAL;
> +        }
> +
> +        rc = remove_all_descendant_nodes(overlay_node);
> +
> +        /* All children nodes are unmapped. Now remove the node itself. */
> +        rc = handle_remove_irq_iommu(overlay_node);
> +        if ( rc )
> +            return rc;
> +
> +        read_lock(&dt_host->lock);
> +
> +        rc = dt_overlay_remove_node(overlay_node);
> +        if ( rc )
> +        {
> +            read_unlock(&dt_host->lock);
> +
> +            return rc;
> +        }
> +
> +        read_unlock(&dt_host->lock);
> +    }
> +
> +    return rc;
> +}
> +
> +/*
> + * First finds the device node to remove. Check if the device is being used by
> + * any dom and finally remove it from dt_host. IOMMU is already being taken care
> + * while destroying the domain.
> + */
> +static long handle_remove_overlay_nodes(void *overlay_fdt,
> +                                        uint32_t overlay_fdt_size)
> +{
> +    int rc = 0;
> +    struct overlay_track *entry, *temp, *track;
> +    bool found_entry = false;
> +
> +    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> +    if ( rc )
> +        return rc;
> +
> +    if ( overlay_node_count(overlay_fdt) == 0 )
> +        return -ENOMEM;
> +
> +    spin_lock(&overlay_lock);
> +
> +    /*
> +     * First check if dtbo is correct i.e. it should one of the dtbo which was
> +     * used when dynamically adding the node.
> +     * Limitation: Cases with same node names but different property are not
> +     * supported currently. We are relying on user to provide the same dtbo
> +     * as it was used when adding the nodes.
> +     */
> +    list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> +    {
> +        if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
> +        {
> +            track = entry;
> +            found_entry = true;
> +            break;
> +        }
> +    }
> +
> +    if ( found_entry == false )
> +    {
> +        rc = -EINVAL;
> +
> +        printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
> +               " Removing nodes is supported for only prior added dtbo. Please"
> +               " provide a valid dtbo which was used to add the nodes.\n");
> +        goto out;
> +
> +    }
> +
> +    rc = remove_nodes(entry);
> +
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "Removing node failed\n");
> +        goto out;
> +    }
> +
> +    list_del(&entry->entry);
> +
> +    xfree(entry->dt_host_new);
> +    xfree(entry->fdt);
> +    xfree(entry->overlay_fdt);
> +
> +    xfree(entry->nodes_address);
> +
> +    xfree(entry);
> +
> +out:
> +    spin_unlock(&overlay_lock);
> +    return rc;
> +}
> +
> +long dt_sysctl(struct xen_sysctl_dt_overlay *op)
> +{
> +    long ret = 0;
No need for assignign a value that will be reassigned anyway.

> +    void *overlay_fdt;
> +
> +    if ( op->overlay_fdt_size <= 0 || op->overlay_fdt_size > 500000 )
FWICS, you want to limit the fdt size to 500KB which should be 512000.
Also, it would be clearer to use KB(500). Otherwise such value is a bit ambiguous.
Also overlay_fdt_size cannot be < 0.

> +        return -EINVAL;
> +
> +    overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
If you alloc the bytes here and the op will not be XEN_SYSCTL_DT_OVERLAY_REMOVE,
then you will end up without freeing it.

> +
> +    if ( overlay_fdt == NULL )
> +        return -ENOMEM;
> +
> +    ret = copy_from_guest(overlay_fdt, op->overlay_fdt, op->overlay_fdt_size);
> +    if ( ret )
> +    {
> +        gprintk(XENLOG_ERR, "copy from guest failed\n");
> +        xfree(overlay_fdt);
> +
> +        return -EFAULT;
> +    }
> +
> +    switch ( op->overlay_op )
> +    {
> +    case XEN_SYSCTL_DT_OVERLAY_REMOVE:
> +        ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
> +        xfree(overlay_fdt);
> +
> +        break;
> +
> +    default:
> +        break;
> +    }
> +
> +    return ret;
> +}
Don't you want to put EMACS comments block here?

> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 02505ab044..bb338b7c27 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -28,6 +28,7 @@
>  #include <xen/pmstat.h>
>  #include <xen/livepatch.h>
>  #include <xen/coverage.h>
> +#include <xen/dt_overlay.h>
> 
>  long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>  {
> @@ -482,6 +483,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>              copyback = 1;
>          break;
> 
> +    case XEN_SYSCTL_dt_overlay:
If you protect xen_sysctl_dt_overlay with ARM ifdefery as Jan suggested,
then you should move this handling to arch_do_sysctl.

> +        ret = dt_sysctl(&op->u.dt_overlay);
> +        break;
> +
>      default:
>          ret = arch_do_sysctl(op, u_sysctl);
>          copyback = 0;
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 5672906729..4bc76bbe27 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1079,6 +1079,23 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
>  #endif
> 
> +#define XEN_SYSCTL_DT_OVERLAY_ADD                   1
I'm not sure whether the ADD macro should be added in this patch.

> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
> +
> +/*
> + * XEN_SYSCTL_dt_overlay
> + * Performs addition/removal of device tree nodes under parent node using dtbo.
> + * This does in three steps:
> + *  - Adds/Removes the nodes from dt_host.
> + *  - Adds/Removes IRQ permission for the nodes.
> + *  - Adds/Removes MMIO accesses.
> + */
> +struct xen_sysctl_dt_overlay {
> +    XEN_GUEST_HANDLE_64(void) overlay_fdt;
FWICS, this is the output variable and it would be beneficial to add a comment.
Also, usually IN variables appear first.

> +    uint32_t overlay_fdt_size;  /* Overlay dtb size. */
> +    uint8_t overlay_op; /* Add or remove. */
These are the input variables, so the comment should be e.g. /* IN: Overlay dtb size */

> +};
> +
>  struct xen_sysctl {
>      uint32_t cmd;
>  #define XEN_SYSCTL_readconsole                    1
> @@ -1109,6 +1126,7 @@ struct xen_sysctl {
>  #define XEN_SYSCTL_livepatch_op                  27
>  /* #define XEN_SYSCTL_set_parameter              28 */
>  #define XEN_SYSCTL_get_cpu_policy                29
> +#define XEN_SYSCTL_dt_overlay                    30
>      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>      union {
>          struct xen_sysctl_readconsole       readconsole;
> @@ -1139,6 +1157,7 @@ struct xen_sysctl {
>  #if defined(__i386__) || defined(__x86_64__)
>          struct xen_sysctl_cpu_policy        cpu_policy;
>  #endif
> +        struct xen_sysctl_dt_overlay        dt_overlay;
>          uint8_t                             pad[128];
>      } u;
>  };
> diff --git a/xen/include/xen/dt_overlay.h b/xen/include/xen/dt_overlay.h
> new file mode 100644
> index 0000000000..30f4b86586
> --- /dev/null
> +++ b/xen/include/xen/dt_overlay.h
> @@ -0,0 +1,55 @@
> +/*
Missing SPDX comment at the top of the files.

> + * xen/common/dt_overlay.h
Incorrect path.

> + *
> + * Device tree overlay support in Xen.
> + *
> + * Copyright (c) 2022 AMD Inc.
> + * Written by Vikram Garhwal <vikram.garhwal@amd.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#ifndef __XEN_DT_SYSCTL_H__
> +#define __XEN_DT_SYSCTL_H__
> +
> +#include <xen/list.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/device_tree.h>
> +#include <xen/rangeset.h>
> +
> +/*
> + * overlay_node_track describes information about added nodes through dtbo.
> + * @entry: List pointer.
> + * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
> + * @fdt: Stores the fdt.
> + * @nodes_fullname: Stores the full name of nodes.
> + * @nodes_irq: Stores the IRQ added from overlay dtb.
> + * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
> + * @num_nodes: Stores total number of nodes in overlay dtb.
> + */
> +struct overlay_track {
> +    struct list_head entry;
> +    struct dt_device_node *dt_host_new;
> +    void *fdt;
> +    void *overlay_fdt;
> +    unsigned long *nodes_address;
> +    unsigned int num_nodes;
> +};
> +
> +struct xen_sysctl_dt_overlay;
> +
> +#ifdef CONFIG_OVERLAY_DTB
> +long dt_sysctl(struct xen_sysctl_dt_overlay *op);
> +#else
> +static inline long dt_sysctl(struct xen_sysctl_dt_overlay *op)
> +{
> +    return -ENOSYS;
> +}
> +#endif
> +#endif
Don't you want to put EMACS comments block here?

> --
> 2.17.1
> 
> 

~Michal

Re: [XEN][RFC PATCH v4 12/16] xen/arm: Implement device tree node removal functionalities

[Vikram Garhwal]

Hi Michal,

On 1/24/23 1:01 AM, Michal Orzel wrote:
> Hi Vikram,
>
> You received extensive feedback for v3 from others, so I will limit my review to just coding
> and general comments.
>
> On 07/12/2022 07:18, Vikram Garhwal wrote:
>>
>> Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes added using
>> device tree overlay.
>>
>> xl dt_overlay remove file.dtbo:
>>      Removes all the nodes in a given dtbo.
>>      First, removes IRQ permissions and MMIO accesses. Next, it finds the nodes
>>      in dt_host and delete the device node entries from dt_host.
>>
>>      The nodes get removed only if it is not used by any of dom0 or domio.
>>
>> Also, added overlay_track struct to keep the track of added node through device
>> tree overlay. overlay_track has dt_host_new which is unflattened form of updated
>> fdt and name of overlay nodes. When a node is removed, we also free the memory
>> used by overlay_track for the particular overlay node.
>>
>> Nested overlay removal is supported in sequential manner only i.e. if
>> overlay_child nests under overlay_parent, it is assumed that user first removes
>> overlay_child and then removes overlay_parent.
>>
>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>> ---
>>   xen/common/Makefile          |   1 +
>>   xen/common/dt_overlay.c      | 411 +++++++++++++++++++++++++++++++++++
>>   xen/common/sysctl.c          |   5 +
>>   xen/include/public/sysctl.h  |  19 ++
>>   xen/include/xen/dt_overlay.h |  55 +++++
>>   5 files changed, 491 insertions(+)
>>   create mode 100644 xen/common/dt_overlay.c
>>   create mode 100644 xen/include/xen/dt_overlay.h
>>
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index 3baf83d527..58a35f55b2 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
>>   obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
>>   obj-$(CONFIG_IOREQ_SERVER) += dm.o
>>   obj-y += domain.o
>> +obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o
>>   obj-y += event_2l.o
>>   obj-y += event_channel.o
>>   obj-y += event_fifo.o
>> diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
>> new file mode 100644
>> index 0000000000..477341f0aa
>> --- /dev/null
>> +++ b/xen/common/dt_overlay.c
>> @@ -0,0 +1,411 @@
>> +/*
>> + * xen/common/dt_overlay.c
> New files should start with SPDX comment expressing license.
>
>> + *
>> + * Device tree overlay support in Xen.
>> + *
>> + * Copyright (c) 2022 AMD Inc.
>> + * Written by Vikram Garhwal <vikram.garhwal@amd.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms and conditions of the GNU General Public
>> + * License, version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <xen/iocap.h>
>> +#include <xen/xmalloc.h>
>> +#include <asm/domain_build.h>
>> +#include <xen/dt_overlay.h>
>> +#include <xen/guest_access.h>
>> +
>> +static LIST_HEAD(overlay_tracker);
>> +static DEFINE_SPINLOCK(overlay_lock);
>> +
>> +/* Find last descendants of the device_node. */
>> +static struct dt_device_node *find_last_descendants_node(
>> +                                            struct dt_device_node *device_node)
>> +{
>> +    struct dt_device_node *child_node;
>> +
>> +    for ( child_node = device_node->child; child_node->sibling != NULL;
>> +          child_node = child_node->sibling )
>> +    {
>> +    }
>> +
>> +    /* If last child_node also have children. */
>> +    if ( child_node->child )
>> +        child_node = find_last_descendants_node(child_node);
> Please add a blank line here.
>
>> +    return child_node;
>> +}
>> +
>> +static int dt_overlay_remove_node(struct dt_device_node *device_node)
>> +{
>> +    struct dt_device_node *np;
>> +    struct dt_device_node *parent_node;
>> +    struct dt_device_node *device_node_last_descendant = device_node->child;
>> +
>> +    parent_node = device_node->parent;
>> +
>> +    if ( parent_node == NULL )
>> +    {
>> +        dt_dprintk("%s's parent node not found\n", device_node->name);
>> +        return -EFAULT;
>> +    }
>> +
>> +    np = parent_node->child;
>> +
>> +    if ( np == NULL )
>> +    {
>> +        dt_dprintk("parent node %s's not found\n", parent_node->name);
>> +        return -EFAULT;
>> +    }
>> +
>> +    /* If node to be removed is only child node or first child. */
>> +    if ( !dt_node_cmp(np->full_name, device_node->full_name) )
>> +    {
>> +        parent_node->child = np->sibling;
>> +
>> +        /*
>> +         * Iterate over all child nodes of device_node. Given that we are
>> +         * removing parent node, we need to remove all it's descendants too.
>> +         */
>> +        if ( device_node_last_descendant )
>> +        {
>> +            device_node_last_descendant =
>> +                                        find_last_descendants_node(device_node);
>> +            parent_node->allnext = device_node_last_descendant->allnext;
>> +        }
>> +        else
>> +            parent_node->allnext = np->allnext;
>> +
>> +        return 0;
>> +    }
>> +
>> +    for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
>> +    {
>> +        if ( !dt_node_cmp(np->sibling->full_name, device_node->full_name) )
>> +        {
>> +            /* Found the node. Now we remove it. */
>> +            np->sibling = np->sibling->sibling;
>> +
>> +            if ( np->child )
>> +                np = find_last_descendants_node(np);
>> +
>> +            /*
>> +             * Iterate over all child nodes of device_node. Given that we are
>> +             * removing parent node, we need to remove all it's descendants too.
>> +             */
>> +            if ( device_node_last_descendant )
>> +                device_node_last_descendant =
>> +                                        find_last_descendants_node(device_node);
>> +
>> +            if ( device_node_last_descendant )
>> +                np->allnext = device_node_last_descendant->allnext;
>> +            else
>> +                np->allnext = np->allnext->allnext;
>> +
>> +            break;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/* Basic sanity check for the dtbo tool stack provided to Xen. */
>> +static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
>> +{
>> +    if ( (fdt_totalsize(overlay_fdt) != overlay_fdt_size) ||
>> +          fdt_check_header(overlay_fdt) )
>> +    {
>> +        printk(XENLOG_ERR "The overlay FDT is not a valid Flat Device Tree\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +/* Count number of nodes till one level of __overlay__ tag. */
>> +static unsigned int overlay_node_count(void *fdto)
>> +{
>> +    unsigned int num_overlay_nodes = 0;
>> +    int fragment;
>> +
>> +    fdt_for_each_subnode(fragment, fdto, 0)
>> +    {
>> +        int subnode;
>> +        int overlay;
>> +
>> +        overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
>> +
>> +        /*
>> +         * overlay value can be < 0. But fdt_for_each_subnode() loop checks for
>> +         * overlay >= 0. So, no need for a overlay>=0 check here.
>> +         */
>> +        fdt_for_each_subnode(subnode, fdto, overlay)
>> +        {
>> +            num_overlay_nodes++;
>> +        }
>> +    }
>> +
>> +    return num_overlay_nodes;
>> +}
>> +
>> +static int handle_remove_irq_iommu(struct dt_device_node *device_node)
>> +{
>> +    int rc = 0;
>> +    struct domain *d = hardware_domain;
>> +    domid_t domid = 0;
> No need for this assignment.
>
>> +    unsigned int naddr, len;
>> +    unsigned int i, nirq;
>> +    u64 addr, size;
> We should not be using types like these anymore. Use uint64_t.
>
>> +
>> +    domid = dt_device_used_by(device_node);
>> +
>> +    dt_dprintk("Checking if node %s is used by any domain\n",
>> +               device_node->full_name);
>> +
>> +    /* Remove the node iff it's assigned to domain 0 or domain io. */
>> +    if ( domid != 0 && domid != DOMID_IO )
>> +    {
>> +        printk(XENLOG_ERR "Device %s as it is being used by domain %d. Removing nodes failed\n",
>> +               device_node->full_name, domid);
>> +        return -EINVAL;
>> +    }
>> +
>> +    dt_dprintk("Removing node: %s\n", device_node->full_name);
>> +
>> +    nirq = dt_number_of_irq(device_node);
>> +
>> +    /* Remove IRQ permission */
>> +    for ( i = 0; i < nirq; i++ )
>> +    {
>> +        rc = platform_get_irq(device_node, i);;
>> +
>> +        if ( irq_access_permitted(d, rc) == false )
>> +        {
>> +            printk(XENLOG_ERR "IRQ %d is not routed to domain %d\n", rc,
>> +                   domid);
>> +            return -EINVAL;
>> +        }
>> +        /*
>> +         * TODO: We don't handle shared IRQs for now. So, it is assumed that
>> +         * the IRQs was not shared with another devices.
>> +         */
>> +        rc = irq_deny_access(d, rc);
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_ERR "unable to revoke access for irq %u for %s\n",
>> +                   i, device_node->full_name);
>> +            return rc;
>> +        }
>> +    }
>> +
>> +    /* Check if iommu property exists. */
>> +    if ( dt_get_property(device_node, "iommus", &len) )
>> +    {
>> +
> Remove extra line.
>
>> +        rc = iommu_remove_dt_device(device_node);
>> +        if ( rc != 0 && rc != -ENXIO )
>> +            return rc;
>> +    }
>> +
>> +    naddr = dt_number_of_address(device_node);
>> +
>> +    /* Remove mmio access. */
>> +    for ( i = 0; i < naddr; i++ )
>> +    {
>> +        rc = dt_device_get_address(device_node, i, &addr, &size);
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
>> +                   i, dt_node_full_name(device_node));
>> +            return rc;
>> +        }
>> +
>> +        rc = iomem_deny_access(d, paddr_to_pfn(addr),
>> +                               paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_ERR "Unable to remove dom%d access to"
>> +                   " 0x%"PRIx64" - 0x%"PRIx64"\n",
>> +                   d->domain_id,
>> +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>> +            return rc;
>> +        }
> What about removing p2m mappings (comment from Julien on v3)?

The main purpose of this series to address first part of dynamic programming i.e.
making Xen aware of new device tree node which means updating the dt_host with overlay node
information. Here we are adding/removing node from dt_host, and checking/set IOMMU and IRQ
permission but never mapping them to any domain. Right now, mapping/Un-mapping will happen
only when a new domU is created/destroyed using "xl create".

To map IOREQ and IOMMU during runtime, there will be another small series after this one where we
will do the actual IOMMU and IRQ mapping do a running domain and will call unmap_mmio_regions()
to remove the mapping.

Here is the patch for that series:
	https://github.com/Xilinx/xen/commit/76fcd5defc1c7c375cb99ac73a4d1138aa87d474
I will clean it and send once current series is done.

Also, If you see addition part of this series, we put "skip_mapping = true" which means
map_range_to_domain()->map_range_p2mt() is never called. The only thing function which will
be called is iomem_permit_access() while adding the node and here we are calling opposite
function to remove the access.

Please let me know if my explanation is not clear.

I addressed rest of your comments in next series.

Regards,
Vikram

>
>> +
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +/* Removes all descendants of the given node. */
>> +static int remove_all_descendant_nodes(struct dt_device_node *device_node)
>> +{
>> +    int rc = 0;
>> +    struct dt_device_node *child_node;
>> +
>> +    for ( child_node = device_node->child; child_node != NULL;
>> +         child_node = child_node->sibling )
>> +    {
>> +        if ( child_node->child )
>> +            remove_all_descendant_nodes(child_node);
>> +
>> +        rc = handle_remove_irq_iommu(child_node);
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +/* Remove nodes from dt_host. */
>> +static int remove_nodes(const struct overlay_track *tracker)
>> +{
>> +    int rc = 0;
>> +    struct dt_device_node *overlay_node;
>> +    unsigned int j;
>> +
>> +    for ( j = 0; j < tracker->num_nodes; j++ )
>> +    {
>> +        overlay_node = (struct dt_device_node *)tracker->nodes_address[j];
>> +        if ( overlay_node == NULL )
>> +        {
>> +            printk(XENLOG_ERR "Device %s is not present in the tree. Removing nodes failed\n",
>> +                   overlay_node->full_name);
>> +            return -EINVAL;
>> +        }
>> +
>> +        rc = remove_all_descendant_nodes(overlay_node);
>> +
>> +        /* All children nodes are unmapped. Now remove the node itself. */
>> +        rc = handle_remove_irq_iommu(overlay_node);
>> +        if ( rc )
>> +            return rc;
>> +
>> +        read_lock(&dt_host->lock);
>> +
>> +        rc = dt_overlay_remove_node(overlay_node);
>> +        if ( rc )
>> +        {
>> +            read_unlock(&dt_host->lock);
>> +
>> +            return rc;
>> +        }
>> +
>> +        read_unlock(&dt_host->lock);
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +/*
>> + * First finds the device node to remove. Check if the device is being used by
>> + * any dom and finally remove it from dt_host. IOMMU is already being taken care
>> + * while destroying the domain.
>> + */
>> +static long handle_remove_overlay_nodes(void *overlay_fdt,
>> +                                        uint32_t overlay_fdt_size)
>> +{
>> +    int rc = 0;
>> +    struct overlay_track *entry, *temp, *track;
>> +    bool found_entry = false;
>> +
>> +    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    if ( overlay_node_count(overlay_fdt) == 0 )
>> +        return -ENOMEM;
>> +
>> +    spin_lock(&overlay_lock);
>> +
>> +    /*
>> +     * First check if dtbo is correct i.e. it should one of the dtbo which was
>> +     * used when dynamically adding the node.
>> +     * Limitation: Cases with same node names but different property are not
>> +     * supported currently. We are relying on user to provide the same dtbo
>> +     * as it was used when adding the nodes.
>> +     */
>> +    list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
>> +    {
>> +        if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
>> +        {
>> +            track = entry;
>> +            found_entry = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if ( found_entry == false )
>> +    {
>> +        rc = -EINVAL;
>> +
>> +        printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
>> +               " Removing nodes is supported for only prior added dtbo. Please"
>> +               " provide a valid dtbo which was used to add the nodes.\n");
>> +        goto out;
>> +
>> +    }
>> +
>> +    rc = remove_nodes(entry);
>> +
>> +    if ( rc )
>> +    {
>> +        printk(XENLOG_ERR "Removing node failed\n");
>> +        goto out;
>> +    }
>> +
>> +    list_del(&entry->entry);
>> +
>> +    xfree(entry->dt_host_new);
>> +    xfree(entry->fdt);
>> +    xfree(entry->overlay_fdt);
>> +
>> +    xfree(entry->nodes_address);
>> +
>> +    xfree(entry);
>> +
>> +out:
>> +    spin_unlock(&overlay_lock);
>> +    return rc;
>> +}
>> +
>> +long dt_sysctl(struct xen_sysctl_dt_overlay *op)
>> +{
>> +    long ret = 0;
> No need for assignign a value that will be reassigned anyway.
>
>> +    void *overlay_fdt;
>> +
>> +    if ( op->overlay_fdt_size <= 0 || op->overlay_fdt_size > 500000 )
> FWICS, you want to limit the fdt size to 500KB which should be 512000.
> Also, it would be clearer to use KB(500). Otherwise such value is a bit ambiguous.
> Also overlay_fdt_size cannot be < 0.
>
>> +        return -EINVAL;
>> +
>> +    overlay_fdt = xmalloc_bytes(op->overlay_fdt_size);
> If you alloc the bytes here and the op will not be XEN_SYSCTL_DT_OVERLAY_REMOVE,
> then you will end up without freeing it.
>
>> +
>> +    if ( overlay_fdt == NULL )
>> +        return -ENOMEM;
>> +
>> +    ret = copy_from_guest(overlay_fdt, op->overlay_fdt, op->overlay_fdt_size);
>> +    if ( ret )
>> +    {
>> +        gprintk(XENLOG_ERR, "copy from guest failed\n");
>> +        xfree(overlay_fdt);
>> +
>> +        return -EFAULT;
>> +    }
>> +
>> +    switch ( op->overlay_op )
>> +    {
>> +    case XEN_SYSCTL_DT_OVERLAY_REMOVE:
>> +        ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
>> +        xfree(overlay_fdt);
>> +
>> +        break;
>> +
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return ret;
>> +}
> Don't you want to put EMACS comments block here?
>
>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
>> index 02505ab044..bb338b7c27 100644
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -28,6 +28,7 @@
>>   #include <xen/pmstat.h>
>>   #include <xen/livepatch.h>
>>   #include <xen/coverage.h>
>> +#include <xen/dt_overlay.h>
>>
>>   long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>   {
>> @@ -482,6 +483,10 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>>               copyback = 1;
>>           break;
>>
>> +    case XEN_SYSCTL_dt_overlay:
> If you protect xen_sysctl_dt_overlay with ARM ifdefery as Jan suggested,
> then you should move this handling to arch_do_sysctl.
>
>> +        ret = dt_sysctl(&op->u.dt_overlay);
>> +        break;
>> +
>>       default:
>>           ret = arch_do_sysctl(op, u_sysctl);
>>           copyback = 0;
>> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
>> index 5672906729..4bc76bbe27 100644
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1079,6 +1079,23 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
>>   DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
>>   #endif
>>
>> +#define XEN_SYSCTL_DT_OVERLAY_ADD                   1
> I'm not sure whether the ADD macro should be added in this patch.
>
>> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
>> +
>> +/*
>> + * XEN_SYSCTL_dt_overlay
>> + * Performs addition/removal of device tree nodes under parent node using dtbo.
>> + * This does in three steps:
>> + *  - Adds/Removes the nodes from dt_host.
>> + *  - Adds/Removes IRQ permission for the nodes.
>> + *  - Adds/Removes MMIO accesses.
>> + */
>> +struct xen_sysctl_dt_overlay {
>> +    XEN_GUEST_HANDLE_64(void) overlay_fdt;
> FWICS, this is the output variable and it would be beneficial to add a comment.
> Also, usually IN variables appear first.
>
>> +    uint32_t overlay_fdt_size;  /* Overlay dtb size. */
>> +    uint8_t overlay_op; /* Add or remove. */
> These are the input variables, so the comment should be e.g. /* IN: Overlay dtb size */
>
>> +};
>> +
>>   struct xen_sysctl {
>>       uint32_t cmd;
>>   #define XEN_SYSCTL_readconsole                    1
>> @@ -1109,6 +1126,7 @@ struct xen_sysctl {
>>   #define XEN_SYSCTL_livepatch_op                  27
>>   /* #define XEN_SYSCTL_set_parameter              28 */
>>   #define XEN_SYSCTL_get_cpu_policy                29
>> +#define XEN_SYSCTL_dt_overlay                    30
>>       uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>>       union {
>>           struct xen_sysctl_readconsole       readconsole;
>> @@ -1139,6 +1157,7 @@ struct xen_sysctl {
>>   #if defined(__i386__) || defined(__x86_64__)
>>           struct xen_sysctl_cpu_policy        cpu_policy;
>>   #endif
>> +        struct xen_sysctl_dt_overlay        dt_overlay;
>>           uint8_t                             pad[128];
>>       } u;
>>   };
>> diff --git a/xen/include/xen/dt_overlay.h b/xen/include/xen/dt_overlay.h
>> new file mode 100644
>> index 0000000000..30f4b86586
>> --- /dev/null
>> +++ b/xen/include/xen/dt_overlay.h
>> @@ -0,0 +1,55 @@
>> +/*
> Missing SPDX comment at the top of the files.
>
>> + * xen/common/dt_overlay.h
> Incorrect path.
>
>> + *
>> + * Device tree overlay support in Xen.
>> + *
>> + * Copyright (c) 2022 AMD Inc.
>> + * Written by Vikram Garhwal <vikram.garhwal@amd.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms and conditions of the GNU General Public
>> + * License, version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +#ifndef __XEN_DT_SYSCTL_H__
>> +#define __XEN_DT_SYSCTL_H__
>> +
>> +#include <xen/list.h>
>> +#include <xen/libfdt/libfdt.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/rangeset.h>
>> +
>> +/*
>> + * overlay_node_track describes information about added nodes through dtbo.
>> + * @entry: List pointer.
>> + * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
>> + * @fdt: Stores the fdt.
>> + * @nodes_fullname: Stores the full name of nodes.
>> + * @nodes_irq: Stores the IRQ added from overlay dtb.
>> + * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
>> + * @num_nodes: Stores total number of nodes in overlay dtb.
>> + */
>> +struct overlay_track {
>> +    struct list_head entry;
>> +    struct dt_device_node *dt_host_new;
>> +    void *fdt;
>> +    void *overlay_fdt;
>> +    unsigned long *nodes_address;
>> +    unsigned int num_nodes;
>> +};
>> +
>> +struct xen_sysctl_dt_overlay;
>> +
>> +#ifdef CONFIG_OVERLAY_DTB
>> +long dt_sysctl(struct xen_sysctl_dt_overlay *op);
>> +#else
>> +static inline long dt_sysctl(struct xen_sysctl_dt_overlay *op)
>> +{
>> +    return -ENOSYS;
>> +}
>> +#endif
>> +#endif
> Don't you want to put EMACS comments block here?
>
>> --
>> 2.17.1
>>
>>
> ~Michal

[XEN][RFC PATCH v4 13/16] xen/arm: Implement device tree node addition functionalities

[Vikram Garhwal]

Update sysctl XEN_SYSCTL_dt_overlay to enable support for dtbo nodes addition
using device tree overlay.

xl dt_overlay add file.dtbo:
    Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
    device_tree_flattened) is created and updated with overlay nodes. This
    updated fdt is further unflattened to a dt_host_new. Next, it checks if any
    of the overlay nodes already exists in the dt_host. If overlay nodes doesn't
    exist then find the overlay nodes in dt_host_new, find the overlay node's
    parent in dt_host and add the nodes as child under their parent in the
    dt_host. The node is attached as the last node under target parent.

    Finally, add IRQs, add device to IOMMUs, set permissions and map MMIO for the
    overlay node.

When a node is added using overlay, a new entry is allocated in the
overlay_track to keep the track of memory allocation due to addition of overlay
node. This is helpful for freeing the memory allocated when a device tree node
is removed.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 xen/common/dt_overlay.c | 465 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 465 insertions(+)

diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
index 477341f0aa..f5426b9dab 100644
--- a/xen/common/dt_overlay.c
+++ b/xen/common/dt_overlay.c
@@ -38,9 +38,29 @@ static struct dt_device_node *find_last_descendants_node(
     /* If last child_node also have children. */
     if ( child_node->child )
         child_node = find_last_descendants_node(child_node);
+
     return child_node;
 }
 
+/*
+ * Returns next node to the input node. If node has children then return
+ * last descendant's next node.
+*/
+static struct dt_device_node *dt_find_next_node(struct dt_device_node *dt,
+                                                struct dt_device_node *node)
+{
+    struct dt_device_node *np;
+
+    dt_for_each_device_node(dt, np)
+        if ( np == node )
+            break;
+
+    if ( np->child )
+        np = find_last_descendants_node(np);
+
+    return np->allnext;
+}
+
 static int dt_overlay_remove_node(struct dt_device_node *device_node)
 {
     struct dt_device_node *np;
@@ -114,6 +134,74 @@ static int dt_overlay_remove_node(struct dt_device_node *device_node)
     return 0;
 }
 
+static int dt_overlay_add_node(struct dt_device_node *device_node,
+                               const char *parent_node_path)
+{
+    struct dt_device_node *parent_node;
+    struct dt_device_node *np, *np_last_descendant;
+    struct dt_device_node *next_node;
+    struct dt_device_node *device_node_last_descendant;
+
+    parent_node = dt_find_node_by_path(parent_node_path);
+
+    if ( parent_node == NULL )
+    {
+        dt_dprintk("Node not found. Overlay node will not be added\n");
+        return -EINVAL;
+    }
+
+    /* If parent has no child. */
+    if ( parent_node->child == NULL )
+    {
+        next_node = parent_node->allnext;
+        device_node->parent = parent_node;
+        parent_node->allnext = device_node;
+        parent_node->child = device_node;
+    }
+    else
+    {
+        /* If parent has at least one child node.
+         * Iterate to the last child node of parent.
+         */
+        for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
+        {
+        }
+
+        /* Iterate over all child nodes of np node. */
+        if ( np->child )
+        {
+            np_last_descendant = find_last_descendants_node(np);
+
+            next_node = np_last_descendant->allnext;
+            np_last_descendant->allnext = device_node;
+        }
+        else
+        {
+            next_node = np->allnext;
+            np->allnext = device_node;
+        }
+
+        device_node->parent = parent_node;
+        np->sibling = device_node;
+        np->sibling->sibling = NULL;
+    }
+
+    /* Iterate over all child nodes of device_node to add children too. */
+    if ( device_node->child )
+    {
+        device_node_last_descendant = find_last_descendants_node(device_node);
+        /* Plug next_node at the end of last children of device_node. */
+        device_node_last_descendant->allnext = next_node;
+    }
+    else
+    {
+        /* Now plug next_node at the end of device_node. */
+        device_node->allnext = next_node;
+    }
+
+    return 0;
+}
+
 /* Basic sanity check for the dtbo tool stack provided to Xen. */
 static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
 {
@@ -153,6 +241,79 @@ static unsigned int overlay_node_count(void *fdto)
     return num_overlay_nodes;
 }
 
+/*
+ * overlay_get_nodes_info will get full name with path for all the nodes which
+ * are in one level of __overlay__ tag. This is useful when checking node for
+ * duplication i.e. dtbo tries to add nodes which already exists in device tree.
+ */
+static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path,
+                                  unsigned int num_overlay_nodes)
+{
+    int fragment;
+    unsigned int node_num = 0;
+
+    *nodes_full_path = xzalloc_bytes(num_overlay_nodes * sizeof(char *));
+
+    if ( *nodes_full_path == NULL )
+        return -ENOMEM;
+
+    fdt_for_each_subnode(fragment, fdto, 0)
+    {
+        int target;
+        int overlay;
+        int subnode;
+        const char *target_path;
+
+        target = fdt_overlay_target_offset(device_tree_flattened, fdto,
+                                           fragment, &target_path);
+        if ( target < 0 )
+            return target;
+
+        overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+
+        /*
+         * overlay value can be < 0. But fdt_for_each_subnode() loop checks for
+         * overlay >= 0. So, no need for a overlay>=0 check here.
+         */
+        fdt_for_each_subnode(subnode, fdto, overlay)
+        {
+            const char *node_name = NULL;
+            int node_name_len = 0;
+            unsigned int target_path_len = strlen(target_path);
+            unsigned int node_full_name_len = 0;
+
+            node_name = fdt_get_name(fdto, subnode, &node_name_len);
+
+            if ( node_name == NULL )
+                return -EINVAL;
+
+            /*
+             * Magic number 2 is for adding '/'. This is done to keep the
+             * node_full_name in the correct full node name format.
+             */
+            node_full_name_len = target_path_len + node_name_len + 2;
+
+            (*nodes_full_path)[node_num] = xmalloc_bytes(node_full_name_len);
+
+            if ( (*nodes_full_path)[node_num] == NULL )
+                return -ENOMEM;
+
+            memcpy((*nodes_full_path)[node_num], target_path, target_path_len);
+
+            (*nodes_full_path)[node_num][target_path_len] = '/';
+
+            memcpy((*nodes_full_path)[node_num] + target_path_len + 1,
+                    node_name, node_name_len);
+
+            (*nodes_full_path)[node_num][node_full_name_len - 1] = '\0';
+
+            node_num++;
+        }
+    }
+
+    return 0;
+}
+
 static int handle_remove_irq_iommu(struct dt_device_node *device_node)
 {
     int rc = 0;
@@ -373,6 +534,302 @@ out:
     return rc;
 }
 
+/*
+ * Handles IRQ and IOMMU mapping for the overlay_node and all descendants of the
+ * overlay_nodes.
+ */
+static int handle_add_irq_iommu(struct domain *d,
+                                struct dt_device_node *overlay_node)
+{
+    int rc = 0;
+    unsigned int naddr, i, len;
+    u64 addr, size;
+    struct dt_device_node *np;
+
+    /* First let's handle the interrupts. */
+    rc = handle_device_interrupts(d, overlay_node, false);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Interrupt failed\n");
+        return rc;
+    }
+
+    /* Check if iommu property exists. */
+    if ( dt_get_property(overlay_node, "iommus", &len) )
+    {
+
+        /* Add device to IOMMUs. */
+        rc = iommu_add_dt_device(overlay_node);
+        if ( rc < 0 )
+        {
+            printk(XENLOG_ERR "Failed to add %s to the IOMMU\n",
+                   dt_node_full_name(overlay_node));
+            return rc;
+        }
+    }
+
+    /* Set permissions. */
+    naddr = dt_number_of_address(overlay_node);
+
+    dt_dprintk("%s passthrough = %d naddr = %u\n",
+               dt_node_full_name(overlay_node), false, naddr);
+
+    /* Give permission for map MMIOs */
+    for ( i = 0; i < naddr; i++ )
+    {
+        struct map_range_data mr_data = { .d = d,
+                                          .p2mt = p2m_mmio_direct_c,
+                                          .skip_mapping = true };
+
+        rc = dt_device_get_address(overlay_node, i, &addr, &size);
+        if ( rc )
+        {
+            printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
+                   i, dt_node_full_name(overlay_node));
+            return rc;
+        }
+
+        rc = map_range_to_domain(overlay_node, addr, size, &mr_data);
+        if ( rc )
+            return rc;
+    }
+
+    /* Map IRQ and IOMMU for overlay_node's children. */
+    for ( np = overlay_node->child; np != NULL; np = np->sibling)
+    {
+        rc = handle_add_irq_iommu(d, np);
+    }
+
+    return rc;
+}
+
+/*
+ * Adds device tree nodes under target node.
+ * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This
+ * is done to avoid the removal of device_tree generation, iomem regions mapping
+ * to hardware domain done by handle_node().
+ */
+static long handle_add_overlay_nodes(void *overlay_fdt,
+                                     uint32_t overlay_fdt_size)
+{
+    int rc = 0, j = 0, i;
+    struct dt_device_node *overlay_node, *prev_node, *next_node;
+    struct domain *d = hardware_domain;
+    struct overlay_track *tr = NULL;
+    char **nodes_full_path = NULL;
+    unsigned int new_fdt_size;
+
+    tr = xzalloc(struct overlay_track);
+    if ( tr == NULL )
+    {
+        return -ENOMEM;
+    }
+
+    new_fdt_size = fdt_totalsize(device_tree_flattened) +
+                                 fdt_totalsize(overlay_fdt);
+
+    tr->fdt = xzalloc_bytes(new_fdt_size);
+    if ( tr->fdt == NULL )
+        return -ENOMEM;
+
+    tr->num_nodes = overlay_node_count(overlay_fdt);
+    if ( tr->num_nodes == 0 )
+    {
+        xfree(tr->fdt);
+        return -ENOMEM;
+    }
+
+    tr->nodes_address = xzalloc_bytes(tr->num_nodes * sizeof(unsigned long));
+    if ( tr->nodes_address == NULL )
+    {
+        xfree(tr->fdt);
+        return -ENOMEM;
+    }
+
+    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+    if ( rc )
+    {
+        xfree(tr->fdt);
+        return rc;
+    }
+
+    /*
+     * Keep a copy of overlay_fdt as fdt_overlay_apply will change the input
+     * overlay's content(magic) when applying overlay.
+     */
+    tr->overlay_fdt = xzalloc_bytes(overlay_fdt_size);
+    if ( tr->overlay_fdt == NULL )
+    {
+        xfree(tr->fdt);
+        return -ENOMEM;
+    }
+
+    memcpy(tr->overlay_fdt, overlay_fdt, overlay_fdt_size);
+
+    spin_lock(&overlay_lock);
+
+    memcpy(tr->fdt, device_tree_flattened,
+           fdt_totalsize(device_tree_flattened));
+
+    /* Open tr->fdt with more space to accommodate the overlay_fdt. */
+    fdt_open_into(tr->fdt, tr->fdt, new_fdt_size);
+
+    /*
+     * overlay_get_nodes_info is called to get the node information from dtbo.
+     * This is done before fdt_overlay_apply() because the overlay apply will
+     * erase the magic of overlay_fdt.
+     */
+    rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
+                                tr->num_nodes);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Getting nodes information failed with error %d\n",
+               rc);
+        goto err;
+    }
+
+    rc = fdt_overlay_apply(tr->fdt, overlay_fdt);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc);
+        goto err;
+    }
+
+    /*
+     * Check if any of the node already exists in dt_host. If node already exits
+     * we can return here as this overlay_fdt is not suitable for overlay ops.
+     */
+    for ( j = 0; j < tr->num_nodes; j++ )
+    {
+        overlay_node = dt_find_node_by_path(nodes_full_path[j]);
+        if ( overlay_node != NULL )
+        {
+            printk(XENLOG_ERR "node %s exists in device tree\n",
+                   nodes_full_path[j]);
+            rc = -EINVAL;
+            goto err;
+        }
+    }
+
+    /* Unflatten the tr->fdt into a new dt_host. */
+    rc = unflatten_device_tree(tr->fdt, &tr->dt_host_new);
+    if ( rc < 0 )
+        goto err;
+
+    for ( j = 0; j < tr->num_nodes; j++ )
+    {
+        dt_dprintk("Adding node: %s\n", nodes_full_path[j]);
+
+        /* Find the newly added node in tr->dt_host_new by it's full path. */
+        overlay_node = device_tree_find_node_by_path(tr->dt_host_new,
+                                                     nodes_full_path[j]);
+        if ( overlay_node == NULL )
+        {
+            dt_dprintk("%s node not found\n", nodes_full_path[j]);
+            rc = -EFAULT;
+            goto remove_node;
+        }
+
+        /*
+         * Find previous and next node to overlay_node in dt_host_new. We will
+         * need these nodes to fix the dt_host_new mapping. When overlay_node is
+         * take out of dt_host_new tree and added to dt_host, link between
+         * previous node and next_node is broken. We will need to refresh
+         * dt_host_new with correct linking for any other overlay nodes
+         * extraction in future.
+         */
+        dt_for_each_device_node(tr->dt_host_new, prev_node)
+            if ( prev_node->allnext == overlay_node )
+                break;
+
+        next_node = dt_find_next_node(tr->dt_host_new, overlay_node);
+
+        read_lock(&dt_host->lock);
+
+        /* Add the node to dt_host. */
+        rc = dt_overlay_add_node(overlay_node, overlay_node->parent->full_name);
+        if ( rc )
+        {
+            read_unlock(&dt_host->lock);
+
+            /* Node not added in dt_host. */
+            goto remove_node;
+        }
+
+        read_unlock(&dt_host->lock);
+
+        prev_node->allnext = next_node;
+
+        overlay_node = dt_find_node_by_path(overlay_node->full_name);
+        if ( overlay_node == NULL )
+        {
+            /* Sanity check. But code will never come here. */
+            ASSERT_UNREACHABLE();
+            goto remove_node;
+        }
+
+        rc = handle_add_irq_iommu(d, overlay_node);
+
+        /* Keep overlay_node address in tracker. */
+        tr->nodes_address[j] = (unsigned long)overlay_node;
+    }
+
+    INIT_LIST_HEAD(&tr->entry);
+    list_add_tail(&tr->entry, &overlay_tracker);
+
+    spin_unlock(&overlay_lock);
+
+    if ( nodes_full_path != NULL )
+    {
+        for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL;
+              i++ )
+        {
+            xfree(nodes_full_path[i]);
+        }
+        xfree(nodes_full_path);
+    }
+
+    return rc;
+
+/*
+ * Failure case. We need to remove the nodes, free tracker(if tr exists) and
+ * tr->dt_host_new.
+ */
+remove_node:
+    tr->num_nodes = j;
+    rc = remove_nodes(tr);
+
+    if ( rc )
+    {
+        /* If removing node fails, this may cause memory leaks. */
+        printk(XENLOG_ERR "Removing node failed.\n");
+        spin_unlock(&overlay_lock);
+        return rc;
+    }
+
+err:
+    spin_unlock(&overlay_lock);
+
+    xfree(tr->dt_host_new);
+    xfree(tr->fdt);
+    xfree(tr->overlay_fdt);
+    xfree(tr->nodes_address);
+
+    if ( nodes_full_path != NULL )
+    {
+        for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL;
+              i++ )
+        {
+            xfree(nodes_full_path[i]);
+        }
+        xfree(nodes_full_path);
+    }
+
+    xfree(tr);
+
+    return rc;
+}
+
 long dt_sysctl(struct xen_sysctl_dt_overlay *op)
 {
     long ret = 0;
@@ -397,6 +854,14 @@ long dt_sysctl(struct xen_sysctl_dt_overlay *op)
 
     switch ( op->overlay_op )
     {
+    case XEN_SYSCTL_DT_OVERLAY_ADD:
+        ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
+
+        if ( ret )
+            xfree(overlay_fdt);
+
+        break;
+
     case XEN_SYSCTL_DT_OVERLAY_REMOVE:
         ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
         xfree(overlay_fdt);
-- 
2.17.1

Re: [XEN][RFC PATCH v4 13/16] xen/arm: Implement device tree node addition functionalities

[Michal Orzel]

Hi Vikram,

On 07/12/2022 07:18, Vikram Garhwal wrote:
> 
> 
> Update sysctl XEN_SYSCTL_dt_overlay to enable support for dtbo nodes addition
> using device tree overlay.
> 
> xl dt_overlay add file.dtbo:
>     Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
>     device_tree_flattened) is created and updated with overlay nodes. This
>     updated fdt is further unflattened to a dt_host_new. Next, it checks if any
>     of the overlay nodes already exists in the dt_host. If overlay nodes doesn't
>     exist then find the overlay nodes in dt_host_new, find the overlay node's
>     parent in dt_host and add the nodes as child under their parent in the
>     dt_host. The node is attached as the last node under target parent.
> 
>     Finally, add IRQs, add device to IOMMUs, set permissions and map MMIO for the
>     overlay node.
> 
> When a node is added using overlay, a new entry is allocated in the
> overlay_track to keep the track of memory allocation due to addition of overlay
> node. This is helpful for freeing the memory allocated when a device tree node
> is removed.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> ---
>  xen/common/dt_overlay.c | 465 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 465 insertions(+)
> 
> diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
> index 477341f0aa..f5426b9dab 100644
> --- a/xen/common/dt_overlay.c
> +++ b/xen/common/dt_overlay.c
> @@ -38,9 +38,29 @@ static struct dt_device_node *find_last_descendants_node(
>      /* If last child_node also have children. */
>      if ( child_node->child )
>          child_node = find_last_descendants_node(child_node);
> +
This should be done in a previous patch as I pointed out in patch 12.

>      return child_node;
>  }
> 
> +/*
> + * Returns next node to the input node. If node has children then return
> + * last descendant's next node.
> +*/
> +static struct dt_device_node *dt_find_next_node(struct dt_device_node *dt,
> +                                                struct dt_device_node *node)
I think the node should be const.

> +{
> +    struct dt_device_node *np;
> +
> +    dt_for_each_device_node(dt, np)
> +        if ( np == node )
> +            break;
> +
> +    if ( np->child )
> +        np = find_last_descendants_node(np);
> +
> +    return np->allnext;
> +}
> +
>  static int dt_overlay_remove_node(struct dt_device_node *device_node)
>  {
>      struct dt_device_node *np;
> @@ -114,6 +134,74 @@ static int dt_overlay_remove_node(struct dt_device_node *device_node)
>      return 0;
>  }
> 
> +static int dt_overlay_add_node(struct dt_device_node *device_node,
> +                               const char *parent_node_path)
> +{
> +    struct dt_device_node *parent_node;
> +    struct dt_device_node *np, *np_last_descendant;
> +    struct dt_device_node *next_node;
> +    struct dt_device_node *device_node_last_descendant;
> +
> +    parent_node = dt_find_node_by_path(parent_node_path);
> +
> +    if ( parent_node == NULL )
> +    {
> +        dt_dprintk("Node not found. Overlay node will not be added\n");
> +        return -EINVAL;
> +    }
> +
> +    /* If parent has no child. */
> +    if ( parent_node->child == NULL )
> +    {
> +        next_node = parent_node->allnext;
> +        device_node->parent = parent_node;
> +        parent_node->allnext = device_node;
> +        parent_node->child = device_node;
> +    }
> +    else
> +    {
> +        /* If parent has at least one child node.
> +         * Iterate to the last child node of parent.
> +         */
> +        for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
> +        {
> +        }
Instead of {} you could just put ; at the end of for statement.

> +
> +        /* Iterate over all child nodes of np node. */
> +        if ( np->child )
> +        {
> +            np_last_descendant = find_last_descendants_node(np);
> +
> +            next_node = np_last_descendant->allnext;
> +            np_last_descendant->allnext = device_node;
> +        }
> +        else
> +        {
> +            next_node = np->allnext;
> +            np->allnext = device_node;
> +        }
> +
> +        device_node->parent = parent_node;
> +        np->sibling = device_node;
> +        np->sibling->sibling = NULL;
> +    }
> +
> +    /* Iterate over all child nodes of device_node to add children too. */
> +    if ( device_node->child )
> +    {
> +        device_node_last_descendant = find_last_descendants_node(device_node);
> +        /* Plug next_node at the end of last children of device_node. */
> +        device_node_last_descendant->allnext = next_node;
> +    }
> +    else
> +    {
> +        /* Now plug next_node at the end of device_node. */
> +        device_node->allnext = next_node;
> +    }
> +
> +    return 0;
> +}
> +
>  /* Basic sanity check for the dtbo tool stack provided to Xen. */
>  static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
>  {
> @@ -153,6 +241,79 @@ static unsigned int overlay_node_count(void *fdto)
>      return num_overlay_nodes;
>  }
> 
> +/*
> + * overlay_get_nodes_info will get full name with path for all the nodes which
> + * are in one level of __overlay__ tag. This is useful when checking node for
> + * duplication i.e. dtbo tries to add nodes which already exists in device tree.
> + */
> +static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path,
> +                                  unsigned int num_overlay_nodes)
> +{
> +    int fragment;
> +    unsigned int node_num = 0;
node_num should be moved under the second for loop.

> +
> +    *nodes_full_path = xzalloc_bytes(num_overlay_nodes * sizeof(char *));
> +
> +    if ( *nodes_full_path == NULL )
> +        return -ENOMEM;
> +
> +    fdt_for_each_subnode(fragment, fdto, 0)
> +    {
> +        int target;
> +        int overlay;
> +        int subnode;
> +        const char *target_path;
> +
> +        target = fdt_overlay_target_offset(device_tree_flattened, fdto,
> +                                           fragment, &target_path);
> +        if ( target < 0 )
> +            return target;
> +
> +        overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +
> +        /*
> +         * overlay value can be < 0. But fdt_for_each_subnode() loop checks for
> +         * overlay >= 0. So, no need for a overlay>=0 check here.
> +         */
> +        fdt_for_each_subnode(subnode, fdto, overlay)
> +        {
> +            const char *node_name = NULL;
> +            int node_name_len = 0;
No need for assignment.

> +            unsigned int target_path_len = strlen(target_path);
> +            unsigned int node_full_name_len = 0;
No need for assignment.
> +
> +            node_name = fdt_get_name(fdto, subnode, &node_name_len);
> +
> +            if ( node_name == NULL )
> +                return -EINVAL;
In case of node_name being NULL, the error code is stored in node_name_len, so
don't you want to return it instead of -EINVAL?

> +
> +            /*
> +             * Magic number 2 is for adding '/'. This is done to keep the
> +             * node_full_name in the correct full node name format.
> +             */
> +            node_full_name_len = target_path_len + node_name_len + 2;
> +
> +            (*nodes_full_path)[node_num] = xmalloc_bytes(node_full_name_len);
> +
> +            if ( (*nodes_full_path)[node_num] == NULL )
> +                return -ENOMEM;
> +
> +            memcpy((*nodes_full_path)[node_num], target_path, target_path_len);
> +
> +            (*nodes_full_path)[node_num][target_path_len] = '/';
> +
> +            memcpy((*nodes_full_path)[node_num] + target_path_len + 1,
> +                    node_name, node_name_len);
> +
> +            (*nodes_full_path)[node_num][node_full_name_len - 1] = '\0';
> +
> +            node_num++;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int handle_remove_irq_iommu(struct dt_device_node *device_node)
>  {
>      int rc = 0;
> @@ -373,6 +534,302 @@ out:
>      return rc;
>  }
> 
> +/*
> + * Handles IRQ and IOMMU mapping for the overlay_node and all descendants of the
> + * overlay_nodes.
> + */
> +static int handle_add_irq_iommu(struct domain *d,
> +                                struct dt_device_node *overlay_node)
> +{
> +    int rc = 0;
No need for assignment.

> +    unsigned int naddr, i, len;
> +    u64 addr, size;
Should be uint64_t.

> +    struct dt_device_node *np;
> +
> +    /* First let's handle the interrupts. */
> +    rc = handle_device_interrupts(d, overlay_node, false);
> +    if ( rc )
To match the handle_device_interrupts behavior on failure, you should check ( rc < 0 )

> +    {
> +        printk(XENLOG_ERR "Interrupt failed\n");
> +        return rc;
> +    }
> +
> +    /* Check if iommu property exists. */
> +    if ( dt_get_property(overlay_node, "iommus", &len) )
> +    {
> +
> +        /* Add device to IOMMUs. */
> +        rc = iommu_add_dt_device(overlay_node);
> +        if ( rc < 0 )
> +        {
> +            printk(XENLOG_ERR "Failed to add %s to the IOMMU\n",
> +                   dt_node_full_name(overlay_node));
> +            return rc;
> +        }
> +    }
> +
> +    /* Set permissions. */
> +    naddr = dt_number_of_address(overlay_node);
> +
> +    dt_dprintk("%s passthrough = %d naddr = %u\n",
> +               dt_node_full_name(overlay_node), false, naddr);
> +
> +    /* Give permission for map MMIOs */
> +    for ( i = 0; i < naddr; i++ )
> +    {
> +        struct map_range_data mr_data = { .d = d,
> +                                          .p2mt = p2m_mmio_direct_c,
> +                                          .skip_mapping = true };
> +
> +        rc = dt_device_get_address(overlay_node, i, &addr, &size);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> +                   i, dt_node_full_name(overlay_node));
> +            return rc;
> +        }
> +
> +        rc = map_range_to_domain(overlay_node, addr, size, &mr_data);
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    /* Map IRQ and IOMMU for overlay_node's children. */
> +    for ( np = overlay_node->child; np != NULL; np = np->sibling)
> +    {
> +        rc = handle_add_irq_iommu(d, np);
Shouldn't you stop looping if rc not zero?

> +    }
> +
> +    return rc;
> +}
> +
> +/*
> + * Adds device tree nodes under target node.
> + * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This
> + * is done to avoid the removal of device_tree generation, iomem regions mapping
> + * to hardware domain done by handle_node().
> + */
> +static long handle_add_overlay_nodes(void *overlay_fdt,
> +                                     uint32_t overlay_fdt_size)
> +{
> +    int rc = 0, j = 0, i;
No need for assignments.

> +    struct dt_device_node *overlay_node, *prev_node, *next_node;
> +    struct domain *d = hardware_domain;
> +    struct overlay_track *tr = NULL;
> +    char **nodes_full_path = NULL;
> +    unsigned int new_fdt_size;
> +
> +    tr = xzalloc(struct overlay_track);
> +    if ( tr == NULL )
> +    {
No need for braces.

> +        return -ENOMEM;
> +    }
> +
> +    new_fdt_size = fdt_totalsize(device_tree_flattened) +
> +                                 fdt_totalsize(overlay_fdt);
> +
> +    tr->fdt = xzalloc_bytes(new_fdt_size);
> +    if ( tr->fdt == NULL )
What about allocated tr? Shouldn't you free it? Also this applies to ...

> +        return -ENOMEM;
> +
> +    tr->num_nodes = overlay_node_count(overlay_fdt);
> +    if ( tr->num_nodes == 0 )
> +    {
> +        xfree(tr->fdt);
here and ...

> +        return -ENOMEM;
> +    }
> +
> +    tr->nodes_address = xzalloc_bytes(tr->num_nodes * sizeof(unsigned long));
> +    if ( tr->nodes_address == NULL )
> +    {
> +        xfree(tr->fdt);
here and ...

> +        return -ENOMEM;
> +    }
> +
> +    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> +    if ( rc )
> +    {
> +        xfree(tr->fdt);
here and ...

> +        return rc;
> +    }
> +
> +    /*
> +     * Keep a copy of overlay_fdt as fdt_overlay_apply will change the input
> +     * overlay's content(magic) when applying overlay.
> +     */
> +    tr->overlay_fdt = xzalloc_bytes(overlay_fdt_size);
> +    if ( tr->overlay_fdt == NULL )
> +    {
> +        xfree(tr->fdt);
here.

> +        return -ENOMEM;
> +    }
> +
> +    memcpy(tr->overlay_fdt, overlay_fdt, overlay_fdt_size);
> +
> +    spin_lock(&overlay_lock);
> +
> +    memcpy(tr->fdt, device_tree_flattened,
> +           fdt_totalsize(device_tree_flattened));
> +
> +    /* Open tr->fdt with more space to accommodate the overlay_fdt. */
> +    fdt_open_into(tr->fdt, tr->fdt, new_fdt_size);
> +
> +    /*
> +     * overlay_get_nodes_info is called to get the node information from dtbo.
> +     * This is done before fdt_overlay_apply() because the overlay apply will
> +     * erase the magic of overlay_fdt.
> +     */
> +    rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
> +                                tr->num_nodes);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "Getting nodes information failed with error %d\n",
> +               rc);
> +        goto err;
> +    }
> +
> +    rc = fdt_overlay_apply(tr->fdt, overlay_fdt);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc);
> +        goto err;
> +    }
> +
> +    /*
> +     * Check if any of the node already exists in dt_host. If node already exits
> +     * we can return here as this overlay_fdt is not suitable for overlay ops.
> +     */
> +    for ( j = 0; j < tr->num_nodes; j++ )
> +    {
> +        overlay_node = dt_find_node_by_path(nodes_full_path[j]);
> +        if ( overlay_node != NULL )
> +        {
> +            printk(XENLOG_ERR "node %s exists in device tree\n",
> +                   nodes_full_path[j]);
> +            rc = -EINVAL;
> +            goto err;
> +        }
> +    }
> +
> +    /* Unflatten the tr->fdt into a new dt_host. */
> +    rc = unflatten_device_tree(tr->fdt, &tr->dt_host_new);
> +    if ( rc < 0 )
> +        goto err;
> +
> +    for ( j = 0; j < tr->num_nodes; j++ )
> +    {
> +        dt_dprintk("Adding node: %s\n", nodes_full_path[j]);
> +
> +        /* Find the newly added node in tr->dt_host_new by it's full path. */
> +        overlay_node = device_tree_find_node_by_path(tr->dt_host_new,
> +                                                     nodes_full_path[j]);
> +        if ( overlay_node == NULL )
> +        {
> +            dt_dprintk("%s node not found\n", nodes_full_path[j]);
> +            rc = -EFAULT;
> +            goto remove_node;
> +        }
> +
> +        /*
> +         * Find previous and next node to overlay_node in dt_host_new. We will
> +         * need these nodes to fix the dt_host_new mapping. When overlay_node is
> +         * take out of dt_host_new tree and added to dt_host, link between
> +         * previous node and next_node is broken. We will need to refresh
> +         * dt_host_new with correct linking for any other overlay nodes
> +         * extraction in future.
> +         */
> +        dt_for_each_device_node(tr->dt_host_new, prev_node)
> +            if ( prev_node->allnext == overlay_node )
> +                break;
> +
> +        next_node = dt_find_next_node(tr->dt_host_new, overlay_node);
> +
> +        read_lock(&dt_host->lock);
> +
> +        /* Add the node to dt_host. */
> +        rc = dt_overlay_add_node(overlay_node, overlay_node->parent->full_name);
> +        if ( rc )
> +        {
> +            read_unlock(&dt_host->lock);
> +
> +            /* Node not added in dt_host. */
> +            goto remove_node;
> +        }
> +
> +        read_unlock(&dt_host->lock);
> +
> +        prev_node->allnext = next_node;
> +
> +        overlay_node = dt_find_node_by_path(overlay_node->full_name);
> +        if ( overlay_node == NULL )
> +        {
> +            /* Sanity check. But code will never come here. */
> +            ASSERT_UNREACHABLE();
> +            goto remove_node;
> +        }
> +
> +        rc = handle_add_irq_iommu(d, overlay_node);
Shouldn't you check rc and exit the loop earlier in case of failure?

> +
> +        /* Keep overlay_node address in tracker. */
> +        tr->nodes_address[j] = (unsigned long)overlay_node;
> +    }
> +
> +    INIT_LIST_HEAD(&tr->entry);
> +    list_add_tail(&tr->entry, &overlay_tracker);
> +
> +    spin_unlock(&overlay_lock);
> +
> +    if ( nodes_full_path != NULL )
> +    {
> +        for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL;
> +              i++ )
> +        {
> +            xfree(nodes_full_path[i]);
> +        }
> +        xfree(nodes_full_path);
> +    }
> +
> +    return rc;
> +
> +/*
> + * Failure case. We need to remove the nodes, free tracker(if tr exists) and
> + * tr->dt_host_new.
> + */
> +remove_node:
> +    tr->num_nodes = j;
> +    rc = remove_nodes(tr);
> +
> +    if ( rc )
> +    {
> +        /* If removing node fails, this may cause memory leaks. */
> +        printk(XENLOG_ERR "Removing node failed.\n");
> +        spin_unlock(&overlay_lock);
> +        return rc;
> +    }
> +
> +err:
> +    spin_unlock(&overlay_lock);
> +
> +    xfree(tr->dt_host_new);
> +    xfree(tr->fdt);
> +    xfree(tr->overlay_fdt);
> +    xfree(tr->nodes_address);
> +
> +    if ( nodes_full_path != NULL )
> +    {
> +        for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL;
> +              i++ )
> +        {
> +            xfree(nodes_full_path[i]);
> +        }
> +        xfree(nodes_full_path);
> +    }
> +
> +    xfree(tr);
> +
> +    return rc;
> +}
> +
>  long dt_sysctl(struct xen_sysctl_dt_overlay *op)
>  {
>      long ret = 0;
> @@ -397,6 +854,14 @@ long dt_sysctl(struct xen_sysctl_dt_overlay *op)
> 
>      switch ( op->overlay_op )
>      {
> +    case XEN_SYSCTL_DT_OVERLAY_ADD:
> +        ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
> +
> +        if ( ret )
> +            xfree(overlay_fdt);
> +
> +        break;
> +
>      case XEN_SYSCTL_DT_OVERLAY_REMOVE:
>          ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size);
>          xfree(overlay_fdt);
> --
> 2.17.1
> 
> 

~Michal

[XEN][RFC PATCH v4 14/16] tools/libs/ctrl: Implement new xc interfaces for dt overlay

[Vikram Garhwal]

xc_dt_overlay() sends the device tree binary overlay, size of .dtbo and overlay
operation type i.e. add or remove to xen.

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 tools/include/xenctrl.h         |  3 ++
 tools/libs/ctrl/Makefile.common |  1 +
 tools/libs/ctrl/xc_dt_overlay.c | 51 +++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+)
 create mode 100644 tools/libs/ctrl/xc_dt_overlay.c

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 0c8b4c3aa7..a71bc0bb1d 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2633,6 +2633,9 @@ int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint32
 int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
                          xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
 
+int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
+                  uint32_t overlay_fdt_size, uint8_t overlay_op);
+
 /* Compat shims */
 #include "xenctrl_compat.h"
 
diff --git a/tools/libs/ctrl/Makefile.common b/tools/libs/ctrl/Makefile.common
index 0a09c28fd3..247afbe5f9 100644
--- a/tools/libs/ctrl/Makefile.common
+++ b/tools/libs/ctrl/Makefile.common
@@ -24,6 +24,7 @@ OBJS-y       += xc_hcall_buf.o
 OBJS-y       += xc_foreign_memory.o
 OBJS-y       += xc_kexec.o
 OBJS-y       += xc_resource.o
+OBJS-$(CONFIG_ARM)  += xc_dt_overlay.o
 OBJS-$(CONFIG_X86) += xc_psr.o
 OBJS-$(CONFIG_X86) += xc_pagetab.o
 OBJS-$(CONFIG_Linux) += xc_linux.o
diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c
new file mode 100644
index 0000000000..325c9ef4c0
--- /dev/null
+++ b/tools/libs/ctrl/xc_dt_overlay.c
@@ -0,0 +1,51 @@
+/*
+ *
+ * Device Tree Overlay functions.
+ * Copyright (C) 2021 Xilinx Inc.
+ * Author Vikram Garhwal <fnu.vikram@xilinx.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "xc_bitops.h"
+#include "xc_private.h"
+#include <xen/hvm/hvm_op.h>
+#include <libfdt.h>
+
+int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
+                  uint32_t overlay_fdt_size, uint8_t overlay_op)
+{
+    int err;
+    DECLARE_SYSCTL;
+
+    DECLARE_HYPERCALL_BOUNCE(overlay_fdt, overlay_fdt_size,
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+    if ( (err = xc_hypercall_bounce_pre(xch, overlay_fdt)) )
+        goto err;
+
+    sysctl.cmd = XEN_SYSCTL_dt_overlay;
+    sysctl.u.dt_overlay.overlay_op = overlay_op;
+    sysctl.u.dt_overlay.overlay_fdt_size = overlay_fdt_size;
+
+    set_xen_guest_handle(sysctl.u.dt_overlay.overlay_fdt, overlay_fdt);
+
+    if ( (err = do_sysctl(xch, &sysctl)) != 0 )
+        PERROR("%s failed", __func__);
+
+err:
+    xc_hypercall_bounce_post(xch, overlay_fdt);
+
+    return err;
+}
-- 
2.17.1

Re: [XEN][RFC PATCH v4 14/16] tools/libs/ctrl: Implement new xc interfaces for dt overlay

[Anthony PERARD]

On Tue, Dec 06, 2022 at 10:18:13PM -0800, Vikram Garhwal wrote:
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 0c8b4c3aa7..a71bc0bb1d 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2633,6 +2633,9 @@ int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint32
>  int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
>                           xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
>  
> +int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
> +                  uint32_t overlay_fdt_size, uint8_t overlay_op);
> +

Since xc_dt_overlay.c is only built on CONFIG_ARM=y, could you enclose
that prototype in "#if defined(__arm__) || defined(__aarch64__)"?

>  /* Compat shims */
>  #include "xenctrl_compat.h"
>  
> diff --git a/tools/libs/ctrl/Makefile.common b/tools/libs/ctrl/Makefile.common
> index 0a09c28fd3..247afbe5f9 100644
> --- a/tools/libs/ctrl/Makefile.common
> +++ b/tools/libs/ctrl/Makefile.common
> @@ -24,6 +24,7 @@ OBJS-y       += xc_hcall_buf.o
>  OBJS-y       += xc_foreign_memory.o
>  OBJS-y       += xc_kexec.o
>  OBJS-y       += xc_resource.o
> +OBJS-$(CONFIG_ARM)  += xc_dt_overlay.o
>  OBJS-$(CONFIG_X86) += xc_psr.o
>  OBJS-$(CONFIG_X86) += xc_pagetab.o
>  OBJS-$(CONFIG_Linux) += xc_linux.o
> diff --git a/tools/libs/ctrl/xc_dt_overlay.c b/tools/libs/ctrl/xc_dt_overlay.c
> new file mode 100644
> index 0000000000..325c9ef4c0
> --- /dev/null
> +++ b/tools/libs/ctrl/xc_dt_overlay.c
> +#include "xc_bitops.h"
> +#include "xc_private.h"
> +#include <xen/hvm/hvm_op.h>
> +#include <libfdt.h>

Why all these headers? "xc_private.h" seems sufficient.
There isn't any bitmap operation, so xc_bitops.h seems unused.
The device tree isn't manipulated so libfdt.h seems unused.
hvm_op.h seems to be for HVMOP, not SYSCTL hypercall, xc_private.h might
already include the right header to do SYSCTL.

Thanks,

-- 
Anthony PERARD

[XEN][RFC PATCH v4 15/16] tools/libs/light: Implement new libxl functions for device tree overlay ops

[Vikram Garhwal]

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
---
 tools/include/libxl.h               |  8 ++++
 tools/libs/light/Makefile           |  3 ++
 tools/libs/light/libxl_dt_overlay.c | 72 +++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)
 create mode 100644 tools/libs/light/libxl_dt_overlay.c

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 2321a648a5..82fc7ce6b9 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -245,6 +245,11 @@
  */
 #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
 
+/**
+ * This means Device Tree Overlay is supported.
+ */
+#define LIBXL_HAVE_DT_OVERLAY 1
+
 /*
  * libxl_domain_build_info has device_model_user to specify the user to
  * run the device model with. See docs/misc/qemu-deprivilege.txt.
@@ -2448,6 +2453,9 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
                                         int *num);
 void libxl_device_pci_list_free(libxl_device_pci* list, int num);
 
+int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
+                     uint32_t overlay_size, uint8_t overlay_op);
+
 /*
  * Turns the current process into a backend device service daemon
  * for a driver domain.
diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
index 374be1cfab..2fde58246e 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -111,6 +111,9 @@ OBJS-y += _libxl_types.o
 OBJS-y += libxl_flask.o
 OBJS-y += _libxl_types_internal.o
 
+# Device tree overlay is enabled only for ARM architecture.
+OBJS-$(CONFIG_ARM) += libxl_dt_overlay.o
+
 ifeq ($(CONFIG_LIBNL),y)
 CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
 endif
diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
new file mode 100644
index 0000000000..38cab880a0
--- /dev/null
+++ b/tools/libs/light/libxl_dt_overlay.c
@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2021 Xilinx Inc.
+ * Author Vikram Garhwal <fnu.vikram@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxl_internal.h"
+#include <libfdt.h>
+#include <xenguest.h>
+#include <xenctrl.h>
+
+static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
+{
+    int r;
+
+    if (fdt_magic(fdt) != FDT_MAGIC) {
+        LOG(ERROR, "Overlay FDT is not a valid Flat Device Tree");
+        return ERROR_FAIL;
+    }
+
+    r = fdt_check_header(fdt);
+    if (r) {
+        LOG(ERROR, "Failed to check the overlay FDT (%d)", r);
+        return ERROR_FAIL;
+    }
+
+    if (fdt_totalsize(fdt) > size) {
+        LOG(ERROR, "Overlay FDT totalsize is too big");
+        return ERROR_FAIL;
+    }
+
+    return 0;
+}
+
+int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, uint32_t overlay_dt_size,
+                     uint8_t overlay_op)
+{
+    int rc;
+    int r;
+    GC_INIT(ctx);
+
+    if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
+        LOG(ERROR, "Overlay DTB check failed");
+        rc = ERROR_FAIL;
+        goto out;
+    } else {
+        LOG(DEBUG, "Overlay DTB check passed");
+        rc = 0;
+    }
+
+    r = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
+
+    if (r) {
+        LOG(ERROR, "%s: Adding/Removing overlay dtb failed.", __func__);
+        rc = ERROR_FAIL;
+    }
+
+out:
+    GC_FREE;
+    return rc;
+}
+
-- 
2.17.1

Re: [XEN][RFC PATCH v4 15/16] tools/libs/light: Implement new libxl functions for device tree overlay ops

[Anthony PERARD]

On Tue, Dec 06, 2022 at 10:18:14PM -0800, Vikram Garhwal wrote:
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 2321a648a5..82fc7ce6b9 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -245,6 +245,11 @@
>   */
>  #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>  
> +/**
> + * This means Device Tree Overlay is supported.
> + */
> +#define LIBXL_HAVE_DT_OVERLAY 1
> +
>  /*
>   * libxl_domain_build_info has device_model_user to specify the user to
>   * run the device model with. See docs/misc/qemu-deprivilege.txt.
> @@ -2448,6 +2453,9 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
>                                          int *num);
>  void libxl_device_pci_list_free(libxl_device_pci* list, int num);
>  
> +int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
> +                     uint32_t overlay_size, uint8_t overlay_op);
> +

Could you guard both the LIBXL_HAVE_* macro and this prototype with "#if
arm"? Since the dt_overlay operation to libxl built on arm.

>  /*
>   * Turns the current process into a backend device service daemon
>   * for a driver domain.
> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> index 374be1cfab..2fde58246e 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -111,6 +111,9 @@ OBJS-y += _libxl_types.o
>  OBJS-y += libxl_flask.o
>  OBJS-y += _libxl_types_internal.o
>  
> +# Device tree overlay is enabled only for ARM architecture.
> +OBJS-$(CONFIG_ARM) += libxl_dt_overlay.o
> +
>  ifeq ($(CONFIG_LIBNL),y)
>  CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
>  endif
> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
> new file mode 100644
> index 0000000000..38cab880a0
> --- /dev/null
> +++ b/tools/libs/light/libxl_dt_overlay.c
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include "libxl_internal.h"
> +#include <libfdt.h>
> +#include <xenguest.h>
> +#include <xenctrl.h>

Don't you need just xenctrl.h and not xenguest.h? (They both already are
libxl_internal.h so I'm not sure if xenguest.h is needed., but
xc_dt_overlay() is in xenctrl.h)


Thanks,

-- 
Anthony PERARD

Re: [XEN][RFC PATCH v4 15/16] tools/libs/light: Implement new libxl functions for device tree overlay ops

[Vikram Garhwal]

Hi Anthony,

On 12/9/22 8:59 AM, Anthony PERARD wrote:
> On Tue, Dec 06, 2022 at 10:18:14PM -0800, Vikram Garhwal wrote:
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index 2321a648a5..82fc7ce6b9 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -245,6 +245,11 @@
>>    */
>>   #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>>   
>> +/**
>> + * This means Device Tree Overlay is supported.
>> + */
>> +#define LIBXL_HAVE_DT_OVERLAY 1
>> +
>>   /*
>>    * libxl_domain_build_info has device_model_user to specify the user to
>>    * run the device model with. See docs/misc/qemu-deprivilege.txt.
>> @@ -2448,6 +2453,9 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
>>                                           int *num);
>>   void libxl_device_pci_list_free(libxl_device_pci* list, int num);
>>   
>> +int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
>> +                     uint32_t overlay_size, uint8_t overlay_op);
>> +
> Could you guard both the LIBXL_HAVE_* macro and this prototype with "#if
> arm"? Since the dt_overlay operation to libxl built on arm.
Will do this in v5
>
>>   /*
>>    * Turns the current process into a backend device service daemon
>>    * for a driver domain.
>> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
>> index 374be1cfab..2fde58246e 100644
>> --- a/tools/libs/light/Makefile
>> +++ b/tools/libs/light/Makefile
>> @@ -111,6 +111,9 @@ OBJS-y += _libxl_types.o
>>   OBJS-y += libxl_flask.o
>>   OBJS-y += _libxl_types_internal.o
>>   
>> +# Device tree overlay is enabled only for ARM architecture.
>> +OBJS-$(CONFIG_ARM) += libxl_dt_overlay.o
>> +
>>   ifeq ($(CONFIG_LIBNL),y)
>>   CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
>>   endif
>> diff --git a/tools/libs/light/libxl_dt_overlay.c b/tools/libs/light/libxl_dt_overlay.c
>> new file mode 100644
>> index 0000000000..38cab880a0
>> --- /dev/null
>> +++ b/tools/libs/light/libxl_dt_overlay.c
>> +#include "libxl_osdeps.h" /* must come before any other headers */
>> +#include "libxl_internal.h"
>> +#include <libfdt.h>
>> +#include <xenguest.h>
>> +#include <xenctrl.h>
> Don't you need just xenctrl.h and not xenguest.h? (They both already are
> libxl_internal.h so I'm not sure if xenguest.h is needed., but
> xc_dt_overlay() is in xenctrl.h)
Thanks for spotting this. I will remove xenguest.h
>
> Thanks,
>

[XEN][RFC PATCH v4 16/16] tools/xl: Add new xl command overlay for device tree overlay support

[Vikram Garhwal]

Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
 tools/xl/xl.h           |  1 +
 tools/xl/xl_cmdtable.c  |  6 ++++++
 tools/xl/xl_vmcontrol.c | 48 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 7c9aff6ad7..6e95dfe6ea 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -138,6 +138,7 @@ int main_shutdown(int argc, char **argv);
 int main_reboot(int argc, char **argv);
 int main_list(int argc, char **argv);
 int main_vm_list(int argc, char **argv);
+int main_dt_overlay(int argc, char **argv);
 int main_create(int argc, char **argv);
 int main_config_update(int argc, char **argv);
 int main_button_press(int argc, char **argv);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 35182ca196..868364c58d 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -630,6 +630,12 @@ const struct cmd_spec cmd_table[] = {
       "Issue a qemu monitor command to the device model of a domain",
       "<Domain> <Command>",
     },
+    { "dt_overlay",
+      &main_dt_overlay, 0, 1,
+      "Add/Remove a device tree overlay",
+      "add/remove <.dtbo>"
+      "-h print this help\n"
+    },
 };
 
 const int cmdtable_len = ARRAY_SIZE(cmd_table);
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 5518c78dc6..b5f04e2741 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1265,6 +1265,54 @@ int main_create(int argc, char **argv)
     return 0;
 }
 
+int main_dt_overlay(int argc, char **argv)
+{
+    const char *overlay_ops = NULL;
+    const char *overlay_config_file = NULL;
+    void *overlay_dtb = NULL;
+    int rc;
+    uint8_t op;
+    int overlay_dtb_size = 0;
+    const int overlay_add_op = 1;
+    const int overlay_remove_op = 2;
+
+    if (argc < 2) {
+        help("dt_overlay");
+        return EXIT_FAILURE;
+    }
+
+    overlay_ops = argv[1];
+    overlay_config_file = argv[2];
+
+    if (strcmp(overlay_ops, "add") == 0)
+        op = overlay_add_op;
+    else if (strcmp(overlay_ops, "remove") == 0)
+        op = overlay_remove_op;
+    else {
+        fprintf(stderr, "Invalid dt overlay operation\n");
+        return ERROR_FAIL;
+    }
+
+    if (overlay_config_file) {
+        rc = libxl_read_file_contents(ctx, overlay_config_file,
+                                      &overlay_dtb, &overlay_dtb_size);
+
+        if (rc) {
+            fprintf(stderr, "failed to read the overlay device tree file %s\n",
+                    overlay_config_file);
+            free(overlay_dtb);
+            return ERROR_FAIL;
+        }
+    } else {
+        fprintf(stderr, "overlay dtbo file not provided\n");
+        return ERROR_FAIL;
+    }
+
+    rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
+
+    free(overlay_dtb);
+    return rc;
+}
 /*
  * Local variables:
  * mode: C
-- 
2.17.1

Re: [XEN][RFC PATCH v4 16/16] tools/xl: Add new xl command overlay for device tree overlay support

[Anthony PERARD]

On Tue, Dec 06, 2022 at 10:18:15PM -0800, Vikram Garhwal wrote:
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 35182ca196..868364c58d 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -630,6 +630,12 @@ const struct cmd_spec cmd_table[] = {
>        "Issue a qemu monitor command to the device model of a domain",
>        "<Domain> <Command>",
>      },
> +    { "dt_overlay",

Command with multiple words are using '-' instead of '_', could you
rename the command to "dt-overlay"?

> +      &main_dt_overlay, 0, 1,
> +      "Add/Remove a device tree overlay",
> +      "add/remove <.dtbo>"
> +      "-h print this help\n"
> +    },
>  };
>  
>  const int cmdtable_len = ARRAY_SIZE(cmd_table);
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 5518c78dc6..b5f04e2741 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1265,6 +1265,54 @@ int main_create(int argc, char **argv)
>      return 0;
>  }
>  
> +int main_dt_overlay(int argc, char **argv)
> +{
> +    const char *overlay_ops = NULL;
> +    const char *overlay_config_file = NULL;
> +    void *overlay_dtb = NULL;
> +    int rc;
> +    uint8_t op;
> +    int overlay_dtb_size = 0;
> +    const int overlay_add_op = 1;
> +    const int overlay_remove_op = 2;
> +
> +    if (argc < 2) {
> +        help("dt_overlay");
> +        return EXIT_FAILURE;
> +    }
> +
> +    overlay_ops = argv[1];
> +    overlay_config_file = argv[2];
> +
> +    if (strcmp(overlay_ops, "add") == 0)
> +        op = overlay_add_op;
> +    else if (strcmp(overlay_ops, "remove") == 0)
> +        op = overlay_remove_op;
> +    else {
> +        fprintf(stderr, "Invalid dt overlay operation\n");
> +        return ERROR_FAIL;

ERROR_FAIL isn't really a value to be used when exiting the programme,
it's value is -3. It's from libxl API.

Instead, could you return EXIT_FAILURE?

> +    }
> +
> +    if (overlay_config_file) {
> +        rc = libxl_read_file_contents(ctx, overlay_config_file,
> +                                      &overlay_dtb, &overlay_dtb_size);
> +
> +        if (rc) {
> +            fprintf(stderr, "failed to read the overlay device tree file %s\n",
> +                    overlay_config_file);
> +            free(overlay_dtb);
> +            return ERROR_FAIL;
> +        }
> +    } else {
> +        fprintf(stderr, "overlay dtbo file not provided\n");
> +        return ERROR_FAIL;
> +    }
> +
> +    rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);

Value returned by libxl_*() are going to be negative when there's an
error, so not something to be return by main(). Could you check if
there's an error and return EXIT_FAILURE instead?

> +    free(overlay_dtb);
> +    return rc;
> +}

Thanks,

-- 
Anthony PERARD

Re: [XEN][RFC PATCH v4 16/16] tools/xl: Add new xl command overlay for device tree overlay support

[Vikram Garhwal]

Hi Anthony,

On 12/9/22 9:55 AM, Anthony PERARD wrote:
> On Tue, Dec 06, 2022 at 10:18:15PM -0800, Vikram Garhwal wrote:
>> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
>> index 35182ca196..868364c58d 100644
>> --- a/tools/xl/xl_cmdtable.c
>> +++ b/tools/xl/xl_cmdtable.c
>> @@ -630,6 +630,12 @@ const struct cmd_spec cmd_table[] = {
>>         "Issue a qemu monitor command to the device model of a domain",
>>         "<Domain> <Command>",
>>       },
>> +    { "dt_overlay",
> Command with multiple words are using '-' instead of '_', could you
> rename the command to "dt-overlay"?
understood.
>
>> +      &main_dt_overlay, 0, 1,
>> +      "Add/Remove a device tree overlay",
>> +      "add/remove <.dtbo>"
>> +      "-h print this help\n"
>> +    },
>>   };
>>   
>>   const int cmdtable_len = ARRAY_SIZE(cmd_table);
>> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
>> index 5518c78dc6..b5f04e2741 100644
>> --- a/tools/xl/xl_vmcontrol.c
>> +++ b/tools/xl/xl_vmcontrol.c
>> @@ -1265,6 +1265,54 @@ int main_create(int argc, char **argv)
>>       return 0;
>>   }
>>   
>> +int main_dt_overlay(int argc, char **argv)
>> +{
>> +    const char *overlay_ops = NULL;
>> +    const char *overlay_config_file = NULL;
>> +    void *overlay_dtb = NULL;
>> +    int rc;
>> +    uint8_t op;
>> +    int overlay_dtb_size = 0;
>> +    const int overlay_add_op = 1;
>> +    const int overlay_remove_op = 2;
>> +
>> +    if (argc < 2) {
>> +        help("dt_overlay");
>> +        return EXIT_FAILURE;
>> +    }
>> +
>> +    overlay_ops = argv[1];
>> +    overlay_config_file = argv[2];
>> +
>> +    if (strcmp(overlay_ops, "add") == 0)
>> +        op = overlay_add_op;
>> +    else if (strcmp(overlay_ops, "remove") == 0)
>> +        op = overlay_remove_op;
>> +    else {
>> +        fprintf(stderr, "Invalid dt overlay operation\n");
>> +        return ERROR_FAIL;
> ERROR_FAIL isn't really a value to be used when exiting the programme,
> it's value is -3. It's from libxl API.
>
> Instead, could you return EXIT_FAILURE?
Will address this in next version.
>> +    }
>> +
>> +    if (overlay_config_file) {
>> +        rc = libxl_read_file_contents(ctx, overlay_config_file,
>> +                                      &overlay_dtb, &overlay_dtb_size);
>> +
>> +        if (rc) {
>> +            fprintf(stderr, "failed to read the overlay device tree file %s\n",
>> +                    overlay_config_file);
>> +            free(overlay_dtb);
>> +            return ERROR_FAIL;
>> +        }
>> +    } else {
>> +        fprintf(stderr, "overlay dtbo file not provided\n");
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
> Value returned by libxl_*() are going to be negative when there's an
> error, so not something to be return by main(). Could you check if
> there's an error and return EXIT_FAILURE instead?
Okay.
>> +    free(overlay_dtb);
>> +    return rc;
>> +}
> Thanks,
>

Re: [XEN][RFC PATCH v4 07/16] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Michal Orzel]

Hi Vikram,

On 07/12/2022 07:18, Vikram Garhwal wrote:
> 
> 
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_lock().
s/lock/locked/

> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  xen/drivers/passthrough/device_tree.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 1c32d7b50c..bb4cf7784d 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -83,16 +83,15 @@ fail:
>      return rc;
>  }
> 
> -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> +static bool_t
> +    iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev)
This should not be indented
>  {
>      bool_t assigned = 0;
> 
>      if ( !dt_device_is_protected(dev) )
>          return 0;
> 
> -    spin_lock(&dtdevs_lock);
>      assigned = !list_empty(&dev->domain_list);
> -    spin_unlock(&dtdevs_lock);
> 
>      return assigned;
>  }
> @@ -213,27 +212,43 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>          if ( (d && d->is_dying) || domctl->u.assign_device.flags )
>              break;
> 
> +        spin_lock(&dtdevs_lock);
> +
>          ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>                                      domctl->u.assign_device.u.dt.size,
>                                      &dev);
>          if ( ret )
> +        {
> +            spin_unlock(&dtdevs_lock);
> +
I think removing a blank line here and in other places would look better.

~Michal