kvm-unittest: fix build with gcc 4.3.X and older

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

To fix, let's just use %eax %al etc.

Quoted 8 lines · Michael S. Tsirkin

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

Is it worth to support such ancient compiler? Nobody complained till now.

BTW with your patch I still cannot compile with 4.2:

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

Quoted 2 lines

To fix, let's just use %eax %al etc.

Only %d0 does not work and dropping "d" fixes it since compiler can figure out correct register from variable size. The patch bellow fixes compilation for 4.2.

Quoted 11 lines · Gleb Natapov

Quoted 8 lines · Michael S. Tsirkin

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

Is it worth to support such ancient compiler? Nobody complained till now.

Well it's not as widely used as kvm itself yet :)
The patch seems simple enough though.

Quoted 4 lines

BTW with your patch I still cannot compile with 4.2:

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

OK that's easy to fix.

Quoted 2 lines

Quoted 2 lines

To fix, let's just use %eax %al etc.

Quoted 3 lines

Only %d0 does not work and dropping "d" fixes it since compiler can figure out correct register from variable size. The patch bellow fixes compilation for 4.2.

It does produce warnings with -Wall though:
Assembler messages:
Warning: using %ax' instead of %eax' due to `w' suffix
Warning: using %al' instead of %eax' due to `b' suffix

Quoted 29 lines · Michael S. Tsirkin

Quoted 11 lines · Gleb Natapov

Quoted 8 lines · Michael S. Tsirkin

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

Is it worth to support such ancient compiler? Nobody complained till now.

Well it's not as widely used as kvm itself yet :)
The patch seems simple enough though.

Quoted 4 lines

BTW with your patch I still cannot compile with 4.2:

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

OK that's easy to fix.

Quoted 5 lines

Quoted 2 lines

To fix, let's just use %eax %al etc.

Only %d0 does not work and dropping "d" fixes it since compiler can figure out correct register from variable size. The patch bellow fixes compilation for 4.2.

Quoted 5 lines

It does produce warnings with -Wall though:
Assembler messages:
Warning: using %ax' instead of %eax' due to `w' suffix
Warning: using %al' instead of %eax' due to `b' suffix

Not for me. No warning with 4.7.3 and 4.2. .s file produced by gcc shows correct assembly with my patch.

--
Gleb.

Quoted 35 lines · Gleb Natapov

Quoted 34 lines · Michael S. Tsirkin

Quoted 11 lines · Gleb Natapov

Quoted 8 lines · Michael S. Tsirkin

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

Is it worth to support such ancient compiler? Nobody complained till now.

Well it's not as widely used as kvm itself yet :)
The patch seems simple enough though.

Quoted 4 lines

BTW with your patch I still cannot compile with 4.2:

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

OK that's easy to fix.

Quoted 5 lines

Quoted 2 lines

To fix, let's just use %eax %al etc.

Only %d0 does not work and dropping "d" fixes it since compiler can figure out correct register from variable size. The patch bellow fixes compilation for 4.2.

It does produce warnings with -Wall though:
Assembler messages:
Warning: using %ax' instead of %eax' due to `w' suffix
Warning: using %al' instead of %eax' due to `b' suffix

Quoted 2 lines

Not for me. No warning with 4.7.3 and 4.2. .s file produced by gcc shows correct assembly with my patch.

4.3 doesn't.

We have the "a" constraint there anyway - so what's the issue with just writing %eax etc? I think it's safer than relying on compiler to do the right thing.

Quoted 36 lines · Michael S. Tsirkin

Quoted 35 lines · Gleb Natapov

Quoted 34 lines · Michael S. Tsirkin

Quoted 11 lines · Gleb Natapov

Quoted 8 lines · Michael S. Tsirkin

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

Is it worth to support such ancient compiler? Nobody complained till now.

Well it's not as widely used as kvm itself yet :)
The patch seems simple enough though.

Quoted 4 lines

BTW with your patch I still cannot compile with 4.2:

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

OK that's easy to fix.

Quoted 5 lines

Quoted 2 lines

To fix, let's just use %eax %al etc.

Only %d0 does not work and dropping "d" fixes it since compiler can figure out correct register from variable size. The patch bellow fixes compilation for 4.2.

It does produce warnings with -Wall though:
Assembler messages:
Warning: using %ax' instead of %eax' due to `w' suffix
Warning: using %al' instead of %eax' due to `b' suffix

Quoted 5 lines

Quoted 2 lines

Not for me. No warning with 4.7.3 and 4.2. .s file produced by gcc shows correct assembly with my patch.

4.3 doesn't.

I pretty much doubt that 4.2 and 4.7 produce correct assembly and 4.3 does not. Which file is it? Send me .s file produced by it.

Quoted 4 lines

We have the "a" constraint there anyway - so what's the issue with just writing %eax etc? I think it's safer than relying on compiler to do the right thing.

It just papers over the problem. Compiler should either complain that it does not know what %w0 or complain that variable length does not match assembly

or use correct register.

--
Gleb.

Quoted 40 lines · Gleb Natapov

Quoted 39 lines · Michael S. Tsirkin

Quoted 37 lines · Gleb Natapov

Quoted 25 lines · Michael S. Tsirkin

Quoted 11 lines · Gleb Natapov

Quoted 8 lines · Michael S. Tsirkin

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

Is it worth to support such ancient compiler? Nobody complained till now.

Well it's not as widely used as kvm itself yet :)
The patch seems simple enough though.

Quoted 4 lines

BTW with your patch I still cannot compile with 4.2:

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

OK that's easy to fix.

Quoted 2 lines

Quoted 2 lines

To fix, let's just use %eax %al etc.

Quoted 9 lines

Quoted 3 lines

Only %d0 does not work and dropping "d" fixes it since compiler can figure out correct register from variable size. The patch bellow fixes compilation for 4.2.

It does produce warnings with -Wall though:
Assembler messages:
Warning: using %ax' instead of %eax' due to `w' suffix
Warning: using %al' instead of %eax' due to `b' suffix

Not for me. No warning with 4.7.3 and 4.2. .s file produced by gcc shows correct assembly with my patch.

Quoted 4 lines

Quoted 2 lines

4.3 doesn't.

I pretty much doubt that 4.2 and 4.7 produce correct assembly and 4.3 does not. Which file is it? Send me .s file produced by it.

Tried to get it but problem went away after I reset and re-applied your patch. I think I had some other change in there that
caused it, and blamed your patch incorrectly. Sorry.

Quoted 7 lines

Quoted 4 lines

We have the "a" constraint there anyway - so what's the issue with just writing %eax etc? I think it's safer than relying on compiler to do the right thing.

It just papers over the problem. Compiler should either complain that it does not know what %w0 or complain that variable length does not match assembly

Of course it can't. Compiler does not parse assembly at all:
these are just constant strings as far as it's concerned.

Quoted 1 line

or use correct register.

Actually it does: it seems to correctly deduce
size from variable type.

So your patch looks good to me, please apply.

Quoted 7 lines

Quoted 3 lines

It just papers over the problem. Compiler should either complain that it does not know what %w0 or complain that variable length does not match assembly

Of course it can't. Compiler does not parse assembly at all:
these are just constant strings as far as it's concerned.

Compiler does not pars assembly itself but it parses things like %w0 to know what assembly to emit. That is why it complained about %d0 in the first place.

Quoted 7 lines · Gleb Natapov

Quoted 7 lines

Quoted 3 lines

It just papers over the problem. Compiler should either complain that it does not know what %w0 or complain that variable length does not match assembly

Of course it can't. Compiler does not parse assembly at all:
these are just constant strings as far as it's concerned.

Quoted 3 lines

Compiler does not pars assembly itself but it parses things like %w0 to know what assembly to emit. That is why it complained about %d0 in the first place.

Right. I meant that with this: "outl %0" it has no chance to figure out that outl needs eax. It has to go by variable type.

Quoted 8 lines

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

Is it worth to support such ancient compiler? Nobody complained till now. BTW with your patch I still cannot compile with 4.2:

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

Quoted 2 lines

To fix, let's just use %eax %al etc.

Only %d0 does not work and dropping "d" fixes it since compiler can figure out correct register from variable size. The patch bellow fixes compilation for 4.2.

lib/x86/pci.c +0 -1

index f95cd88..231668a 100644
@@ -3,13 +3,13 @@
 
 static void outl(unsigned short port, unsigned val)
 {
-    asm volatile("outl %d0, %w1" : : "a"(val), "Nd"(port));

x86/vmexit.c +0 -1

 static unsigned int inb(unsigned short port)
--

Gleb.

Il 17/10/2013 08:27, Gleb Natapov ha scritto:

Quoted 30 lines · Michael S. Tsirkin

Quoted 8 lines

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

Is it worth to support such ancient compiler? Nobody complained till now. BTW with your patch I still cannot compile with 4.2:

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

Quoted 2 lines

To fix, let's just use %eax %al etc.

Only %d0 does not work and dropping "d" fixes it since compiler can figure out correct register from variable size. The patch bellow fixes compilation for 4.2.

lib/x86/pci.c +0 -1

index f95cd88..231668a 100644
@@ -3,13 +3,13 @@
 
 static void outl(unsigned short port, unsigned val)
 {
-    asm volatile("outl %d0, %w1" : : "a"(val), "Nd"(port));

lib/x86/pci.c line 14

Quoted 10 lines

lib/x86/pci.c +2 -1

+    asm volatile("outl %0, %w1" : : "a"(val), "Nd"(port));
 }
 
 static unsigned inl(unsigned short port)
 {
     unsigned data;
-    asm volatile("inl %w1, %d0" : "=a"(data) : "Nd"(port));
+    asm volatile("inl %w1, %0" : "=a"(data) : "Nd"(port));
     return data;
 }

This is okay.

x86/s3.c static inline int rtc_in(u8 reg) · line 148

Quoted 13 lines

lib/x86/pci.c +0 -0

 static uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)

x86/s3.c +1 -1

index 71d3ff9..d568aa7 100644
@@ -143,14 +143,14 @@ static inline int rtc_in(u8 reg)
 {
     u8 x = reg;
     asm volatile("outb %b1, $0x70; inb $0x71, %b0"
-		 : "+a"(x) : "0"(x));
+		 : "=a"(x) : "0"(x));
     return x;
 }

This should be wrong. GCC should complain that the same operand is used for both input and output but has an "=" constraint.

x86/s3.c static inline int rtc_in(u8 reg) · line 154

Quoted 6 lines

x86/s3.c +1 -1

 static inline void rtc_out(u8 reg, u8 val)
 {
     asm volatile("outb %b1, $0x70; mov %b2, %b1; outb %b1, $0x71"
-		 : "+a"(reg) : "0"(reg), "ri"(val));
+		 : "=a"(reg) : "0"(reg), "ri"(val));
 }

Same here.

But I'm not sure what is the error message for older GCC for s3.c, as I wrote in reply to Michael.

x86/vmexit.c static void outw(unsigned short port, unsigned val) · line 30

Quoted 12 lines

x86/s3.c +0 -0

 extern char resume_start, resume_end;

x86/vmexit.c +1 -1

index 3b945de..7e9af15 100644
@@ -26,7 +26,7 @@ static void outw(unsigned short port, unsigned val)
 
 static void outl(unsigned short port, unsigned val)
 {
-    asm volatile("outl %d0, %w1" : : "a"(val), "Nd"(port));
+    asm volatile("outl %0, %w1" : : "a"(val), "Nd"(port));
 }

Okay.

Paolo

Quoted 4 lines

x86/vmexit.c +0 -1

 static unsigned int inb(unsigned short port)
--

Gleb.

Quoted 21 lines

Quoted 21 lines

Quoted 8 lines

x86/s3.c static inline int rtc_in(u8 reg) · line 148

Quoted 8 lines

x86/s3.c +1 -1

@@ -143,14 +143,14 @@ static inline int rtc_in(u8 reg)
 {
     u8 x = reg;
     asm volatile("outb %b1, $0x70; inb $0x71, %b0"
-		 : "+a"(x) : "0"(x));
+		 : "=a"(x) : "0"(x));
     return x;
 }

This should be wrong. GCC should complain that the same operand is used for both input and output but has an "=" constraint.

Quoted 6 lines

x86/s3.c static inline int rtc_in(u8 reg) · line 154

Quoted 6 lines

x86/s3.c +1 -1

 static inline void rtc_out(u8 reg, u8 val)
 {
     asm volatile("outb %b1, $0x70; mov %b2, %b1; outb %b1, $0x71"
-		 : "+a"(reg) : "0"(reg), "ri"(val));
+		 : "=a"(reg) : "0"(reg), "ri"(val));
 }

Same here.

Quoted 3 lines

Quoted 3 lines

But I'm not sure what is the error message for older GCC for s3.c, as I wrote in reply to Michael.

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

And I am puzzled by this too.

Il 17/10/2013 12:58, Gleb Natapov ha scritto:

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 8 lines

x86/s3.c static inline int rtc_in(u8 reg) · line 148

Quoted 8 lines

x86/s3.c +1 -1

@@ -143,14 +143,14 @@ static inline int rtc_in(u8 reg)
 {
     u8 x = reg;
     asm volatile("outb %b1, $0x70; inb $0x71, %b0"
-		 : "+a"(x) : "0"(x));
+		 : "=a"(x) : "0"(x));
     return x;
 }

This should be wrong. GCC should complain that the same operand is used for both input and output but has an "=" constraint.

Quoted 6 lines

x86/s3.c static inline int rtc_in(u8 reg) · line 154

Quoted 6 lines

x86/s3.c +1 -1

 static inline void rtc_out(u8 reg, u8 val)
 {
     asm volatile("outb %b1, $0x70; mov %b2, %b1; outb %b1, $0x71"
-		 : "+a"(reg) : "0"(reg), "ri"(val));
+		 : "=a"(reg) : "0"(reg), "ri"(val));
 }

Same here.

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

But I'm not sure what is the error message for older GCC for s3.c, as I wrote in reply to Michael.

x86/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

And I am puzzled by this too.

Hmm, looks like my version is the incorrect one and, if you use "+a" you need not use the matching input constraint. So it's either your version, or one that removes the "0" altogether.

Thus,

Reviewed-by Paolo Bonzini <[email protected]>

Paolo

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

To fix, let's just use %eax %al etc.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 lib/x86/pci.c | 4 ++--
 x86/s3.c      | 4 ++--
 x86/vmexit.c  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

lib/x86/pci.c +2 -2

index f95cd88..08f7ebf 100644
@@ -3,13 +3,13 @@
 
 static void outl(unsigned short port, unsigned val)
 {
-    asm volatile("outl %d0, %w1" : : "a"(val), "Nd"(port));
+    asm volatile("outl %%eax, %w1" : : "a"(val), "Nd"(port));
 }
 
 static unsigned inl(unsigned short port)
 {
     unsigned data;
-    asm volatile("inl %w1, %d0" : "=a"(data) : "Nd"(port));
+    asm volatile("inl %w1, %%eax" : "=a"(data) : "Nd"(port));
     return data;
 }
 static uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)

x86/s3.c +0 -0

index 1feb452..eeffa17 100644

x86/s3.c +0 -0

 }
 
 extern char resume_start, resume_end;

x86/vmexit.c +1 -1

index 3b945de..29bc582 100644
@@ -26,7 +26,7 @@ static void outw(unsigned short port, unsigned val)
 
 static void outl(unsigned short port, unsigned val)
 {
-    asm volatile("outl %d0, %w1" : : "a"(val), "Nd"(port));
+    asm volatile("outl %%eax, %w1" : : "a"(val), "Nd"(port));
 }
 
 static unsigned int inb(unsigned short port)

Il 16/10/2013 21:46, Michael S. Tsirkin ha scritto:

Quoted 4 lines

Old GCC didn't let you reference variable by number if it is listed with a specific register constraint, on the assumption you can just use the register name explicitly.

Tell us the truth, you made this up. :) Who doesn't do that for invalid asm error messages...

Quoted 4 lines

Build fails with errors like this:
a.c:6: error: invalid 'asm': invalid operand code 'd'

To fix, let's just use %eax %al etc.

The problem is that %d0 is wrong. The "d" modifier is for "print duplicated register operand for AVX instruction" (whatever that means) according to GCC source code (gcc/config/i386/i386.md).

You need to use %k0 according to the same file.

Quoted 33 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 lib/x86/pci.c | 4 ++--
 x86/s3.c      | 4 ++--
 x86/vmexit.c  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

lib/x86/pci.c +2 -2

index f95cd88..08f7ebf 100644
@@ -3,13 +3,13 @@
 
 static void outl(unsigned short port, unsigned val)
 {
-    asm volatile("outl %d0, %w1" : : "a"(val), "Nd"(port));
+    asm volatile("outl %%eax, %w1" : : "a"(val), "Nd"(port));
 }
 
 static unsigned inl(unsigned short port)
 {
     unsigned data;
-    asm volatile("inl %w1, %d0" : "=a"(data) : "Nd"(port));
+    asm volatile("inl %w1, %%eax" : "=a"(data) : "Nd"(port));
     return data;
 }
 static uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)

x86/s3.c +0 -0

index 1feb452..eeffa17 100644
@@ -149,8 +149,8 @@ static inline int rtc_in(u8 reg)
 

x86/s3.c static inline int rtc_in(u8 reg) · line 153

Quoted 6 lines

x86/s3.c +2 -2

 static inline void rtc_out(u8 reg, u8 val)
 {
-    asm volatile("outb %b1, $0x70; mov %b2, %b1; outb %b1, $0x71"
-		 : "+a"(reg) : "0"(reg), "ri"(val));
+    asm volatile("outb %%al, $0x70; mov %b1, %%al; outb %%al, $0x71"
+		 : "+a"(reg) : "ri"(val));

Are you sure about this? The error message here is definitely not "invalid operand code 'b'".

That said, the patch is correct. It's only the commit message that is wrong.

I'm not sure about Gleb's patch. I'll reply to his message.

Paolo

Quoted 17 lines

x86/s3.c +0 -0

 }
 
 extern char resume_start, resume_end;

x86/vmexit.c +1 -1

index 3b945de..29bc582 100644
@@ -26,7 +26,7 @@ static void outw(unsigned short port, unsigned val)
 
 static void outl(unsigned short port, unsigned val)
 {
-    asm volatile("outl %d0, %w1" : : "a"(val), "Nd"(port));
+    asm volatile("outl %%eax, %w1" : : "a"(val), "Nd"(port));
 }
 
 static unsigned int inb(unsigned short port)