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(-)

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)
-- 
2.39.5

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