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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.)

> If you agree, I can send a patch to unbreak CI unless you want to do this.

Please do.

Thanks, Jan