Re: [XEN][PATCH v12 20/20] tools/xl: Add new xl command overlay for device tree overlay support

[Michal Orzel <michal.orzel@xxxxxxx>]

On 06/09/2023 09:57, Jan Beulich wrote:
> 
> 
> On 06.09.2023 09:32, Michal Orzel wrote:
>>
>>
>> On 06/09/2023 08:55, Jan Beulich wrote:
>>>
>>>
>>> On 06.09.2023 03:16, Vikram Garhwal wrote:
>>>> --- a/tools/xl/xl_vmcontrol.c
>>>> +++ b/tools/xl/xl_vmcontrol.c
>>>> @@ -1265,6 +1265,58 @@ int main_create(int argc, char **argv)
>>>>      return 0;
>>>>  }
>>>>
>>>> +int main_dt_overlay(int argc, char **argv)
>>>> +{
>>>> +    const char *overlay_ops = NULL;
>>>> +    const char *overlay_config_file = NULL;
>>>> +    void *overlay_dtb = NULL;
>>>> +    int rc;
>>>> +    uint8_t op;
>>>> +    int overlay_dtb_size = 0;
>>>> +    const int overlay_add_op = 1;
>>>> +    const int overlay_remove_op = 2;
>>>> +
>>>> +    if (argc < 2) {
>>>> +        help("dt_overlay");
>>>> +        return EXIT_FAILURE;
>>>> +    }
>>>> +
>>>> +    overlay_ops = argv[1];
>>>> +    overlay_config_file = argv[2];
>>>> +
>>>> +    if (strcmp(overlay_ops, "add") == 0)
>>>> +        op = overlay_add_op;
>>>> +    else if (strcmp(overlay_ops, "remove") == 0)
>>>> +        op = overlay_remove_op;
>>>> +    else {
>>>> +        fprintf(stderr, "Invalid dt overlay operation\n");
>>>> +        return EXIT_FAILURE;
>>>> +    }
>>>> +
>>>> +    if (overlay_config_file) {
>>>> +        rc = libxl_read_file_contents(ctx, overlay_config_file,
>>>> +                                      &overlay_dtb, &overlay_dtb_size);
>>>> +
>>>> +        if (rc) {
>>>> +            fprintf(stderr, "failed to read the overlay device tree file 
>>>> %s\n",
>>>> +                    overlay_config_file);
>>>> +            free(overlay_dtb);
>>>> +            return ERROR_FAIL;
>>>> +        }
>>>> +    } else {
>>>> +        fprintf(stderr, "overlay dtbo file not provided\n");
>>>> +        return ERROR_FAIL;
>>>> +    }
>>>> +
>>>> +    rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
>>>
>>> Because of this being Arm-only (as validly pointed out by osstest), I expect
>>> the entire function here as well as its entry in cmd_table[] want to be
>>> Arm-specific, too? Of course it would be nice to not key this to __arm__ /
>>> __aarch64__, but to something that would not need touching again if the
>>> underlying infrastructure was made available to, say, RISC-V as well. But of
>>> course - right now the goal needs to be to address the CI and osstest
>>> breakage.
>> I agree. I would suggest to guard it with LIBXL_HAVE_DT_OVERLAY which is for 
>> now
>> only defined for arm32/arm64. This way the code will not need to be modified 
>> if other
>> arch gain support for the feature.
> 
> Ah yes, that ought to work. While there perhaps also replace the conditional
> around the declaration of the function in libxl.h. (But of course Anthony
> may tell me/us that this isn't the way to go.)
Hmm, if we change guards for libxl_dt_overlay(), what about xc_dt_overlay()
for which we cannot use LIBXL guard? Is it ok in that case or better just focus
on the fix.

~Michal