| Message ID | 20250403175744.1538469-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/amd: Convert wrmsr_amd_safe() to use asm goto() | expand |
On 03.04.2025 19:57, Andrew Cooper wrote: > Bloat-o-meter reports: > > add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-29 (-29) > Function old new delta > _probe_mask_msr 99 94 -5 > init_amd 2418 2394 -24 > > but that's because .fixup doesn't contain sized/typed symbols. This also > drops two "mov -EFAULT, %reg; jmp ...;" sequences too. The net saving is -50. > > wrmsr_amd_safe()'s return value is only checked against 0 (if at all), and > because of this, the compiler can now avoid manifesting 0/-EFAULT entirely, > and the %[fault] label simply lands on the right basic block. > > Convert to Xen style while rewriting. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > > Slightly RFC. We use -EIO elsewhere for this purpose, and nothing in this > logic cares. {rd,wr}msr_safe() both use -EFAULT. What's "elsewhere" here? > I was pleasently surprised by the manifestation of -EFAULT going away > entirely. I fear I don't understand this, given the -EFAULT is still there in the new code. Irrespective of these remarks: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 04/04/2025 8:48 am, Jan Beulich wrote: > On 03.04.2025 19:57, Andrew Cooper wrote: >> Bloat-o-meter reports: >> >> add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-29 (-29) >> Function old new delta >> _probe_mask_msr 99 94 -5 >> init_amd 2418 2394 -24 >> >> but that's because .fixup doesn't contain sized/typed symbols. This also >> drops two "mov -EFAULT, %reg; jmp ...;" sequences too. The net saving is -50. >> >> wrmsr_amd_safe()'s return value is only checked against 0 (if at all), and >> because of this, the compiler can now avoid manifesting 0/-EFAULT entirely, >> and the %[fault] label simply lands on the right basic block. >> >> Convert to Xen style while rewriting. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> >> Slightly RFC. We use -EIO elsewhere for this purpose, and nothing in this >> logic cares. > {rd,wr}msr_safe() both use -EFAULT. What's "elsewhere" here? Oh, that would be Linux. Sorry, too much time spent staring at the same logic in different codebases. > >> I was pleasently surprised by the manifestation of -EFAULT going away >> entirely. > I fear I don't understand this, given the -EFAULT is still there in the > new code. I meant about what the optimiser can do. It hadn't occurred to me that that was a valid transformation. > > Irrespective of these remarks: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. ~Andrew
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index 597b0f073d55..ce4e1df71064 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -74,21 +74,19 @@ static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo, } static inline int wrmsr_amd_safe(unsigned int msr, unsigned int lo, - unsigned int hi) + unsigned int hi) { - int err; + asm goto ( "1: wrmsr\n\t" + _ASM_EXTABLE(1b, %l[fault]) + : + : "c" (msr), "a" (lo), "d" (hi), "D" (0x9c5a203a) + : + : fault ); - asm volatile("1: wrmsr\n2:\n" - ".section .fixup,\"ax\"\n" - "3: movl %6,%0\n" - " jmp 2b\n" - ".previous\n" - _ASM_EXTABLE(1b, 3b) - : "=r" (err) - : "c" (msr), "a" (lo), "d" (hi), "D" (0x9c5a203a), - "0" (0), "i" (-EFAULT)); + return 0; - return err; + fault: + return -EFAULT; } static void wrmsr_amd(unsigned int msr, uint64_t val)
Bloat-o-meter reports: add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-29 (-29) Function old new delta _probe_mask_msr 99 94 -5 init_amd 2418 2394 -24 but that's because .fixup doesn't contain sized/typed symbols. This also drops two "mov -EFAULT, %reg; jmp ...;" sequences too. The net saving is -50. wrmsr_amd_safe()'s return value is only checked against 0 (if at all), and because of this, the compiler can now avoid manifesting 0/-EFAULT entirely, and the %[fault] label simply lands on the right basic block. Convert to Xen style while rewriting. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> Slightly RFC. We use -EIO elsewhere for this purpose, and nothing in this logic cares. I was pleasently surprised by the manifestation of -EFAULT going away entirely. I expect converting wrmsr_safe() is going to be far greater win. Converting rdmsr_amd_safe() (and friends) is going to be much harder, because they have outputs, which GCCs before this year cannot do safely (there was a serious bug, found and backported). I think I'm going to port Linux's CC_HAS_ASM_GOTO_OUTPUT infrastructure, so we can at least provide a better wrapper on new-enough toolchains. --- xen/arch/x86/cpu/amd.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)