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
