Re: [PATCH v2 5/6] xen/arm: exclude xen,reg from domU extended regions

[Stewart Hildebrand <stewart.hildebrand@amd.com>]

On 5/9/25 02:54, Orzel, Michal wrote:
> On 08/05/2025 15:20, Stewart Hildebrand wrote:
>> diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
>> index 7a6cd67c22f1..1939c3ebf7dc 100644
>> --- a/xen/include/xen/fdt-kernel.h
>> +++ b/xen/include/xen/fdt-kernel.h
>> @@ -24,6 +24,7 @@ struct kernel_info {
>>  #ifdef CONFIG_STATIC_SHM
>>      struct shared_meminfo shm_mem;
>>  #endif
>> +    struct rangeset *xen_reg_assigned;
> The purpose of your newly introduced xen_reg_assigned is to keep track of these
> ranges so that we can remove them from extended regions. The concept of extended
> regions exists only for Arm today. Therefore I'm not sure why making all these
> common i.e. entry in struct, rangeset allocation, etc. The other aspect is that
> extended regions may be disabled by the user and you would still allocate
> rangeset and add xen,reg to it for no purpose - i.e. dead code.

How about an arch hook? E.g. see work-in-progress/untested patch at the
end.

> Also, what about direct-mapped domUs? We don't seem to take xen,reg into account
> there.

Right, we ought to take xen,reg into account for direct-map domUs too.
This is because, even though the domU is direct-mapped, xen,reg can
still set up a translated mapping (gfn != mfn). Also, xen,reg doesn't
need to correspond to a real device, it can be any arbitrary mapping.
I'll send a patch.

> P.S.
> After recent dom0less code movement there are some issues that I reported to
> Oleksii. Long story short, we shouldn't be making the code common (e.g. static
> mem, shmem, domain type) that is implemented for now only for one arch. If the
> need arises in the future, the feature code together with callers can be moved
> to common. At the moment, we have some features being in arch specific
> directories but callers in common code and #ifdef-ed (making the stubs
> unreachable). That's not great.
> 
> ~Michal



diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b189a7cfae9f..f099e27d846c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -929,6 +929,31 @@ out:
     return res;
 }
 
+int __init arch_add_xen_reg(struct kernel_info *kinfo, paddr_t gstart,
+                            paddr_t size)
+{
+    if ( !opt_ext_regions )
+        return 0;
+
+    if ( !kinfo->arch.xen_reg_assigned )
+    {
+        kinfo->arch.xen_reg_assigned = rangeset_new(NULL, NULL, 0);
+
+        if ( !kinfo->arch.xen_reg_assigned )
+            return -ENOMEM;
+    }
+
+    return rangeset_add_range(kinfo->arch.xen_reg_assigned, PFN_DOWN(gstart),
+                              PFN_DOWN(gstart + size - 1));
+}
+
+int __init arch_cleanup(struct kernel_info *kinfo)
+{
+    rangeset_destroy(kinfo->arch.xen_reg_assigned);
+
+    return 0;
+}
+
 static int __init find_domU_holes(const struct kernel_info *kinfo,
                                   struct membanks *ext_regions)
 {
@@ -973,9 +998,9 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
     if ( res )
         goto out;
 
-    if ( kinfo->xen_reg_assigned )
+    if ( kinfo->arch.xen_reg_assigned )
     {
-        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
+        res = rangeset_subtract(mem_holes, kinfo->arch.xen_reg_assigned);
         if ( res )
             goto out;
     }
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 7c3b7fde5b64..8d6bd2dd77f9 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -16,6 +16,8 @@ struct arch_kernel_info
 
     /* Enable pl011 emulation */
     bool vpl011;
+
+    struct rangeset *xen_reg_assigned;
 };
 
 #endif /* #ifdef __ARCH_ARM_KERNEL_H__ */
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index 2c56f13771ab..654575612744 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -146,14 +146,6 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
     int res;
     paddr_t mstart, size, gstart;
 
-    if ( !kinfo->xen_reg_assigned )
-    {
-        kinfo->xen_reg_assigned = rangeset_new(NULL, NULL, 0);
-
-        if ( !kinfo->xen_reg_assigned )
-            return -ENOMEM;
-    }
-
     /* xen,reg specifies where to map the MMIO region */
     cell = (const __be32 *)xen_reg->data;
     len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
@@ -196,8 +188,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
             return -EFAULT;
         }
 
-        res = rangeset_add_range(kinfo->xen_reg_assigned, PFN_DOWN(gstart),
-                                 PFN_DOWN(gstart + size - 1));
+        res = arch_add_xen_reg(kinfo, gstart, size);
         if ( res )
             return res;
     }
@@ -828,10 +819,10 @@ static int __init construct_domU(struct domain *d,
     domain_vcpu_affinity(d, node);
 
     rc = alloc_xenstore_params(&kinfo);
+    if ( rc < 0 )
+        return rc;
 
-    rangeset_destroy(kinfo.xen_reg_assigned);
-
-    return rc;
+    return arch_cleanup(&kinfo);
 }
 
 void __init create_domUs(void)
diff --git a/xen/include/asm-generic/dom0less-build.h b/xen/include/asm-generic/dom0less-build.h
index e0ad0429ec74..3e577e4dbe10 100644
--- a/xen/include/asm-generic/dom0less-build.h
+++ b/xen/include/asm-generic/dom0less-build.h
@@ -61,6 +61,10 @@ void set_domain_type(struct domain *d, struct kernel_info *kinfo);
 int init_intc_phandle(struct kernel_info *kinfo, const char *name,
                       const int node_next, const void *pfdt);
 
+int arch_add_xen_reg(struct kernel_info *kinfo, paddr_t gstart, paddr_t size);
+
+int arch_cleanup(struct kernel_info *kinfo);
+
 #else /* !CONFIG_DOM0LESS_BOOT */
 
 static inline void create_domUs(void) {}
diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
index 1939c3ebf7dc..7a6cd67c22f1 100644
--- a/xen/include/xen/fdt-kernel.h
+++ b/xen/include/xen/fdt-kernel.h
@@ -24,7 +24,6 @@ struct kernel_info {
 #ifdef CONFIG_STATIC_SHM
     struct shared_meminfo shm_mem;
 #endif
-    struct rangeset *xen_reg_assigned;
 
     /* kernel entry point */
     paddr_t entry;