Re: [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.05.2024 12:03, Henry Wang wrote:
> +static long handle_detach_overlay_nodes(struct domain *d,
> +                                        const void *overlay_fdt,
> +                                        uint32_t overlay_fdt_size)
> +{
> +    int rc;
> +    unsigned int j;
> +    struct overlay_track *entry, *temp, *track;
> +    bool found_entry = false;
> +
> +    rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> +    if ( rc )
> +        return rc;
> +
> +    spin_lock(&overlay_lock);
> +
> +    /*
> +     * First check if dtbo is correct i.e. it should one of the dtbo which 
> was
> +     * used when dynamically adding the node.
> +     * Limitation: Cases with same node names but different property are not
> +     * supported currently. We are relying on user to provide the same dtbo
> +     * as it was used when adding the nodes.
> +     */
> +    list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
> +    {
> +        if ( memcmp(entry->overlay_fdt, overlay_fdt, overlay_fdt_size) == 0 )
> +        {
> +            track = entry;

Random question (not doing a full review of the DT code): What use is
this (and the track variable itself)? It's never used further down afaics.
Same for attach.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1190,6 +1190,17 @@ struct xen_domctl_vmtrace_op {
>  typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
>  
> +#if defined(__arm__) || defined (__aarch64__)

Nit: Consistent use of blanks please (also again below).

> +struct xen_domctl_dt_overlay {
> +    XEN_GUEST_HANDLE_64(const_void) overlay_fdt;  /* IN: overlay fdt. */
> +    uint32_t overlay_fdt_size;              /* IN: Overlay dtb size. */
> +#define XEN_DOMCTL_DT_OVERLAY_ATTACH                3
> +#define XEN_DOMCTL_DT_OVERLAY_DETACH                4

While the numbers don't really matter much, picking 3 and 4 rather than,
say, 1 and 2 still looks a little odd.

> --- a/xen/include/xen/dt-overlay.h
> +++ b/xen/include/xen/dt-overlay.h
> @@ -14,6 +14,7 @@
>  #include <xen/device_tree.h>
>  #include <xen/list.h>
>  #include <xen/rangeset.h>
> +#include <public/domctl.h>

Why? All you need here ...

> @@ -42,12 +43,18 @@ struct xen_sysctl_dt_overlay;
>  
>  #ifdef CONFIG_OVERLAY_DTB
>  long dt_overlay_sysctl(struct xen_sysctl_dt_overlay *op);
> +long dt_overlay_domctl(struct domain *d, struct xen_domctl_dt_overlay *op);

... is a forward declaration of struct xen_domctl_dt_overlay.

Jan