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 10:06, Michal Orzel wrote:
> 
> 
> 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?

I'd key that to some suitable sysctl definition from the public header. If
need be, the sub-op #define itself could be made conditional and then be
used for that purpose.

> Is it ok in that case or better just focus > on the fix.

Personally I'd consider dealing with just the breakage sufficient for the
moment. The libxc part then of course still wants dealing with later on,
preferably in time for 4.18.

Jan