Patchwork [05/28] kvm tools: 64-bit tidy; use PRIx64 when printf'ing u64s and link appropriately

login
register
mail settings
Submitter Matt Evans
Date Dec. 6, 2011, 3:38 a.m.
Message ID <4EDD8E4D.5000309@ozlabs.org>
Download mbox | patch
Permalink /patch/129500/
State New
Headers show

Comments

Matt Evans - Dec. 6, 2011, 3:38 a.m.
On LP64 systems our u64s are just longs; remove the %llx'es in favour of PRIx64
etc.

This patch also adds CFLAGS to the final link, so that any -m64 is obeyed when
linking, too.

Signed-off-by: Matt Evans <matt@ozlabs.org>
---
 tools/kvm/Makefile       |    2 +-
 tools/kvm/builtin-run.c  |   14 ++++++++------
 tools/kvm/builtin-stat.c |    4 +++-
 tools/kvm/disk/core.c    |    4 +++-
 tools/kvm/mmio.c         |    4 +++-
 5 files changed, 18 insertions(+), 10 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin - Dec. 6, 2011, 8:13 a.m.
Ingo actually got us to remove all the PRI* specifiers, but that was
back when we only did x86 :)

Ingo, does it make sense to use them now when we support different
architectures?

On Tue, 2011-12-06 at 14:38 +1100, Matt Evans wrote:
> On LP64 systems our u64s are just longs; remove the %llx'es in favour of PRIx64
> etc.
> 
> This patch also adds CFLAGS to the final link, so that any -m64 is obeyed when
> linking, too.
> 
> Signed-off-by: Matt Evans <matt@ozlabs.org>
> ---
>  tools/kvm/Makefile       |    2 +-
>  tools/kvm/builtin-run.c  |   14 ++++++++------
>  tools/kvm/builtin-stat.c |    4 +++-
>  tools/kvm/disk/core.c    |    4 +++-
>  tools/kvm/mmio.c         |    4 +++-
>  5 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile
> index 009a6ba..57dc521 100644
> --- a/tools/kvm/Makefile
> +++ b/tools/kvm/Makefile
> @@ -218,7 +218,7 @@ KVMTOOLS-VERSION-FILE:
>  
>  $(PROGRAM): $(DEPS) $(OBJS)
>  	$(E) "  LINK    " $@
> -	$(Q) $(CC) $(OBJS) $(LIBS) -o $@
> +	$(Q) $(CC) $(CFLAGS) $(OBJS) $(LIBS) -o $@
>  
>  $(GUEST_INIT): guest/init.c
>  	$(E) "  LINK    " $@
> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> index e4aa87e..7cf208d 100644
> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -42,6 +42,8 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#define __STDC_FORMAT_MACROS
> +#include <inttypes.h>
>  #include <ctype.h>
>  #include <stdio.h>
>  
> @@ -383,8 +385,8 @@ static int shmem_parser(const struct option *opt, const char *arg, int unset)
>  		strcpy(handle, default_handle);
>  	}
>  	if (verbose) {
> -		pr_info("shmem: phys_addr = %llx", phys_addr);
> -		pr_info("shmem: size      = %llx", size);
> +		pr_info("shmem: phys_addr = %"PRIx64, phys_addr);
> +		pr_info("shmem: size      = %"PRIx64, size);
>  		pr_info("shmem: handle    = %s", handle);
>  		pr_info("shmem: create    = %d", create);
>  	}
> @@ -545,7 +547,7 @@ panic_kvm:
>  		current_kvm_cpu->kvm_run->exit_reason,
>  		kvm_exit_reasons[current_kvm_cpu->kvm_run->exit_reason]);
>  	if (current_kvm_cpu->kvm_run->exit_reason == KVM_EXIT_UNKNOWN)
> -		fprintf(stderr, "KVM exit code: 0x%Lu\n",
> +		fprintf(stderr, "KVM exit code: 0x%"PRIx64"\n",
>  			current_kvm_cpu->kvm_run->hw.hardware_exit_reason);
>  
>  	kvm_cpu__set_debug_fd(STDOUT_FILENO);
> @@ -760,10 +762,10 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  		ram_size	= get_ram_size(nrcpus);
>  
>  	if (ram_size < MIN_RAM_SIZE_MB)
> -		die("Not enough memory specified: %lluMB (min %lluMB)", ram_size, MIN_RAM_SIZE_MB);
> +		die("Not enough memory specified: %"PRIu64"MB (min %lluMB)", ram_size, MIN_RAM_SIZE_MB);
>  
>  	if (ram_size > host_ram_size())
> -		pr_warning("Guest memory size %lluMB exceeds host physical RAM size %lluMB", ram_size, host_ram_size());
> +		pr_warning("Guest memory size %"PRIu64"MB exceeds host physical RAM size %"PRIu64"MB", ram_size, host_ram_size());
>  
>  	ram_size <<= MB_SHIFT;
>  
> @@ -878,7 +880,7 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  		virtio_blk__init_all(kvm);
>  	}
>  
> -	printf("  # kvm run -k %s -m %Lu -c %d --name %s\n", kernel_filename, ram_size / 1024 / 1024, nrcpus, guest_name);
> +	printf("  # kvm run -k %s -m %"PRId64" -c %d --name %s\n", kernel_filename, ram_size / 1024 / 1024, nrcpus, guest_name);
>  
>  	if (!kvm__load_kernel(kvm, kernel_filename, initrd_filename,
>  				real_cmdline, vidmode))
> diff --git a/tools/kvm/builtin-stat.c b/tools/kvm/builtin-stat.c
> index e28eb5b..c1f2605 100644
> --- a/tools/kvm/builtin-stat.c
> +++ b/tools/kvm/builtin-stat.c
> @@ -9,6 +9,8 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <signal.h>
> +#define __STDC_FORMAT_MACROS
> +#include <inttypes.h>
>  
>  #include <linux/virtio_balloon.h>
>  
> @@ -97,7 +99,7 @@ static int do_memstat(const char *name, int sock)
>  			printf("The total amount of memory available (in bytes):");
>  			break;
>  		}
> -		printf("%llu\n", stats[i].val);
> +		printf("%"PRId64"\n", stats[i].val);
>  	}
>  	printf("\n");
>  
> diff --git a/tools/kvm/disk/core.c b/tools/kvm/disk/core.c
> index 4915efd..a135851 100644
> --- a/tools/kvm/disk/core.c
> +++ b/tools/kvm/disk/core.c
> @@ -4,6 +4,8 @@
>  
>  #include <sys/eventfd.h>
>  #include <sys/poll.h>
> +#define __STDC_FORMAT_MACROS
> +#include <inttypes.h>
>  
>  #define AIO_MAX 32
>  
> @@ -232,7 +234,7 @@ ssize_t disk_image__get_serial(struct disk_image *disk, void *buffer, ssize_t *l
>  	if (fstat(disk->fd, &st) != 0)
>  		return 0;
>  
> -	*len = snprintf(buffer, *len, "%llu%llu%llu", (u64)st.st_dev, (u64)st.st_rdev, (u64)st.st_ino);
> +	*len = snprintf(buffer, *len, "%"PRId64"%"PRId64"%"PRId64, (u64)st.st_dev, (u64)st.st_rdev, (u64)st.st_ino);
>  	return *len;
>  }
>  
> diff --git a/tools/kvm/mmio.c b/tools/kvm/mmio.c
> index de7320f..1158bff 100644
> --- a/tools/kvm/mmio.c
> +++ b/tools/kvm/mmio.c
> @@ -9,6 +9,8 @@
>  #include <linux/kvm.h>
>  #include <linux/types.h>
>  #include <linux/rbtree.h>
> +#define __STDC_FORMAT_MACROS
> +#include <inttypes.h>
>  
>  #define mmio_node(n) rb_entry(n, struct mmio_mapping, node)
>  
> @@ -124,7 +126,7 @@ bool kvm__emulate_mmio(struct kvm *kvm, u64 phys_addr, u8 *data, u32 len, u8 is_
>  	if (mmio)
>  		mmio->mmio_fn(phys_addr, data, len, is_write, mmio->ptr);
>  	else
> -		fprintf(stderr, "Warning: Ignoring MMIO %s at %016llx (length %u)\n",
> +		fprintf(stderr, "Warning: Ignoring MMIO %s at %016"PRIx64" (length %u)\n",
>  			to_direction(is_write), phys_addr, len);
>  	br_read_unlock();
>  
> --
> 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
Ingo Molnar - Dec. 6, 2011, 8:28 a.m.
* Sasha Levin <levinsasha928@gmail.com> wrote:

> Ingo actually got us to remove all the PRI* specifiers, but 
> that was back when we only did x86 :)
> 
> Ingo, does it make sense to use them now when we support 
> different architectures?

Not at all - ppc should use a sane u64/s64 definition, i.e. 
int-ll64.h instead of the int-l64.h crap.

The powerpc maintainers indicated that they'd fix that, a couple 
of years ago, but nothing appears to have come out of that.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Mackerras - Dec. 6, 2011, 10:05 a.m.
On Tue, Dec 06, 2011 at 09:28:27AM +0100, Ingo Molnar wrote:
> 
> * Sasha Levin <levinsasha928@gmail.com> wrote:
> 
> > Ingo actually got us to remove all the PRI* specifiers, but 
> > that was back when we only did x86 :)
> > 
> > Ingo, does it make sense to use them now when we support 
> > different architectures?
> 
> Not at all - ppc should use a sane u64/s64 definition, i.e. 
> int-ll64.h instead of the int-l64.h crap.
> 
> The powerpc maintainers indicated that they'd fix that, a couple 
> of years ago, but nothing appears to have come out of that.

We changed it for the kernel, but not for userspace (i.e. glibc)
because of concerns about possibly breaking existing userspace
programs.  I haven't looked closely at Matt's patches, but it should
be possible to use [un]signed long long for the u64/s64 types, I would
think.

Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar - Dec. 6, 2011, 10:24 a.m.
* Paul Mackerras <paulus@samba.org> wrote:

> On Tue, Dec 06, 2011 at 09:28:27AM +0100, Ingo Molnar wrote:
> > 
> > * Sasha Levin <levinsasha928@gmail.com> wrote:
> > 
> > > Ingo actually got us to remove all the PRI* specifiers, but 
> > > that was back when we only did x86 :)
> > > 
> > > Ingo, does it make sense to use them now when we support 
> > > different architectures?
> > 
> > Not at all - ppc should use a sane u64/s64 definition, i.e. 
> > int-ll64.h instead of the int-l64.h crap.
> > 
> > The powerpc maintainers indicated that they'd fix that, a couple 
> > of years ago, but nothing appears to have come out of that.
> 
> We changed it for the kernel, but not for userspace (i.e. 
> glibc) because of concerns about possibly breaking existing 
> userspace programs. [...]

Indeed, you do:
 
 #if defined(__powerpc64__) && !defined(__KERNEL__)
 # include <asm-generic/int-l64.h>
 #else
 # include <asm-generic/int-ll64.h>
 #endif

which correctly uses int-ll64.h everywhere except 64-bit 
userspace inclusions. So i take back my 'nothing appears to have 
come out of that' comment - it's all nicely fixed!

> [...]  I haven't looked closely at Matt's 
> patches, but it should be possible to use [un]signed long long 
> for the u64/s64 types, I would think.

In tools/kvm/ we are using our own u64/s64 definitions, not 
glibc's, so i think it should be fine - as long as we don't pick 
up int-l64.h accidentally via the 
arch/powerpc/include/asm/types.h exception for user-space.

Stray uses of int64 should be converted to u64 (i see some in 
tools/kvm/virtio/9p-pdu.c) but otherwise we should be ok - 
unless i'm missing some complication.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Evans - Dec. 7, 2011, 7:01 a.m.
Hi Ingo,


On 06/12/11 21:24, Ingo Molnar wrote:
> 
> * Paul Mackerras <paulus@samba.org> wrote:
> 
>> On Tue, Dec 06, 2011 at 09:28:27AM +0100, Ingo Molnar wrote:
>>>
>>> * Sasha Levin <levinsasha928@gmail.com> wrote:
>>>
>>>> Ingo actually got us to remove all the PRI* specifiers, but 
>>>> that was back when we only did x86 :)
>>>>
>>>> Ingo, does it make sense to use them now when we support 
>>>> different architectures?
>>>
>>> Not at all - ppc should use a sane u64/s64 definition, i.e. 
>>> int-ll64.h instead of the int-l64.h crap.
>>>
>>> The powerpc maintainers indicated that they'd fix that, a couple 
>>> of years ago, but nothing appears to have come out of that.
>>
>> We changed it for the kernel, but not for userspace (i.e. 
>> glibc) because of concerns about possibly breaking existing 
>> userspace programs. [...]
> 
> Indeed, you do:
>  
>  #if defined(__powerpc64__) && !defined(__KERNEL__)
>  # include <asm-generic/int-l64.h>
>  #else
>  # include <asm-generic/int-ll64.h>
>  #endif
> 
> which correctly uses int-ll64.h everywhere except 64-bit 
> userspace inclusions. So i take back my 'nothing appears to have 
> come out of that' comment - it's all nicely fixed!
> 
>> [...]  I haven't looked closely at Matt's 
>> patches, but it should be possible to use [un]signed long long 
>> for the u64/s64 types, I would think.
> 
> In tools/kvm/ we are using our own u64/s64 definitions, not 
> glibc's, so i think it should be fine - as long as we don't pick 
> up int-l64.h accidentally via the 
> arch/powerpc/include/asm/types.h exception for user-space.

That's what's happening here; we're __powerpc64__ and !__KERNEL__,
tools/kvm/include/linux/types.h includes asm/types.h so gets the int-l64.h
definition of __u64, as above.  :/

builtin-run.c:389: error: format `%llx' expects type `long long unsigned int', but argument 2 has type `u64'

etc. etc.

Cheers,


Matt

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar - Dec. 7, 2011, 8:16 a.m.
* Matt Evans <matt@ozlabs.org> wrote:

> >> [...]  I haven't looked closely at Matt's 
> >> patches, but it should be possible to use [un]signed long long 
> >> for the u64/s64 types, I would think.
> > 
> > In tools/kvm/ we are using our own u64/s64 definitions, not 
> > glibc's, so i think it should be fine - as long as we don't pick 
> > up int-l64.h accidentally via the 
> > arch/powerpc/include/asm/types.h exception for user-space.
> 
> That's what's happening here; we're __powerpc64__ and 
> !__KERNEL__, tools/kvm/include/linux/types.h includes 
> asm/types.h so gets the int-l64.h definition of __u64, as 
> above.  :/
> 
> builtin-run.c:389: error: format `%llx' expects type `long 
> long unsigned int', but argument 2 has type `u64'

So either define __KERNEL__ or patch a __NEW_USERSPACE__ define 
into power/asm/types.h and use it - if the PowerPC folks agree 
with that approach.

Sane userspace should not be prevented from using the same sane 
types the kernel is already using :-)

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paolo Bonzini - Dec. 7, 2011, 12:49 p.m.
On 12/07/2011 09:16 AM, Ingo Molnar wrote:
>> >  That's what's happening here; we're __powerpc64__ and
>> >  !__KERNEL__, tools/kvm/include/linux/types.h includes
>> >  asm/types.h so gets the int-l64.h definition of __u64, as
>> >  above.  :/
>> >
>> >  builtin-run.c:389: error: format `%llx' expects type `long
>> >  long unsigned int', but argument 2 has type `u64'
> So either define __KERNEL__ or patch a __NEW_USERSPACE__ define
> into power/asm/types.h and use it - if the PowerPC folks agree
> with that approach.
>
> Sane userspace should not be prevented from using the same sane
> types the kernel is already using:-)

I should dig up that patent of mine for "apparatus for conversion of 
circular motion to rectilinear ('wheel')".

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pekka Enberg - Dec. 7, 2011, 5:14 p.m.
On Wed, 7 Dec 2011, Ingo Molnar wrote:

>
> * Matt Evans <matt@ozlabs.org> wrote:
>
>>>> [...]  I haven't looked closely at Matt's
>>>> patches, but it should be possible to use [un]signed long long
>>>> for the u64/s64 types, I would think.
>>>
>>> In tools/kvm/ we are using our own u64/s64 definitions, not
>>> glibc's, so i think it should be fine - as long as we don't pick
>>> up int-l64.h accidentally via the
>>> arch/powerpc/include/asm/types.h exception for user-space.
>>
>> That's what's happening here; we're __powerpc64__ and
>> !__KERNEL__, tools/kvm/include/linux/types.h includes
>> asm/types.h so gets the int-l64.h definition of __u64, as
>> above.  :/
>>
>> builtin-run.c:389: error: format `%llx' expects type `long
>> long unsigned int', but argument 2 has type `u64'
>
> So either define __KERNEL__ or patch a __NEW_USERSPACE__ define
> into power/asm/types.h and use it - if the PowerPC folks agree
> with that approach.
>
> Sane userspace should not be prevented from using the same sane
> types the kernel is already using :-)

How does perf handle this? I'm sure it has the exact same issue, doesn't 
it?

 			Pekka
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pekka Enberg - Dec. 7, 2011, 5:21 p.m.
On 12/07/2011 09:16 AM, Ingo Molnar wrote:
>>> >  That's what's happening here; we're __powerpc64__ and
>>> >  !__KERNEL__, tools/kvm/include/linux/types.h includes
>>> >  asm/types.h so gets the int-l64.h definition of __u64, as
>>> >  above.  :/
>>> >
>>> >  builtin-run.c:389: error: format `%llx' expects type `long
>>> >  long unsigned int', but argument 2 has type `u64'
>> So either define __KERNEL__ or patch a __NEW_USERSPACE__ define
>> into power/asm/types.h and use it - if the PowerPC folks agree
>> with that approach.
>> 
>> Sane userspace should not be prevented from using the same sane
>> types the kernel is already using:-)

On Wed, 7 Dec 2011, Paolo Bonzini wrote:
> I should dig up that patent of mine for "apparatus for conversion of circular 
> motion to rectilinear ('wheel')".

What's your point?

We use kernel types for obvious reasons and it trying to use <stdint.h> 
for the PPC port is just begging for trouble.

 			Pekka
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Evans - Dec. 8, 2011, 3:14 a.m.
On 08/12/11 04:14, Pekka Enberg wrote:
> On Wed, 7 Dec 2011, Ingo Molnar wrote:
> 
>>
>> * Matt Evans <matt@ozlabs.org> wrote:
>>
>>>>> [...]  I haven't looked closely at Matt's
>>>>> patches, but it should be possible to use [un]signed long long
>>>>> for the u64/s64 types, I would think.
>>>>
>>>> In tools/kvm/ we are using our own u64/s64 definitions, not
>>>> glibc's, so i think it should be fine - as long as we don't pick
>>>> up int-l64.h accidentally via the
>>>> arch/powerpc/include/asm/types.h exception for user-space.
>>>
>>> That's what's happening here; we're __powerpc64__ and
>>> !__KERNEL__, tools/kvm/include/linux/types.h includes
>>> asm/types.h so gets the int-l64.h definition of __u64, as
>>> above.  :/
>>>
>>> builtin-run.c:389: error: format `%llx' expects type `long
>>> long unsigned int', but argument 2 has type `u64'
>>
>> So either define __KERNEL__ or patch a __NEW_USERSPACE__ define
>> into power/asm/types.h and use it - if the PowerPC folks agree
>> with that approach.
>>
>> Sane userspace should not be prevented from using the same sane
>> types the kernel is already using :-)
> 
> How does perf handle this? I'm sure it has the exact same issue, doesn't 
> it?

It does; ironically it uses PRIblah, so I had followed its example.


Cheers,


Matt

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar - Dec. 8, 2011, 4:49 a.m.
* Matt Evans <matt@ozlabs.org> wrote:

> On 08/12/11 04:14, Pekka Enberg wrote:
> > On Wed, 7 Dec 2011, Ingo Molnar wrote:
> > 
> >>
> >> * Matt Evans <matt@ozlabs.org> wrote:
> >>
> >>>>> [...]  I haven't looked closely at Matt's
> >>>>> patches, but it should be possible to use [un]signed long long
> >>>>> for the u64/s64 types, I would think.
> >>>>
> >>>> In tools/kvm/ we are using our own u64/s64 definitions, not
> >>>> glibc's, so i think it should be fine - as long as we don't pick
> >>>> up int-l64.h accidentally via the
> >>>> arch/powerpc/include/asm/types.h exception for user-space.
> >>>
> >>> That's what's happening here; we're __powerpc64__ and
> >>> !__KERNEL__, tools/kvm/include/linux/types.h includes
> >>> asm/types.h so gets the int-l64.h definition of __u64, as
> >>> above.  :/
> >>>
> >>> builtin-run.c:389: error: format `%llx' expects type `long
> >>> long unsigned int', but argument 2 has type `u64'
> >>
> >> So either define __KERNEL__ or patch a __NEW_USERSPACE__ define
> >> into power/asm/types.h and use it - if the PowerPC folks agree
> >> with that approach.
> >>
> >> Sane userspace should not be prevented from using the same sane
> >> types the kernel is already using :-)
> > 
> > How does perf handle this? I'm sure it has the exact same 
> > issue, doesn't it?
> 
> It does; ironically it uses PRIblah, so I had followed its 
> example.

Sadly it regressed lately in that area - it was certainly 
PRIblah-less in the early stages :-)

Pekka, how do the headers react if we define __KERNEL__? 
Alternatively, allowing an override in powerpc/types.h beyond 
__KERNEL__ looks sensible as well.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Evans - Dec. 8, 2011, 4:56 a.m.
On 08/12/11 15:49, Ingo Molnar wrote:
> 
> * Matt Evans <matt@ozlabs.org> wrote:
> 
>> On 08/12/11 04:14, Pekka Enberg wrote:
>>> On Wed, 7 Dec 2011, Ingo Molnar wrote:
>>>
>>>>
>>>> * Matt Evans <matt@ozlabs.org> wrote:
>>>>
>>>>>>> [...]  I haven't looked closely at Matt's
>>>>>>> patches, but it should be possible to use [un]signed long long
>>>>>>> for the u64/s64 types, I would think.
>>>>>>
>>>>>> In tools/kvm/ we are using our own u64/s64 definitions, not
>>>>>> glibc's, so i think it should be fine - as long as we don't pick
>>>>>> up int-l64.h accidentally via the
>>>>>> arch/powerpc/include/asm/types.h exception for user-space.
>>>>>
>>>>> That's what's happening here; we're __powerpc64__ and
>>>>> !__KERNEL__, tools/kvm/include/linux/types.h includes
>>>>> asm/types.h so gets the int-l64.h definition of __u64, as
>>>>> above.  :/
>>>>>
>>>>> builtin-run.c:389: error: format `%llx' expects type `long
>>>>> long unsigned int', but argument 2 has type `u64'
>>>>
>>>> So either define __KERNEL__ or patch a __NEW_USERSPACE__ define
>>>> into power/asm/types.h and use it - if the PowerPC folks agree
>>>> with that approach.
>>>>
>>>> Sane userspace should not be prevented from using the same sane
>>>> types the kernel is already using :-)
>>>
>>> How does perf handle this? I'm sure it has the exact same 
>>> issue, doesn't it?
>>
>> It does; ironically it uses PRIblah, so I had followed its 
>> example.
> 
> Sadly it regressed lately in that area - it was certainly 
> PRIblah-less in the early stages :-)

Oh dear :-)

> Pekka, how do the headers react if we define __KERNEL__? 
> Alternatively, allowing an override in powerpc/types.h beyond 
> __KERNEL__ looks sensible as well.

Well, I just tried it and it ended in tears; various things bring in tons more
(ppc) stuff from asm/, quite a few conflicts.

I've resorted to defining __KERNEL__ in linux/types.h *only* around #include
<asm/types.h> (i.e. undefining it afterwards).  This picks up the correct
int-ll64.h on PPC64 and doesn't break everything else.

Cheers,


Matt
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Evans - Dec. 8, 2011, 5:47 a.m.
On 08/12/11 15:56, Matt Evans wrote:
> On 08/12/11 15:49, Ingo Molnar wrote:
>>
>> * Matt Evans <matt@ozlabs.org> wrote:
>>
>>> On 08/12/11 04:14, Pekka Enberg wrote:
>>>> On Wed, 7 Dec 2011, Ingo Molnar wrote:
>>>>
>>>>>
>>>>> * Matt Evans <matt@ozlabs.org> wrote:
>>>>>
>>>>>>>> [...]  I haven't looked closely at Matt's
>>>>>>>> patches, but it should be possible to use [un]signed long long
>>>>>>>> for the u64/s64 types, I would think.
>>>>>>>
>>>>>>> In tools/kvm/ we are using our own u64/s64 definitions, not
>>>>>>> glibc's, so i think it should be fine - as long as we don't pick
>>>>>>> up int-l64.h accidentally via the
>>>>>>> arch/powerpc/include/asm/types.h exception for user-space.
>>>>>>
>>>>>> That's what's happening here; we're __powerpc64__ and
>>>>>> !__KERNEL__, tools/kvm/include/linux/types.h includes
>>>>>> asm/types.h so gets the int-l64.h definition of __u64, as
>>>>>> above.  :/
>>>>>>
>>>>>> builtin-run.c:389: error: format `%llx' expects type `long
>>>>>> long unsigned int', but argument 2 has type `u64'
>>>>>
>>>>> So either define __KERNEL__ or patch a __NEW_USERSPACE__ define
>>>>> into power/asm/types.h and use it - if the PowerPC folks agree
>>>>> with that approach.
>>>>>
>>>>> Sane userspace should not be prevented from using the same sane
>>>>> types the kernel is already using :-)
>>>>
>>>> How does perf handle this? I'm sure it has the exact same 
>>>> issue, doesn't it?
>>>
>>> It does; ironically it uses PRIblah, so I had followed its 
>>> example.
>>
>> Sadly it regressed lately in that area - it was certainly 
>> PRIblah-less in the early stages :-)
> 
> Oh dear :-)
> 
>> Pekka, how do the headers react if we define __KERNEL__? 
>> Alternatively, allowing an override in powerpc/types.h beyond 
>> __KERNEL__ looks sensible as well.
> 
> Well, I just tried it and it ended in tears; various things bring in tons more
> (ppc) stuff from asm/, quite a few conflicts.
> 
> I've resorted to defining __KERNEL__ in linux/types.h *only* around #include
> <asm/types.h> (i.e. undefining it afterwards).  This picks up the correct
> int-ll64.h on PPC64 and doesn't break everything else.

I spoke too soon; this screws x86 (who?), in that BITS_PER_LONG's redefined as
../../include/asm-generic/bitsperlong.h now kicks in if __KERNEL__'s defined.
Defining __KERNEL__ feels a bit nasty, esp. considering these knock-on effects.

Since tools/kvm/include/linux/types.h only requires __u32, __u64 et al from
<asm/types.h>, wouldn't it be most straightforward to just #include
<asm-generic/int-ll64.h>?  This avoids #define __KERNEL__ breaking other
includes brought into userland, avoids changing system headers/distros, and
includes the file we're really interested in on both x86 & PPC.

Cheers,


Matt
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar - Dec. 8, 2011, 5:49 a.m.
* Matt Evans <matt@ozlabs.org> wrote:

> Since tools/kvm/include/linux/types.h only requires __u32, 
> __u64 et al from <asm/types.h>, wouldn't it be most 
> straightforward to just #include <asm-generic/int-ll64.h>?  
> This avoids #define __KERNEL__ breaking other includes brought 
> into userland, avoids changing system headers/distros, and 
> includes the file we're really interested in on both x86 & 
> PPC.

No system headers need to be changed: you can just patch 
powerpc/types.h with the extra __SANE_USERSPACE__ define (or any 
suitable name) and the KVM tool should work as-is.

The beauty of tools/kvm/ being integrated into the kernel tree.

Really, if that works and if the PowerPC folks agree then we 
have an immediate, fully adequate, instantly applicable 
solution.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Evans - Dec. 8, 2011, 6:17 a.m.
On 08/12/11 16:49, Ingo Molnar wrote:
> 
> * Matt Evans <matt@ozlabs.org> wrote:
> 
>> Since tools/kvm/include/linux/types.h only requires __u32, 
>> __u64 et al from <asm/types.h>, wouldn't it be most 
>> straightforward to just #include <asm-generic/int-ll64.h>?  
>> This avoids #define __KERNEL__ breaking other includes brought 
>> into userland, avoids changing system headers/distros, and 
>> includes the file we're really interested in on both x86 & 
>> PPC.
> 
> No system headers need to be changed: you can just patch 
> powerpc/types.h with the extra __SANE_USERSPACE__ define (or any 
> suitable name) and the KVM tool should work as-is.
> 
> The beauty of tools/kvm/ being integrated into the kernel tree.
> 
> Really, if that works and if the PowerPC folks agree then we 
> have an immediate, fully adequate, instantly applicable 
> solution.

Ahh, I finally understand you-- I'd earlier thought you were referring to the
system headers' asm/types.h, etc.  Thanks for your patience on this very trivial
point :-)  I'll add __SANE_USERSPACE_TYPES__ and we'll see how the others like
it. >:)  (Paul, that sound OK?)


Thanks,


Matt
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile
index 009a6ba..57dc521 100644
--- a/tools/kvm/Makefile
+++ b/tools/kvm/Makefile
@@ -218,7 +218,7 @@  KVMTOOLS-VERSION-FILE:
 
 $(PROGRAM): $(DEPS) $(OBJS)
 	$(E) "  LINK    " $@
-	$(Q) $(CC) $(OBJS) $(LIBS) -o $@
+	$(Q) $(CC) $(CFLAGS) $(OBJS) $(LIBS) -o $@
 
 $(GUEST_INIT): guest/init.c
 	$(E) "  LINK    " $@
diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
index e4aa87e..7cf208d 100644
--- a/tools/kvm/builtin-run.c
+++ b/tools/kvm/builtin-run.c
@@ -42,6 +42,8 @@ 
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#define __STDC_FORMAT_MACROS
+#include <inttypes.h>
 #include <ctype.h>
 #include <stdio.h>
 
@@ -383,8 +385,8 @@  static int shmem_parser(const struct option *opt, const char *arg, int unset)
 		strcpy(handle, default_handle);
 	}
 	if (verbose) {
-		pr_info("shmem: phys_addr = %llx", phys_addr);
-		pr_info("shmem: size      = %llx", size);
+		pr_info("shmem: phys_addr = %"PRIx64, phys_addr);
+		pr_info("shmem: size      = %"PRIx64, size);
 		pr_info("shmem: handle    = %s", handle);
 		pr_info("shmem: create    = %d", create);
 	}
@@ -545,7 +547,7 @@  panic_kvm:
 		current_kvm_cpu->kvm_run->exit_reason,
 		kvm_exit_reasons[current_kvm_cpu->kvm_run->exit_reason]);
 	if (current_kvm_cpu->kvm_run->exit_reason == KVM_EXIT_UNKNOWN)
-		fprintf(stderr, "KVM exit code: 0x%Lu\n",
+		fprintf(stderr, "KVM exit code: 0x%"PRIx64"\n",
 			current_kvm_cpu->kvm_run->hw.hardware_exit_reason);
 
 	kvm_cpu__set_debug_fd(STDOUT_FILENO);
@@ -760,10 +762,10 @@  int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 		ram_size	= get_ram_size(nrcpus);
 
 	if (ram_size < MIN_RAM_SIZE_MB)
-		die("Not enough memory specified: %lluMB (min %lluMB)", ram_size, MIN_RAM_SIZE_MB);
+		die("Not enough memory specified: %"PRIu64"MB (min %lluMB)", ram_size, MIN_RAM_SIZE_MB);
 
 	if (ram_size > host_ram_size())
-		pr_warning("Guest memory size %lluMB exceeds host physical RAM size %lluMB", ram_size, host_ram_size());
+		pr_warning("Guest memory size %"PRIu64"MB exceeds host physical RAM size %"PRIu64"MB", ram_size, host_ram_size());
 
 	ram_size <<= MB_SHIFT;
 
@@ -878,7 +880,7 @@  int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 		virtio_blk__init_all(kvm);
 	}
 
-	printf("  # kvm run -k %s -m %Lu -c %d --name %s\n", kernel_filename, ram_size / 1024 / 1024, nrcpus, guest_name);
+	printf("  # kvm run -k %s -m %"PRId64" -c %d --name %s\n", kernel_filename, ram_size / 1024 / 1024, nrcpus, guest_name);
 
 	if (!kvm__load_kernel(kvm, kernel_filename, initrd_filename,
 				real_cmdline, vidmode))
diff --git a/tools/kvm/builtin-stat.c b/tools/kvm/builtin-stat.c
index e28eb5b..c1f2605 100644
--- a/tools/kvm/builtin-stat.c
+++ b/tools/kvm/builtin-stat.c
@@ -9,6 +9,8 @@ 
 #include <stdio.h>
 #include <string.h>
 #include <signal.h>
+#define __STDC_FORMAT_MACROS
+#include <inttypes.h>
 
 #include <linux/virtio_balloon.h>
 
@@ -97,7 +99,7 @@  static int do_memstat(const char *name, int sock)
 			printf("The total amount of memory available (in bytes):");
 			break;
 		}
-		printf("%llu\n", stats[i].val);
+		printf("%"PRId64"\n", stats[i].val);
 	}
 	printf("\n");
 
diff --git a/tools/kvm/disk/core.c b/tools/kvm/disk/core.c
index 4915efd..a135851 100644
--- a/tools/kvm/disk/core.c
+++ b/tools/kvm/disk/core.c
@@ -4,6 +4,8 @@ 
 
 #include <sys/eventfd.h>
 #include <sys/poll.h>
+#define __STDC_FORMAT_MACROS
+#include <inttypes.h>
 
 #define AIO_MAX 32
 
@@ -232,7 +234,7 @@  ssize_t disk_image__get_serial(struct disk_image *disk, void *buffer, ssize_t *l
 	if (fstat(disk->fd, &st) != 0)
 		return 0;
 
-	*len = snprintf(buffer, *len, "%llu%llu%llu", (u64)st.st_dev, (u64)st.st_rdev, (u64)st.st_ino);
+	*len = snprintf(buffer, *len, "%"PRId64"%"PRId64"%"PRId64, (u64)st.st_dev, (u64)st.st_rdev, (u64)st.st_ino);
 	return *len;
 }
 
diff --git a/tools/kvm/mmio.c b/tools/kvm/mmio.c
index de7320f..1158bff 100644
--- a/tools/kvm/mmio.c
+++ b/tools/kvm/mmio.c
@@ -9,6 +9,8 @@ 
 #include <linux/kvm.h>
 #include <linux/types.h>
 #include <linux/rbtree.h>
+#define __STDC_FORMAT_MACROS
+#include <inttypes.h>
 
 #define mmio_node(n) rb_entry(n, struct mmio_mapping, node)
 
@@ -124,7 +126,7 @@  bool kvm__emulate_mmio(struct kvm *kvm, u64 phys_addr, u8 *data, u32 len, u8 is_
 	if (mmio)
 		mmio->mmio_fn(phys_addr, data, len, is_write, mmio->ptr);
 	else
-		fprintf(stderr, "Warning: Ignoring MMIO %s at %016llx (length %u)\n",
+		fprintf(stderr, "Warning: Ignoring MMIO %s at %016"PRIx64" (length %u)\n",
 			to_direction(is_write), phys_addr, len);
 	br_read_unlock();