Re: [Xen-devel] [PATCH v3 3/6] xen/arm: assign devices to boot domains

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Stefano Stabellini writes:

> Scan the user provided dtb fragment at boot. For each device node, map
> memory to guests, and route interrupts and setup the iommu.
>
> The iommu is setup by passing the node of the device to assign on the
> host device tree. The path is specified in the device tree fragment as
> the "xen,path" string property.
>
> The memory region to remap is specified by the "xen,reg" property.
> (Perhaps it might be possible to use "range" instead of "xen,regs". This
> is something to investigate.)
>
> The interrupts are taken from the host device tree corresponding node.
> To map the interrupt call handle_interrupts, which is shared with the
> existing dom0 path.
>
> Add a interrupt-parent property automatically to the guest device tree
> when the interrupt-parent should be the GIC. Copy over the interrupt
> property from the host device tree node.
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
> Changes in v3:
> - improve commit message
> - remove superfluous cast
> - merge code with the copy code
> - add interrup-parent
> - demove depth > 2 check
> - reuse code from handle_interrupts
> - copy interrupts from host dt
>
> Changes in v2:
> - rename "path" to "xen,path"
> - grammar fix
> - use gaddr_to_gfn and maddr_to_mfn
> - remove depth <= 2 limitation in scanning the dtb fragment
> - introduce and parse xen,reg
> - code style
> - support more than one interrupt per device
> - specify only the GIC is supported
> ---
>  xen/arch/arm/domain_build.c | 66 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 70bcdc449d..0057a509d1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1712,6 +1712,9 @@ static int __init handle_properties(struct domain *d, 
> void *fdt, const void *pfd
>  {
>      int propoff, nameoff, r;
>      const struct fdt_property *prop;
> +    struct dt_device_node *node;
> +    const __be32 *cell;
> +    int i, len;
>
>      for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
>            propoff >= 0;
> @@ -1726,6 +1729,69 @@ static int __init handle_properties(struct domain *d, 
> void *fdt, const void *pfd
>                           prop->data, fdt32_to_cpu(prop->len));
>          if ( r )
>              return r;
> +
> +        if ( strcmp("xen,reg", fdt_string(pfdt, nameoff)) == 0 )
IIRC, in your other patch series you used dt_*_cmp function there.
Should it be dt_prop_cmp() in this case?

> +        {
> +            paddr_t mstart, size, gstart;
> +            cell = (const __be32 *)prop->data;
> +            len = fdt32_to_cpu(prop->len) /
> +                ((address_cells*2 + size_cells) * sizeof (u32));
Coding style: address_cells * 2. Also "sizeof(u32)".

> +
> +            for ( i = 0; i < len; i++ )
> +            {
> +                mstart = dt_next_cell(address_cells, &cell);
> +                size = dt_next_cell(size_cells, &cell);
> +                gstart = dt_next_cell(address_cells, &cell);
> +
> +                r = guest_physmap_add_entry(d, gaddr_to_gfn(gstart),
> +                        maddr_to_mfn(mstart),
> +                        get_order_from_bytes(size),
> +                        p2m_mmio_direct_dev);
> +                if ( r < 0 )
> +                {
> +                    dprintk(XENLOG_ERR,
> +                            "Failed to map %"PRIpaddr" to the guest 
> at%"PRIpaddr"\n",
> +                            mstart, gstart);
> +                    return -EFAULT;
> +                }
> +            }
> +        }
> +
> +        if ( strcmp("xen,path", fdt_string(pfdt, nameoff)) == 0 )
The same question about dt_prop_cmp.

> +        {
> +            node = dt_find_node_by_path(prop->data);
> +            if ( node != NULL )
> +                r = iommu_assign_dt_device(d, node);
> +            else
> +            {
> +                dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> +                        (char *)prop->data);
> +                return -EINVAL;
> +            }
> +
> +            r = handle_interrupts(d, node, true);
> +            if ( r < 0 )
> +                return r;
> +            if ( r > 0 )
nitpicking: how about
  if ( r == 0 )
       continue;
?
This will save one level of nesting.

Also, now I can see why handle_interrupts() returns either <0, 0 or 1.
But this was not clear in the patch #1. Additionally, need_mapping is
always true in this case. So actually you can check only for (r < 0)

> +            {
> +                unsigned int intlen;
> +                const u32* intspec;
> +
> +                /* generate interrupt-parent to point to the virtual GIC */
> +                r = fdt_property_u32(fdt, "interrupt-parent", 
> GUEST_PHANDLE_GIC);
> +                if ( r )
> +                    return r;
> +
> +                /* copy interrupts/interrupts-extended from the host DT node 
> */
I can't see "interrupts-extended" handling in the code below.

> +                intspec = dt_get_property(node, "interrupts", &intlen);
> +                if ( intspec == NULL )
> +                    return -EFAULT;
> +
> +                r = fdt_property(fdt, "interrupts", intspec, intlen);
> +                if ( r )
> +                    return r;
> +            }
> +        }
>      }
>
>      /* FDT_ERR_NOTFOUND => There is no more properties for this node */


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel