Re: [Xen-devel] [PATCH v5 8/8] xen: add runtime parameter access support to hypfs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.02.2020 09:22, Jürgen Groß wrote:
> On 19.02.20 17:44, Jan Beulich wrote:
>> On 19.02.2020 09:11, Juergen Gross wrote:
>>> --- a/xen/arch/arm/xen.lds.S
>>> +++ b/xen/arch/arm/xen.lds.S
>>> @@ -89,6 +89,11 @@ SECTIONS
>>>          __start_schedulers_array = .;
>>>          *(.data.schedulers)
>>>          __end_schedulers_array = .;
>>> +
>>> +       . = ALIGN(8);
>>> +       __paramhypfs_start = .;
>>> +       *(.data.paramhypfs)
>>> +       __paramhypfs_end = .;
>>
>> Do you really need 8-byte alignment even on 32-bit Arm?
> 
> I just followed the general pattern in this file.

Well, it'll be the Arm maintainers to judge anyway. It merely
caught my eye.

>>> @@ -99,28 +101,33 @@ static int parse_gnttab_limit(const char *param, const 
>>> char *arg,
>>>           return -ERANGE;
>>>   
>>>       *valp = val;
>>> +    snprintf(par_val, PAR_VAL_SZ, "%lu", val);
>>>   
>>>       return 0;
>>>   }
>>>   
>>>   unsigned int __read_mostly opt_max_grant_frames = 64;
>>> +static char gnttab_max_frames_val[PAR_VAL_SZ] = "64";
>>
>> This and the other option are plain integer ones from a presentation
>> pov, so it would be nice to get away here without the extra buffers.
> 
> There is more than pure integer semantics here: the value is limited,
> so I can't just use the normal integer assignment.

Which is why I've said "from a presentation pov", i.e. when consuming
the value for passing back as a string.

> Its not as if those functions are performance critical, and special
> casing for sparing the buffer memory will probably waste more memory
> due to larger code size.

Well, it's not so much the memory savings, but the reduction of
redundant information being kept. But if it's not sufficiently simple
to generalize, so be it then.

>>> @@ -23,10 +24,17 @@ struct kernel_param {
>>>       } par;
>>>   };
>>>   
>>> +struct param_hypfs {
>>> +    const struct kernel_param *param;
>>
>> As long as this is here, I don't think ...
>>
>>> @@ -76,40 +84,87 @@ extern const struct kernel_param __param_start[], 
>>> __param_end[];
>>>             .type = OPT_IGNORE }
>>>   
>>>   #define __rtparam         __param(__dataparam)
>>> +#define __paramfs         static __paramhypfs \
>>> +    __attribute__((__aligned__(sizeof(void *)))) struct param_hypfs
>>
>> ... you need the alignment attribute here. But see below.
>>
>>> -#define custom_runtime_only_param(_name, _var) \
>>> +#define custom_runtime_only_param(_name, _var, contvar) \
>>>       __rtparam __rtpar_##_var = \
>>>         { .name = _name, \
>>>             .type = OPT_CUSTOM, \
>>> -          .par.func = _var }
>>> +          .par.func = _var }; \
>>> +    __paramfs __parfs_##_var = \
>>> +        { .param = &__rtpar_##_var, \
>>
>> Instead of a pointer, can't the param struct be part of this
>> bigger structure?
> 
> I have some further patches in my pipeline which will do even more,
> by removing the sysctl for setting parameters and using hypfs for
> that purpose in libxl. This will remove the need for the runtime
> copy of struct kernel_param completely.

Ah, good to know. While reviewing this I was wondering about exactly
this.

> But r

???

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel