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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.05.2024 03:36, Henry Wang wrote:
> On 5/16/2024 8:31 PM, Jan Beulich wrote:
>> On 16.05.2024 12:03, Henry Wang wrote:
>>> --- 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!

While I think tying together libxl and domctl values isn't a very good idea,
if you really want to do so, you'll want to add suitable checking somewhere,
alongside comments. The comments ought to keep people from changing the
values then, while the checks would need to be there for people not paying
attention.

Jan