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

[Henry Wang <xin.wang2@xxxxxxx>]

Hi Jan,

As usual, thanks for the review!

On 5/16/2024 8:31 PM, Jan Beulich wrote:
> On 16.05.2024 12:03, Henry Wang wrote:
>
> +    /*
> +     * 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.
I think you are correct, it is a copy paste of the existing code and the 
track variable is indeed useless. So in v3, I will simply drop it and 
mention this clean-up in commit message. Also I realized that the exact 
logic of finding the entry is duplicated third times, so I will also 
extract the logic to a function.
> > --- 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).
Good catch. Will fix it.

> > +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.
Well although I agree with you it is indeed a bit odd, the problem of 
this is that, in current implementation I reused the libxl_dt_overlay() 
(with proper backward compatible) to deliver the sysctl and domctl 
depend on the op, and we have:
#define LIBXL_DT_OVERLAY_ADD                   1
#define LIBXL_DT_OVERLAY_REMOVE                2
#define LIBXL_DT_OVERLAY_ATTACH                3
#define LIBXL_DT_OVERLAY_DETACH                4

Then the op-number is passed from the toolstack to Xen, and checked in 
dt_overlay_domctl(). So with this implementation the attach/detach op 
number should be 3 and 4 since 1 and 2 have different meanings.
But I realized that I can also implement a similar API, say 
libxl_dt_overlay_domain() and that way we can reuse 1 and 2 and there is 
not even need to provide backward compatible of libxl_dt_overlay(). So 
would you mind sharing your preference on which approach would you like 
more? Thanks!
> > --- 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.
Oh indeed. Will fix this. Thanks!

Kind regards,
Henry

> Jan