| Message ID | 20240717111231.3517605-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [for-4.20] hvmloader: Rework hypercall infrastructure | expand |
On 17.07.2024 13:12, Andrew Cooper wrote: > Right now, the hypercall page is at a hardcoded physical address, and making > hypercalls involves indirect calls to compile-time constant addresses. > e.g. HYPERCALL_memory_op is: > > mov $0x80180,%eax > call *%eax > > Instead, import the hypercall infrastructure from XTF to have hypercall_page[] > fully within the hvmloader image, and prepared at compile time rather than > runtime. This uses direct calls, so HYPERCALL_memory_op now disassembles as: > > call 132180 <HYPERCALL_memory_op> > > which is faster and clearer. Just to mention it - even with a fixed address using indirect calls shouldn't have been necessary (minus assembler bugs, that is). > Remove the loop over multiple hypercall pages. It was long ago realised to be > an unworkable design, and eax fixed in the ABI to 1. > > Pass -z noexecstack to LD to stop newer bintuils complaining about the absence > of .note.GNU-stack. hvmloader is not a regular binary, and in fact its stack > is always executable owing to operating in unpaged mode. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > > There doesn't appear to be any sensible AFLAGS infrastructure to set > -D__ASSEMBLY__. Opecoding it once seemed like the least bad option. I agree. > --- > tools/firmware/hvmloader/Makefile | 3 + > tools/firmware/hvmloader/config.h | 1 - > tools/firmware/hvmloader/hvmloader.c | 7 +- > tools/firmware/hvmloader/hvmloader.lds | 4 +- > tools/firmware/hvmloader/hypercall.h | 121 ++++++---------------- > tools/firmware/hvmloader/hypercall_page.S | 67 ++++++++++++ > 6 files changed, 105 insertions(+), 98 deletions(-) > create mode 100644 tools/firmware/hvmloader/hypercall_page.S May I ask that the new file use a hyphen in place of the underscore? > @@ -142,8 +141,7 @@ static void init_hypercalls(void) > > /* Fill in hypercall transfer pages. */ > cpuid(base + 2, &eax, &ebx, &ecx, &edx); > - for ( i = 0; i < eax; i++ ) > - wrmsr(ebx, HYPERCALL_PHYSICAL_ADDRESS + (i << 12) + i); > + wrmsr(ebx, (unsigned long)hypercall_page); Convert the comment to singular then, too? > --- a/tools/firmware/hvmloader/hvmloader.lds > +++ b/tools/firmware/hvmloader/hvmloader.lds > @@ -7,9 +7,9 @@ SECTIONS > * NB: there's no need to use the AT keyword in order to set the LMA, by > * default the linker will use VMA = LMA unless specified otherwise. > */ > - .text : { *(.text) *(.text.*) } > + .text : { *(.text) *(.text.*)} Likely merely a leftover from some experimentation? > --- a/tools/firmware/hvmloader/hypercall.h > +++ b/tools/firmware/hvmloader/hypercall.h > @@ -35,148 +35,91 @@ > #include <xen/xen.h> > #include "config.h" > > -#define hcall_addr(name) \ > - ((unsigned long)HYPERCALL_PHYSICAL_ADDRESS + __HYPERVISOR_##name * 32) > - > -#define _hypercall0(type, name) \ > -({ \ > - long __res; \ > - asm volatile ( \ > - "call *%%eax" \ > - : "=a" (__res) \ > - : "0" (hcall_addr(name)) \ > - : "memory" ); \ > - (type)__res; \ > -}) > - > -#define _hypercall1(type, name, a1) \ > -({ \ > - long __res, __ign1; \ > - asm volatile ( \ > - "call *%%eax" \ > - : "=a" (__res), "=b" (__ign1) \ > - : "0" (hcall_addr(name)), \ > - "1" ((long)(a1)) \ > - : "memory" ); \ > - (type)__res; \ > -}) > - > -#define _hypercall2(type, name, a1, a2) \ > -({ \ > - long __res, __ign1, __ign2; \ > - asm volatile ( \ > - "call *%%eax" \ > - : "=a" (__res), "=b" (__ign1), "=c" (__ign2) \ > - : "0" (hcall_addr(name)), \ > - "1" ((long)(a1)), "2" ((long)(a2)) \ > - : "memory" ); \ > - (type)__res; \ > -}) > - > -#define _hypercall3(type, name, a1, a2, a3) \ > -({ \ > - long __res, __ign1, __ign2, __ign3; \ > - asm volatile ( \ > - "call *%%eax" \ > - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ > - "=d" (__ign3) \ > - : "0" (hcall_addr(name)), \ > - "1" ((long)(a1)), "2" ((long)(a2)), \ > - "3" ((long)(a3)) \ > - : "memory" ); \ > - (type)__res; \ > -}) > - > -#define _hypercall4(type, name, a1, a2, a3, a4) \ > -({ \ > - long __res, __ign1, __ign2, __ign3, __ign4; \ > - asm volatile ( \ > - "call *%%eax" \ > - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ > - "=d" (__ign3), "=S" (__ign4) \ > - : "0" (hcall_addr(name)), \ > - "1" ((long)(a1)), "2" ((long)(a2)), \ > - "3" ((long)(a3)), "4" ((long)(a4)) \ > - : "memory" ); \ > - (type)__res; \ > -}) > - > -#define _hypercall5(type, name, a1, a2, a3, a4, a5) \ > -({ \ > - long __res, __ign1, __ign2, __ign3, __ign4, __ign5; \ > - asm volatile ( \ > - "call *%%eax" \ > - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ > - "=d" (__ign3), "=S" (__ign4), "=D" (__ign5) \ > - : "0" (hcall_addr(name)), \ > - "1" ((long)(a1)), "2" ((long)(a2)), \ > - "3" ((long)(a3)), "4" ((long)(a4)), \ > - "5" ((long)(a5)) \ > - : "memory" ); \ > - (type)__res; \ > -}) > +extern const char hypercall_page[]; > + > +#define _hypercall2(type, hcall, a1, a2) \ > + ({ \ > + long res, _a1 = (long)(a1), _a2 = (long)(a2); \ > + asm volatile ( \ > + "call hypercall_page + %c[offset]" \ > + : "=a" (res), "+b" (_a1), "+c" (_a2) \ > + : [offset] "i" (hcall * 32) \ > + : "memory" ); \ > + (type)res; \ > + }) > + > +#define _hypercall3(type, hcall, a1, a2, a3) \ > + ({ \ > + long res, _a1 = (long)(a1), _a2 = (long)(a2), _a3 = (long)(a3); \ > + asm volatile ( \ > + "call hypercall_page + %c[offset]" \ > + : "=a" (res), "+b" (_a1), "+c" (_a2), "+d" (_a3) \ > + : [offset] "i" (hcall * 32) \ > + : "memory" ); \ > + (type)res; \ > + }) Not having _hypercall0() and _hypercall1() anymore is certainly a little odd. If one needed to use such a hypercall, even if only for debugging, the extra work needed (every time) would be larger than necessary. I'm definitely less worried about _hypercall4() and _hypercall5(). In any event the removal of any wrappers could do with mentioning in the description, to indicate it's deliberate (and why). > static inline int > hypercall_sched_op( > int cmd, void *arg) > { > - return _hypercall2(int, sched_op, cmd, arg); > + return _hypercall2(int, __HYPERVISOR_sched_op, cmd, arg); > } I know you don't really like token concatenation in cases like these ones, but these adjustments all don't look as if they were necessary right here. The new macros use the macro parameter only in ways where token pasting would continue to work, afaics. And in the new assembly file you use very similar token pasting anyway. > --- /dev/null > +++ b/tools/firmware/hvmloader/hypercall_page.S > @@ -0,0 +1,67 @@ > +#include <xen/xen.h> > + > + .section ".hcall", "aw" Why "aw"? I'd have expected "awx" if you really think the writable part needs expressing here, or else "ax". Otherwise I think a brief comment as wanted as to the absence of x for something that is executable. Also may I ask that you add @progbits? > + .align 4096 PAGE_SIZE? And then again ... > + .globl hypercall_page > +hypercall_page: > + /* Poisoned with `ret` for safety before hypercalls are set up. */ > + .fill 4096, 1, 0xc3 > + .type hypercall_page, STT_OBJECT > + .size hypercall_page, 4096 ... here? As to the "poisoning" - how does RET provide any safety? If a call happened early, the uncertainty of what %eax is set to would make it unpredictable how such a caller would further behave. Imo better to crash / hang in such a case, perhaps by using INT3 instead. I notice that matches earlier behavior, but there the comment at least validly said "rendering them no-ops" (yet still without considering possible problems resulting from doing so). > +#define DECLARE_HYPERCALL(name) \ > + .globl HYPERCALL_ ## name; \ > + .type HYPERCALL_ ## name, STT_FUNC; \ > + .size HYPERCALL_ ## name, 32; \ > + .set HYPERCALL_ ## name, hypercall_page + __HYPERVISOR_ ## name * 32 > + > +DECLARE_HYPERCALL(set_trap_table) > +DECLARE_HYPERCALL(mmu_update) > +DECLARE_HYPERCALL(set_gdt) > +DECLARE_HYPERCALL(stack_switch) > +DECLARE_HYPERCALL(set_callbacks) > +DECLARE_HYPERCALL(fpu_taskswitch) > +DECLARE_HYPERCALL(sched_op_compat) > +DECLARE_HYPERCALL(platform_op) > +DECLARE_HYPERCALL(set_debugreg) > +DECLARE_HYPERCALL(get_debugreg) > +DECLARE_HYPERCALL(update_descriptor) > +DECLARE_HYPERCALL(memory_op) > +DECLARE_HYPERCALL(multicall) > +DECLARE_HYPERCALL(update_va_mapping) > +DECLARE_HYPERCALL(set_timer_op) > +DECLARE_HYPERCALL(event_channel_op_compat) > +DECLARE_HYPERCALL(xen_version) > +DECLARE_HYPERCALL(console_io) > +DECLARE_HYPERCALL(physdev_op_compat) > +DECLARE_HYPERCALL(grant_table_op) > +DECLARE_HYPERCALL(vm_assist) > +DECLARE_HYPERCALL(update_va_mapping_otherdomain) > +DECLARE_HYPERCALL(iret) > +DECLARE_HYPERCALL(vcpu_op) > +DECLARE_HYPERCALL(set_segment_base) > +DECLARE_HYPERCALL(mmuext_op) > +DECLARE_HYPERCALL(xsm_op) > +DECLARE_HYPERCALL(nmi_op) > +DECLARE_HYPERCALL(sched_op) > +DECLARE_HYPERCALL(callback_op) > +DECLARE_HYPERCALL(xenoprof_op) > +DECLARE_HYPERCALL(event_channel_op) > +DECLARE_HYPERCALL(physdev_op) > +DECLARE_HYPERCALL(hvm_op) > +DECLARE_HYPERCALL(sysctl) > +DECLARE_HYPERCALL(domctl) > +DECLARE_HYPERCALL(kexec_op) > +DECLARE_HYPERCALL(tmem_op) We're not going to ever need this in hvmloader, are we? There are quite a few more that I'd suggest to leave out, but this one stands out for no longer existing even in the hypervisor. Jan > +DECLARE_HYPERCALL(argo_op) > +DECLARE_HYPERCALL(xenpmu_op) > + > +DECLARE_HYPERCALL(arch_0) > +DECLARE_HYPERCALL(arch_1) > +DECLARE_HYPERCALL(arch_2) > +DECLARE_HYPERCALL(arch_3) > +DECLARE_HYPERCALL(arch_4) > +DECLARE_HYPERCALL(arch_5) > +DECLARE_HYPERCALL(arch_6) > +DECLARE_HYPERCALL(arch_7)
On 17/07/2024 1:37 pm, Jan Beulich wrote: > On 17.07.2024 13:12, Andrew Cooper wrote: >> Right now, the hypercall page is at a hardcoded physical address, and making >> hypercalls involves indirect calls to compile-time constant addresses. >> e.g. HYPERCALL_memory_op is: >> >> mov $0x80180,%eax >> call *%eax >> >> Instead, import the hypercall infrastructure from XTF to have hypercall_page[] >> fully within the hvmloader image, and prepared at compile time rather than >> runtime. This uses direct calls, so HYPERCALL_memory_op now disassembles as: >> >> call 132180 <HYPERCALL_memory_op> >> >> which is faster and clearer. > Just to mention it - even with a fixed address using indirect calls shouldn't > have been necessary (minus assembler bugs, that is). Indeed. The very proto-form of XTF to investigate XSA-106 was done in HVMLoader because I needed something HVM based (rather than the MiniOS based PV work I'd been doing up to that point). Starting XTF from scratch was a very deliberate decision so as not to starting with technical debt such as this... > >> Remove the loop over multiple hypercall pages. It was long ago realised to be >> an unworkable design, and eax fixed in the ABI to 1. >> >> Pass -z noexecstack to LD to stop newer bintuils complaining about the absence >> of .note.GNU-stack. hvmloader is not a regular binary, and in fact its stack >> is always executable owing to operating in unpaged mode. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> >> There doesn't appear to be any sensible AFLAGS infrastructure to set >> -D__ASSEMBLY__. Opecoding it once seemed like the least bad option. > I agree. > >> --- >> tools/firmware/hvmloader/Makefile | 3 + >> tools/firmware/hvmloader/config.h | 1 - >> tools/firmware/hvmloader/hvmloader.c | 7 +- >> tools/firmware/hvmloader/hvmloader.lds | 4 +- >> tools/firmware/hvmloader/hypercall.h | 121 ++++++---------------- >> tools/firmware/hvmloader/hypercall_page.S | 67 ++++++++++++ >> 6 files changed, 105 insertions(+), 98 deletions(-) >> create mode 100644 tools/firmware/hvmloader/hypercall_page.S > May I ask that the new file use a hyphen in place of the underscore? > >> @@ -142,8 +141,7 @@ static void init_hypercalls(void) >> >> /* Fill in hypercall transfer pages. */ >> cpuid(base + 2, &eax, &ebx, &ecx, &edx); >> - for ( i = 0; i < eax; i++ ) >> - wrmsr(ebx, HYPERCALL_PHYSICAL_ADDRESS + (i << 12) + i); >> + wrmsr(ebx, (unsigned long)hypercall_page); > Convert the comment to singular then, too? > >> --- a/tools/firmware/hvmloader/hvmloader.lds >> +++ b/tools/firmware/hvmloader/hvmloader.lds >> @@ -7,9 +7,9 @@ SECTIONS >> * NB: there's no need to use the AT keyword in order to set the LMA, by >> * default the linker will use VMA = LMA unless specified otherwise. >> */ >> - .text : { *(.text) *(.text.*) } >> + .text : { *(.text) *(.text.*)} > Likely merely a leftover from some experimentation? Ah yes. I originally had .text.hcall which doesn't work properly after .text.* But I also didn't want it disassembled by default, hence ending up in data. > >> --- a/tools/firmware/hvmloader/hypercall.h >> +++ b/tools/firmware/hvmloader/hypercall.h >> @@ -35,148 +35,91 @@ >> #include <xen/xen.h> >> #include "config.h" >> >> -#define hcall_addr(name) \ >> - ((unsigned long)HYPERCALL_PHYSICAL_ADDRESS + __HYPERVISOR_##name * 32) >> - >> -#define _hypercall0(type, name) \ >> -({ \ >> - long __res; \ >> - asm volatile ( \ >> - "call *%%eax" \ >> - : "=a" (__res) \ >> - : "0" (hcall_addr(name)) \ >> - : "memory" ); \ >> - (type)__res; \ >> -}) >> - >> -#define _hypercall1(type, name, a1) \ >> -({ \ >> - long __res, __ign1; \ >> - asm volatile ( \ >> - "call *%%eax" \ >> - : "=a" (__res), "=b" (__ign1) \ >> - : "0" (hcall_addr(name)), \ >> - "1" ((long)(a1)) \ >> - : "memory" ); \ >> - (type)__res; \ >> -}) >> - >> -#define _hypercall2(type, name, a1, a2) \ >> -({ \ >> - long __res, __ign1, __ign2; \ >> - asm volatile ( \ >> - "call *%%eax" \ >> - : "=a" (__res), "=b" (__ign1), "=c" (__ign2) \ >> - : "0" (hcall_addr(name)), \ >> - "1" ((long)(a1)), "2" ((long)(a2)) \ >> - : "memory" ); \ >> - (type)__res; \ >> -}) >> - >> -#define _hypercall3(type, name, a1, a2, a3) \ >> -({ \ >> - long __res, __ign1, __ign2, __ign3; \ >> - asm volatile ( \ >> - "call *%%eax" \ >> - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ >> - "=d" (__ign3) \ >> - : "0" (hcall_addr(name)), \ >> - "1" ((long)(a1)), "2" ((long)(a2)), \ >> - "3" ((long)(a3)) \ >> - : "memory" ); \ >> - (type)__res; \ >> -}) >> - >> -#define _hypercall4(type, name, a1, a2, a3, a4) \ >> -({ \ >> - long __res, __ign1, __ign2, __ign3, __ign4; \ >> - asm volatile ( \ >> - "call *%%eax" \ >> - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ >> - "=d" (__ign3), "=S" (__ign4) \ >> - : "0" (hcall_addr(name)), \ >> - "1" ((long)(a1)), "2" ((long)(a2)), \ >> - "3" ((long)(a3)), "4" ((long)(a4)) \ >> - : "memory" ); \ >> - (type)__res; \ >> -}) >> - >> -#define _hypercall5(type, name, a1, a2, a3, a4, a5) \ >> -({ \ >> - long __res, __ign1, __ign2, __ign3, __ign4, __ign5; \ >> - asm volatile ( \ >> - "call *%%eax" \ >> - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ >> - "=d" (__ign3), "=S" (__ign4), "=D" (__ign5) \ >> - : "0" (hcall_addr(name)), \ >> - "1" ((long)(a1)), "2" ((long)(a2)), \ >> - "3" ((long)(a3)), "4" ((long)(a4)), \ >> - "5" ((long)(a5)) \ >> - : "memory" ); \ >> - (type)__res; \ >> -}) >> +extern const char hypercall_page[]; >> + >> +#define _hypercall2(type, hcall, a1, a2) \ >> + ({ \ >> + long res, _a1 = (long)(a1), _a2 = (long)(a2); \ >> + asm volatile ( \ >> + "call hypercall_page + %c[offset]" \ >> + : "=a" (res), "+b" (_a1), "+c" (_a2) \ >> + : [offset] "i" (hcall * 32) \ >> + : "memory" ); \ >> + (type)res; \ >> + }) >> + >> +#define _hypercall3(type, hcall, a1, a2, a3) \ >> + ({ \ >> + long res, _a1 = (long)(a1), _a2 = (long)(a2), _a3 = (long)(a3); \ >> + asm volatile ( \ >> + "call hypercall_page + %c[offset]" \ >> + : "=a" (res), "+b" (_a1), "+c" (_a2), "+d" (_a3) \ >> + : [offset] "i" (hcall * 32) \ >> + : "memory" ); \ >> + (type)res; \ >> + }) > Not having _hypercall0() and _hypercall1() anymore is certainly a little > odd. If one needed to use such a hypercall, even if only for debugging, > the extra work needed (every time) would be larger than necessary. I'm > definitely less worried about _hypercall4() and _hypercall5(). > > In any event the removal of any wrappers could do with mentioning in the > description, to indicate it's deliberate (and why). I suppose leaving 0 and 1 behind is fine. That's easy enough. > >> static inline int >> hypercall_sched_op( >> int cmd, void *arg) >> { >> - return _hypercall2(int, sched_op, cmd, arg); >> + return _hypercall2(int, __HYPERVISOR_sched_op, cmd, arg); >> } > I know you don't really like token concatenation in cases like these ones, > but these adjustments all don't look as if they were necessary right here. > The new macros use the macro parameter only in ways where token pasting > would continue to work, afaics. And in the new assembly file you use very > similar token pasting anyway. That's because my judgement is not about simply tokenisation (or not). It's about not using ## when it is likely to interfere with grep/cscope/tags/etc. The assembly file both isn't really interesting to find this way, and needs ## in order to work the way it does (differing prefixes in the hypercall names). > >> --- /dev/null >> +++ b/tools/firmware/hvmloader/hypercall_page.S >> @@ -0,0 +1,67 @@ >> +#include <xen/xen.h> >> + >> + .section ".hcall", "aw" > Why "aw"? I'd have expected "awx" if you really think the writable part > needs expressing here, or else "ax". Otherwise I think a brief comment as > wanted as to the absence of x for something that is executable. It's because it's merged with .data (so it doesn't pollute the disassembly). "awx" causes a section flags mismatch. > Also may I ask that you add @progbits? Ok. > >> + .align 4096 > PAGE_SIZE? And then again ... > >> + .globl hypercall_page >> +hypercall_page: >> + /* Poisoned with `ret` for safety before hypercalls are set up. */ >> + .fill 4096, 1, 0xc3 >> + .type hypercall_page, STT_OBJECT >> + .size hypercall_page, 4096 > ... here? HVMLoader doesn't have a suitable constant, and doesn't have _AC(). Although we probably can just get away with (1 << PAGE_SHIFT) and drop the ul. > > As to the "poisoning" - how does RET provide any safety? If a call happened > early, the uncertainty of what %eax is set to would make it unpredictable > how such a caller would further behave. Imo better to crash / hang in such > a case, perhaps by using INT3 instead. > > I notice that matches earlier behavior, but there the comment at least > validly said "rendering them no-ops" (yet still without considering > possible problems resulting from doing so). That's a complicated one. I can't remember why I chose that exact phraseology, but it's not really about accidentally-too-early case, it's about error handling. HVMLoader doesn't have an IDT, so any exception is fatal. But that's also something that ideally wants fixing (if it weren't for the fact that it's more likely that I'll complete plans to remove hvmloader completely before having time to do an IDT). But for code which does have a panic() function, there's console_io (logging) and sched_op (SHUTDOWN_crash) which you want to use just in case they do work, before moving onto other methods of terminating. > >> +#define DECLARE_HYPERCALL(name) \ >> + .globl HYPERCALL_ ## name; \ >> + .type HYPERCALL_ ## name, STT_FUNC; \ >> + .size HYPERCALL_ ## name, 32; \ >> + .set HYPERCALL_ ## name, hypercall_page + __HYPERVISOR_ ## name * 32 >> + >> +DECLARE_HYPERCALL(set_trap_table) >> +DECLARE_HYPERCALL(mmu_update) >> +DECLARE_HYPERCALL(set_gdt) >> +DECLARE_HYPERCALL(stack_switch) >> +DECLARE_HYPERCALL(set_callbacks) >> +DECLARE_HYPERCALL(fpu_taskswitch) >> +DECLARE_HYPERCALL(sched_op_compat) >> +DECLARE_HYPERCALL(platform_op) >> +DECLARE_HYPERCALL(set_debugreg) >> +DECLARE_HYPERCALL(get_debugreg) >> +DECLARE_HYPERCALL(update_descriptor) >> +DECLARE_HYPERCALL(memory_op) >> +DECLARE_HYPERCALL(multicall) >> +DECLARE_HYPERCALL(update_va_mapping) >> +DECLARE_HYPERCALL(set_timer_op) >> +DECLARE_HYPERCALL(event_channel_op_compat) >> +DECLARE_HYPERCALL(xen_version) >> +DECLARE_HYPERCALL(console_io) >> +DECLARE_HYPERCALL(physdev_op_compat) >> +DECLARE_HYPERCALL(grant_table_op) >> +DECLARE_HYPERCALL(vm_assist) >> +DECLARE_HYPERCALL(update_va_mapping_otherdomain) >> +DECLARE_HYPERCALL(iret) >> +DECLARE_HYPERCALL(vcpu_op) >> +DECLARE_HYPERCALL(set_segment_base) >> +DECLARE_HYPERCALL(mmuext_op) >> +DECLARE_HYPERCALL(xsm_op) >> +DECLARE_HYPERCALL(nmi_op) >> +DECLARE_HYPERCALL(sched_op) >> +DECLARE_HYPERCALL(callback_op) >> +DECLARE_HYPERCALL(xenoprof_op) >> +DECLARE_HYPERCALL(event_channel_op) >> +DECLARE_HYPERCALL(physdev_op) >> +DECLARE_HYPERCALL(hvm_op) >> +DECLARE_HYPERCALL(sysctl) >> +DECLARE_HYPERCALL(domctl) >> +DECLARE_HYPERCALL(kexec_op) >> +DECLARE_HYPERCALL(tmem_op) > We're not going to ever need this in hvmloader, are we? There are quite a > few more that I'd suggest to leave out, but this one stands out for no > longer existing even in the hypervisor. I suspect I can trim this to just what HVMLoader needs. It's not as if we're expecting a major change in functionality. ~Andrew
On 24.07.2024 19:24, Andrew Cooper wrote: > On 17/07/2024 1:37 pm, Jan Beulich wrote: >> On 17.07.2024 13:12, Andrew Cooper wrote: >>> static inline int >>> hypercall_sched_op( >>> int cmd, void *arg) >>> { >>> - return _hypercall2(int, sched_op, cmd, arg); >>> + return _hypercall2(int, __HYPERVISOR_sched_op, cmd, arg); >>> } >> I know you don't really like token concatenation in cases like these ones, >> but these adjustments all don't look as if they were necessary right here. >> The new macros use the macro parameter only in ways where token pasting >> would continue to work, afaics. And in the new assembly file you use very >> similar token pasting anyway. > > That's because my judgement is not about simply tokenisation (or not). > It's about not using ## when it is likely to interfere with > grep/cscope/tags/etc. > > The assembly file both isn't really interesting to find this way, and > needs ## in order to work the way it does (differing prefixes in the > hypercall names). The "not interesting" part is very subjective. If one really wanted to find everything, this still would get in the way. And there being differing prefixes in the other case would then simply require passing two arguments to the macro instead of just one (which imo is undesirable, but that is along with considering the change above undesirable as well). I'm not bothered enough to request to undo this altogether, or to split this into a separate change, but I wonder whether in the opposite case you wouldn't demand either or even both. >>> + .align 4096 >> PAGE_SIZE? And then again ... >> >>> + .globl hypercall_page >>> +hypercall_page: >>> + /* Poisoned with `ret` for safety before hypercalls are set up. */ >>> + .fill 4096, 1, 0xc3 >>> + .type hypercall_page, STT_OBJECT >>> + .size hypercall_page, 4096 >> ... here? > > HVMLoader doesn't have a suitable constant, and doesn't have _AC(). > > Although we probably can just get away with (1 << PAGE_SHIFT) and drop > the ul. Oh, right, we still mean to be compatible with gas not ignoring the ul suffix on numbers. >> As to the "poisoning" - how does RET provide any safety? If a call happened >> early, the uncertainty of what %eax is set to would make it unpredictable >> how such a caller would further behave. Imo better to crash / hang in such >> a case, perhaps by using INT3 instead. >> >> I notice that matches earlier behavior, but there the comment at least >> validly said "rendering them no-ops" (yet still without considering >> possible problems resulting from doing so). > > That's a complicated one. I can't remember why I chose that exact > phraseology, but it's not really about accidentally-too-early case, it's > about error handling. > > HVMLoader doesn't have an IDT, so any exception is fatal. But that's > also something that ideally wants fixing (if it weren't for the fact > that it's more likely that I'll complete plans to remove hvmloader > completely before having time to do an IDT). > > But for code which does have a panic() function, there's console_io > (logging) and sched_op (SHUTDOWN_crash) which you want to use just in > case they do work, before moving onto other methods of terminating. All fine, but that still leaves this RET filling yielding random behavior at possible early call sites. At the very least the pre-fill should result in errors to be observed by too-early callers. For this to at least vaguely resemble something valid to call "poisoned", that is. Jan
On Thu, Jul 25, 2024 at 08:18:27AM +0200, Jan Beulich wrote: > On 24.07.2024 19:24, Andrew Cooper wrote: > > On 17/07/2024 1:37 pm, Jan Beulich wrote: > >> On 17.07.2024 13:12, Andrew Cooper wrote: > >>> static inline int > >>> hypercall_sched_op( > >>> int cmd, void *arg) > >>> { > >>> - return _hypercall2(int, sched_op, cmd, arg); > >>> + return _hypercall2(int, __HYPERVISOR_sched_op, cmd, arg); > >>> } > >> I know you don't really like token concatenation in cases like these ones, > >> but these adjustments all don't look as if they were necessary right here. > >> The new macros use the macro parameter only in ways where token pasting > >> would continue to work, afaics. And in the new assembly file you use very > >> similar token pasting anyway. > > > > That's because my judgement is not about simply tokenisation (or not). > > It's about not using ## when it is likely to interfere with > > grep/cscope/tags/etc. > > > > The assembly file both isn't really interesting to find this way, and > > needs ## in order to work the way it does (differing prefixes in the > > hypercall names). > > The "not interesting" part is very subjective. If one really wanted to find > everything, this still would get in the way. And there being differing > prefixes in the other case would then simply require passing two arguments > to the macro instead of just one (which imo is undesirable, but that is > along with considering the change above undesirable as well). > > I'm not bothered enough to request to undo this altogether, or to split > this into a separate change, but I wonder whether in the opposite case you > wouldn't demand either or even both. > > >>> + .align 4096 > >> PAGE_SIZE? And then again ... > >> > >>> + .globl hypercall_page > >>> +hypercall_page: > >>> + /* Poisoned with `ret` for safety before hypercalls are set up. */ > >>> + .fill 4096, 1, 0xc3 > >>> + .type hypercall_page, STT_OBJECT > >>> + .size hypercall_page, 4096 > >> ... here? > > > > HVMLoader doesn't have a suitable constant, and doesn't have _AC(). > > > > Although we probably can just get away with (1 << PAGE_SHIFT) and drop > > the ul. > > Oh, right, we still mean to be compatible with gas not ignoring the ul > suffix on numbers. > > >> As to the "poisoning" - how does RET provide any safety? If a call happened > >> early, the uncertainty of what %eax is set to would make it unpredictable > >> how such a caller would further behave. Imo better to crash / hang in such > >> a case, perhaps by using INT3 instead. > >> > >> I notice that matches earlier behavior, but there the comment at least > >> validly said "rendering them no-ops" (yet still without considering > >> possible problems resulting from doing so). > > > > That's a complicated one. I can't remember why I chose that exact > > phraseology, but it's not really about accidentally-too-early case, it's > > about error handling. > > > > HVMLoader doesn't have an IDT, so any exception is fatal. But that's > > also something that ideally wants fixing (if it weren't for the fact > > that it's more likely that I'll complete plans to remove hvmloader > > completely before having time to do an IDT). > > > > But for code which does have a panic() function, there's console_io > > (logging) and sched_op (SHUTDOWN_crash) which you want to use just in > > case they do work, before moving onto other methods of terminating. > > All fine, but that still leaves this RET filling yielding random behavior > at possible early call sites. At the very least the pre-fill should result > in errors to be observed by too-early callers. For this to at least vaguely > resemble something valid to call "poisoned", that is. FWIW, I've not long ago [0] switched FreeBSD hypercall page to be poisoned with int3, as it was previously filled with nops which resulted in very funny traces when the hypercall page was used prior to being initialized. I also considered filling with ret, but I think that's likely to mask or make uninitialized usages harder to spot. It's IMO better to get a triple-fault than silence attempts to use the hypercall page too early. Thanks, Roger. [0] https://cgit.freebsd.org/src/commit/?id=e283c994ab270706142ef5dde9092950000af901
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile index 84cba171cd6b..0e1dce26b342 100644 --- a/tools/firmware/hvmloader/Makefile +++ b/tools/firmware/hvmloader/Makefile @@ -34,6 +34,7 @@ OBJS = hvmloader.o mp_tables.o util.o smbios.o OBJS += smp.o cacheattr.o xenbus.o vnuma.o OBJS += e820.o pci.o pir.o ctype.o OBJS += hvm_param.o +OBJS += hypercall_page.o OBJS += ovmf.o seabios.o ifeq ($(debug),y) OBJS += tests.o @@ -64,6 +65,7 @@ endif # Suppress the warning about LOAD segments with RWX permissions, as what we # build isn't a normal user-mode executable. LDFLAGS-$(call ld-option,--warn-rwx-segments) := --no-warn-rwx-segments +LDFLAGS-y += -z noexecstack .PHONY: all all: hvmloader @@ -74,6 +76,7 @@ acpi: rombios.o: roms.inc smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\"" +hypercall_page.o: CFLAGS += -D__ASSEMBLY__ ACPI_PATH = ../../libacpi DSDT_FILES = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h index cd716bf39245..4ff7aa9d4483 100644 --- a/tools/firmware/hvmloader/config.h +++ b/tools/firmware/hvmloader/config.h @@ -64,7 +64,6 @@ extern bool acpi_enabled; /* Memory map. */ #define SCRATCH_PHYSICAL_ADDRESS 0x00010000 -#define HYPERCALL_PHYSICAL_ADDRESS 0x00080000 #define VGABIOS_PHYSICAL_ADDRESS 0x000C0000 #define HVMLOADER_PHYSICAL_ADDRESS 0x00100000 /* Special BIOS mappings, etc. are allocated from here upwards... */ diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c index f8af88fabf24..cd81eb412840 100644 --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -120,7 +120,6 @@ bool acpi_enabled; static void init_hypercalls(void) { uint32_t eax, ebx, ecx, edx; - unsigned long i; char signature[13]; xen_extraversion_t extraversion; uint32_t base; @@ -142,8 +141,7 @@ static void init_hypercalls(void) /* Fill in hypercall transfer pages. */ cpuid(base + 2, &eax, &ebx, &ecx, &edx); - for ( i = 0; i < eax; i++ ) - wrmsr(ebx, HYPERCALL_PHYSICAL_ADDRESS + (i << 12) + i); + wrmsr(ebx, (unsigned long)hypercall_page); /* Print version information. */ cpuid(base + 1, &eax, &ebx, &ecx, &edx); @@ -324,9 +322,6 @@ int main(void) const struct bios_config *bios; const struct hvm_modlist_entry *bios_module; - /* Initialise hypercall stubs with RET, rendering them no-ops. */ - memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE); - printf("HVM Loader\n"); BUG_ON(hvm_start_info->magic != XEN_HVM_START_MAGIC_VALUE); diff --git a/tools/firmware/hvmloader/hvmloader.lds b/tools/firmware/hvmloader/hvmloader.lds index edb1a12dc305..559cd2107f6a 100644 --- a/tools/firmware/hvmloader/hvmloader.lds +++ b/tools/firmware/hvmloader/hvmloader.lds @@ -7,9 +7,9 @@ SECTIONS * NB: there's no need to use the AT keyword in order to set the LMA, by * default the linker will use VMA = LMA unless specified otherwise. */ - .text : { *(.text) *(.text.*) } + .text : { *(.text) *(.text.*)} .rodata : { *(.rodata) *(.rodata.*) } - .data : { *(.data) *(.data.*) } + .data : { *(.data) *(.data.*) *(.hcall) } .bss : { *(.bss) *(.bss.*) } _end = .; } diff --git a/tools/firmware/hvmloader/hypercall.h b/tools/firmware/hvmloader/hypercall.h index 5368c3072007..a55051c5350f 100644 --- a/tools/firmware/hvmloader/hypercall.h +++ b/tools/firmware/hvmloader/hypercall.h @@ -35,148 +35,91 @@ #include <xen/xen.h> #include "config.h" -#define hcall_addr(name) \ - ((unsigned long)HYPERCALL_PHYSICAL_ADDRESS + __HYPERVISOR_##name * 32) - -#define _hypercall0(type, name) \ -({ \ - long __res; \ - asm volatile ( \ - "call *%%eax" \ - : "=a" (__res) \ - : "0" (hcall_addr(name)) \ - : "memory" ); \ - (type)__res; \ -}) - -#define _hypercall1(type, name, a1) \ -({ \ - long __res, __ign1; \ - asm volatile ( \ - "call *%%eax" \ - : "=a" (__res), "=b" (__ign1) \ - : "0" (hcall_addr(name)), \ - "1" ((long)(a1)) \ - : "memory" ); \ - (type)__res; \ -}) - -#define _hypercall2(type, name, a1, a2) \ -({ \ - long __res, __ign1, __ign2; \ - asm volatile ( \ - "call *%%eax" \ - : "=a" (__res), "=b" (__ign1), "=c" (__ign2) \ - : "0" (hcall_addr(name)), \ - "1" ((long)(a1)), "2" ((long)(a2)) \ - : "memory" ); \ - (type)__res; \ -}) - -#define _hypercall3(type, name, a1, a2, a3) \ -({ \ - long __res, __ign1, __ign2, __ign3; \ - asm volatile ( \ - "call *%%eax" \ - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ - "=d" (__ign3) \ - : "0" (hcall_addr(name)), \ - "1" ((long)(a1)), "2" ((long)(a2)), \ - "3" ((long)(a3)) \ - : "memory" ); \ - (type)__res; \ -}) - -#define _hypercall4(type, name, a1, a2, a3, a4) \ -({ \ - long __res, __ign1, __ign2, __ign3, __ign4; \ - asm volatile ( \ - "call *%%eax" \ - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ - "=d" (__ign3), "=S" (__ign4) \ - : "0" (hcall_addr(name)), \ - "1" ((long)(a1)), "2" ((long)(a2)), \ - "3" ((long)(a3)), "4" ((long)(a4)) \ - : "memory" ); \ - (type)__res; \ -}) - -#define _hypercall5(type, name, a1, a2, a3, a4, a5) \ -({ \ - long __res, __ign1, __ign2, __ign3, __ign4, __ign5; \ - asm volatile ( \ - "call *%%eax" \ - : "=a" (__res), "=b" (__ign1), "=c" (__ign2), \ - "=d" (__ign3), "=S" (__ign4), "=D" (__ign5) \ - : "0" (hcall_addr(name)), \ - "1" ((long)(a1)), "2" ((long)(a2)), \ - "3" ((long)(a3)), "4" ((long)(a4)), \ - "5" ((long)(a5)) \ - : "memory" ); \ - (type)__res; \ -}) +extern const char hypercall_page[]; + +#define _hypercall2(type, hcall, a1, a2) \ + ({ \ + long res, _a1 = (long)(a1), _a2 = (long)(a2); \ + asm volatile ( \ + "call hypercall_page + %c[offset]" \ + : "=a" (res), "+b" (_a1), "+c" (_a2) \ + : [offset] "i" (hcall * 32) \ + : "memory" ); \ + (type)res; \ + }) + +#define _hypercall3(type, hcall, a1, a2, a3) \ + ({ \ + long res, _a1 = (long)(a1), _a2 = (long)(a2), _a3 = (long)(a3); \ + asm volatile ( \ + "call hypercall_page + %c[offset]" \ + : "=a" (res), "+b" (_a1), "+c" (_a2), "+d" (_a3) \ + : [offset] "i" (hcall * 32) \ + : "memory" ); \ + (type)res; \ + }) static inline int hypercall_sched_op( int cmd, void *arg) { - return _hypercall2(int, sched_op, cmd, arg); + return _hypercall2(int, __HYPERVISOR_sched_op, cmd, arg); } static inline int hypercall_memory_op( unsigned int cmd, void *arg) { - return _hypercall2(int, memory_op, cmd, arg); + return _hypercall2(int, __HYPERVISOR_memory_op, cmd, arg); } static inline int hypercall_multicall( void *call_list, int nr_calls) { - return _hypercall2(int, multicall, call_list, nr_calls); + return _hypercall2(int, __HYPERVISOR_multicall, call_list, nr_calls); } static inline int hypercall_event_channel_op( int cmd, void *arg) { - return _hypercall2(int, event_channel_op, cmd, arg); + return _hypercall2(int, __HYPERVISOR_event_channel_op, cmd, arg); } static inline int hypercall_xen_version( int cmd, void *arg) { - return _hypercall2(int, xen_version, cmd, arg); + return _hypercall2(int, __HYPERVISOR_xen_version, cmd, arg); } static inline int hypercall_console_io( int cmd, int count, char *str) { - return _hypercall3(int, console_io, cmd, count, str); + return _hypercall3(int, __HYPERVISOR_console_io, cmd, count, str); } static inline int hypercall_vm_assist( unsigned int cmd, unsigned int type) { - return _hypercall2(int, vm_assist, cmd, type); + return _hypercall2(int, __HYPERVISOR_vm_assist, cmd, type); } static inline int hypercall_vcpu_op( int cmd, int vcpuid, void *extra_args) { - return _hypercall3(int, vcpu_op, cmd, vcpuid, extra_args); + return _hypercall3(int, __HYPERVISOR_vcpu_op, cmd, vcpuid, extra_args); } static inline int hypercall_hvm_op( int cmd, void *arg) { - return _hypercall2(int, hvm_op, cmd, arg); + return _hypercall2(int, __HYPERVISOR_hvm_op, cmd, arg); } #endif /* __HVMLOADER_HYPERCALL_H__ */ diff --git a/tools/firmware/hvmloader/hypercall_page.S b/tools/firmware/hvmloader/hypercall_page.S new file mode 100644 index 000000000000..75428591d19a --- /dev/null +++ b/tools/firmware/hvmloader/hypercall_page.S @@ -0,0 +1,67 @@ +#include <xen/xen.h> + + .section ".hcall", "aw" + .align 4096 + + .globl hypercall_page +hypercall_page: + /* Poisoned with `ret` for safety before hypercalls are set up. */ + .fill 4096, 1, 0xc3 + .type hypercall_page, STT_OBJECT + .size hypercall_page, 4096 + +#define DECLARE_HYPERCALL(name) \ + .globl HYPERCALL_ ## name; \ + .type HYPERCALL_ ## name, STT_FUNC; \ + .size HYPERCALL_ ## name, 32; \ + .set HYPERCALL_ ## name, hypercall_page + __HYPERVISOR_ ## name * 32 + +DECLARE_HYPERCALL(set_trap_table) +DECLARE_HYPERCALL(mmu_update) +DECLARE_HYPERCALL(set_gdt) +DECLARE_HYPERCALL(stack_switch) +DECLARE_HYPERCALL(set_callbacks) +DECLARE_HYPERCALL(fpu_taskswitch) +DECLARE_HYPERCALL(sched_op_compat) +DECLARE_HYPERCALL(platform_op) +DECLARE_HYPERCALL(set_debugreg) +DECLARE_HYPERCALL(get_debugreg) +DECLARE_HYPERCALL(update_descriptor) +DECLARE_HYPERCALL(memory_op) +DECLARE_HYPERCALL(multicall) +DECLARE_HYPERCALL(update_va_mapping) +DECLARE_HYPERCALL(set_timer_op) +DECLARE_HYPERCALL(event_channel_op_compat) +DECLARE_HYPERCALL(xen_version) +DECLARE_HYPERCALL(console_io) +DECLARE_HYPERCALL(physdev_op_compat) +DECLARE_HYPERCALL(grant_table_op) +DECLARE_HYPERCALL(vm_assist) +DECLARE_HYPERCALL(update_va_mapping_otherdomain) +DECLARE_HYPERCALL(iret) +DECLARE_HYPERCALL(vcpu_op) +DECLARE_HYPERCALL(set_segment_base) +DECLARE_HYPERCALL(mmuext_op) +DECLARE_HYPERCALL(xsm_op) +DECLARE_HYPERCALL(nmi_op) +DECLARE_HYPERCALL(sched_op) +DECLARE_HYPERCALL(callback_op) +DECLARE_HYPERCALL(xenoprof_op) +DECLARE_HYPERCALL(event_channel_op) +DECLARE_HYPERCALL(physdev_op) +DECLARE_HYPERCALL(hvm_op) +DECLARE_HYPERCALL(sysctl) +DECLARE_HYPERCALL(domctl) +DECLARE_HYPERCALL(kexec_op) +DECLARE_HYPERCALL(tmem_op) +DECLARE_HYPERCALL(argo_op) +DECLARE_HYPERCALL(xenpmu_op) + +DECLARE_HYPERCALL(arch_0) +DECLARE_HYPERCALL(arch_1) +DECLARE_HYPERCALL(arch_2) +DECLARE_HYPERCALL(arch_3) +DECLARE_HYPERCALL(arch_4) +DECLARE_HYPERCALL(arch_5) +DECLARE_HYPERCALL(arch_6) +DECLARE_HYPERCALL(arch_7)
Right now, the hypercall page is at a hardcoded physical address, and making hypercalls involves indirect calls to compile-time constant addresses. e.g. HYPERCALL_memory_op is: mov $0x80180,%eax call *%eax Instead, import the hypercall infrastructure from XTF to have hypercall_page[] fully within the hvmloader image, and prepared at compile time rather than runtime. This uses direct calls, so HYPERCALL_memory_op now disassembles as: call 132180 <HYPERCALL_memory_op> which is faster and clearer. Remove the loop over multiple hypercall pages. It was long ago realised to be an unworkable design, and eax fixed in the ABI to 1. Pass -z noexecstack to LD to stop newer bintuils complaining about the absence of .note.GNU-stack. hvmloader is not a regular binary, and in fact its stack is always executable owing to operating in unpaged mode. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> There doesn't appear to be any sensible AFLAGS infrastructure to set -D__ASSEMBLY__. Opecoding it once seemed like the least bad option. --- tools/firmware/hvmloader/Makefile | 3 + tools/firmware/hvmloader/config.h | 1 - tools/firmware/hvmloader/hvmloader.c | 7 +- tools/firmware/hvmloader/hvmloader.lds | 4 +- tools/firmware/hvmloader/hypercall.h | 121 ++++++---------------- tools/firmware/hvmloader/hypercall_page.S | 67 ++++++++++++ 6 files changed, 105 insertions(+), 98 deletions(-) create mode 100644 tools/firmware/hvmloader/hypercall_page.S