| Message ID | 20131017062751.GK15657@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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.
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.
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.
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.
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. >
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.
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
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
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)