Re: [PATCH] livepatch: do not use .livepatch.funcs section to store internal state

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 16, 2023 at 03:05:20PM +0100, Jan Beulich wrote:
> On 16.11.2023 14:54, Roger Pau Monné wrote:
> > On Thu, Nov 16, 2023 at 01:39:27PM +0100, Jan Beulich wrote:
> >> On 16.11.2023 12:58, Roger Pau Monne wrote:
> >>> --- a/xen/include/public/sysctl.h
> >>> +++ b/xen/include/public/sysctl.h
> >>> @@ -991,10 +991,7 @@ struct livepatch_func {
> >>>      uint32_t new_size;
> >>>      uint32_t old_size;
> >>>      uint8_t version;        /* MUST be LIVEPATCH_PAYLOAD_VERSION. */
> >>> -    uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
> >>> -    uint8_t applied;
> >>> -    uint8_t patch_offset;
> >>> -    uint8_t _pad[6];
> >>> +    uint8_t _pad[39];
> >>
> >> Should this be LIVEPATCH_OPAQUE_SIZE+8 and ...
> > 
> > Hm, I'm not sure that's any clearer.  In fact I think
> > LIVEPATCH_OPAQUE_SIZE shouldn't have leaked into sysctl.h in the first
> > place.
> > 
> > If we later want to add more fields and bump the version, isn't it
> > easier to have the padding size clearly specified as a number?
> 
> If new fields (beyond the present padding size) would need adding,
> that would constitute an incompatible change anyway.

Not if we bump the version field I think?

As the version is a strict match, bumping the version allows for a
completely new layout to be implemented past the 'version' field.

> Until then imo
> it would be clearer to keep the reference to the original constant.
> But thinking about it again, the difference is perhaps indeed only
> marginal. Anyway, I'll leave this to the livepatch maintainers.
> 
> One further related remark though: Now that you pointed me at the
> other use of the constant in the public header, don't you want to
> update the comment there, for it to not become stale (in referring
> to struct livepatch_func)?

Hm, yes, indeed.  I will wait for further comments before sending just
that comment fix.  I would add:

uint8_t data[LIVEPATCH_OPAQUE_SIZE]; /* Max number of bytes to be patched */

Thanks, Roger.