| Message ID | 20230418092458.15253-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/livepatch: enable livepatching assembly source files | expand |
On 18.04.2023 11:24, Roger Pau Monne wrote: > Some of the assembly entry points cannot be safely patched until it's > safe to use jmp, as livepatch can replace a whole block with a jmp to > a new address, and that won't be safe until speculative mitigations > have been applied. Isn't the issue only with indirect JMP, whereas livepatch uses only direct ones? > --- 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; \ In how far is livepatch susceptible to two .text.* sections of the same name? This can result here and perhaps also for static C functions. > + 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; \ Do these really need to go into config.h, instead of e.g. asm_defns.h? I'd prefer if stuff like this was moved out of here, rather than more things accumulating. (Perhaps these also would better be assembler macros, in which case asm-defns.h might be the place to put them, but I guess that's fairly much a matter of taste.) Couldn't END_LP() set type and size unconditionally? (But see also below.) > --- a/xen/arch/x86/x86_64/entry.S > +++ b/xen/arch/x86/x86_64/entry.S > @@ -660,7 +660,7 @@ ENTRY(early_page_fault) > > ALIGN > /* No special register assumptions. */ > -restore_all_xen: > +START_LP(restore_all_xen) > /* > * Check whether we need to switch to the per-CPU page tables, in > * case we return to late PV exit code (from an NMI or #MC). > @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3) > > RESTORE_ALL adj=8 > iretq > +END_LP(restore_all_xen) While I'm fine with this conversion, ... > ENTRY(common_interrupt) > ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP > @@ -687,6 +688,7 @@ ENTRY(common_interrupt) > SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, %r14=end, %rdx=0, Clob: acd */ > /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ > > +START_LP(common_interrupt_lp) > mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx > mov STACK_CPUINFO_FIELD(use_pv_cr3)(%r14), %bl > mov %rcx, %r15 > @@ -707,6 +709,7 @@ ENTRY(common_interrupt) > mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14) > mov %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14) > jmp ret_from_intr > +END_LP(common_interrupt_lp) ... this one's odd, as it doesn't cover the entire "function". How would you envision we sensibly add ELF metadata also for common_interrupt? Jan
On 18/04/2023 10:24 am, Roger Pau Monne wrote: > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S > index 7675a59ff057..c204634910c4 100644 > --- a/xen/arch/x86/x86_64/entry.S > +++ b/xen/arch/x86/x86_64/entry.S > @@ -660,7 +660,7 @@ ENTRY(early_page_fault) > > ALIGN > /* No special register assumptions. */ > -restore_all_xen: > +START_LP(restore_all_xen) > /* > * Check whether we need to switch to the per-CPU page tables, in > * case we return to late PV exit code (from an NMI or #MC). > @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3) > > RESTORE_ALL adj=8 > iretq > +END_LP(restore_all_xen) While it's useful to have a concrete idea of what is necessary to fix all of this, I do not wish to put in markers like this. This isn't about livepatching - it's about getting sane ELF metadata. This is why I had Jane work on using the Linux macros. They account for *all* interesting ELF metadata, as well as taking care of things like the global function alignment settings, CFI patching space, etc. Putting functions in separate sections should be hidden in the normal SYM_FUNC_START(), and dependent on CONFIG_SPLIT_SECTIONS behind the scenes seeing as we have that as a option already. ~Andrew
On 18/04/2023 12:00 pm, Jan Beulich wrote: > On 18.04.2023 11:24, Roger Pau Monne wrote: >> Some of the assembly entry points cannot be safely patched until it's >> safe to use jmp, as livepatch can replace a whole block with a jmp to >> a new address, and that won't be safe until speculative mitigations >> have been applied. > Isn't the issue only with indirect JMP, whereas livepatch uses only > direct ones? We already have direct jumps prior to speculation safety logic. Livepatching putting more in doesn't change our safety. >> --- 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; \ > In how far is livepatch susceptible to two .text.* sections of the same > name? This can result here and perhaps also for static C functions. Well - the section is the unit of binary-diffing noticing a difference. If we have a naming collision here, then I expect the linker will merge the two section, and the livepatch will end up bigger than it strictly needs to be. ~Andrew
On Tue, Apr 18, 2023 at 01:00:53PM +0200, Jan Beulich wrote: > On 18.04.2023 11:24, Roger Pau Monne wrote: > > Some of the assembly entry points cannot be safely patched until it's > > safe to use jmp, as livepatch can replace a whole block with a jmp to > > a new address, and that won't be safe until speculative mitigations > > have been applied. > > Isn't the issue only with indirect JMP, whereas livepatch uses only > direct ones? Oh, I see, livepatch uses a relative JMP, so that's not affected. > > --- 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; \ > > In how far is livepatch susceptible to two .text.* sections of the same > name? This can result here and perhaps also for static C functions. This is all fine, as long as they are not in the same translation unit. Livepatch creation operates against object files, so as long as there are no section names clashes in a translation unit it should be able to deal with it. > > + 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; \ > > Do these really need to go into config.h, instead of e.g. asm_defns.h? I've just put them together to the ENTRY() macros, but yes, I see no reason to not move them to asm{_,-}defns.h. > I'd prefer if stuff like this was moved out of here, rather than more > things accumulating. (Perhaps these also would better be assembler > macros, in which case asm-defns.h might be the place to put them, but I > guess that's fairly much a matter of taste.) > > 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. > > --- a/xen/arch/x86/x86_64/entry.S > > +++ b/xen/arch/x86/x86_64/entry.S > > @@ -660,7 +660,7 @@ ENTRY(early_page_fault) > > > > ALIGN > > /* No special register assumptions. */ > > -restore_all_xen: > > +START_LP(restore_all_xen) > > /* > > * Check whether we need to switch to the per-CPU page tables, in > > * case we return to late PV exit code (from an NMI or #MC). > > @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3) > > > > RESTORE_ALL adj=8 > > iretq > > +END_LP(restore_all_xen) > > While I'm fine with this conversion, ... So I take that overall you would agree to adding this extra information using a pair of macros similar to the proposed ones. > > ENTRY(common_interrupt) > > ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP > > @@ -687,6 +688,7 @@ ENTRY(common_interrupt) > > SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, %r14=end, %rdx=0, Clob: acd */ > > /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ > > > > +START_LP(common_interrupt_lp) > > mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx > > mov STACK_CPUINFO_FIELD(use_pv_cr3)(%r14), %bl > > mov %rcx, %r15 > > @@ -707,6 +709,7 @@ ENTRY(common_interrupt) > > mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14) > > mov %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14) > > jmp ret_from_intr > > +END_LP(common_interrupt_lp) > > ... this one's odd, as it doesn't cover the entire "function". How would > you envision we sensibly add ELF metadata also for common_interrupt? That was done as to avoid patching the first part of the function that does the speculative mitigations, but since the jmp introduced by livepatch is a relative one we are safe and could indeed patch the whole function. Thanks, Roger.
On Tue, Apr 18, 2023 at 01:17:55PM +0100, Andrew Cooper wrote: > On 18/04/2023 10:24 am, Roger Pau Monne wrote: > > > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S > > index 7675a59ff057..c204634910c4 100644 > > --- a/xen/arch/x86/x86_64/entry.S > > +++ b/xen/arch/x86/x86_64/entry.S > > @@ -660,7 +660,7 @@ ENTRY(early_page_fault) > > > > ALIGN > > /* No special register assumptions. */ > > -restore_all_xen: > > +START_LP(restore_all_xen) > > /* > > * Check whether we need to switch to the per-CPU page tables, in > > * case we return to late PV exit code (from an NMI or #MC). > > @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3) > > > > RESTORE_ALL adj=8 > > iretq > > +END_LP(restore_all_xen) > > > While it's useful to have a concrete idea of what is necessary to fix > all of this, I do not wish to put in markers like this. This isn't > about livepatching - it's about getting sane ELF metadata. Right, Jan has expressed a similar opinion. > This is why I had Jane work on using the Linux macros. They account for > *all* interesting ELF metadata, as well as taking care of things like > the global function alignment settings, CFI patching space, etc. I don't see much issue in using the Linux macros, if we agree we want those. Some of those would likely be unused, do we want to import it wholesale, or just introduce the ones required? Initially I might just introduce SYM_FUNC_START{,_LOCAL}() and SYM_FUNC_END(). > Putting functions in separate sections should be hidden in the normal > SYM_FUNC_START(), and dependent on CONFIG_SPLIT_SECTIONS behind the > scenes seeing as we have that as a option already. Sure. Thanks, Roger.
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.) 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. >>> --- a/xen/arch/x86/x86_64/entry.S >>> +++ b/xen/arch/x86/x86_64/entry.S >>> @@ -660,7 +660,7 @@ ENTRY(early_page_fault) >>> >>> ALIGN >>> /* No special register assumptions. */ >>> -restore_all_xen: >>> +START_LP(restore_all_xen) >>> /* >>> * Check whether we need to switch to the per-CPU page tables, in >>> * case we return to late PV exit code (from an NMI or #MC). >>> @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3) >>> >>> RESTORE_ALL adj=8 >>> iretq >>> +END_LP(restore_all_xen) >> >> While I'm fine with this conversion, ... > > So I take that overall you would agree to adding this extra > information using a pair of macros similar to the proposed ones. Yes (with the above in mind, though). Jan
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? > 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) > >>> --- a/xen/arch/x86/x86_64/entry.S > >>> +++ b/xen/arch/x86/x86_64/entry.S > >>> @@ -660,7 +660,7 @@ ENTRY(early_page_fault) > >>> > >>> ALIGN > >>> /* No special register assumptions. */ > >>> -restore_all_xen: > >>> +START_LP(restore_all_xen) > >>> /* > >>> * Check whether we need to switch to the per-CPU page tables, in > >>> * case we return to late PV exit code (from an NMI or #MC). > >>> @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3) > >>> > >>> RESTORE_ALL adj=8 > >>> iretq > >>> +END_LP(restore_all_xen) > >> > >> While I'm fine with this conversion, ... > > > > So I take that overall you would agree to adding this extra > > information using a pair of macros similar to the proposed ones. > > Yes (with the above in mind, though). Sure, thanks for the feedback. Roger.
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. Jan
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.
On 19.04.2023 13:44, Roger Pau Monné wrote: > 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. Okay, I'm glad we can agree on no SYM_. But what value does START have? And why would the type be (re)specified via ..._END()? FUNC(), DATA(), and END() ought to be all we need. The type would be set by the entry point macros, and the size by END(). To cover local vs global I could live with _LOCAL suffixes, but personally would prefer e.g. LFUNC() and GFUNC(). We could also limit ourselves to FUNC() plus DATA(), and have (non-)global expressed by END() and e.g. LEND() or END_LOCAL(). One less macro, but maybe slightly odd to have the .global directives then at the end rather than at the beginning. Note that this is different from my proposed patch, where I aimed at the change being unintrusive. This includes that this was matching what Arm has (and hence required no change there at all). I think it would certainly be nice if these constructs were as similar as possible between arch-es; some may even be possible to share. Jan
On Wed, Apr 19, 2023 at 02:00:38PM +0200, Jan Beulich wrote: > On 19.04.2023 13:44, Roger Pau Monné wrote: > > 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. > > Okay, I'm glad we can agree on no SYM_. But what value does START have? > And why would the type be (re)specified via ..._END()? FUNC(), DATA(), > and END() ought to be all we need. Does it imply that we would then drop ENTRY()? (seems so, would just like to confirm). > The type would be set by the entry > point macros, and the size by END(). To cover local vs global I could > live with _LOCAL suffixes, but personally would prefer e.g. LFUNC() > and GFUNC(). We could also limit ourselves to FUNC() plus DATA(), and > have (non-)global expressed by END() and e.g. LEND() or END_LOCAL(). > One less macro, but maybe slightly odd to have the .global directives > then at the end rather than at the beginning. Hm, yes, I do find it odd to have the .global at the end. FUNC and FUNC_LOCAL would be my preference, I do find {L,G}FUNC a bit confusing. > > Note that this is different from my proposed patch, where I aimed at > the change being unintrusive. This includes that this was matching > what Arm has (and hence required no change there at all). I think it > would certainly be nice if these constructs were as similar as > possible between arch-es; some may even be possible to share. Well, yes, that would seem desirable as long as we can agree on a set of helper names. Thanks, Roger.
On 19.04.2023 15:36, Roger Pau Monné wrote: > On Wed, Apr 19, 2023 at 02:00:38PM +0200, Jan Beulich wrote: >> On 19.04.2023 13:44, Roger Pau Monné wrote: >>> 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. >> >> Okay, I'm glad we can agree on no SYM_. But what value does START have? >> And why would the type be (re)specified via ..._END()? FUNC(), DATA(), >> and END() ought to be all we need. > > Does it imply that we would then drop ENTRY()? (seems so, would just > like to confirm). Yes. ENTRY() may not go away immediately, but I'd expect it to be phased out. >> The type would be set by the entry >> point macros, and the size by END(). To cover local vs global I could >> live with _LOCAL suffixes, but personally would prefer e.g. LFUNC() >> and GFUNC(). We could also limit ourselves to FUNC() plus DATA(), and >> have (non-)global expressed by END() and e.g. LEND() or END_LOCAL(). >> One less macro, but maybe slightly odd to have the .global directives >> then at the end rather than at the beginning. > > Hm, yes, I do find it odd to have the .global at the end. FUNC and > FUNC_LOCAL would be my preference, I do find {L,G}FUNC a bit > confusing. Well, yes, I was expecting this to be the case. Hence why I said I could live with _LOCAL suffixes, even if they aren't my preference. What we may want to keep in mind is that sooner or later we may want to have non-aligning variants of these. That'll again make for larger names, unless we went with e.g. an optional 2nd parameter which, if absent, means default alignment, while if present it would specify the alignment (which then can be used to effectively specify no alignment). E.g. #define ALIGN(algn...) .balign algn #define GLOBAL(name) \ .globl name; \ .hidden name; \ name: #define FUNC(name, algn...) \ ALIGN(LAST(16, ## algn), 0x90); \ GLOBAL(name); \ .type name, @function with these helpers (and count_args() as we already have it), or ideally something yet more simple (which right now I can't seem to be able to think of): #define ARG1_(x, y...) (x) #define ARG2_(x, y...) (y) #define LAST__(nr) ARG ## nr ## _ #define LAST_(nr) LAST__(nr) #define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) Jan
On Wed, Apr 19, 2023 at 04:39:01PM +0200, Jan Beulich wrote: > On 19.04.2023 15:36, Roger Pau Monné wrote: > > On Wed, Apr 19, 2023 at 02:00:38PM +0200, Jan Beulich wrote: > >> On 19.04.2023 13:44, Roger Pau Monné wrote: > >>> 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. > >> > >> Okay, I'm glad we can agree on no SYM_. But what value does START have? > >> And why would the type be (re)specified via ..._END()? FUNC(), DATA(), > >> and END() ought to be all we need. > > > > Does it imply that we would then drop ENTRY()? (seems so, would just > > like to confirm). > > Yes. ENTRY() may not go away immediately, but I'd expect it to be > phased out. > > >> The type would be set by the entry > >> point macros, and the size by END(). To cover local vs global I could > >> live with _LOCAL suffixes, but personally would prefer e.g. LFUNC() > >> and GFUNC(). We could also limit ourselves to FUNC() plus DATA(), and > >> have (non-)global expressed by END() and e.g. LEND() or END_LOCAL(). > >> One less macro, but maybe slightly odd to have the .global directives > >> then at the end rather than at the beginning. > > > > Hm, yes, I do find it odd to have the .global at the end. FUNC and > > FUNC_LOCAL would be my preference, I do find {L,G}FUNC a bit > > confusing. > > Well, yes, I was expecting this to be the case. Hence why I said I could > live with _LOCAL suffixes, even if they aren't my preference. What we > may want to keep in mind is that sooner or later we may want to have > non-aligning variants of these. That'll again make for larger names, > unless we went with e.g. an optional 2nd parameter which, if absent, > means default alignment, while if present it would specify the alignment > (which then can be used to effectively specify no alignment). E.g. > > #define ALIGN(algn...) .balign algn > > #define GLOBAL(name) \ > .globl name; \ > .hidden name; \ > name: > > #define FUNC(name, algn...) \ > ALIGN(LAST(16, ## algn), 0x90); \ > GLOBAL(name); \ > .type name, @function > > with these helpers (and count_args() as we already have it), or ideally > something yet more simple (which right now I can't seem to be able to > think of): > > #define ARG1_(x, y...) (x) > #define ARG2_(x, y...) (y) > > #define LAST__(nr) ARG ## nr ## _ > #define LAST_(nr) LAST__(nr) > #define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) Would seem acceptable to me. Would you like to make a proposal (likely updating your previous patch) along this lines? Thanks, Roger.
On 19.04.2023 17:57, Roger Pau Monné wrote: > On Wed, Apr 19, 2023 at 04:39:01PM +0200, Jan Beulich wrote: >> On 19.04.2023 15:36, Roger Pau Monné wrote: >>> On Wed, Apr 19, 2023 at 02:00:38PM +0200, Jan Beulich wrote: >>>> On 19.04.2023 13:44, Roger Pau Monné wrote: >>>>> 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. >>>> >>>> Okay, I'm glad we can agree on no SYM_. But what value does START have? >>>> And why would the type be (re)specified via ..._END()? FUNC(), DATA(), >>>> and END() ought to be all we need. >>> >>> Does it imply that we would then drop ENTRY()? (seems so, would just >>> like to confirm). >> >> Yes. ENTRY() may not go away immediately, but I'd expect it to be >> phased out. >> >>>> The type would be set by the entry >>>> point macros, and the size by END(). To cover local vs global I could >>>> live with _LOCAL suffixes, but personally would prefer e.g. LFUNC() >>>> and GFUNC(). We could also limit ourselves to FUNC() plus DATA(), and >>>> have (non-)global expressed by END() and e.g. LEND() or END_LOCAL(). >>>> One less macro, but maybe slightly odd to have the .global directives >>>> then at the end rather than at the beginning. >>> >>> Hm, yes, I do find it odd to have the .global at the end. FUNC and >>> FUNC_LOCAL would be my preference, I do find {L,G}FUNC a bit >>> confusing. >> >> Well, yes, I was expecting this to be the case. Hence why I said I could >> live with _LOCAL suffixes, even if they aren't my preference. What we >> may want to keep in mind is that sooner or later we may want to have >> non-aligning variants of these. That'll again make for larger names, >> unless we went with e.g. an optional 2nd parameter which, if absent, >> means default alignment, while if present it would specify the alignment >> (which then can be used to effectively specify no alignment). E.g. >> >> #define ALIGN(algn...) .balign algn >> >> #define GLOBAL(name) \ >> .globl name; \ >> .hidden name; \ >> name: >> >> #define FUNC(name, algn...) \ >> ALIGN(LAST(16, ## algn), 0x90); \ >> GLOBAL(name); \ >> .type name, @function >> >> with these helpers (and count_args() as we already have it), or ideally >> something yet more simple (which right now I can't seem to be able to >> think of): >> >> #define ARG1_(x, y...) (x) >> #define ARG2_(x, y...) (y) >> >> #define LAST__(nr) ARG ## nr ## _ >> #define LAST_(nr) LAST__(nr) >> #define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) > > Would seem acceptable to me. Would you like to make a proposal > (likely updating your previous patch) along this lines? I wouldn't mind doing so, as long as there was at least a vague chance that this also comes somewhat close to meet Andrew's expectations. Andrew? Jan
diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h index fbc4bb3416bd..68e7fdfe3517 100644 --- 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; \ diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S index 7675a59ff057..c204634910c4 100644 --- a/xen/arch/x86/x86_64/entry.S +++ b/xen/arch/x86/x86_64/entry.S @@ -660,7 +660,7 @@ ENTRY(early_page_fault) ALIGN /* No special register assumptions. */ -restore_all_xen: +START_LP(restore_all_xen) /* * Check whether we need to switch to the per-CPU page tables, in * case we return to late PV exit code (from an NMI or #MC). @@ -677,6 +677,7 @@ UNLIKELY_END(exit_cr3) RESTORE_ALL adj=8 iretq +END_LP(restore_all_xen) ENTRY(common_interrupt) ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP @@ -687,6 +688,7 @@ ENTRY(common_interrupt) SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs, %r14=end, %rdx=0, Clob: acd */ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ +START_LP(common_interrupt_lp) mov STACK_CPUINFO_FIELD(xen_cr3)(%r14), %rcx mov STACK_CPUINFO_FIELD(use_pv_cr3)(%r14), %bl mov %rcx, %r15 @@ -707,6 +709,7 @@ ENTRY(common_interrupt) mov %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14) mov %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14) jmp ret_from_intr +END_LP(common_interrupt_lp) ENTRY(page_fault) ENDBR64
In order to be able to livepatch code from assembly files we need: * Proper function symbols from assembly code, including the size. * Separate sections for each function. However assembly code doesn't really have the concept of a function, and hence the code tends to chain different labels that can also be entry points. In order to be able to livepatch such code we need to enclose the assembly code into isolated function-like blocks, so they can be handled by livepatch. Introduce two new macros to do so, {START,END}_LP() that take a unique function like name, create the function symbol and put the code into a separate text section. Note that START_LP() requires a preceding jump before the section change, so that any preceding code that fallthrough correctly continues execution, as sections can be reordered. Chaining of consecutive livepatchable blocks will also require that the previous section jumps into the next one if required. A couple of shortcomings: * We don't check that the size of the section is enough to fit a jump instruction (ARCH_PATCH_INSN_SIZE). Some logic from the alternatives framework should be used to pad sections if required. * Any labels inside of a {START,END}_LP() section must not be referenced from another section, as the patching would break those. I haven't figured out a way to detect such references. We already use .L to denote local labels, but we would have to be careful. Some of the assembly entry points cannot be safely patched until it's safe to use jmp, as livepatch can replace a whole block with a jmp to a new address, and that won't be safe until speculative mitigations have been applied. I could also look into allowing livepatch of sections where jmp replacement is not safe by requesting in-place code replacement only, we could then maybe allow adding some nop padding to those sections in order to cope with the size increasing in further livepatches. So far this patch only contains two switched functions: restore_all_xen and common_interrupt. I don't really want to switch more code until we agree on the approach, so take this as a kind of RFC patch. Obviously conversion doesn't need to be done in one go, neither all assembly code need to be 'transformed' in this way. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/include/asm/config.h | 14 ++++++++++++++ xen/arch/x86/x86_64/entry.S | 5 ++++- 2 files changed, 18 insertions(+), 1 deletion(-)