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

[Henry Wang <xin.wang2@amd.com>]

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