diff mbox

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

Message ID 20131017062751.GK15657@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gleb Natapov Oct. 17, 2013, 6:27 a.m. UTC
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.
 
--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Gleb Natapov Oct. 17, 2013, 8:20 a.m. UTC | #1
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin Oct. 17, 2013, 8:27 a.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gleb Natapov Oct. 17, 2013, 8:34 a.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin Oct. 17, 2013, 9:28 a.m. UTC | #4
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gleb Natapov Oct. 17, 2013, 9:33 a.m. UTC | #5
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gleb Natapov Oct. 17, 2013, 9:43 a.m. UTC | #6
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin Oct. 17, 2013, 9:44 a.m. UTC | #7
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. 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Oct. 17, 2013, 10:55 a.m. UTC | #8
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.
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gleb Natapov Oct. 17, 2013, 10:58 a.m. UTC | #9
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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini Oct. 17, 2013, 11:02 a.m. UTC | #10
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin Oct. 17, 2013, 11:31 a.m. UTC | #11
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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)