Re: [XEN][PATCH v10 15/20] arm/asm/setup.h: Update struct map_range_data to add rangeset.

[Michal Orzel <michal.orzel@xxxxxxx>]

On 25/08/2023 10:02, Vikram Garhwal wrote:
> Add rangesets for IRQs and IOMEMs. This was done to accommodate dynamic 
> overlay
> node addition/removal operations. With overlay operations, new IRQs and IOMEMs
> are added in dt_host and routed. While removing overlay nodes, nodes are 
> remove
s/remove/removed/

> from dt_host and their IRQs and IOMEMs routing is also removed. Storing IRQs 
> and
> IOMEMs in the rangeset will avoid re-parsing the device tree nodes to get the
> IOMEM and IRQ ranges for overlay remove ops.
> 
> Dynamic overlay node add/remove will be introduced in follow-up patches.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> ---
>  xen/arch/arm/device.c            | 43 +++++++++++++++++++++++++-------
>  xen/arch/arm/domain_build.c      |  4 +--
>  xen/arch/arm/include/asm/setup.h |  5 +++-
>  3 files changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 857f171a27..9df37abac8 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -165,6 +165,14 @@ int map_range_to_domain(const struct dt_device_node *dev,
>      dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
>                 addr, addr + len, mr_data->p2mt);
>  
> +    if ( mr_data->iomem_ranges )
> +    {
> +        res = rangeset_add_range(mr_data->iomem_ranges, paddr_to_pfn(addr),
> +                                 paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
paddr_to_pfn_aligned()?

> +        if ( res )
> +            return res;
> +    }
> +
>      return 0;
>  }
>  
> @@ -178,9 +186,10 @@ int map_range_to_domain(const struct dt_device_node *dev,
>   */
>  int handle_device_interrupts(struct domain *d,
This needs to be renamed as stated in one of the first patches.

>                               struct dt_device_node *dev,
> -                             bool need_mapping)
> +                             bool need_mapping,
> +                             struct rangeset *irq_ranges)
>  {
> -    unsigned int i, nirq;
> +    unsigned int i, nirq, irq;
>      int res;
>      struct dt_raw_irq rirq;
>  
> @@ -208,17 +217,24 @@ int handle_device_interrupts(struct domain *d,
>              continue;
>          }
>  
> -        res = platform_get_irq(dev, i);
> -        if ( res < 0 )
> +        irq = platform_get_irq(dev, i);
platform_get_irq() can return negative and you even have a check for < 0 below.
So you cannot assign it to unsigned.

> +        if ( irq < 0 )
>          {
>              printk(XENLOG_ERR "Unable to get irq %u for %s\n",
>                     i, dt_node_full_name(dev));
> -            return res;
> +            return irq;
>          }
>  
> -        res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> +        res = map_irq_to_domain(d, irq, need_mapping, dt_node_name(dev));
>          if ( res )
>              return res;
> +
> +        if ( irq_ranges )
> +        {
> +            res = rangeset_add_singleton(irq_ranges, irq);
> +            if ( res )
> +                return res;
> +        }
>      }
>  
>      return 0;
> @@ -249,6 +265,11 @@ static int map_dt_irq_to_domain(const struct 
> dt_device_node *dev,
>      }
>  
>      res = map_irq_to_domain(d, irq, !mr_data->skip_mapping, 
> dt_node_name(dev));
> +    if ( res )
> +        return res;
> +
> +    if ( mr_data->irq_ranges )
> +        res = rangeset_add_singleton(mr_data->irq_ranges, irq);
>  
>      return res;
>  }
> @@ -289,7 +310,8 @@ static int map_device_children(const struct 
> dt_device_node *dev,
>   *  - Assign the device to the guest if it's protected by an IOMMU
>   *  - Map the IRQs and iomem regions to DOM0
>   */
> -int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t 
> p2mt)
> +int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t 
> p2mt,
> +                  struct rangeset *iomem_ranges, struct rangeset *irq_ranges)
You modified a function but the corresponding prototype change is missing.

>  {
>      unsigned int naddr;
>      unsigned int i;
> @@ -304,10 +326,13 @@ int handle_device(struct domain *d, struct 
> dt_device_node *dev, p2m_type_t p2mt)
>       *     pci_host_bridge_mappings().
>       */
>      struct map_range_data mr_data = {
> +        .d = d,
There's something wrong. I can see that you removed this line in patch no. 3. 
Why?
I don't see any reason so please fix it (otherwise code state from patch 3 to 
15 is broken since nothing else sets mr_data.d).
When moving the code (which you should just copy/paste), do not perform any 
modification (unless mentioned otherwise).
Also, each patch in a series (except rare occasions) should not cause any 
build/runtime failure.

~Michal