[Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[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'

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

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 lib/x86/pci.c | 4 ++--
 x86/s3.c      | 4 ++--
 x86/vmexit.c  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/x86/pci.c b/lib/x86/pci.c
index f95cd88..08f7ebf 100644
--- a/lib/x86/pci.c
+++ b/lib/x86/pci.c
@@ -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)
diff --git a/x86/s3.c b/x86/s3.c
index 1feb452..eeffa17 100644
--- a/x86/s3.c
+++ b/x86/s3.c
@@ -149,8 +149,8 @@ static inline int rtc_in(u8 reg)
 
 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));
 }
 
 extern char resume_start, resume_end;
diff --git a/x86/vmexit.c b/x86/vmexit.c
index 3b945de..29bc582 100644
--- a/x86/vmexit.c
+++ b/x86/vmexit.c
@@ -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)
-- 
MST

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Gleb Natapov]

On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
> 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'

> 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.
 
diff --git a/lib/x86/pci.c b/lib/x86/pci.c
index f95cd88..231668a 100644
--- a/lib/x86/pci.c
+++ b/lib/x86/pci.c
@@ -3,13 +3,13 @@
 
 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));
 }
 
 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;
 }
 static uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)
diff --git a/x86/s3.c b/x86/s3.c
index 71d3ff9..d568aa7 100644
--- a/x86/s3.c
+++ b/x86/s3.c
@@ -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;
 }
 
 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));
 }
 
 extern char resume_start, resume_end;
diff --git a/x86/vmexit.c b/x86/vmexit.c
index 3b945de..7e9af15 100644
--- a/x86/vmexit.c
+++ b/x86/vmexit.c
@@ -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));
 }
 
 static unsigned int inb(unsigned short port)
--
			Gleb.

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 09:27:51AM +0300, Gleb Natapov wrote:
> On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
> > 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.

> 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.

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

So it's not the compiler, the compiler generated wrong
code with outb %eax. But assembler saw outb and %eax which
are inconsistent and decided that outb should take precedence.

I'm not sure it's wize to rely on this behaviour though.
How's this? This should fix 4.2 for you too.

Maybe we should tweak outb/outw as well to keep it
all consistent. I can do this - what do you think?


diff --git a/lib/x86/pci.c b/lib/x86/pci.c
index f95cd88..08f7ebf 100644
--- a/lib/x86/pci.c
+++ b/lib/x86/pci.c
@@ -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)
diff --git a/x86/s3.c b/x86/s3.c
index 1feb452..bfa4206 100644
--- a/x86/s3.c
+++ b/x86/s3.c
@@ -142,15 +142,15 @@ u32* find_resume_vector_addr(void)
 static inline int rtc_in(u8 reg)
 {
     u8 x = reg;
-    asm volatile("outb %b1, $0x70; inb $0x71, %b0"
-		 : "+a"(x) : "0"(x));
+    asm volatile("outb %%al, $0x70; inb $0x71, %%al"
+		 : "+a"(x));
     return x;
 }
 
 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));
 }
 
 extern char resume_start, resume_end;
diff --git a/x86/vmexit.c b/x86/vmexit.c
index 3b945de..29bc582 100644
--- a/x86/vmexit.c
+++ b/x86/vmexit.c
@@ -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)


-- 
MST

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Gleb Natapov]

On Thu, Oct 17, 2013 at 11:12:31AM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 17, 2013 at 09:27:51AM +0300, Gleb Natapov wrote:
> > On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
> > > 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.
> 
> > 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.
> 
> > > 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
> 
Not for me. No warning with 4.7.3 and 4.2. .s file produced by gcc shows
correct assembly with my patch.

--
			Gleb.

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 11:20:27AM +0300, Gleb Natapov wrote:
> On Thu, Oct 17, 2013 at 11:12:31AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 17, 2013 at 09:27:51AM +0300, Gleb Natapov wrote:
> > > On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
> > > > 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.
> > 
> > > 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.
> > 
> > > > 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
> > 
> 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.

> --
> 			Gleb.

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Gleb Natapov]

On Thu, Oct 17, 2013 at 11:27:37AM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 17, 2013 at 11:20:27AM +0300, Gleb Natapov wrote:
> > On Thu, Oct 17, 2013 at 11:12:31AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Oct 17, 2013 at 09:27:51AM +0300, Gleb Natapov wrote:
> > > > On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
> > > > > 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.
> > > 
> > > > 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.
> > > 
> > > > > 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
> > > 
> > 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.

> 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.

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 11:34:41AM +0300, Gleb Natapov wrote:
> On Thu, Oct 17, 2013 at 11:27:37AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 17, 2013 at 11:20:27AM +0300, Gleb Natapov wrote:
> > > On Thu, Oct 17, 2013 at 11:12:31AM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Oct 17, 2013 at 09:27:51AM +0300, Gleb Natapov wrote:
> > > > > On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
> > > > > > 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.
> > > > 
> > > > > 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.
> > > > 
> > > > > > 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
> > > > 
> > > 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.

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.

> > 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.

> 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.


> --
> 			Gleb.

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Gleb Natapov]

On Thu, Oct 17, 2013 at 12:28:58PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 17, 2013 at 11:34:41AM +0300, Gleb Natapov wrote:
> > On Thu, Oct 17, 2013 at 11:27:37AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Oct 17, 2013 at 11:20:27AM +0300, Gleb Natapov wrote:
> > > > On Thu, Oct 17, 2013 at 11:12:31AM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Oct 17, 2013 at 09:27:51AM +0300, Gleb Natapov wrote:
> > > > > > On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
> > > > > > > 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.
> > > > > 
> > > > > > 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.
> > > > > 
> > > > > > > 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
> > > > > 
> > > > 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.
> 
> 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.
> 
NP, that makes more sense.

> > > 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.
> 
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.

> > 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.
> 
> 
Will do.

--
			Gleb.

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 12:33:39PM +0300, Gleb Natapov wrote:
> > > 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.

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. 

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Gleb Natapov]

On Thu, Oct 17, 2013 at 12:44:46PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 17, 2013 at 12:33:39PM +0300, Gleb Natapov wrote:
> > > > 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.
> 
> 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. 
Yes.

--
			Gleb.

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Paolo Bonzini]

Il 17/10/2013 08:27, Gleb Natapov ha scritto:
> On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
>> 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'
> 
>> 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.
>  
> diff --git a/lib/x86/pci.c b/lib/x86/pci.c
> index f95cd88..231668a 100644
> --- a/lib/x86/pci.c
> +++ b/lib/x86/pci.c
> @@ -3,13 +3,13 @@
>  
>  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));
>  }
>  
>  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.

>  static uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)
> diff --git a/x86/s3.c b/x86/s3.c
> index 71d3ff9..d568aa7 100644
> --- a/x86/s3.c
> +++ b/x86/s3.c
> @@ -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.

>  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.

>  extern char resume_start, resume_end;
> diff --git a/x86/vmexit.c b/x86/vmexit.c
> index 3b945de..7e9af15 100644
> --- a/x86/vmexit.c
> +++ b/x86/vmexit.c
> @@ -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

>  static unsigned int inb(unsigned short port)
> --
> 			Gleb.
> 

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Gleb Natapov]

On Thu, Oct 17, 2013 at 12:55:16PM +0200, Paolo Bonzini wrote:
> Il 17/10/2013 08:27, Gleb Natapov ha scritto:
> > On Wed, Oct 16, 2013 at 10:46:53PM +0300, Michael S. Tsirkin wrote:
> >> 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'
> > 
> >> 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.
> >  
> > diff --git a/lib/x86/pci.c b/lib/x86/pci.c
> > index f95cd88..231668a 100644
> > --- a/lib/x86/pci.c
> > +++ b/lib/x86/pci.c
> > @@ -3,13 +3,13 @@
> >  
> >  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));
> >  }
> >  
> >  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.
> 
> >  static uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)
> > diff --git a/x86/s3.c b/x86/s3.c
> > index 71d3ff9..d568aa7 100644
> > --- a/x86/s3.c
> > +++ b/x86/s3.c
> > @@ -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.
> 
> >  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/s3.c: In function 'main':
x86/s3.c:145: error: inconsistent operand constraints in an 'asm'

And I am puzzled by this too.

> >  extern char resume_start, resume_end;
> > diff --git a/x86/vmexit.c b/x86/vmexit.c
> > index 3b945de..7e9af15 100644
> > --- a/x86/vmexit.c
> > +++ b/x86/vmexit.c
> > @@ -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
> 
> >  static unsigned int inb(unsigned short port)
> > --
> > 			Gleb.
> > 

--
			Gleb.

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Paolo Bonzini]

Il 17/10/2013 12:58, Gleb Natapov ha scritto:
>>> > > @@ -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.
>> > 
>>> > >  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/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 <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 01:02:17PM +0200, Paolo Bonzini wrote:
> Il 17/10/2013 12:58, Gleb Natapov ha scritto:
> >>> > > @@ -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.
> >> > 
> >>> > >  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/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 <pbonzini@redhat.com>

Yea, you can add Reviewed-by: Michael S. Tsirkin <mst@redhat.com> too.

But maybe we can add %k constraints like Paolo said?
Being extra careful ...


> Paolo

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Paolo Bonzini]

Il 16/10/2013 21:46, Michael S. Tsirkin ha scritto:
> 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...

> 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.

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  lib/x86/pci.c | 4 ++--
>  x86/s3.c      | 4 ++--
>  x86/vmexit.c  | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/x86/pci.c b/lib/x86/pci.c
> index f95cd88..08f7ebf 100644
> --- a/lib/x86/pci.c
> +++ b/lib/x86/pci.c
> @@ -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)
> diff --git a/x86/s3.c b/x86/s3.c
> index 1feb452..eeffa17 100644
> --- a/x86/s3.c
> +++ b/x86/s3.c
> @@ -149,8 +149,8 @@ static inline int rtc_in(u8 reg)
>  
>  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

>  }
>  
>  extern char resume_start, resume_end;
> diff --git a/x86/vmexit.c b/x86/vmexit.c
> index 3b945de..29bc582 100644
> --- a/x86/vmexit.c
> +++ b/x86/vmexit.c
> @@ -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)
> 

Re: [Qemu-devel] [PATCH] kvm-unittest: fix build with gcc 4.3.X and older

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 12:53:30PM +0200, Paolo Bonzini wrote:
> Il 16/10/2013 21:46, Michael S. Tsirkin ha scritto:
> > 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...

I guessed at the reason, yes.

> > 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.

Aha!

> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  lib/x86/pci.c | 4 ++--
> >  x86/s3.c      | 4 ++--
> >  x86/vmexit.c  | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/x86/pci.c b/lib/x86/pci.c
> > index f95cd88..08f7ebf 100644
> > --- a/lib/x86/pci.c
> > +++ b/lib/x86/pci.c
> > @@ -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)
> > diff --git a/x86/s3.c b/x86/s3.c
> > index 1feb452..eeffa17 100644
> > --- a/x86/s3.c
> > +++ b/x86/s3.c
> > @@ -149,8 +149,8 @@ static inline int rtc_in(u8 reg)
> >  
> >  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'".

This produced a different error about conflict in constrains I think,
Gleb saw it too.

> 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
> 
> >  }
> >  
> >  extern char resume_start, resume_end;
> > diff --git a/x86/vmexit.c b/x86/vmexit.c
> > index 3b945de..29bc582 100644
> > --- a/x86/vmexit.c
> > +++ b/x86/vmexit.c
> > @@ -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)
> >