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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.11.2021 08:02, Vikram Garhwal wrote:
> Introduce sysctl XEN_SYSCTL_overlay to remove device-tree nodes added using
> device tree overlay.

XEN_SYSCTL_overlay is too generic a name imo.

> @@ -476,6 +781,73 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>              copyback = 1;
>          break;
>  
> +#if defined (CONFIG_OVERLAY_DTB)
> +    case XEN_SYSCTL_overlay:
> +    {
> +        void *overlay_fdt;
> +        char **node_full_path = NULL;
> +        int num_overlay_nodes;
> +
> +        if ( op->u.overlay_dt.overlay_fdt_size > 0 )
> +            overlay_fdt = xmalloc_bytes(op->u.overlay_dt.overlay_fdt_size);
> +        else
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        if ( overlay_fdt == NULL )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }
> +
> +        ret = copy_from_guest(overlay_fdt, op->u.overlay_dt.overlay_fdt,
> +                             op->u.overlay_dt.overlay_fdt_size);
> +        if ( ret )
> +        {
> +            gprintk(XENLOG_ERR, "copy from guest failed\n");
> +            xfree(overlay_fdt);
> +
> +            ret = -EFAULT;
> +            break;
> +        }
> +
> +        if ( op->u.overlay_dt.overlay_op == XEN_SYSCTL_DT_OVERLAY_ADD )
> +        {
> +            ret = handle_add_overlay_nodes(overlay_fdt,
> +                                           
> op->u.overlay_dt.overlay_fdt_size);
> +        } else if ( op->u.overlay_dt.overlay_op ==
> +                                        XEN_SYSCTL_DT_OVERLAY_REMOVE )
> +        {
> +            ret = check_overlay_fdt(overlay_fdt,
> +                                    op->u.overlay_dt.overlay_fdt_size);
> +            if ( ret )
> +            {
> +                ret = -EFAULT;
> +                break;
> +            }
> +
> +            num_overlay_nodes = overlay_node_count(overlay_fdt);
> +            if ( num_overlay_nodes == 0 )
> +            {
> +                ret = -ENOMEM;
> +                break;
> +            }
> +
> +            overlay_get_node_info(overlay_fdt, &node_full_path,
> +                                  num_overlay_nodes);
> +
> +            ret = handle_remove_overlay_nodes(node_full_path,
> +                                              num_overlay_nodes);
> +        }
> +
> +        xfree(node_full_path);
> +
> +        break;
> +    }
> +#endif
> +
>      default:
>          ret = arch_do_sysctl(op, u_sysctl);

Seeing this call is even in (patch) context - would you mind clarifying
why you don't add the new code to arch_do_sysctl() (perhaps by way of
further forwarding to a new dt_sysctl(), which could then live in a DT-
specific source file)?

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1065,6 +1065,25 @@ typedef struct xen_sysctl_cpu_policy 
> xen_sysctl_cpu_policy_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
>  #endif
>  
> +#if defined (CONFIG_OVERLAY_DTB)
> +#define XEN_SYSCTL_DT_OVERLAY_ADD                   1
> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE                2
> +
> +/*
> + * XEN_SYSCTL_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_overlay_dt {
> +    XEN_GUEST_HANDLE_64(void) overlay_fdt;
> +    uint32_t overlay_fdt_size;  /* Overlay dtb size. */
> +    uint8_t overlay_op; /* Add or remove. */
> +};

Please make padding explicit and check that it's zero on input (so
it can later be assigned a purpose without needing to bump the
sysctl interface version).

> @@ -1125,6 +1145,9 @@ struct xen_sysctl {
>  #if defined(__i386__) || defined(__x86_64__)
>          struct xen_sysctl_cpu_policy        cpu_policy;
>  #endif
> +#if defined (CONFIG_OVERLAY_DTB)
> +        struct xen_sysctl_overlay_dt       overlay_dt;
> +#endif

No CONFIG_* dependencies in public headers, please.

Jan