[Qemu-devel] [PATCH 1/3] x86: fix warning without CONFIG_KVM

[Qemu-devel] [PATCH 1/3] x86: fix warning without CONFIG_KVM

[Mark McLoughlin]

Warning is:

 target-i386/helper.c: In function `cpu_x86_cpuid':
 target-i386/helper.c:1373: warning: implicit declaration of function `host_cpuid'

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 target-i386/helper.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index 7ddb4ce..c8b8be9 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1296,10 +1296,10 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
 }
 #endif /* !CONFIG_USER_ONLY */
 
-#if defined(CONFIG_KVM)
 static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
                        uint32_t *ecx, uint32_t *edx)
 {
+#if defined(CONFIG_KVM)
     uint32_t vec[4];
 
 #ifdef __x86_64__
@@ -1327,8 +1327,8 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
 	*ecx = vec[2];
     if (edx)
 	*edx = vec[3];
-}
 #endif
+}
 
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
                    uint32_t *eax, uint32_t *ebx,
-- 
1.5.4.3

[Qemu-devel] [PATCH 2/3] dyngen: fix some warnings about unused functions

[Mark McLoughlin]

Warnings were:

  dyngen.c:292: error: ‘pstrcpy’ defined but not used

Only used with CONFIG_FORMAT_COFF

  dyngen.c:319: error: ‘swab32ss’ defined but not used

With ELF_USES_RELOCA, either swab32ss() or swab64ss() will
be used depending on ELF_CLASS. Easier to just mark them
both as unused.

  dyngen.c:334: error: ‘get16’ defined but not used
  dyngen.c:343: error: ‘get32’ defined but not used
  dyngen.c:352: error: ‘put16’ defined but not used
  dyngen.c:359: error: ‘put32’ defined but not used

Maybe be used for some host arches; easiest to mark them as
unused.

  dyngen.c:425: error: ‘elf_swap_phdr’ defined but not used

This one is completely unused; just delete.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 dyngen.c |   28 ++++++++++------------------
 1 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/dyngen.c b/dyngen.c
index 68f23ad..c55a115 100644
--- a/dyngen.c
+++ b/dyngen.c
@@ -32,6 +32,8 @@
 
 #include "config-host.h"
 
+#define ATTRIBUTE_UNUSED __attribute__((unused))
+
 /* NOTE: we test CONFIG_WIN32 instead of _WIN32 to enabled cross
    compilation */
 #if defined(CONFIG_WIN32)
@@ -288,6 +290,7 @@ static int strstart(const char *str, const char *val, const char **ptr)
     return 1;
 }
 
+#ifdef CONFIG_FORMAT_COFF
 static void pstrcpy(char *buf, int buf_size, const char *str)
 {
     int c;
@@ -304,6 +307,7 @@ static void pstrcpy(char *buf, int buf_size, const char *str)
     }
     *q = '\0';
 }
+#endif /* CONFIG_FORMAT_COFF */
 
 static void swab16s(uint16_t *p)
 {
@@ -315,7 +319,7 @@ static void swab32s(uint32_t *p)
     *p = bswap32(*p);
 }
 
-static void swab32ss(int32_t *p)
+static void ATTRIBUTE_UNUSED swab32ss(int32_t *p)
 {
     *p = bswap32(*p);
 }
@@ -325,12 +329,12 @@ static void swab64s(uint64_t *p)
     *p = bswap64(*p);
 }
 
-static void swab64ss(int64_t *p)
+static void ATTRIBUTE_UNUSED swab64ss(int64_t *p)
 {
     *p = bswap64(*p);
 }
 
-static uint16_t get16(uint16_t *p)
+static uint16_t ATTRIBUTE_UNUSED get16(uint16_t *p)
 {
     uint16_t val;
     val = *p;
@@ -339,7 +343,7 @@ static uint16_t get16(uint16_t *p)
     return val;
 }
 
-static uint32_t get32(uint32_t *p)
+static uint32_t ATTRIBUTE_UNUSED get32(uint32_t *p)
 {
     uint32_t val;
     val = *p;
@@ -348,14 +352,14 @@ static uint32_t get32(uint32_t *p)
     return val;
 }
 
-static void put16(uint16_t *p, uint16_t val)
+static void ATTRIBUTE_UNUSED put16(uint16_t *p, uint16_t val)
 {
     if (do_swap)
         val = bswap16(val);
     *p = val;
 }
 
-static void put32(uint32_t *p, uint32_t val)
+static void ATTRIBUTE_UNUSED put32(uint32_t *p, uint32_t val)
 {
     if (do_swap)
         val = bswap32(val);
@@ -421,18 +425,6 @@ static void elf_swap_shdr(struct elf_shdr *h)
   swabls(&h->	sh_entsize);		/* Entry size if section holds table */
 }
 
-static void elf_swap_phdr(struct elf_phdr *h)
-{
-    swab32s(&h->p_type);			/* Segment type */
-    swabls(&h->p_offset);		/* Segment file offset */
-    swabls(&h->p_vaddr);		/* Segment virtual address */
-    swabls(&h->p_paddr);		/* Segment physical address */
-    swabls(&h->p_filesz);		/* Segment size in file */
-    swabls(&h->p_memsz);		/* Segment size in memory */
-    swab32s(&h->p_flags);		/* Segment flags */
-    swabls(&h->p_align);		/* Segment alignment */
-}
-
 static void elf_swap_rel(ELF_RELOC *rel)
 {
     swabls(&rel->r_offset);
-- 
1.5.4.3

[Qemu-devel] [PATCH 3/3] cris: fix a segfault if pflash drive not found

[Mark McLoughlin]

drive_get_index() returns -1 if a drive isn't found; don't
use -1 to index drives_table.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/etraxfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/etraxfs.c b/hw/etraxfs.c
index 4f93cd8..2f6e2ee 100644
--- a/hw/etraxfs.c
+++ b/hw/etraxfs.c
@@ -83,7 +83,7 @@ void bareetraxfs_init (ram_addr_t ram_size, int vga_ram_size,
     phys_flash = qemu_ram_alloc(FLASH_SIZE);
     i = drive_get_index(IF_PFLASH, 0, 0);
     pflash_cfi02_register(0x0, phys_flash,
-                          drives_table[i].bdrv, (64 * 1024),
+                          i != -1 ? drives_table[i].bdrv : NULL, (64 * 1024),
                           FLASH_SIZE >> 16,
                           1, 2, 0x0000, 0x0000, 0x0000, 0x0000,
                           0x555, 0x2aa);
-- 
1.5.4.3

[Qemu-devel] Re: [PATCH 3/3] cris: fix a segfault if pflash drive not found

[Anthony Liguori]

Mark McLoughlin wrote:
> drive_get_index() returns -1 if a drive isn't found; don't
> use -1 to index drives_table.
>   

Looks fine to me.  Edgar?

Regards,

Anthony Liguori

> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  hw/etraxfs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/etraxfs.c b/hw/etraxfs.c
> index 4f93cd8..2f6e2ee 100644
> --- a/hw/etraxfs.c
> +++ b/hw/etraxfs.c
> @@ -83,7 +83,7 @@ void bareetraxfs_init (ram_addr_t ram_size, int vga_ram_size,
>      phys_flash = qemu_ram_alloc(FLASH_SIZE);
>      i = drive_get_index(IF_PFLASH, 0, 0);
>      pflash_cfi02_register(0x0, phys_flash,
> -                          drives_table[i].bdrv, (64 * 1024),
> +                          i != -1 ? drives_table[i].bdrv : NULL, (64 * 1024),
>                            FLASH_SIZE >> 16,
>                            1, 2, 0x0000, 0x0000, 0x0000, 0x0000,
>                            0x555, 0x2aa);
>   

Re: [Qemu-devel] Re: [PATCH 3/3] cris: fix a segfault if pflash drive not found

[Edgar E. Iglesias]

On Thu, Nov 13, 2008 at 01:37:10PM -0600, Anthony Liguori wrote:
> Mark McLoughlin wrote:
>> drive_get_index() returns -1 if a drive isn't found; don't
>> use -1 to index drives_table.
>>   
>
> Looks fine to me.  Edgar?

Look OK, I'll apply it later tonight.

Thanks

>
> Regards,
>
> Anthony Liguori
>
>> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
>> ---
>>  hw/etraxfs.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/etraxfs.c b/hw/etraxfs.c
>> index 4f93cd8..2f6e2ee 100644
>> --- a/hw/etraxfs.c
>> +++ b/hw/etraxfs.c
>> @@ -83,7 +83,7 @@ void bareetraxfs_init (ram_addr_t ram_size, int 
>> vga_ram_size,
>>      phys_flash = qemu_ram_alloc(FLASH_SIZE);
>>      i = drive_get_index(IF_PFLASH, 0, 0);
>>      pflash_cfi02_register(0x0, phys_flash,
>> -                          drives_table[i].bdrv, (64 * 1024),
>> +                          i != -1 ? drives_table[i].bdrv : NULL, (64 * 
>> 1024),
>>                            FLASH_SIZE >> 16,
>>                            1, 2, 0x0000, 0x0000, 0x0000, 0x0000,
>>                            0x555, 0x2aa);
>>   
>
>

[Qemu-devel] Re: [PATCH 2/3] dyngen: fix some warnings about unused functions

[Jan Kiszka]

Mark McLoughlin wrote:
> Warnings were:
> 
>   dyngen.c:292: error: ‘pstrcpy’ defined but not used
> 
> Only used with CONFIG_FORMAT_COFF
> 
>   dyngen.c:319: error: ‘swab32ss’ defined but not used
> 
> With ELF_USES_RELOCA, either swab32ss() or swab64ss() will
> be used depending on ELF_CLASS. Easier to just mark them
> both as unused.
> 
>   dyngen.c:334: error: ‘get16’ defined but not used
>   dyngen.c:343: error: ‘get32’ defined but not used
>   dyngen.c:352: error: ‘put16’ defined but not used
>   dyngen.c:359: error: ‘put32’ defined but not used
> 
> Maybe be used for some host arches; easiest to mark them as
> unused.
> 
>   dyngen.c:425: error: ‘elf_swap_phdr’ defined but not used
> 
> This one is completely unused; just delete.

As far as I understood, the whole file will disappear soon (once ongoing
PPC conversion to TCG is completed). I gave up hunting after the dyngen
warning mess for this reason.

However, welcome to the club of warning fighters! Hope you'll have more
luck than others (including me). :)

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/3] dyngen: fix some warnings about unused functions

[Anthony Liguori]

Mark McLoughlin wrote:
> Warnings were:
>
>   dyngen.c:292: error: ‘pstrcpy’ defined but not used
>
> Only used with CONFIG_FORMAT_COFF
>
>   dyngen.c:319: error: ‘swab32ss’ defined but not used
>
> With ELF_USES_RELOCA, either swab32ss() or swab64ss() will
> be used depending on ELF_CLASS. Easier to just mark them
> both as unused.
>
>   dyngen.c:334: error: ‘get16’ defined but not used
>   dyngen.c:343: error: ‘get32’ defined but not used
>   dyngen.c:352: error: ‘put16’ defined but not used
>   dyngen.c:359: error: ‘put32’ defined but not used
>
> Maybe be used for some host arches; easiest to mark them as
> unused.
>
>   dyngen.c:425: error: ‘elf_swap_phdr’ defined but not used
>
> This one is completely unused; just delete.
>   

Since dyngen is going away soon, I'm not eager to make changes to it 
that could possibly break on some weird architecture.  Do you think it 
would be terrible to just ignore this for now?

If someone else is more confident that this is okay, I think it's fine 
to commit.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 2/3] dyngen: fix some warnings about unused functions

[Mark McLoughlin]

On Thu, 2008-11-13 at 13:39 -0600, Anthony Liguori wrote:
> Mark McLoughlin wrote:
> > Warnings were:
> >
> >   dyngen.c:292: error: ‘pstrcpy’ defined but not used
> >
> > Only used with CONFIG_FORMAT_COFF
> >
> >   dyngen.c:319: error: ‘swab32ss’ defined but not used
> >
> > With ELF_USES_RELOCA, either swab32ss() or swab64ss() will
> > be used depending on ELF_CLASS. Easier to just mark them
> > both as unused.
> >
> >   dyngen.c:334: error: ‘get16’ defined but not used
> >   dyngen.c:343: error: ‘get32’ defined but not used
> >   dyngen.c:352: error: ‘put16’ defined but not used
> >   dyngen.c:359: error: ‘put32’ defined but not used
> >
> > Maybe be used for some host arches; easiest to mark them as
> > unused.
> >
> >   dyngen.c:425: error: ‘elf_swap_phdr’ defined but not used
> >
> > This one is completely unused; just delete.
> >   
> 
> Since dyngen is going away soon, I'm not eager to make changes to it 
> that could possibly break on some weird architecture.  Do you think it 
> would be terrible to just ignore this for now?

Yeah, that's fine - I was trying to make it possible to build with
--enable-werror to so it would be easier to spot new warnings.

The tcg 'TARGET_LONG_BITS is not defined' warning seems much more nasty
to solve anyway.

Well, slirp too - but you can build with --disable-slirp to get around
that.

Cheers,
Mark.

[Qemu-devel] Re: [PATCH 1/3] x86: fix warning without CONFIG_KVM

[Jan Kiszka]

Mark McLoughlin wrote:
> Warning is:
> 
>  target-i386/helper.c: In function `cpu_x86_cpuid':
>  target-i386/helper.c:1373: warning: implicit declaration of function `host_cpuid'
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  target-i386/helper.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 7ddb4ce..c8b8be9 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1296,10 +1296,10 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr)
>  }
>  #endif /* !CONFIG_USER_ONLY */
>  
> -#if defined(CONFIG_KVM)
>  static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
>                         uint32_t *ecx, uint32_t *edx)
>  {
> +#if defined(CONFIG_KVM)
>      uint32_t vec[4];
>  
>  #ifdef __x86_64__
> @@ -1327,8 +1327,8 @@ static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
>  	*ecx = vec[2];
>      if (edx)
>  	*edx = vec[3];
> -}
>  #endif
> +}

Isn't it cleaner to provide an empty static inline in the !CONFIG_KVM
case? Not all compilers might be smart enough to optimize this function
away.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2 ES-OS
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 1/3] x86: fix warning without CONFIG_KVM

[Anthony Liguori]

Jan Kiszka wrote:
> Isn't it cleaner to provide an empty static inline in the !CONFIG_KVM
> case? Not all compilers might be smart enough to optimize this function
> away.
>   

kvm_enabled() is (0) when !CONFIG_KVM so that's why this is a warning 
and not an error.  The function call disappears during linking from dead 
code elimination.  There is no routine overhead when !CONFIG_KVM.

Regards,

Anthony Liguori

> Jan
>
>   

Re: [Qemu-devel] Re: [PATCH 1/3] x86: fix warning without CONFIG_KVM

[Jamie Lokier]

Anthony Liguori wrote:
> Jan Kiszka wrote:
> >Isn't it cleaner to provide an empty static inline in the !CONFIG_KVM
> >case? Not all compilers might be smart enough to optimize this function
> >away.
> 
> kvm_enabled() is (0) when !CONFIG_KVM so that's why this is a warning 
> and not an error.  The function call disappears during linking from dead 
> code elimination.  There is no routine overhead when !CONFIG_KVM.

I think it's in reference to:

>  static void host_cpuid(uint32_t function, uint32_t *eax, uint32_t *ebx,
>                         uint32_t *ecx, uint32_t *edx)

This compiles to an empty function if !CONFIG_KVM.  Current GCC will
remove the function call by auto-inlining, but some compilers
(including older GCC) will produce an empty function.

If you're saying !CONFIG_KVM results in the _call_ to host_cpuid()
being removed, I'm thinking that the option to provide the guest with
the same features as the host CPU is still useful if KVM is
disabled...  I have a real use-case for this.  It's installing some
versions of Windows in a guest, to be used with KVM after
installation, because the install process is more reliable (and
faster!) with KVM disabled but after that it works.

-- Jamie

[Qemu-devel] Re: [PATCH 1/3] x86: fix warning without CONFIG_KVM

[Anthony Liguori]

Mark McLoughlin wrote:
> Warning is:
>
>  target-i386/helper.c: In function `cpu_x86_cpuid':
>  target-i386/helper.c:1373: warning: implicit declaration of function `host_cpuid'
>
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
>   

Applied.  Thanks.

Regards,

Anthony Liguori