On Wed, Apr 19, 2023 at 10:43:22AM +0200, Jan Beulich wrote:
> On 19.04.2023 10:25, Roger Pau Monné wrote:
> > On Wed, Apr 19, 2023 at 08:17:45AM +0200, Jan Beulich wrote:
> >> On 18.04.2023 15:06, Roger Pau Monné wrote:
> >>> On Tue, Apr 18, 2023 at 01:00:53PM +0200, Jan Beulich wrote:
> >>>> On 18.04.2023 11:24, Roger Pau Monne wrote:
> >>>>> --- a/xen/arch/x86/include/asm/config.h
> >>>>> +++ b/xen/arch/x86/include/asm/config.h
> >>>>> @@ -44,6 +44,20 @@
> >>>>> /* Linkage for x86 */
> >>>>> #ifdef __ASSEMBLY__
> >>>>> #define ALIGN .align 16,0x90
> >>>>> +#ifdef CONFIG_LIVEPATCH
> >>>>> +#define START_LP(name) \
> >>>>> + jmp name; \
> >>>>> + .pushsection .text.name, "ax", @progbits; \
> >>>>> + name:
> >>>>> +#define END_LP(name) \
> >>>>> + .size name, . - name; \
> >>>>> + .type name, @function; \
> >>>>> + .popsection
> >>>>> +#else
> >>>>> +#define START_LP(name) \
> >>>>> + name:
> >>>>> +#define END_LP(name)
> >>>>> +#endif
> >>>>> #define ENTRY(name) \
> >>>>> .globl name; \
> >>>>> ALIGN; \
> >>>>
> >>>> Couldn't END_LP() set type and size unconditionally? (But see also
> >>>> below.)
> >>>
> >>> I see, so that we could also use it for debug purposes. I guess at
> >>> that point it might be better to use {START,END}_FUNC() to note that
> >>> the macros also have an effect beyond that of livepatching.
> >>>
> >>> Maybe also introduce a START_ENTRY() that replaces ENTRY()? Albeit I
> >>> find START_ENTRY a weird name.
> >>
> >> So do I. {START,END}_FUNC() or whatever else are in principle fine, but
> >> I take it that you're aware that we meanwhile have two or even three
> >> concurring proposals on a general scheme of such annotations, and we
> >> don't seem to be able to agree on one. (I guess I'll make a design
> >> session proposal on this topic for Prague.)
> >
> > Oh, I wasn't aware we had other proposals, I've been away on an off
> > quite a lot recently, and haven't been able to keep up with all
> > xen-devel email. Do you have any references at hand?
>
> Andrew said he had posted something long ago, but I didn't recall and
> hence have no reference. My posting from about a year ago is
> https://lists.xen.org/archives/html/xen-devel/2022-04/msg00876.html
> Subsequently Jane went kind of the Linux route:
> https://lists.xen.org/archives/html/xen-devel/2022-08/msg00236.html
>
> >> One thing needs to be clear though: Macros doing things solely needed
> >> for LP need to not have extra effects with it disabled, and such
> >> macros also better wouldn't e.g. insert stray JMP when not really
> >> needed. Hence I expect we still want (some) LP-specific macros besides
> >> whatever we settle on as the generic ones.
> >
> > The stray jmp can be inserted only in the livepatch case, if we end up
> > having to add it.
> >
> > Maybe we should just go with Linux names, so initially I would like to
> > use:
> >
> > SYM_FUNC_START{_NOALIGN}(name)
> > SYM_FUNC_START_LOCAL{_NOALIGN}(name)
> > SYM_FUNC_END(name)
>
> As said in replies on the earlier threads, I think these are overly
> verbose and come in overly many variations.
Right, I would only introduce the ones above and as lonog as I have at
least one user for them. I don't think there's much value in importing
the file wholesale if we have no use case for a lot of the imported
macros.
The main issue with ENTRY() and ENDPROC() / ENDDATA() is that we still
need a tag for local function-like entry point labels, would you then
use PROC() for those? ENTRY_LOCAL()?
I have to admit I prefer the FUNC_START{_LOCAL} for that purpose as I
think it's clearer. I would agree on dropping the SYM_ prefix from
the Linux ones if there's consensus.
I would ideally like to be able to make progress on this before the
XenSummit. I think we all agree we want those annotations, being
blocked on the names of the helpers seems absurd.
Thanks, Roger.
