On Wednesday 26 July 2006 19:27, Jeremy Fitzhardinge wrote:> static inline void raw_local_irq_restore(unsigned long f) > { > __asm__ __volatile__(paravirt_alt("pushl %%ecx; pushl %%edx\n\t" > "pushl %1; call *%0\n\t" > "popl %1; popl %%edx; popl %%ecx", > PARAVIRT_RESTORE_FLAGS) > : : "m" (paravirt_ops.restore_fl), "a"(f) > : "memory"); > } > > Wouldn't adding ecx/edx (and maybe eax, where it has no other use) to > the asm's clobber list be sufficient?It's even better because we currently can't annotate explicit push/pop for the dwarf2 stack unwinder, and it would cause fallbacks to the old inexact unwinder. -Andi
In your sequences in asm/paravirt.h, you explicitly save the caller-save
regs:
static inline void raw_local_irq_restore(unsigned long f)
{
__asm__ __volatile__(paravirt_alt("pushl %%ecx; pushl %%edx\n\t"
"pushl %1; call *%0\n\t"
"popl %1; popl %%edx; popl %%ecx",
PARAVIRT_RESTORE_FLAGS)
: : "m" (paravirt_ops.restore_fl), "a"(f)
: "memory");
}
Wouldn't adding ecx/edx (and maybe eax, where it has no other use) to
the asm's clobber list be sufficient?
This also has the nice effect of making these registers are freely
available for inlined code to use as temps, and making the register
usage match the normal ABI calling convention. On the other hand, I
suppose, if the inlined code doesn't need the registers, it is a waste
to make gcc rearrange things.
Assuming always clobbering ecx and edx would cause too much gcc
reloading, it seems reasonable to me to say that any paravirt code
sequence can use eax as input, output or scratch, and (say) ecx is also
available for scratch. If it needs more (like a call to C function
would), then it needs to save anything else it wants to clobber.
J
Zachary Amsden wrote:> I would actually very much prefer not to have EAX, EDX, ECX clobbered > by these calls at all. There are four performance critical calls that > are interrupt related - and we should implement them entirely in > assembler, with the hope of inlining them someday. We go to some > effort not to clobber anything except for condition codes in our > implementation.OK. I was talking about all the calls in general, but the more performance-critical ones can definitely get special treatment.> If I understand correctly, the Xen implementation clobbers ESI? Why > is that?It needs a temp for address calculation, and it turns out that ESI is unused in entry.S (or at least at all the points it's needed). So there's nothing deeply architectural about the choice; we just need a scratch register here. ECX or EDX would work as well (though perhaps it would require more work in entry.S), if you can cope with clobbering a single register.> Here's our implementation of CLI/STI/PUSHF/POPF for reference.[...] Neat.> VMI_ENTRY(EnableInterrupts) > movb $(EFLAGS_IF >> 8), %ss:SHARED(interruptFlag)+1 > testb $(EFLAGS_IF >> 8), %ss:SHARED(interruptPending)+1 > jnz interruptsSlow > retSo ss:SHARED(foo) is per-CPU data? Or would you do something else in the SMP case? J
On Wed, 2006-07-26 at 10:27 -0700, Jeremy Fitzhardinge wrote:> In your sequences in asm/paravirt.h, you explicitly save the caller-save > regs: > > static inline void raw_local_irq_restore(unsigned long f) > { > __asm__ __volatile__(paravirt_alt("pushl %%ecx; pushl %%edx\n\t" > "pushl %1; call *%0\n\t" > "popl %1; popl %%edx; popl %%ecx", > PARAVIRT_RESTORE_FLAGS) > : : "m" (paravirt_ops.restore_fl), "a"(f) > : "memory"); > } > > Wouldn't adding ecx/edx (and maybe eax, where it has no other use) to > the asm's clobber list be sufficient?Absolutely. I didn't know how much room the replacements would need, so I chose this "no-clobber" approach which takes 12 bytes. If it turns out that is too much, we can change to a clobber. If it turns out to be too little, we'll need to noop pad and/or clobber. For the cases where these are used in asm directly (currently cli and sti replacement), it was simplest to save all regs to have to same non-clobber properties as the raw insns. Hope that clarifies my thinking, Rusty. -- Help! Save Australia from the worst of the DMCA: http://linux.org.au/law