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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 12/05/2025 21:55, Stewart Hildebrand wrote:
> 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.
Still, this is only needed for extended regions and your solution a) does not
mention this fact at all and b) assumes that other arches (let's focus on RISCV
for now) have a plan to use it in the future. If b) is true (I'm not sure
because Oleksii did not move this code to common), then making the hooks global
while extended regions creation logic still being under /arm does not seem
beneficial.

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

~Michal