Re: [PATCH 02/28] kvm tools: Only build/init i8042 on x86

2011-12-06 Thread Matt Evans
On 07/12/11 05:59, Scott Wood wrote:
> On 12/05/2011 09:37 PM, Matt Evans wrote:
>> Not every architecture has an i8042 kbd controller, so only use this when
>> building for x86.
> 
> There are non-x86 machines that have one, though -- does KVM tool have
> any sort of target configuration mechanism?

See my other mail (of 1m ago), there isn't a configuration mechanism nor a way
to select devices at run-time (nor buses to attach them to, etc.).  This patch
is a bit of a hack, though there's no other way without the extra
infrastructure.  This patch will get a 2nd platform going, and can be revisited
when another rears its head.

Cheers,


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


Re: [PATCH 1/8] kvm tools: Add initial SPAPR PPC64 architecture support

2011-12-06 Thread Matt Evans
Hi Scott,

On 07/12/11 05:03, Scott Wood wrote:
> On 12/05/2011 10:05 PM, Matt Evans wrote:
>> This patch adds a new arch directory, powerpc, basic file structure, register
>> setup and where necessary stubs out arch-specific functions (e.g. interrupts,
>> runloop exits) that later patches will provide.  The target is an
>> SPAPR-compliant PPC64 machine (i.e. pSeries); there is no support for PPC32 
>> or
>> 'bare metal' PPC64 guests as yet.  Subsequent patches implement the hcalls 
>> and
>> RTAS required to boot SPAPR pSeries kernels.
> 
> You just sent out 28 patches removing "everything is x86"
> dependencies -- may I suggest that the PPC code be structured so that
> there isn't an "everything on PPC (or even PPC64) is SPAPR" assumption,
> even if SPAPR is initially the only sub-arch present?

I had anticipated this comment (though not the "28 patches" remark, easy now).
It is a fair comment, but you hit the nail on the head with your other mail
(regarding configuring in i8042, presumably to emulate crappy dev boards)
asking whether kvmtool has a config system.  It does not.

Since we currently lack any kind of build-time configuration (or any fancy
run-time -M  a la QEMU) it's a bit hard to cater for multiple
platforms.  I'm aware that the PPC patches are painfully PPC64-with-SPAPR and I
don't present them as perfect, but I really think we need to attack the
configuration stuff before bifurcating.  Is this something you'd like to see to?

(Your comments on the #defines and magic accepted & fixed, thank you.)


Cheers,


Matt



> 
> E.g. this is SPAPR-specific despite being in generically-named
> tools/kvm/powerpc/kvm-cpu.c:
> 
>> +static void kvm_cpu__setup_regs(struct kvm_cpu *vcpu)
>> +{
>> +struct kvm_regs *r = &vcpu->regs;
>> +
>> +if (vcpu->cpu_id == 0) {
>> +r->pc = KERNEL_START_ADDR;
>> +r->gpr[3] = vcpu->kvm->fdt_gra;
>> +r->gpr[5] = 0;
>> +} else {
>> +r->pc = KERNEL_SECONDARY_START_ADDR;
>> +r->gpr[3] = vcpu->cpu_id;
>> +}
>> +r->msr = 0x80001000UL; /* 64bit, non-HV, ME */
>> +
>> +if (ioctl(vcpu->vcpu_fd, KVM_SET_REGS, &vcpu->regs) < 0)
>> +die_perror("KVM_SET_REGS failed");
>> +}
> 
>> diff --git a/tools/kvm/powerpc/include/kvm/kvm-arch.h 
>> b/tools/kvm/powerpc/include/kvm/kvm-arch.h
>> new file mode 100644
>> index 000..722d01c
>> --- /dev/null
>> +++ b/tools/kvm/powerpc/include/kvm/kvm-arch.h
>> @@ -0,0 +1,70 @@
>> +/*
>> + * PPC64 architecture-specific definitions
>> + *
>> + * Copyright 2011 Matt Evans , IBM Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published
>> + * by the Free Software Foundation.
>> + */
>> +
>> +#ifndef KVM__KVM_ARCH_H
>> +#define KVM__KVM_ARCH_H
> [snip]
>> +void ioport__setup_arch(void)
> [snip]
>> +int irq__register_device(u32 dev, u8 *num, u8 *pin, u8 *line)
> 
> I'm seeing a lot of double-underscores -- is this common style in KVM
> tool?  It's reserved for use by the compiler and system library.  It's
> common in the kernel (though not used like this for namespace
> prefixes), but there's no system library involved there.
> 
>> diff --git a/tools/kvm/powerpc/kvm-cpu.c b/tools/kvm/powerpc/kvm-cpu.c
>> new file mode 100644
>> index 000..79422ff
>> --- /dev/null
>> +++ b/tools/kvm/powerpc/kvm-cpu.c
> [snip]
>> +#define MSR_SF  (1UL<<63)
>> +#define MSR_HV  (1UL<<60)
>> +#define MSR_VEC (1UL<<25)
>> +#define MSR_VSX (1UL<<23)
>> +#define MSR_POW (1UL<<18)
>> +#define MSR_EE  (1UL<<15)
>> +#define MSR_PR  (1UL<<14)
>> +#define MSR_FP  (1UL<<13)
>> +#define MSR_ME  (1UL<<12)
>> +#define MSR_FE0 (1UL<<11)
>> +#define MSR_SE  (1UL<<10)
>> +#define MSR_BE  (1UL<<9)
>> +#define MSR_FE1 (1UL<<8)
>> +#define MSR_IR  (1UL<<5)
>> +#define MSR_DR  (1UL<<4)
>> +#define MSR_PMM (1UL<<2)
>> +#define MSR_RI  (1UL<<1)
>> +#define MSR_LE  (1UL<<0)
> 
> Shouldn't these go in a header?
> 
>> +#define HUGETLBFS_MAGIC   0x958458f6
> 
> #include 
> 
> ?
> 
> -Scott

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


Re: [PATCH 07/28] kvm tools: Move 'kvm__recommended_cpus' to arch-specific code

2011-12-06 Thread Matt Evans
On 07/12/11 18:24, Alexander Graf wrote:
> 
> On 07.12.2011, at 08:19, Matt Evans  wrote:
> 
>> On 07/12/11 17:34, Sasha Levin wrote:
>>> On Wed, 2011-12-07 at 17:17 +1100, Matt Evans wrote:
 On 06/12/11 19:20, Sasha Levin wrote:
> Why is it getting moved out of generic code?
>
> This is used to determine the maximum amount of vcpus supported by the
> host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
> KVM_CAP_MAX_VCPUS are not arch specific.

 I checked api.txt and you're right, it isn't arch-specific.  I assumed it 
 was,
 because PPC KVM doesn't support it ;-) I've dropped this patch and in its 
 place
 implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" 
 instead
 of die(); you'll see that when I repost.

 This will have the effect of PPC being limited to 4 CPUs until the kernel
 supports that CAP.  (I'll see about this part too.)
>>>
>>> I went to look at which limitation PPC places on amount of vcpus in
>>> guest, and saw this in kvmppc_core_vcpu_create() in the book3s code:
>>>
>>>vcpu = kvmppc_core_vcpu_create(kvm, id);
>>>vcpu->arch.wqp = &vcpu->wq;
>>>if (!IS_ERR(vcpu))
>>>kvmppc_create_vcpu_debugfs(vcpu, id);
>>>
>>> This is wrong, right? The VCPU is dereferenced before actually checking
>>> that it's not an error.
>>
>> Yeah, that's b0rk.  Alex, a patch below. :)
> 
> Thanks :). Will apply asap but don't have a real keyboard today :).

Ha!  Voice control on your phone, what could go wrong?

> I suppose this is stable material?

Good idea, (and if we're formal,
Signed-off-by: Matt Evans 
).  I suppose no one's seen a vcpu fail to be created, yet.


Thanks,

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


Re: [PATCH 07/28] kvm tools: Move 'kvm__recommended_cpus' to arch-specific code

2011-12-06 Thread Alexander Graf

On 07.12.2011, at 08:19, Matt Evans  wrote:

> On 07/12/11 17:34, Sasha Levin wrote:
>> On Wed, 2011-12-07 at 17:17 +1100, Matt Evans wrote:
>>> On 06/12/11 19:20, Sasha Levin wrote:
 Why is it getting moved out of generic code?
 
 This is used to determine the maximum amount of vcpus supported by the
 host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
 KVM_CAP_MAX_VCPUS are not arch specific.
>>> 
>>> I checked api.txt and you're right, it isn't arch-specific.  I assumed it 
>>> was,
>>> because PPC KVM doesn't support it ;-) I've dropped this patch and in its 
>>> place
>>> implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" 
>>> instead
>>> of die(); you'll see that when I repost.
>>> 
>>> This will have the effect of PPC being limited to 4 CPUs until the kernel
>>> supports that CAP.  (I'll see about this part too.)
>> 
>> I went to look at which limitation PPC places on amount of vcpus in
>> guest, and saw this in kvmppc_core_vcpu_create() in the book3s code:
>> 
>>vcpu = kvmppc_core_vcpu_create(kvm, id);
>>vcpu->arch.wqp = &vcpu->wq;
>>if (!IS_ERR(vcpu))
>>kvmppc_create_vcpu_debugfs(vcpu, id);
>> 
>> This is wrong, right? The VCPU is dereferenced before actually checking
>> that it's not an error.
> 
> Yeah, that's b0rk.  Alex, a patch below. :)

Thanks :). Will apply asap but don't have a real keyboard today :).

I suppose this is stable material?

Alex

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


Re: [PATCH 07/28] kvm tools: Move 'kvm__recommended_cpus' to arch-specific code

2011-12-06 Thread Matt Evans
On 07/12/11 17:34, Sasha Levin wrote:
> On Wed, 2011-12-07 at 17:17 +1100, Matt Evans wrote:
>> On 06/12/11 19:20, Sasha Levin wrote:
>>> Why is it getting moved out of generic code?
>>>
>>> This is used to determine the maximum amount of vcpus supported by the
>>> host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
>>> KVM_CAP_MAX_VCPUS are not arch specific.
>>
>> I checked api.txt and you're right, it isn't arch-specific.  I assumed it 
>> was,
>> because PPC KVM doesn't support it ;-) I've dropped this patch and in its 
>> place
>> implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" 
>> instead
>> of die(); you'll see that when I repost.
>>
>> This will have the effect of PPC being limited to 4 CPUs until the kernel
>> supports that CAP.  (I'll see about this part too.)
> 
> I went to look at which limitation PPC places on amount of vcpus in
> guest, and saw this in kvmppc_core_vcpu_create() in the book3s code:
> 
>   vcpu = kvmppc_core_vcpu_create(kvm, id);
>   vcpu->arch.wqp = &vcpu->wq;
>   if (!IS_ERR(vcpu))
>   kvmppc_create_vcpu_debugfs(vcpu, id);
> 
> This is wrong, right? The VCPU is dereferenced before actually checking
> that it's not an error.

Yeah, that's b0rk.  Alex, a patch below. :)


Cheers,


Matt

---
Subject: [PATCH] KVM: PPC: Fix vcpu_create dereference before validity check.


Signed-off-by: Matt Evans 
---
 arch/powerpc/kvm/powerpc.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 084d1c5..7c7220c 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -285,9 +285,10 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, 
unsigned int id)
 {
struct kvm_vcpu *vcpu;
vcpu = kvmppc_core_vcpu_create(kvm, id);
-   vcpu->arch.wqp = &vcpu->wq;
-   if (!IS_ERR(vcpu))
+   if (!IS_ERR(vcpu)) {
+   vcpu->arch.wqp = &vcpu->wq;
kvmppc_create_vcpu_debugfs(vcpu, id);
+   }
return vcpu;
 }
 
-- 
1.7.0.4

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


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

2011-12-06 Thread Matt Evans
Hi Ingo,


On 06/12/11 21:24, Ingo Molnar wrote:
> 
> * Paul Mackerras  wrote:
> 
>> On Tue, Dec 06, 2011 at 09:28:27AM +0100, Ingo Molnar wrote:
>>>
>>> * Sasha Levin  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 
>  #else
>  # include 
>  #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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 08/28] kvm tools: Fix KVM_RUN exit code check

2011-12-06 Thread Alexander Graf

On 07.12.2011, at 01:32, Matt Evans  wrote:

> On 06/12/11 19:22, Sasha Levin wrote:
>> If KVM_RUN can actually return anything besides 0 or -1 it may be also
>> worthwhile to update Documentation/virtual/kvm/api.txt .
>> 
>> What are the cases where it happens?
> 
> Well, on PPC the internal kvmppc_run_vcpu() returns either RESUME_GUEST (which
> stays in-kernel and drops back to the guest) or RESUME_HOST, which is 
> propagated
> back out to userland as the return value of ioctl(KVM_RUN).  So, anything
> kvmtool sees is either <0 for error or RESUME_HOST, i.e. 2.
> 
> Alex, do you think the PPC KVM code should be forced to 0 on success, or is
> there any value to the expanded the return codes (and updating api.txt) for
> varying kinds of positive success?

I don't think it's worth the potential ABI breakage to change the current 
behavior :). Even if we did change it, you would still have to touch kvm tool 
to work with older kernels.

Alex

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


Re: [PATCH 28/28] kvm tools: Create arch-specific kvm_cpu__emulate_io()

2011-12-06 Thread Matt Evans
On 06/12/11 19:54, Sasha Levin wrote:
> Can we possibly do it by getting the generic code to call both
> 'kvm_cpu__arch_emulate_io' and 'kvm_cpu__arch_emulate_mmio', and have
> the ppc code have an empty static for 'kvm_cpu__arch_emulate_io'?

Yeah that's nicer, I'll make that change... less invasive.


Cheers,


Matt


> 
> On Tue, 2011-12-06 at 14:43 +1100, Matt Evans wrote:
>> Different architectures will deal with MMIO exits differently.  For example,
>> KVM_EXIT_IO is x86-specific, and I/O cycles are often synthesisted by 
>> steering
>> into windows in PCI bridges on other architectures.
>>
>> This patch moves the IO/MMIO exit code from the main runloop into 
>> x86/kvm-cpu.c
>>
>> Signed-off-by: Matt Evans 
>> ---
>>  tools/kvm/include/kvm/kvm-cpu.h |1 +
>>  tools/kvm/kvm-cpu.c |   37 +
>>  tools/kvm/x86/kvm-cpu.c |   37 +
>>  3 files changed, 43 insertions(+), 32 deletions(-)
>>
>> diff --git a/tools/kvm/include/kvm/kvm-cpu.h 
>> b/tools/kvm/include/kvm/kvm-cpu.h
>> index 15618f1..6f38c0c 100644
>> --- a/tools/kvm/include/kvm/kvm-cpu.h
>> +++ b/tools/kvm/include/kvm/kvm-cpu.h
>> @@ -13,6 +13,7 @@ void kvm_cpu__run(struct kvm_cpu *vcpu);
>>  void kvm_cpu__reboot(void);
>>  int kvm_cpu__start(struct kvm_cpu *cpu);
>>  bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu);
>> +bool kvm_cpu__emulate_io(struct kvm_cpu *cpu, struct kvm_run *kvm_run);
>>  
>>  int kvm_cpu__get_debug_fd(void);
>>  void kvm_cpu__set_debug_fd(int fd);
>> diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
>> index 884a89f..c9fbc81 100644
>> --- a/tools/kvm/kvm-cpu.c
>> +++ b/tools/kvm/kvm-cpu.c
>> @@ -103,49 +103,22 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
>>  kvm_cpu__show_registers(cpu);
>>  kvm_cpu__show_code(cpu);
>>  break;
>> -case KVM_EXIT_IO: {
>> -bool ret;
>> -
>> -ret = kvm__emulate_io(cpu->kvm,
>> -cpu->kvm_run->io.port,
>> -(u8 *)cpu->kvm_run +
>> -cpu->kvm_run->io.data_offset,
>> -cpu->kvm_run->io.direction,
>> -cpu->kvm_run->io.size,
>> -cpu->kvm_run->io.count);
>> -
>> -if (!ret)
>> +case KVM_EXIT_IO:
>> +case KVM_EXIT_MMIO:
>> +if (!kvm_cpu__emulate_io(cpu, cpu->kvm_run))
>>  goto panic_kvm;
>>  break;
>> -}
>> -case KVM_EXIT_MMIO: {
>> -bool ret;
>> -
>> -ret = kvm__emulate_mmio(cpu->kvm,
>> -cpu->kvm_run->mmio.phys_addr,
>> -cpu->kvm_run->mmio.data,
>> -cpu->kvm_run->mmio.len,
>> -cpu->kvm_run->mmio.is_write);
>> -
>> -if (!ret)
>> -goto panic_kvm;
>> -break;
>> -}
>>  case KVM_EXIT_INTR:
>>  if (cpu->is_running)
>>  break;
>>  goto exit_kvm;
>>  case KVM_EXIT_SHUTDOWN:
>>  goto exit_kvm;
>> -default: {
>> -bool ret;
>> -
>> -ret = kvm_cpu__handle_exit(cpu);
>> -if (!ret)
>> +default:
>> +if (!kvm_cpu__handle_exit(cpu))
>>  goto panic_kvm;
>>  break;
>>  }
>> -}
>>  kvm_cpu__handle_coalesced_mmio(cpu);
>>  }
>>  
>> diff --git a/tools/kvm/x86/kvm-cpu.c b/tools/kvm/x86/kvm-cpu.c
>> index a0d10cc..665d742 100644
>> --- a/tools/kvm/x86/kvm-cpu.c
>> +++ b/tools/kvm/x86/kvm-cpu.c
>> @@ -217,6 +217,43 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>  return false;
>>  }
>>  
>> +bool kvm_cpu__emulate_io(struct kvm_cpu *cpu, struct kvm_run *kvm_run)
>> +{
>> +bool ret;
>> +switch (kvm_run->exit_reason) {
>> +case KVM_EXIT_IO: {
>> +ret = kvm__emulate_io(cpu->kvm,
>> +  cpu->kvm_run->io.port,
>> +  (u8 *)cpu->kvm_run +
>> +  cpu->kvm_run->io.data_offset,
>> +  cpu->kvm_run->io.direction,
>> +  cpu->kvm_run->io.size,
>> +  cpu->kvm_run->io.count);
>> +
>> +if (!ret)
>> +goto panic_kvm;
>> +break;
>> +}
>> +case KVM_EXIT_MMIO: {
>> +ret = kvm__emulate_mmio(cpu->kvm,
>> +cpu->kvm_run->mmio.phy

Re: [PATCH 07/28] kvm tools: Move 'kvm__recommended_cpus' to arch-specific code

2011-12-06 Thread Sasha Levin
On Wed, 2011-12-07 at 17:17 +1100, Matt Evans wrote:
> On 06/12/11 19:20, Sasha Levin wrote:
> > Why is it getting moved out of generic code?
> > 
> > This is used to determine the maximum amount of vcpus supported by the
> > host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
> > KVM_CAP_MAX_VCPUS are not arch specific.
> 
> I checked api.txt and you're right, it isn't arch-specific.  I assumed it was,
> because PPC KVM doesn't support it ;-) I've dropped this patch and in its 
> place
> implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" 
> instead
> of die(); you'll see that when I repost.
> 
> This will have the effect of PPC being limited to 4 CPUs until the kernel
> supports that CAP.  (I'll see about this part too.)

I went to look at which limitation PPC places on amount of vcpus in
guest, and saw this in kvmppc_core_vcpu_create() in the book3s code:

vcpu = kvmppc_core_vcpu_create(kvm, id);
vcpu->arch.wqp = &vcpu->wq;
if (!IS_ERR(vcpu))
kvmppc_create_vcpu_debugfs(vcpu, id);

This is wrong, right? The VCPU is dereferenced before actually checking
that it's not an error.

-- 

Sasha.

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


Re: [PATCH 16/28] kvm tools: Allow load_flat_binary() to load an initrd alongside

2011-12-06 Thread Cyrill Gorcunov
On Wed, Dec 07, 2011 at 11:42:27AM +1100, Matt Evans wrote:
> Hi Cyrill,
> 
> On 06/12/11 23:04, Cyrill Gorcunov wrote:
> > On Tue, Dec 06, 2011 at 12:29:48PM +0200, Pekka Enberg wrote:
> > ...
> >>
> >> Otherwise looks OK to me. Cyrill?
> >>
> > 
> > It might be not seen from patch (or my local kvm repo
> > is not yet updated well) but I somehow miss who will be
> > reading initrd in case of loading flat image? If noone,
> > then what's the point to pass fd there at all?
> 
> I pass in the initrd fd in generic code in preparation for the PPC support in
> "[PATCH 1/8] kvm tools: Add initial SPAPR PPC64 architecture support" which 
> uses
> it.  As you saw, I haven't made x86 load the initrd in the case of a flat 
> kernel
> binary being loaded; I didn't think a flat kernel can have a non-embedded 
> initrd
> on x86?  (My x86 is rusty, this may not be true ;) bootparams are [b]zImage
> only?)
> 

Ah, I see. Sorry for confusion. Thanks.

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


Re: [PATCH 07/28] kvm tools: Move 'kvm__recommended_cpus' to arch-specific code

2011-12-06 Thread Matt Evans
On 06/12/11 19:20, Sasha Levin wrote:
> Why is it getting moved out of generic code?
> 
> This is used to determine the maximum amount of vcpus supported by the
> host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
> KVM_CAP_MAX_VCPUS are not arch specific.

I checked api.txt and you're right, it isn't arch-specific.  I assumed it was,
because PPC KVM doesn't support it ;-) I've dropped this patch and in its place
implemented the api.txt suggestion of "if KVM_CAP_NR_VCPUS fails, use 4" instead
of die(); you'll see that when I repost.

This will have the effect of PPC being limited to 4 CPUs until the kernel
supports that CAP.  (I'll see about this part too.)


Thanks,


Matt


> 
> On Tue, 2011-12-06 at 14:39 +1100, Matt Evans wrote:
>> Architectures can recommend/count/determine number of CPUs differently, so 
>> move
>> this out of generic code.
>>
>> Signed-off-by: Matt Evans 
>> ---
>>  tools/kvm/kvm.c |   30 --
>>  tools/kvm/x86/kvm.c |   30 ++
>>  2 files changed, 30 insertions(+), 30 deletions(-)
>>
>> diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
>> index 7ce1640..e526483 100644
>> --- a/tools/kvm/kvm.c
>> +++ b/tools/kvm/kvm.c
>> @@ -259,17 +259,6 @@ void kvm__register_mem(struct kvm *kvm, u64 guest_phys, 
>> u64 size, void *userspac
>>  die_perror("KVM_SET_USER_MEMORY_REGION ioctl");
>>  }
>>  
>> -int kvm__recommended_cpus(struct kvm *kvm)
>> -{
>> -int ret;
>> -
>> -ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS);
>> -if (ret <= 0)
>> -die_perror("KVM_CAP_NR_VCPUS");
>> -
>> -return ret;
>> -}
>> -
>>  static void kvm__pid(int fd, u32 type, u32 len, u8 *msg)
>>  {
>>  pid_t pid = getpid();
>> @@ -282,25 +271,6 @@ static void kvm__pid(int fd, u32 type, u32 len, u8 *msg)
>>  pr_warning("Failed sending PID");
>>  }
>>  
>> -/*
>> - * The following hack should be removed once 'x86: Raise the hard
>> - * VCPU count limit' makes it's way into the mainline.
>> - */
>> -#ifndef KVM_CAP_MAX_VCPUS
>> -#define KVM_CAP_MAX_VCPUS 66
>> -#endif
>> -
>> -int kvm__max_cpus(struct kvm *kvm)
>> -{
>> -int ret;
>> -
>> -ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS);
>> -if (ret <= 0)
>> -ret = kvm__recommended_cpus(kvm);
>> -
>> -return ret;
>> -}
>> -
>>  struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, const char *name)
>>  {
>>  struct kvm *kvm;
>> diff --git a/tools/kvm/x86/kvm.c b/tools/kvm/x86/kvm.c
>> index ac6c91e..75e4a52 100644
>> --- a/tools/kvm/x86/kvm.c
>> +++ b/tools/kvm/x86/kvm.c
>> @@ -76,6 +76,36 @@ bool kvm__arch_cpu_supports_vm(void)
>>  return regs.ecx & (1 << feature);
>>  }
>>  
>> +int kvm__recommended_cpus(struct kvm *kvm)
>> +{
>> +int ret;
>> +
>> +ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS);
>> +if (ret <= 0)
>> +die_perror("KVM_CAP_NR_VCPUS");
>> +
>> +return ret;
>> +}
>> +
>> +/*
>> + * The following hack should be removed once 'x86: Raise the hard
>> + * VCPU count limit' makes it's way into the mainline.
>> + */
>> +#ifndef KVM_CAP_MAX_VCPUS
>> +#define KVM_CAP_MAX_VCPUS 66
>> +#endif
>> +
>> +int kvm__max_cpus(struct kvm *kvm)
>> +{
>> +int ret;
>> +
>> +ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS);
>> +if (ret <= 0)
>> +ret = kvm__recommended_cpus(kvm);
>> +
>> +return ret;
>> +}
>> +
>>  /*
>>   * Allocating RAM size bigger than 4GB requires us to leave a gap
>>   * in the RAM which is used for PCI MMIO, hotplug, and unconfigured
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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


Re: [PATCH 21/28] kvm tools: Add --hugetlbfs option to specify memory path

2011-12-06 Thread Sasha Levin
On Wed, 2011-12-07 at 11:35 +1100, Matt Evans wrote:
> On 06/12/11 19:32, Sasha Levin wrote:
> > I'm seeing hugetlbfs_path being passed around, but I don't see anything
> > that actually does something with it.
> > 
> > Did it get into a different patch? or maybe it wasn't 'git add'ed?
> 
> It's actually moved in preparation for the later "[PATCH 1/8] kvm tools: Add
> initial SPAPR PPC64 architecture support" patch, which uses it.  I could've
> mushed that into the later PPC series but I thought it'd be clearer to do the
> generic/x86 stuff separately.

I actually would like to see everything moved into the generic code out
of the PPC series :)

hugetlb allocation should probably be done in the generic code as it
would work on x86 as well.

-- 

Sasha.

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


Re: [PATCH 17/28] kvm tools: Only call symbol__init() if we have BFD

2011-12-06 Thread Matt Evans
On 06/12/11 19:26, Sasha Levin wrote:
> It's optional, but when CONFIG_HAS_BFD is not defined symbol__init() is
> defined as an empty static function.
> 
> Why was there a need to wrap it in a #ifdef here?

That's an excellent question and I think the true answer is just embarrassing so
I'll just drop this patch from the series. ;-)

Thanks,


Matt

> 
> On Tue, 2011-12-06 at 14:41 +1100, Matt Evans wrote:
>> CONFIG_HAS_BFD is optional, symbol.c inclusion is optional -- so make its 
>> init
>> call dependent on CONFIG_HAS_BFD.
>>
>> Signed-off-by: Matt Evans 
>> ---
>>  tools/kvm/builtin-run.c |3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
>> index 1257c90..aaa5132 100644
>> --- a/tools/kvm/builtin-run.c
>> +++ b/tools/kvm/builtin-run.c
>> @@ -798,8 +798,9 @@ int kvm_cmd_run(int argc, const char **argv, const char 
>> *prefix)
>>  if (!script)
>>  script = DEFAULT_SCRIPT;
>>  
>> +#ifdef CONFIG_HAS_BFD
>>  symbol__init(vmlinux_filename);
>> -
>> +#endif
>>  term_init();
>>  
>>  if (!guest_name) {
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Matt Evans
On 07/12/11 00:38, Cyrill Gorcunov wrote:
> On Tue, Dec 06, 2011 at 03:29:00PM +0200, Pekka Enberg wrote:
>>>
>>> Hehe, this is because it should be rtaher defined as
>>>
>>> union pci_config_address {
>>> struct {
>>> #if __BYTE_ORDER == __LITTLE_ENDIAN
>>>   unsignedzeros   : 2;
>>>   unsignedregister_number : 6;
>>> #else
>>>   ...
>>> #endif
>>> }
>>> u32 w;
>>> };
>>
>> Yup, that fixes it for me.
>>
> 
> Good. Matt, mind to update?

Absolutely -- thank you for digging in and debugging that!  I'll repost a V2.


Cheers,


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


Re: [PATCH 14/28] kvm tools: Fix term_getc(), term_getc_iov() endian bugs

2011-12-06 Thread Matt Evans
Hi Asias,

On 06/12/11 23:00, Asias He wrote:
> On 12/06/2011 06:24 PM, Pekka Enberg wrote:
>> On Tue, Dec 6, 2011 at 5:40 AM, Matt Evans  wrote:
>>> term_getc()'s int c has one byte written into it (at its lowest address) by
>>> read_in_full().  This is expected to be the least significant byte, but that
>>> isn't the case on BE!  Use correct type, unsigned char.  A similar issue 
>>> exists
>>> in term_getc_iov(), which needs to write a char to the iov rather than an 
>>> int.
>>>
>>> Signed-off-by: Matt Evans 
>>> ---
>>>  tools/kvm/term.c |5 ++---
>>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/kvm/term.c b/tools/kvm/term.c
>>> index fb5d71c..440884e 100644
>>> --- a/tools/kvm/term.c
>>> +++ b/tools/kvm/term.c
>>> @@ -30,11 +30,10 @@ int term_fds[4][2];
>>>
>>>  int term_getc(int who, int term)
>>>  {
>>> -   int c;
>>> +   unsigned char c;
>>>
>>>if (who != active_console)
>>>return -1;
>>> -
> 
> We can drop this line too.
> 
>   c &= 0xff;

D'oh!  Of course it can -- I'll remove it & repost.

>>>if (read_in_full(term_fds[term][TERM_FD_IN], &c, 1) < 0)
>>>return -1;
>>>
>>> @@ -84,7 +83,7 @@ int term_getc_iov(int who, struct iovec *iov, int iovcnt, 
>>> int term)
>>>if (c < 0)
>>>return 0;
>>>
>>> -   *((int *)iov[TERM_FD_IN].iov_base)  = c;
>>> +   *((char *)iov[TERM_FD_IN].iov_base) = (char)c;
>>>
>>>return sizeof(char);
>>>  }
>>
>> Looks OK to me. Asias?
>>
> 
> Otherwise, looks fine to me.

Thanks for reviewing!


Matt

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


Re: [PATCH 16/28] kvm tools: Allow load_flat_binary() to load an initrd alongside

2011-12-06 Thread Matt Evans
Hi Cyrill,

On 06/12/11 23:04, Cyrill Gorcunov wrote:
> On Tue, Dec 06, 2011 at 12:29:48PM +0200, Pekka Enberg wrote:
> ...
>>
>> Otherwise looks OK to me. Cyrill?
>>
> 
> It might be not seen from patch (or my local kvm repo
> is not yet updated well) but I somehow miss who will be
> reading initrd in case of loading flat image? If noone,
> then what's the point to pass fd there at all?

I pass in the initrd fd in generic code in preparation for the PPC support in
"[PATCH 1/8] kvm tools: Add initial SPAPR PPC64 architecture support" which uses
it.  As you saw, I haven't made x86 load the initrd in the case of a flat kernel
binary being loaded; I didn't think a flat kernel can have a non-embedded initrd
on x86?  (My x86 is rusty, this may not be true ;) bootparams are [b]zImage
only?)


Thanks for reviewing,


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


Re: [PATCH 21/28] kvm tools: Add --hugetlbfs option to specify memory path

2011-12-06 Thread Matt Evans
On 06/12/11 19:32, Sasha Levin wrote:
> I'm seeing hugetlbfs_path being passed around, but I don't see anything
> that actually does something with it.
> 
> Did it get into a different patch? or maybe it wasn't 'git add'ed?

It's actually moved in preparation for the later "[PATCH 1/8] kvm tools: Add
initial SPAPR PPC64 architecture support" patch, which uses it.  I could've
mushed that into the later PPC series but I thought it'd be clearer to do the
generic/x86 stuff separately.


Matt

> 
> On Tue, 2011-12-06 at 14:41 +1100, Matt Evans wrote:
>> Some architectures may want to use hugetlbfs to mmap() their guest memory, so
>> allow a path to be specified on the commandline and pass it to 
>> kvm__arch_init().
>>
>> Signed-off-by: Matt Evans 
>> ---
>>  tools/kvm/builtin-run.c |4 +++-
>>  tools/kvm/include/kvm/kvm.h |4 ++--
>>  tools/kvm/kvm.c |4 ++--
>>  tools/kvm/x86/kvm.c |2 +-
>>  4 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
>> index 84aa931..4c88169 100644
>> --- a/tools/kvm/builtin-run.c
>> +++ b/tools/kvm/builtin-run.c
>> @@ -84,6 +84,7 @@ static const char *guest_mac;
>>  static const char *host_mac;
>>  static const char *script;
>>  static const char *guest_name;
>> +static const char *hugetlbfs_path;
>>  static struct virtio_net_params *net_params;
>>  static bool single_step;
>>  static bool readonly_image[MAX_DISK_IMAGES];
>> @@ -422,6 +423,7 @@ static const struct option options[] = {
>>  OPT_CALLBACK('\0', "tty", NULL, "tty id",
>>   "Remap guest TTY into a pty on the host",
>>   tty_parser),
>> +OPT_STRING('\0', "hugetlbfs", &hugetlbfs_path, "path", "Hugetlbfs 
>> path"),
>>  
>>  OPT_GROUP("Kernel options:"),
>>  OPT_STRING('k', "kernel", &kernel_filename, "kernel",
>> @@ -808,7 +810,7 @@ int kvm_cmd_run(int argc, const char **argv, const char 
>> *prefix)
>>  guest_name = default_name;
>>  }
>>  
>> -kvm = kvm__init(dev, ram_size, guest_name);
>> +kvm = kvm__init(dev, hugetlbfs_path, ram_size, guest_name);
>>  
>>  kvm->single_step = single_step;
>>  
>> diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
>> index 5fe6e75..7159952 100644
>> --- a/tools/kvm/include/kvm/kvm.h
>> +++ b/tools/kvm/include/kvm/kvm.h
>> @@ -30,7 +30,7 @@ struct kvm_ext {
>>  void kvm__set_dir(const char *fmt, ...);
>>  const char *kvm__get_dir(void);
>>  
>> -struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, const char *name);
>> +struct kvm *kvm__init(const char *kvm_dev, const char *hugetlbfs_path, u64 
>> ram_size, const char *name);
>>  int kvm__recommended_cpus(struct kvm *kvm);
>>  int kvm__max_cpus(struct kvm *kvm);
>>  void kvm__init_ram(struct kvm *kvm);
>> @@ -54,7 +54,7 @@ int kvm__enumerate_instances(int (*callback)(const char 
>> *name, int pid));
>>  void kvm__remove_socket(const char *name);
>>  
>>  void kvm__arch_set_cmdline(char *cmdline, bool video);
>> -void kvm__arch_init(struct kvm *kvm, const char *kvm_dev, u64 ram_size, 
>> const char *name);
>> +void kvm__arch_init(struct kvm *kvm, const char *kvm_dev, const char 
>> *hugetlbfs_path, u64 ram_size, const char *name);
>>  void kvm__arch_setup_firmware(struct kvm *kvm);
>>  bool kvm__arch_cpu_supports_vm(void);
>>  void kvm__arch_periodic_poll(struct kvm *kvm);
>> diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
>> index 6f33e1a..503ceae 100644
>> --- a/tools/kvm/kvm.c
>> +++ b/tools/kvm/kvm.c
>> @@ -272,7 +272,7 @@ static void kvm__pid(int fd, u32 type, u32 len, u8 *msg)
>>  pr_warning("Failed sending PID");
>>  }
>>  
>> -struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, const char *name)
>> +struct kvm *kvm__init(const char *kvm_dev, const char *hugetlbfs_path, u64 
>> ram_size, const char *name)
>>  {
>>  struct kvm *kvm;
>>  int ret;
>> @@ -305,7 +305,7 @@ struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, 
>> const char *name)
>>  if (kvm__check_extensions(kvm))
>>  die("A required KVM extention is not supported by OS");
>>  
>> -kvm__arch_init(kvm, kvm_dev, ram_size, name);
>> +kvm__arch_init(kvm, kvm_dev, hugetlbfs_path, ram_size, name);
>>  
>>  kvm->name = name;
>>  
>> diff --git a/tools/kvm/x86/kvm.c b/tools/kvm/x86/kvm.c
>> index 4ac21c0..76f805f 100644
>> --- a/tools/kvm/x86/kvm.c
>> +++ b/tools/kvm/x86/kvm.c
>> @@ -161,7 +161,7 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>>  }
>>  
>>  /* Architecture-specific KVM init */
>> -void kvm__arch_init(struct kvm *kvm, const char *kvm_dev, u64 ram_size, 
>> const char *name)
>> +void kvm__arch_init(struct kvm *kvm, const char *kvm_dev, const char 
>> *hugetlbfs_path, u64 ram_size, const char *name)
>>  {
>>  struct kvm_pit_config pit_config = { .flags = 0, };
>>  int ret;
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majord...@vger.kern

Re: [PATCH 08/28] kvm tools: Fix KVM_RUN exit code check

2011-12-06 Thread Matt Evans
On 06/12/11 19:22, Sasha Levin wrote:
> If KVM_RUN can actually return anything besides 0 or -1 it may be also
> worthwhile to update Documentation/virtual/kvm/api.txt .
> 
> What are the cases where it happens?

Well, on PPC the internal kvmppc_run_vcpu() returns either RESUME_GUEST (which
stays in-kernel and drops back to the guest) or RESUME_HOST, which is propagated
back out to userland as the return value of ioctl(KVM_RUN).  So, anything
kvmtool sees is either <0 for error or RESUME_HOST, i.e. 2.

Alex, do you think the PPC KVM code should be forced to 0 on success, or is
there any value to the expanded the return codes (and updating api.txt) for
varying kinds of positive success?


Cheers,


Matt


> 
> On Tue, 2011-12-06 at 14:39 +1100, Matt Evans wrote:
>> kvm_cpu__run() currently die()s if KVM_RUN returns non-zero.  Some 
>> architectures
>> may return positive values in non-error cases, whereas real errors are always
>> negative return values.  Check for those instead.
>>
>> Signed-off-by: Matt Evans 
>> ---
>>  tools/kvm/kvm-cpu.c |2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
>> index 9bc0796..884a89f 100644
>> --- a/tools/kvm/kvm-cpu.c
>> +++ b/tools/kvm/kvm-cpu.c
>> @@ -30,7 +30,7 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
>>  int err;
>>  
>>  err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
>> -if (err && (errno != EINTR && errno != EAGAIN))
>> +if (err < 0 && (errno != EINTR && errno != EAGAIN))
>>  die_perror("KVM_RUN failed");
>>  }
>>  
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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


Re: [PATCH 02/28] kvm tools: Only build/init i8042 on x86

2011-12-06 Thread Scott Wood
On 12/05/2011 09:37 PM, Matt Evans wrote:
> Not every architecture has an i8042 kbd controller, so only use this when
> building for x86.

There are non-x86 machines that have one, though -- does KVM tool have
any sort of target configuration mechanism?

-Scott

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


Re: [PATCH 1/8] kvm tools: Add initial SPAPR PPC64 architecture support

2011-12-06 Thread Scott Wood
On 12/06/2011 12:33 PM, Pekka Enberg wrote:
> On Tue, Dec 6, 2011 at 8:03 PM, Scott Wood  wrote:
>> I'm seeing a lot of double-underscores -- is this common style in KVM
>> tool?  It's reserved for use by the compiler and system library.  It's
>> common in the kernel (though not used like this for namespace
>> prefixes), but there's no system library involved there.
> 
> Yes, they are KVM tool coding style which we took from perf. Double
> underscore _prefixes_ are reserved in userspace but there's no reason
> we can't use them in identifiers like we do.

OK, it looks like it's just C++ that also reserves non-leading double
underscores -- sorry about that.

-Scott

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


Re: [PATCH 1/8] kvm tools: Add initial SPAPR PPC64 architecture support

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 8:03 PM, Scott Wood  wrote:
> I'm seeing a lot of double-underscores -- is this common style in KVM
> tool?  It's reserved for use by the compiler and system library.  It's
> common in the kernel (though not used like this for namespace
> prefixes), but there's no system library involved there.

Yes, they are KVM tool coding style which we took from perf. Double
underscore _prefixes_ are reserved in userspace but there's no reason
we can't use them in identifiers like we do.

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


Re: [PATCH 1/8] kvm tools: Add initial SPAPR PPC64 architecture support

2011-12-06 Thread Scott Wood
On 12/05/2011 10:05 PM, Matt Evans wrote:
> This patch adds a new arch directory, powerpc, basic file structure, register
> setup and where necessary stubs out arch-specific functions (e.g. interrupts,
> runloop exits) that later patches will provide.  The target is an
> SPAPR-compliant PPC64 machine (i.e. pSeries); there is no support for PPC32 or
> 'bare metal' PPC64 guests as yet.  Subsequent patches implement the hcalls and
> RTAS required to boot SPAPR pSeries kernels.

You just sent out 28 patches removing "everything is x86"
dependencies -- may I suggest that the PPC code be structured so that
there isn't an "everything on PPC (or even PPC64) is SPAPR" assumption,
even if SPAPR is initially the only sub-arch present?

E.g. this is SPAPR-specific despite being in generically-named
tools/kvm/powerpc/kvm-cpu.c:

> +static void kvm_cpu__setup_regs(struct kvm_cpu *vcpu)
> +{
> + struct kvm_regs *r = &vcpu->regs;
> +
> + if (vcpu->cpu_id == 0) {
> + r->pc = KERNEL_START_ADDR;
> + r->gpr[3] = vcpu->kvm->fdt_gra;
> + r->gpr[5] = 0;
> + } else {
> + r->pc = KERNEL_SECONDARY_START_ADDR;
> + r->gpr[3] = vcpu->cpu_id;
> + }
> + r->msr = 0x80001000UL; /* 64bit, non-HV, ME */
> +
> + if (ioctl(vcpu->vcpu_fd, KVM_SET_REGS, &vcpu->regs) < 0)
> + die_perror("KVM_SET_REGS failed");
> +}

> diff --git a/tools/kvm/powerpc/include/kvm/kvm-arch.h 
> b/tools/kvm/powerpc/include/kvm/kvm-arch.h
> new file mode 100644
> index 000..722d01c
> --- /dev/null
> +++ b/tools/kvm/powerpc/include/kvm/kvm-arch.h
> @@ -0,0 +1,70 @@
> +/*
> + * PPC64 architecture-specific definitions
> + *
> + * Copyright 2011 Matt Evans , IBM Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#ifndef KVM__KVM_ARCH_H
> +#define KVM__KVM_ARCH_H
[snip]
> +void ioport__setup_arch(void)
[snip]
> +int irq__register_device(u32 dev, u8 *num, u8 *pin, u8 *line)

I'm seeing a lot of double-underscores -- is this common style in KVM
tool?  It's reserved for use by the compiler and system library.  It's
common in the kernel (though not used like this for namespace
prefixes), but there's no system library involved there.

> diff --git a/tools/kvm/powerpc/kvm-cpu.c b/tools/kvm/powerpc/kvm-cpu.c
> new file mode 100644
> index 000..79422ff
> --- /dev/null
> +++ b/tools/kvm/powerpc/kvm-cpu.c
[snip]
> +#define MSR_SF   (1UL<<63)
> +#define MSR_HV   (1UL<<60)
> +#define MSR_VEC  (1UL<<25)
> +#define MSR_VSX  (1UL<<23)
> +#define MSR_POW  (1UL<<18)
> +#define MSR_EE   (1UL<<15)
> +#define MSR_PR   (1UL<<14)
> +#define MSR_FP   (1UL<<13)
> +#define MSR_ME   (1UL<<12)
> +#define MSR_FE0  (1UL<<11)
> +#define MSR_SE   (1UL<<10)
> +#define MSR_BE   (1UL<<9)
> +#define MSR_FE1  (1UL<<8)
> +#define MSR_IR   (1UL<<5)
> +#define MSR_DR   (1UL<<4)
> +#define MSR_PMM  (1UL<<2)
> +#define MSR_RI   (1UL<<1)
> +#define MSR_LE   (1UL<<0)

Shouldn't these go in a header?

> +#define HUGETLBFS_MAGIC   0x958458f6

#include 

?

-Scott

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


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Cyrill Gorcunov
On Tue, Dec 06, 2011 at 03:29:00PM +0200, Pekka Enberg wrote:
> >
> >Hehe, this is because it should be rtaher defined as
> >
> >union pci_config_address {
> >struct {
> > #if __BYTE_ORDER == __LITTLE_ENDIAN
> >   unsignedzeros   : 2;
> >   unsignedregister_number : 6;
> > #else
> >   ...
> > #endif
> >}
> >u32 w;
> >};
> 
> Yup, that fixes it for me.
> 

Good. Matt, mind to update?

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


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Pekka Enberg

On Tue, 6 Dec 2011, Cyrill Gorcunov wrote:

On Tue, Dec 06, 2011 at 01:58:24PM +0200, Pekka Enberg wrote:

On Tue, 2011-12-06 at 15:47 +0400, Cyrill Gorcunov wrote:

On Tue, Dec 06, 2011 at 01:41:56PM +0200, Pekka Enberg wrote:

On Tue, Dec 6, 2011 at 12:28 PM, Cyrill Gorcunov  wrote:

On Tue, Dec 06, 2011 at 12:25:29PM +0200, Pekka Enberg wrote:

On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:

vesa, pci-shmem and virtio-pci devices need to set up config space with
little-endian conversions (as config space is LE).  The pci_config_address
bitfield also needs to be reversed when building on BE systems.

Signed-off-by: Matt Evans 


Looks OK to me. Sasha, Cyrill?



BIOS part looks pretty good to me. LE/BE part as well. Thanks Matt!


Hmm. This seems to break "make check" for me:



If you change back to

 static struct pci_device_header pci_shmem_pci_device = {
...
.class  = 0xFF, /* misc pci device */
...
 };

does it help?


No but dropping these hunks fixes it for me:

@@ -17,7 +18,8 @@
 #define PCI_CONFIG_BUS_FORWARD 0xcfa
 #define PCI_IO_SIZE0x100

-struct pci_config_address {
+union pci_config_address {
+#if __BYTE_ORDER == __LITTLE_ENDIAN
   unsignedzeros   : 2;/* 1  .. 0  */
   unsignedregister_number : 6;/* 7  .. 2  */
   unsignedfunction_number : 3;/* 10 .. 8  */
@@ -25,6 +27,16 @@ struct pci_config_address {
   unsignedbus_number  : 8;/* 23 .. 16 */
   unsignedreserved: 7;/* 30 .. 24 */
   unsignedenable_bit  : 1;/* 31   */
+#else
+   unsignedenable_bit  : 1;/* 31   */
+   unsignedreserved: 7;/* 30 .. 24 */
+   unsignedbus_number  : 8;/* 23 .. 16 */
+   unsigneddevice_number   : 5;/* 15 .. 11 */
+   unsignedfunction_number : 3;/* 10 .. 8  */
+   unsignedregister_number : 6;/* 7  .. 2  */
+   unsignedzeros   : 2;/* 1  .. 0  */
+#endif
+   u32 w;
 };

Pekka



Hehe, this is because it should be rtaher defined as

union pci_config_address {
struct {
 #if __BYTE_ORDER == __LITTLE_ENDIAN
   unsignedzeros   : 2;
   unsignedregister_number : 6;
 #else
   ...
 #endif
}
u32 w;
};


Yup, that fixes it for me.

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


Re: [PATCH 24/28] kvm tools: Fix virtio-pci endian bug when reading VIRTIO_PCI_QUEUE_NUM

2011-12-06 Thread Sasha Levin
On Tue, 2011-12-06 at 12:26 +0200, Pekka Enberg wrote:
> On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
> > The field size is currently wrong, read into a 32bit word instead of 16.  
> > This
> > casues trouble when BE.
> >
> > Signed-off-by: Matt Evans 
> > ---
> >  tools/kvm/virtio/pci.c |3 +--
> >  1 files changed, 1 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/kvm/virtio/pci.c b/tools/kvm/virtio/pci.c
> > index 0ae93fb..6b27ff8 100644
> > --- a/tools/kvm/virtio/pci.c
> > +++ b/tools/kvm/virtio/pci.c
> > @@ -116,8 +116,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, 
> > struct kvm *kvm, u16 port,
> >break;
> >case VIRTIO_PCI_QUEUE_NUM:
> >val = vtrans->virtio_ops->get_size_vq(kvm, vpci->dev, 
> > vpci->queue_selector);
> > -   ioport__write32(data, val);
> > -   break;
> > +   ioport__write16(data, val);
> >break;
> >case VIRTIO_PCI_STATUS:
> >ioport__write8(data, vpci->status);
> 
> Looks good to me. Asias, Sasha?

Looks good.

-- 

Sasha.

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


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Cyrill Gorcunov
On Tue, Dec 06, 2011 at 01:58:24PM +0200, Pekka Enberg wrote:
> On Tue, 2011-12-06 at 15:47 +0400, Cyrill Gorcunov wrote:
> > On Tue, Dec 06, 2011 at 01:41:56PM +0200, Pekka Enberg wrote:
> > > On Tue, Dec 6, 2011 at 12:28 PM, Cyrill Gorcunov  
> > > wrote:
> > > > On Tue, Dec 06, 2011 at 12:25:29PM +0200, Pekka Enberg wrote:
> > > >> On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
> > > >> > vesa, pci-shmem and virtio-pci devices need to set up config space 
> > > >> > with
> > > >> > little-endian conversions (as config space is LE).  The 
> > > >> > pci_config_address
> > > >> > bitfield also needs to be reversed when building on BE systems.
> > > >> >
> > > >> > Signed-off-by: Matt Evans 
> > > >>
> > > >> Looks OK to me. Sasha, Cyrill?
> > > >>
> > > >
> > > > BIOS part looks pretty good to me. LE/BE part as well. Thanks Matt!
> > > 
> > > Hmm. This seems to break "make check" for me:
> > >
> > 
> > If you change back to
> > 
> >  static struct pci_device_header pci_shmem_pci_device = {
> > ...
> > .class  = 0xFF, /* misc pci device */
> > ...
> >  };
> > 
> > does it help?
> 
> No but dropping these hunks fixes it for me:
> 
> @@ -17,7 +18,8 @@
>  #define PCI_CONFIG_BUS_FORWARD 0xcfa
>  #define PCI_IO_SIZE0x100
> 
> -struct pci_config_address {
> +union pci_config_address {
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
>unsignedzeros   : 2;/* 1  .. 0  */
>unsignedregister_number : 6;/* 7  .. 2  */
>unsignedfunction_number : 3;/* 10 .. 8  */
> @@ -25,6 +27,16 @@ struct pci_config_address {
>unsignedbus_number  : 8;/* 23 .. 16 */
>unsignedreserved: 7;/* 30 .. 24 */
>unsignedenable_bit  : 1;/* 31   */
> +#else
> +   unsignedenable_bit  : 1;/* 31   */
> +   unsignedreserved: 7;/* 30 .. 24 */
> +   unsignedbus_number  : 8;/* 23 .. 16 */
> +   unsigneddevice_number   : 5;/* 15 .. 11 */
> +   unsignedfunction_number : 3;/* 10 .. 8  */
> +   unsignedregister_number : 6;/* 7  .. 2  */
> +   unsignedzeros   : 2;/* 1  .. 0  */
> +#endif
> +   u32 w;
>  };
> 
>   Pekka
> 

Hehe, this is because it should be rtaher defined as

union pci_config_address {
 struct {
  #if __BYTE_ORDER == __LITTLE_ENDIAN
unsignedzeros   : 2;
unsignedregister_number : 6;
  #else
...
  #endif
 }
 u32 w;
};

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


Re: [PATCH 16/28] kvm tools: Allow load_flat_binary() to load an initrd alongside

2011-12-06 Thread Cyrill Gorcunov
On Tue, Dec 06, 2011 at 12:29:48PM +0200, Pekka Enberg wrote:
...
> 
> Otherwise looks OK to me. Cyrill?
> 

It might be not seen from patch (or my local kvm repo
is not yet updated well) but I somehow miss who will be
reading initrd in case of loading flat image? If noone,
then what's the point to pass fd there at all?

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


Re: [PATCH 14/28] kvm tools: Fix term_getc(), term_getc_iov() endian bugs

2011-12-06 Thread Asias He
On 12/06/2011 06:24 PM, Pekka Enberg wrote:
> On Tue, Dec 6, 2011 at 5:40 AM, Matt Evans  wrote:
>> term_getc()'s int c has one byte written into it (at its lowest address) by
>> read_in_full().  This is expected to be the least significant byte, but that
>> isn't the case on BE!  Use correct type, unsigned char.  A similar issue 
>> exists
>> in term_getc_iov(), which needs to write a char to the iov rather than an 
>> int.
>>
>> Signed-off-by: Matt Evans 
>> ---
>>  tools/kvm/term.c |5 ++---
>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/kvm/term.c b/tools/kvm/term.c
>> index fb5d71c..440884e 100644
>> --- a/tools/kvm/term.c
>> +++ b/tools/kvm/term.c
>> @@ -30,11 +30,10 @@ int term_fds[4][2];
>>
>>  int term_getc(int who, int term)
>>  {
>> -   int c;
>> +   unsigned char c;
>>
>>if (who != active_console)
>>return -1;
>> -

We can drop this line too.

c &= 0xff;



>>if (read_in_full(term_fds[term][TERM_FD_IN], &c, 1) < 0)
>>return -1;
>>
>> @@ -84,7 +83,7 @@ int term_getc_iov(int who, struct iovec *iov, int iovcnt, 
>> int term)
>>if (c < 0)
>>return 0;
>>
>> -   *((int *)iov[TERM_FD_IN].iov_base)  = c;
>> +   *((char *)iov[TERM_FD_IN].iov_base) = (char)c;
>>
>>return sizeof(char);
>>  }
> 
> Looks OK to me. Asias?
> 

Otherwise, looks fine to me.


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


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Pekka Enberg
On Tue, 2011-12-06 at 15:47 +0400, Cyrill Gorcunov wrote:
> On Tue, Dec 06, 2011 at 01:41:56PM +0200, Pekka Enberg wrote:
> > On Tue, Dec 6, 2011 at 12:28 PM, Cyrill Gorcunov  wrote:
> > > On Tue, Dec 06, 2011 at 12:25:29PM +0200, Pekka Enberg wrote:
> > >> On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
> > >> > vesa, pci-shmem and virtio-pci devices need to set up config space with
> > >> > little-endian conversions (as config space is LE).  The 
> > >> > pci_config_address
> > >> > bitfield also needs to be reversed when building on BE systems.
> > >> >
> > >> > Signed-off-by: Matt Evans 
> > >>
> > >> Looks OK to me. Sasha, Cyrill?
> > >>
> > >
> > > BIOS part looks pretty good to me. LE/BE part as well. Thanks Matt!
> > 
> > Hmm. This seems to break "make check" for me:
> >
> 
> If you change back to
> 
>  static struct pci_device_header pci_shmem_pci_device = {
>   ...
>   .class  = 0xFF, /* misc pci device */
>   ...
>  };
> 
> does it help?

No but dropping these hunks fixes it for me:

@@ -17,7 +18,8 @@
 #define PCI_CONFIG_BUS_FORWARD 0xcfa
 #define PCI_IO_SIZE0x100

-struct pci_config_address {
+union pci_config_address {
+#if __BYTE_ORDER == __LITTLE_ENDIAN
   unsignedzeros   : 2;/* 1  .. 0  */
   unsignedregister_number : 6;/* 7  .. 2  */
   unsignedfunction_number : 3;/* 10 .. 8  */
@@ -25,6 +27,16 @@ struct pci_config_address {
   unsignedbus_number  : 8;/* 23 .. 16 */
   unsignedreserved: 7;/* 30 .. 24 */
   unsignedenable_bit  : 1;/* 31   */
+#else
+   unsignedenable_bit  : 1;/* 31   */
+   unsignedreserved: 7;/* 30 .. 24 */
+   unsignedbus_number  : 8;/* 23 .. 16 */
+   unsigneddevice_number   : 5;/* 15 .. 11 */
+   unsignedfunction_number : 3;/* 10 .. 8  */
+   unsignedregister_number : 6;/* 7  .. 2  */
+   unsignedzeros   : 2;/* 1  .. 0  */
+#endif
+   u32 w;
 };

Pekka

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


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Cyrill Gorcunov
On Tue, Dec 06, 2011 at 01:41:56PM +0200, Pekka Enberg wrote:
> On Tue, Dec 6, 2011 at 12:28 PM, Cyrill Gorcunov  wrote:
> > On Tue, Dec 06, 2011 at 12:25:29PM +0200, Pekka Enberg wrote:
> >> On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
> >> > vesa, pci-shmem and virtio-pci devices need to set up config space with
> >> > little-endian conversions (as config space is LE).  The 
> >> > pci_config_address
> >> > bitfield also needs to be reversed when building on BE systems.
> >> >
> >> > Signed-off-by: Matt Evans 
> >>
> >> Looks OK to me. Sasha, Cyrill?
> >>
> >
> > BIOS part looks pretty good to me. LE/BE part as well. Thanks Matt!
> 
> Hmm. This seems to break "make check" for me:
>

If you change back to

 static struct pci_device_header pci_shmem_pci_device = {
...
.class  = 0xFF, /* misc pci device */
...
 };

does it help?

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


Re: "KVM: PPC: booke: Improve timer register emulation" breaks Book3s HV

2011-12-06 Thread Alexander Graf

On 06.12.2011, at 12:43, Paul Mackerras wrote:

> On Tue, Dec 06, 2011 at 03:03:00PM +1100, Paul Mackerras wrote:
>> I'm not sure why yet, but commit 8a97c432 ("KVM: PPC: booke: Improve
>> timer register emulation") in Alex's kvm-ppc-next branch is breaking
>> Book3S HV KVM on POWER7.  Guest cpus fail to spin up, and even with
>> just one cpu, the guest stalls every so often.  If I stop the guest
>> and inspect the state with qemu, PC is at 0x900.  Reverting 8a97c432
>> makes it work properly again.
> 
> This fixes it:
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 534cbe1..83bcb44 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -560,7 +560,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
> kvm_run *run)
> 
> void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
> {
> - if (waitqueue_active(&vcpu->wq)) {
> + if (waitqueue_active(vcpu->arch.wqp)) {

:(

>   wake_up_interruptible(vcpu->arch.wqp);
>   vcpu->stat.halt_wakeup++;
>   } else if (vcpu->cpu != -1) {
> 
> Alex, do you want to roll that in with Scott's patch, or do you want me
> to send a separate patch on top?

I usually like praise where praise belongs, so I'll put this right after 
Scott's patch if you give me a nice and shiny patch description with a 
signed-off-by line :)


Alex

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


Re: "KVM: PPC: booke: Improve timer register emulation" breaks Book3s HV

2011-12-06 Thread Paul Mackerras
On Tue, Dec 06, 2011 at 03:03:00PM +1100, Paul Mackerras wrote:
> I'm not sure why yet, but commit 8a97c432 ("KVM: PPC: booke: Improve
> timer register emulation") in Alex's kvm-ppc-next branch is breaking
> Book3S HV KVM on POWER7.  Guest cpus fail to spin up, and even with
> just one cpu, the guest stalls every so often.  If I stop the guest
> and inspect the state with qemu, PC is at 0x900.  Reverting 8a97c432
> makes it work properly again.

This fixes it:

diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 534cbe1..83bcb44 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -560,7 +560,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
kvm_run *run)
 
 void kvm_vcpu_kick(struct kvm_vcpu *vcpu)
 {
-   if (waitqueue_active(&vcpu->wq)) {
+   if (waitqueue_active(vcpu->arch.wqp)) {
wake_up_interruptible(vcpu->arch.wqp);
vcpu->stat.halt_wakeup++;
} else if (vcpu->cpu != -1) {

Alex, do you want to roll that in with Scott's patch, or do you want me
to send a separate patch on top?

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


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 12:28 PM, Cyrill Gorcunov  wrote:
> On Tue, Dec 06, 2011 at 12:25:29PM +0200, Pekka Enberg wrote:
>> On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
>> > vesa, pci-shmem and virtio-pci devices need to set up config space with
>> > little-endian conversions (as config space is LE).  The pci_config_address
>> > bitfield also needs to be reversed when building on BE systems.
>> >
>> > Signed-off-by: Matt Evans 
>>
>> Looks OK to me. Sasha, Cyrill?
>>
>
> BIOS part looks pretty good to me. LE/BE part as well. Thanks Matt!

Hmm. This seems to break "make check" for me:

./kvm run -d tests/boot/boot_test.iso -p "init=init"
  # kvm run -k ../../arch/x86/boot/bzImage -m 448 -c 4 --name guest-2845
[0.00] Initializing cgroup subsys cpuset
[0.00] Initializing cgroup subsys cpu
[0.00] Linux version 3.2.0-rc3 (penberg@tux) (gcc version
4.6.0 20110603 (Red Hat 4.6.0-10) (GCC) ) #67 SMP Thu Nov 24 11:05:24
EET 2011
[0.00] Command line: noapic noacpi pci=conf1 reboot=k panic=1
i8042.direct=1 i8042.dumbkbd=1 i8042.nopnp=1 console=ttyS0
earlyprintk=serial i8042.noaux=1 init=init root=/dev/vda rw
[0.00] BIOS-provided physical RAM map:
[0.00]  BIOS-e820:  - 0009fc00 (usable)
[0.00]  BIOS-e820: 0009fc00 - 000a (reserved)
[0.00]  BIOS-e820: 000f - 000f (reserved)
[0.00]  BIOS-e820: 0010 - 1c00 (usable)
[0.00] bootconsole [earlyser0] enabled
[0.00] NX (Execute Disable) protection: active
[0.00] DMI not present or invalid.
[0.00] No AGP bridge found
[0.00] last_pfn = 0x1c000 max_arch_pfn = 0x4
[0.00] x86 PAT enabled: cpu 0, old 0x70106, new 0x7010600070106
[0.00] CPU MTRRs all blank - virtualized system.
[0.00] found SMP MP-table at [880f0390] f0390
[0.00] init_memory_mapping: -1c00
[0.00] ACPI Error: A valid RSDP was not found (20110623/tbxfroot-219)
[0.00] No NUMA configuration found
[0.00] Faking a node at -1c00
[0.00] Initmem setup node 0 -1c00
[0.00]   NODE_DATA [1bfec000 - 1bff]
[0.00] kvm-clock: Using msrs 4b564d01 and 4b564d00
[0.00] kvm-clock: cpu 0, msr 0:1b71301, boot clock
[0.00] Zone PFN ranges:
[0.00]   DMA  0x0010 -> 0x1000
[0.00]   DMA320x1000 -> 0x0010
[0.00]   Normal   empty
[0.00] Movable zone start PFN for each node
[0.00] early_node_map[2] active PFN ranges
[0.00] 0: 0x0010 -> 0x009f
[0.00] 0: 0x0100 -> 0x0001c000
[0.00] SFI: Simple Firmware Interface v0.81 http://simplefirmware.org
[0.00] Intel MultiProcessor Specification v1.4
[0.00] MPTABLE: OEM ID: KVMCPU00
[0.00] MPTABLE: Product ID: 0.1
[0.00] MPTABLE: APIC at: 0xFEE0
[0.00] Processor #0 (Bootup-CPU)
[0.00] Processor #1
[0.00] Processor #2
[0.00] Processor #3
[0.00] IOAPIC[0]: apic_id 5, version 17, address 0xfec0, GSI 0-23
[0.00] Processors: 4
[0.00] SMP: Allowing 4 CPUs, 0 hotplug CPUs
[0.00] PM: Registered nosave memory: 0009f000 - 000a
[0.00] PM: Registered nosave memory: 000a - 000f
[0.00] PM: Registered nosave memory: 000f - 000ff000
[0.00] PM: Registered nosave memory: 000ff000 - 0010
[0.00] Allocating PCI resources starting at 1c00 (gap:
1c00:e400)
[0.00] Booting paravirtualized kernel on KVM
[0.00] setup_percpu: NR_CPUS:256 nr_cpumask_bits:256
nr_cpu_ids:4 nr_node_ids:1
[0.00] PERCPU: Embedded 27 pages/cpu @88001bc0 s77888
r8192 d24512 u524288
[0.00] kvm-clock: cpu 0, msr 0:1bc12301, primary cpu clock
[0.00] KVM setup async PF for cpu 0
[0.00] kvm-stealtime: cpu 0, msr 1bc0d000
[0.00] Built 1 zonelists in Node order, mobility grouping on.
Total pages: 112778
[0.00] Policy zone: DMA32
[0.00] Kernel command line: noapic noacpi pci=conf1 reboot=k
panic=1 i8042.direct=1 i8042.dumbkbd=1 i8042.nopnp=1 console=ttyS0
earlyprintk=serial i8042.noaux=1 init=init root=/dev/vda rw
[0.00] PID hash table entries: 2048 (order: 2, 16384 bytes)
[0.00] xsave/xrstor: enabled xstate_bv 0x7, cntxt size 0x340
[0.00] Checking aperture...
[0.00] No AGP bridge found
[0.00] Memory: 435408k/458752k available (4752k kernel code,
452k absent, 22892k reserved, 6886k data, 908k init)
[0.00] SLUB: Genslabs=15, HWalign=64, Order=0-3, MinObjects=0,
CPUs=4, Nodes=1
[0.00] Hierarchical RCU implementation.
[0.00]  RCU dyntick-idle grace-period acceleration is enabled.
[

Re: [PATCH 24/28] kvm tools: Fix virtio-pci endian bug when reading VIRTIO_PCI_QUEUE_NUM

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 1:28 PM, Asias He  wrote:
>>> @@ -116,8 +116,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, 
>>> struct kvm *kvm, u16 port,
>>>                break;
>>>        case VIRTIO_PCI_QUEUE_NUM:
>>>                val = vtrans->virtio_ops->get_size_vq(kvm, vpci->dev, 
>>> vpci->queue_selector);
>>> -               ioport__write32(data, val);
>>> -               break;
>>> +               ioport__write16(data, val);
>>>                break;
>>>        case VIRTIO_PCI_STATUS:
>>>                ioport__write8(data, vpci->status);
>>
>> Looks good to me. Asias, Sasha?
>
> Looks good to me.

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


Re: [PATCH 24/28] kvm tools: Fix virtio-pci endian bug when reading VIRTIO_PCI_QUEUE_NUM

2011-12-06 Thread Asias He
On 12/06/2011 06:26 PM, Pekka Enberg wrote:
> On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
>> The field size is currently wrong, read into a 32bit word instead of 16.  
>> This
>> casues trouble when BE.
>>
>> Signed-off-by: Matt Evans 
>> ---
>>  tools/kvm/virtio/pci.c |3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/kvm/virtio/pci.c b/tools/kvm/virtio/pci.c
>> index 0ae93fb..6b27ff8 100644
>> --- a/tools/kvm/virtio/pci.c
>> +++ b/tools/kvm/virtio/pci.c
>> @@ -116,8 +116,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, 
>> struct kvm *kvm, u16 port,
>>break;
>>case VIRTIO_PCI_QUEUE_NUM:
>>val = vtrans->virtio_ops->get_size_vq(kvm, vpci->dev, 
>> vpci->queue_selector);
>> -   ioport__write32(data, val);
>> -   break;
>> +   ioport__write16(data, val);
>>break;
>>case VIRTIO_PCI_STATUS:
>>ioport__write8(data, vpci->status);
> 
> Looks good to me. Asias, Sasha?

Looks good to me.

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


Re: [PATCH 16/28] kvm tools: Allow load_flat_binary() to load an initrd alongside

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 5:41 AM, Matt Evans  wrote:
> This patch passes the initrd fd and commandline to load_flat_binary(), which 
> may
> be used to load both the kernel & an initrd (stashing or inserting the
> commandline as appropriate) in the same way that load_bzimage() does.  This is
> especially useful when load_bzimage() is unused for a particular
> architecture. :-)
>
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/include/kvm/kvm.h |    2 +-
>  tools/kvm/kvm.c             |   10 ++
>  tools/kvm/x86/kvm.c         |   12 +---
>  3 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
> index fae2ba9..5fe6e75 100644
> --- a/tools/kvm/include/kvm/kvm.h
> +++ b/tools/kvm/include/kvm/kvm.h
> @@ -59,7 +59,7 @@ void kvm__arch_setup_firmware(struct kvm *kvm);
>  bool kvm__arch_cpu_supports_vm(void);
>  void kvm__arch_periodic_poll(struct kvm *kvm);
>
> -int load_flat_binary(struct kvm *kvm, int fd);
> +int load_flat_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const 
> char *kernel_cmdline);
>  bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd, const char 
> *kernel_cmdline, u16 vidmode);
>
>  /*
> diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
> index 457de1a..6f33e1a 100644
> --- a/tools/kvm/kvm.c
> +++ b/tools/kvm/kvm.c
> @@ -354,23 +354,25 @@ bool kvm__load_kernel(struct kvm *kvm, const char 
> *kernel_filename,
>
>        ret = load_bzimage(kvm, fd_kernel, fd_initrd, kernel_cmdline, vidmode);
>
> -       if (initrd_filename)
> -               close(fd_initrd);
> -
>        if (ret)
>                goto found_kernel;
>
>        pr_warning("%s is not a bzImage. Trying to load it as a flat 
> binary...", kernel_filename);
>
> -       ret = load_flat_binary(kvm, fd_kernel);
> +       ret = load_flat_binary(kvm, fd_kernel, fd_initrd, kernel_cmdline);
> +
>        if (ret)
>                goto found_kernel;
>
> +       if (initrd_filename)
> +               close(fd_initrd);
>        close(fd_kernel);
>
>        die("%s is not a valid bzImage or flat binary", kernel_filename);
>
>  found_kernel:
> +       if (initrd_filename)
> +               close(fd_initrd);
>        close(fd_kernel);
>
>        return ret;
> diff --git a/tools/kvm/x86/kvm.c b/tools/kvm/x86/kvm.c
> index 7071dc6..4ac21c0 100644
> --- a/tools/kvm/x86/kvm.c
> +++ b/tools/kvm/x86/kvm.c
> @@ -227,17 +227,23 @@ void kvm__irq_trigger(struct kvm *kvm, int irq)
>  #define BOOT_PROTOCOL_REQUIRED 0x206
>  #define LOAD_HIGH              0x01
>
> -int load_flat_binary(struct kvm *kvm, int fd)
> +int load_flat_binary(struct kvm *kvm, int fd_kernel, int fd_initrd, const 
> char *kernel_cmdline)
>  {
>        void *p;
>        int nr;
>
> -       if (lseek(fd, 0, SEEK_SET) < 0)
> +       /* Some architectures may support loading an initrd alongside the 
> flat kernel,
> +        * but we do not.
> +        */

Minor nit - we use comment format where the first line is left empty from text:

       /*
        * Some architectures may support loading an initrd alongside
the flat kernel,
        * but we do not.
        */

> +       if (fd_initrd != -1)
> +               pr_warning("Loading initrd with flat binary not supported.");
> +
> +       if (lseek(fd_kernel, 0, SEEK_SET) < 0)
>                die_perror("lseek");
>
>        p = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, BOOT_LOADER_IP);
>
> -       while ((nr = read(fd, p, 65536)) > 0)
> +       while ((nr = read(fd_kernel, p, 65536)) > 0)
>                p += nr;
>
>        kvm->boot_selector      = BOOT_LOADER_SELECTOR;
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Otherwise looks OK to me. Cyrill?
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Cyrill Gorcunov
On Tue, Dec 06, 2011 at 12:25:29PM +0200, Pekka Enberg wrote:
> On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
> > vesa, pci-shmem and virtio-pci devices need to set up config space with
> > little-endian conversions (as config space is LE).  The pci_config_address
> > bitfield also needs to be reversed when building on BE systems.
> >
> > Signed-off-by: Matt Evans 
> 
> Looks OK to me. Sasha, Cyrill?
> 

BIOS part looks pretty good to me. LE/BE part as well. Thanks Matt!

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


Re: [PATCH 15/28] kvm tools: Allow initrd_check() to match a cpio

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 5:40 AM, Matt Evans  wrote:
> cpios are valid as initrds too, so allow them through the check.
>
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/kvm.c |    8 +---
>  1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
> index 33243f1..457de1a 100644
> --- a/tools/kvm/kvm.c
> +++ b/tools/kvm/kvm.c
> @@ -317,10 +317,11 @@ struct kvm *kvm__init(const char *kvm_dev, u64 
> ram_size, const char *name)
>  /* RFC 1952 */
>  #define GZIP_ID1               0x1f
>  #define GZIP_ID2               0x8b
> -
> +#define CPIO_MAGIC             "0707"
> +/* initrd may be gzipped, or a plain cpio */
>  static bool initrd_check(int fd)
>  {
> -       unsigned char id[2];
> +       unsigned char id[4];
>
>        if (read_in_full(fd, id, ARRAY_SIZE(id)) < 0)
>                return false;
> @@ -328,7 +329,8 @@ static bool initrd_check(int fd)
>        if (lseek(fd, 0, SEEK_SET) < 0)
>                die_perror("lseek");
>
> -       return id[0] == GZIP_ID1 && id[1] == GZIP_ID2;
> +       return (id[0] == GZIP_ID1 && id[1] == GZIP_ID2) ||
> +               !memcmp(id, CPIO_MAGIC, 4);
>  }
>
>  bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,

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


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

2011-12-06 Thread Ingo Molnar

* Paul Mackerras  wrote:

> On Tue, Dec 06, 2011 at 09:28:27AM +0100, Ingo Molnar wrote:
> > 
> > * Sasha Levin  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 
 #else
 # include 
 #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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 24/28] kvm tools: Fix virtio-pci endian bug when reading VIRTIO_PCI_QUEUE_NUM

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
> The field size is currently wrong, read into a 32bit word instead of 16.  This
> casues trouble when BE.
>
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/virtio/pci.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/tools/kvm/virtio/pci.c b/tools/kvm/virtio/pci.c
> index 0ae93fb..6b27ff8 100644
> --- a/tools/kvm/virtio/pci.c
> +++ b/tools/kvm/virtio/pci.c
> @@ -116,8 +116,7 @@ static bool virtio_pci__io_in(struct ioport *ioport, 
> struct kvm *kvm, u16 port,
>                break;
>        case VIRTIO_PCI_QUEUE_NUM:
>                val = vtrans->virtio_ops->get_size_vq(kvm, vpci->dev, 
> vpci->queue_selector);
> -               ioport__write32(data, val);
> -               break;
> +               ioport__write16(data, val);
>                break;
>        case VIRTIO_PCI_STATUS:
>                ioport__write8(data, vpci->status);

Looks good to me. Asias, Sasha?
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 23/28] kvm tools: Endian-sanitise pci.h and PCI device setup

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 5:42 AM, Matt Evans  wrote:
> vesa, pci-shmem and virtio-pci devices need to set up config space with
> little-endian conversions (as config space is LE).  The pci_config_address
> bitfield also needs to be reversed when building on BE systems.
>
> Signed-off-by: Matt Evans 

Looks OK to me. Sasha, Cyrill?
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 14/28] kvm tools: Fix term_getc(), term_getc_iov() endian bugs

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 5:40 AM, Matt Evans  wrote:
> term_getc()'s int c has one byte written into it (at its lowest address) by
> read_in_full().  This is expected to be the least significant byte, but that
> isn't the case on BE!  Use correct type, unsigned char.  A similar issue 
> exists
> in term_getc_iov(), which needs to write a char to the iov rather than an int.
>
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/term.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/kvm/term.c b/tools/kvm/term.c
> index fb5d71c..440884e 100644
> --- a/tools/kvm/term.c
> +++ b/tools/kvm/term.c
> @@ -30,11 +30,10 @@ int term_fds[4][2];
>
>  int term_getc(int who, int term)
>  {
> -       int c;
> +       unsigned char c;
>
>        if (who != active_console)
>                return -1;
> -
>        if (read_in_full(term_fds[term][TERM_FD_IN], &c, 1) < 0)
>                return -1;
>
> @@ -84,7 +83,7 @@ int term_getc_iov(int who, struct iovec *iov, int iovcnt, 
> int term)
>        if (c < 0)
>                return 0;
>
> -       *((int *)iov[TERM_FD_IN].iov_base)      = c;
> +       *((char *)iov[TERM_FD_IN].iov_base)     = (char)c;
>
>        return sizeof(char);
>  }

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


Re: [PATCH 01/28] kvm tools: Split x86 arch-specific bits into x86/

2011-12-06 Thread Pekka Enberg
On Tue, Dec 6, 2011 at 10:07 AM, Sasha Levin  wrote:
> The code doesn't build after this patch due to missing header issues
> which you fixed in patches #10 & #11. Could you please move those two to
> the beginning of the series for the sake of bisectablilty?

I did that myself. Patches 10, 11, and 1 applied, thanks!
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2011-12-06 Thread Paul Mackerras
On Tue, Dec 06, 2011 at 09:28:27AM +0100, Ingo Molnar wrote:
> 
> * Sasha Levin  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 00/28] kvm tools: Prepare kvmtool for another architecture

2011-12-06 Thread Sasha Levin
Awesome work :)

I really like the clean split we'll have between arch specific and
generic code after this series.

Not only we get ppc support, you've also made it very easy to add future
support for more archs.

On Tue, 2011-12-06 at 14:35 +1100, Matt Evans wrote:
> Hi,
> 
> 
> This patch series rearranges and tidies various parts of kvmtool to pave the 
> way
> for the addition of support for another architecture -- SPAPR PPC64.  A second
> patch series will follow to present the PPC64 support.
> 
> kvmtool is extremely x86-specific, so a fair chunk of refactoring into "common
> code" vs "architecture-specific code" is performed in this set.  It also has a
> (refreshingly small) set of endian bugs that are fixed, plus assumptions about
> the hardware presented to the guest.
> 
> I've started the series with the main meat-- moving/renaming things like bios,
> CPU setup, guest address space layout, interrupts, ioports etc., into a new 
> x86/
> directory.  The Makefile determines an architecture and builds the appropriate
> dir, devices, etc.
> 
> Follow-on patches change some of the mechanics, for example modifying the loop
> around ioctl(KVM_RUN) so that whilst it stays generic, it calls into
> arch-specific code to handle specific exit reasons, MMIO etc.  The builtin-run
> initialisation path is rationalised so that PCI & IRQs are initialised before
> devices, and all of this happens before arch-specific code is given the chance
> to initialise any firmware and generate any device trees.
> 
> Most of this series is fairly trivial, in moving code, making definitions
> arch-local or available via a header, endian sanitisation.  The PCI code 
> changes
> are probably most 'interesting', in that I have made the config space accesses
> available to those not using the PC ioport access method, plus wrapped
> initialisations of config space with cpu_to_leXX accesses.
> 
> If there's anything in this series that'll cause the world to end, or stain, 
> do
> let me know. :)
> 
> 
> Cheers,
> 
> 
> Matt
> 
> 
> 
> Matt Evans (28):
>   kvm tools: Split x86 arch-specific bits into x86/
>   kvm tools: Only build/init i8042 on x86
>   kvm tools: Add Makefile parameter for kernel include path
>   kvm tools: Re-arrange Makefile to heed CFLAGS before checking for
> optional libs
>   kvm tools: 64-bit tidy; use PRIx64 when printf'ing u64s and link
> appropriately
>   kvm tools: Add arch-specific KVM_RUN exit handling via
> kvm_cpu__handle_exit()
>   kvm tools: Move 'kvm__recommended_cpus' to arch-specific code
>   kvm tools: Fix KVM_RUN exit code check
>   kvm tools: Add kvm__arch_periodic_poll()
>   kvm tools: term.h needs to include stdbool.h
>   kvm tools: kvm.c needs to include sys/stat.h for mkdir
>   kvm tools: Move arch-specific cmdline init into
> kvm__arch_set_cmdline()
>   kvm tools: Add CONSOLE_HV term type and allow it to be selected
>   kvm tools: Fix term_getc(), term_getc_iov() endian bugs
>   kvm tools: Allow initrd_check() to match a cpio
>   kvm tools: Allow load_flat_binary() to load an initrd alongside
>   kvm tools: Only call symbol__init() if we have BFD
>   kvm tools: Initialise PCI before devices start getting registered
> with PCI
>   kvm tools: Perform CPU and firmware setup after devices are added
>   kvm tools: Init IRQs after determining nrcpus
>   kvm tools: Add --hugetlbfs option to specify memory path
>   kvm tools: Move PCI_MAX_DEVICES to pci.h
>   kvm tools: Endian-sanitise pci.h and PCI device setup
>   kvm tools: Fix virtio-pci endian bug when reading
> VIRTIO_PCI_QUEUE_NUM
>   kvm tools: Correctly set virtio-pci bar_size and remove hardwired
> address
>   kvm tools: Add pci__config_{rd,wr}(), pci__find_dev() and fix PCI
> config register addressing
>   kvm tools: Arch-specific define for PCI MMIO allocation area
>   kvm tools: Create arch-specific kvm_cpu__emulate_io()
> 
>  tools/kvm/Makefile  |  139 +---
>  tools/kvm/builtin-run.c |   82 +++--
>  tools/kvm/builtin-stat.c|4 +-
>  tools/kvm/disk/core.c   |4 +-
>  tools/kvm/hw/pci-shmem.c|   23 +-
>  tools/kvm/hw/vesa.c |   15 +-
>  tools/kvm/include/kvm/ioport.h  |   13 +-
>  tools/kvm/include/kvm/kvm-cpu.h |   30 +--
>  tools/kvm/include/kvm/kvm.h |   62 +---
>  tools/kvm/include/kvm/pci.h |   30 ++-
>  tools/kvm/include/kvm/term.h|2 +
>  tools/kvm/ioport.c  |   54 ---
>  tools/kvm/kvm-cpu.c |  407 +-
>  tools/kvm/kvm.c |  374 +---
>  tools/kvm/mmio.c|4 +-
>  tools/kvm/pci.c |   76 +++--
>  tools/kvm/term.c|5 +-
>  tools/k

Re: [PATCH 28/28] kvm tools: Create arch-specific kvm_cpu__emulate_io()

2011-12-06 Thread Sasha Levin
Can we possibly do it by getting the generic code to call both
'kvm_cpu__arch_emulate_io' and 'kvm_cpu__arch_emulate_mmio', and have
the ppc code have an empty static for 'kvm_cpu__arch_emulate_io'?

On Tue, 2011-12-06 at 14:43 +1100, Matt Evans wrote:
> Different architectures will deal with MMIO exits differently.  For example,
> KVM_EXIT_IO is x86-specific, and I/O cycles are often synthesisted by steering
> into windows in PCI bridges on other architectures.
> 
> This patch moves the IO/MMIO exit code from the main runloop into 
> x86/kvm-cpu.c
> 
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/include/kvm/kvm-cpu.h |1 +
>  tools/kvm/kvm-cpu.c |   37 +
>  tools/kvm/x86/kvm-cpu.c |   37 +
>  3 files changed, 43 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/kvm/include/kvm/kvm-cpu.h b/tools/kvm/include/kvm/kvm-cpu.h
> index 15618f1..6f38c0c 100644
> --- a/tools/kvm/include/kvm/kvm-cpu.h
> +++ b/tools/kvm/include/kvm/kvm-cpu.h
> @@ -13,6 +13,7 @@ void kvm_cpu__run(struct kvm_cpu *vcpu);
>  void kvm_cpu__reboot(void);
>  int kvm_cpu__start(struct kvm_cpu *cpu);
>  bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu);
> +bool kvm_cpu__emulate_io(struct kvm_cpu *cpu, struct kvm_run *kvm_run);
>  
>  int kvm_cpu__get_debug_fd(void);
>  void kvm_cpu__set_debug_fd(int fd);
> diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
> index 884a89f..c9fbc81 100644
> --- a/tools/kvm/kvm-cpu.c
> +++ b/tools/kvm/kvm-cpu.c
> @@ -103,49 +103,22 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
>   kvm_cpu__show_registers(cpu);
>   kvm_cpu__show_code(cpu);
>   break;
> - case KVM_EXIT_IO: {
> - bool ret;
> -
> - ret = kvm__emulate_io(cpu->kvm,
> - cpu->kvm_run->io.port,
> - (u8 *)cpu->kvm_run +
> - cpu->kvm_run->io.data_offset,
> - cpu->kvm_run->io.direction,
> - cpu->kvm_run->io.size,
> - cpu->kvm_run->io.count);
> -
> - if (!ret)
> + case KVM_EXIT_IO:
> + case KVM_EXIT_MMIO:
> + if (!kvm_cpu__emulate_io(cpu, cpu->kvm_run))
>   goto panic_kvm;
>   break;
> - }
> - case KVM_EXIT_MMIO: {
> - bool ret;
> -
> - ret = kvm__emulate_mmio(cpu->kvm,
> - cpu->kvm_run->mmio.phys_addr,
> - cpu->kvm_run->mmio.data,
> - cpu->kvm_run->mmio.len,
> - cpu->kvm_run->mmio.is_write);
> -
> - if (!ret)
> - goto panic_kvm;
> - break;
> - }
>   case KVM_EXIT_INTR:
>   if (cpu->is_running)
>   break;
>   goto exit_kvm;
>   case KVM_EXIT_SHUTDOWN:
>   goto exit_kvm;
> - default: {
> - bool ret;
> -
> - ret = kvm_cpu__handle_exit(cpu);
> - if (!ret)
> + default:
> + if (!kvm_cpu__handle_exit(cpu))
>   goto panic_kvm;
>   break;
>   }
> - }
>   kvm_cpu__handle_coalesced_mmio(cpu);
>   }
>  
> diff --git a/tools/kvm/x86/kvm-cpu.c b/tools/kvm/x86/kvm-cpu.c
> index a0d10cc..665d742 100644
> --- a/tools/kvm/x86/kvm-cpu.c
> +++ b/tools/kvm/x86/kvm-cpu.c
> @@ -217,6 +217,43 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>   return false;
>  }
>  
> +bool kvm_cpu__emulate_io(struct kvm_cpu *cpu, struct kvm_run *kvm_run)
> +{
> + bool ret;
> + switch (kvm_run->exit_reason) {
> + case KVM_EXIT_IO: {
> + ret = kvm__emulate_io(cpu->kvm,
> +   cpu->kvm_run->io.port,
> +   (u8 *)cpu->kvm_run +
> +   cpu->kvm_run->io.data_offset,
> +   cpu->kvm_run->io.direction,
> +   cpu->kvm_run->io.size,
> +   cpu->kvm_run->io.count);
> +
> + if (!ret)
> + goto panic_kvm;
> + break;
> + }
> + case KVM_EXIT_MMIO: {
> + ret = kvm__emulate_mmio(cpu->kvm,
> + cpu->kvm_run->mmio.phys_addr,
> + cpu->kvm_run->mmio.data,
> + cpu->kvm_run->mmio.len,
> + cp

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

2011-12-06 Thread Ingo Molnar

* Sasha Levin  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 21/28] kvm tools: Add --hugetlbfs option to specify memory path

2011-12-06 Thread Sasha Levin
I'm seeing hugetlbfs_path being passed around, but I don't see anything
that actually does something with it.

Did it get into a different patch? or maybe it wasn't 'git add'ed?

On Tue, 2011-12-06 at 14:41 +1100, Matt Evans wrote:
> Some architectures may want to use hugetlbfs to mmap() their guest memory, so
> allow a path to be specified on the commandline and pass it to 
> kvm__arch_init().
> 
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/builtin-run.c |4 +++-
>  tools/kvm/include/kvm/kvm.h |4 ++--
>  tools/kvm/kvm.c |4 ++--
>  tools/kvm/x86/kvm.c |2 +-
>  4 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> index 84aa931..4c88169 100644
> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -84,6 +84,7 @@ static const char *guest_mac;
>  static const char *host_mac;
>  static const char *script;
>  static const char *guest_name;
> +static const char *hugetlbfs_path;
>  static struct virtio_net_params *net_params;
>  static bool single_step;
>  static bool readonly_image[MAX_DISK_IMAGES];
> @@ -422,6 +423,7 @@ static const struct option options[] = {
>   OPT_CALLBACK('\0', "tty", NULL, "tty id",
>"Remap guest TTY into a pty on the host",
>tty_parser),
> + OPT_STRING('\0', "hugetlbfs", &hugetlbfs_path, "path", "Hugetlbfs 
> path"),
>  
>   OPT_GROUP("Kernel options:"),
>   OPT_STRING('k', "kernel", &kernel_filename, "kernel",
> @@ -808,7 +810,7 @@ int kvm_cmd_run(int argc, const char **argv, const char 
> *prefix)
>   guest_name = default_name;
>   }
>  
> - kvm = kvm__init(dev, ram_size, guest_name);
> + kvm = kvm__init(dev, hugetlbfs_path, ram_size, guest_name);
>  
>   kvm->single_step = single_step;
>  
> diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
> index 5fe6e75..7159952 100644
> --- a/tools/kvm/include/kvm/kvm.h
> +++ b/tools/kvm/include/kvm/kvm.h
> @@ -30,7 +30,7 @@ struct kvm_ext {
>  void kvm__set_dir(const char *fmt, ...);
>  const char *kvm__get_dir(void);
>  
> -struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, const char *name);
> +struct kvm *kvm__init(const char *kvm_dev, const char *hugetlbfs_path, u64 
> ram_size, const char *name);
>  int kvm__recommended_cpus(struct kvm *kvm);
>  int kvm__max_cpus(struct kvm *kvm);
>  void kvm__init_ram(struct kvm *kvm);
> @@ -54,7 +54,7 @@ int kvm__enumerate_instances(int (*callback)(const char 
> *name, int pid));
>  void kvm__remove_socket(const char *name);
>  
>  void kvm__arch_set_cmdline(char *cmdline, bool video);
> -void kvm__arch_init(struct kvm *kvm, const char *kvm_dev, u64 ram_size, 
> const char *name);
> +void kvm__arch_init(struct kvm *kvm, const char *kvm_dev, const char 
> *hugetlbfs_path, u64 ram_size, const char *name);
>  void kvm__arch_setup_firmware(struct kvm *kvm);
>  bool kvm__arch_cpu_supports_vm(void);
>  void kvm__arch_periodic_poll(struct kvm *kvm);
> diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
> index 6f33e1a..503ceae 100644
> --- a/tools/kvm/kvm.c
> +++ b/tools/kvm/kvm.c
> @@ -272,7 +272,7 @@ static void kvm__pid(int fd, u32 type, u32 len, u8 *msg)
>   pr_warning("Failed sending PID");
>  }
>  
> -struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, const char *name)
> +struct kvm *kvm__init(const char *kvm_dev, const char *hugetlbfs_path, u64 
> ram_size, const char *name)
>  {
>   struct kvm *kvm;
>   int ret;
> @@ -305,7 +305,7 @@ struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, 
> const char *name)
>   if (kvm__check_extensions(kvm))
>   die("A required KVM extention is not supported by OS");
>  
> - kvm__arch_init(kvm, kvm_dev, ram_size, name);
> + kvm__arch_init(kvm, kvm_dev, hugetlbfs_path, ram_size, name);
>  
>   kvm->name = name;
>  
> diff --git a/tools/kvm/x86/kvm.c b/tools/kvm/x86/kvm.c
> index 4ac21c0..76f805f 100644
> --- a/tools/kvm/x86/kvm.c
> +++ b/tools/kvm/x86/kvm.c
> @@ -161,7 +161,7 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>  }
>  
>  /* Architecture-specific KVM init */
> -void kvm__arch_init(struct kvm *kvm, const char *kvm_dev, u64 ram_size, 
> const char *name)
> +void kvm__arch_init(struct kvm *kvm, const char *kvm_dev, const char 
> *hugetlbfs_path, u64 ram_size, const char *name)
>  {
>   struct kvm_pit_config pit_config = { .flags = 0, };
>   int ret;
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Sasha.

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


Re: [PATCH 17/28] kvm tools: Only call symbol__init() if we have BFD

2011-12-06 Thread Sasha Levin
It's optional, but when CONFIG_HAS_BFD is not defined symbol__init() is
defined as an empty static function.

Why was there a need to wrap it in a #ifdef here?

On Tue, 2011-12-06 at 14:41 +1100, Matt Evans wrote:
> CONFIG_HAS_BFD is optional, symbol.c inclusion is optional -- so make its init
> call dependent on CONFIG_HAS_BFD.
> 
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/builtin-run.c |3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> index 1257c90..aaa5132 100644
> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -798,8 +798,9 @@ int kvm_cmd_run(int argc, const char **argv, const char 
> *prefix)
>   if (!script)
>   script = DEFAULT_SCRIPT;
>  
> +#ifdef CONFIG_HAS_BFD
>   symbol__init(vmlinux_filename);
> -
> +#endif
>   term_init();
>  
>   if (!guest_name) {
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Sasha.

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


Re: [PATCH 08/28] kvm tools: Fix KVM_RUN exit code check

2011-12-06 Thread Sasha Levin
If KVM_RUN can actually return anything besides 0 or -1 it may be also
worthwhile to update Documentation/virtual/kvm/api.txt .

What are the cases where it happens?

On Tue, 2011-12-06 at 14:39 +1100, Matt Evans wrote:
> kvm_cpu__run() currently die()s if KVM_RUN returns non-zero.  Some 
> architectures
> may return positive values in non-error cases, whereas real errors are always
> negative return values.  Check for those instead.
> 
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/kvm-cpu.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
> index 9bc0796..884a89f 100644
> --- a/tools/kvm/kvm-cpu.c
> +++ b/tools/kvm/kvm-cpu.c
> @@ -30,7 +30,7 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
>   int err;
>  
>   err = ioctl(vcpu->vcpu_fd, KVM_RUN, 0);
> - if (err && (errno != EINTR && errno != EAGAIN))
> + if (err < 0 && (errno != EINTR && errno != EAGAIN))
>   die_perror("KVM_RUN failed");
>  }
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Sasha.

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


Re: [PATCH 07/28] kvm tools: Move 'kvm__recommended_cpus' to arch-specific code

2011-12-06 Thread Sasha Levin
Why is it getting moved out of generic code?

This is used to determine the maximum amount of vcpus supported by the
host for a single guest, and as far as I know KVM_CAP_NR_VCPUS and
KVM_CAP_MAX_VCPUS are not arch specific.

On Tue, 2011-12-06 at 14:39 +1100, Matt Evans wrote:
> Architectures can recommend/count/determine number of CPUs differently, so 
> move
> this out of generic code.
> 
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/kvm.c |   30 --
>  tools/kvm/x86/kvm.c |   30 ++
>  2 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
> index 7ce1640..e526483 100644
> --- a/tools/kvm/kvm.c
> +++ b/tools/kvm/kvm.c
> @@ -259,17 +259,6 @@ void kvm__register_mem(struct kvm *kvm, u64 guest_phys, 
> u64 size, void *userspac
>   die_perror("KVM_SET_USER_MEMORY_REGION ioctl");
>  }
>  
> -int kvm__recommended_cpus(struct kvm *kvm)
> -{
> - int ret;
> -
> - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS);
> - if (ret <= 0)
> - die_perror("KVM_CAP_NR_VCPUS");
> -
> - return ret;
> -}
> -
>  static void kvm__pid(int fd, u32 type, u32 len, u8 *msg)
>  {
>   pid_t pid = getpid();
> @@ -282,25 +271,6 @@ static void kvm__pid(int fd, u32 type, u32 len, u8 *msg)
>   pr_warning("Failed sending PID");
>  }
>  
> -/*
> - * The following hack should be removed once 'x86: Raise the hard
> - * VCPU count limit' makes it's way into the mainline.
> - */
> -#ifndef KVM_CAP_MAX_VCPUS
> -#define KVM_CAP_MAX_VCPUS 66
> -#endif
> -
> -int kvm__max_cpus(struct kvm *kvm)
> -{
> - int ret;
> -
> - ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS);
> - if (ret <= 0)
> - ret = kvm__recommended_cpus(kvm);
> -
> - return ret;
> -}
> -
>  struct kvm *kvm__init(const char *kvm_dev, u64 ram_size, const char *name)
>  {
>   struct kvm *kvm;
> diff --git a/tools/kvm/x86/kvm.c b/tools/kvm/x86/kvm.c
> index ac6c91e..75e4a52 100644
> --- a/tools/kvm/x86/kvm.c
> +++ b/tools/kvm/x86/kvm.c
> @@ -76,6 +76,36 @@ bool kvm__arch_cpu_supports_vm(void)
>   return regs.ecx & (1 << feature);
>  }
>  
> +int kvm__recommended_cpus(struct kvm *kvm)
> +{
> + int ret;
> +
> + ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS);
> + if (ret <= 0)
> + die_perror("KVM_CAP_NR_VCPUS");
> +
> + return ret;
> +}
> +
> +/*
> + * The following hack should be removed once 'x86: Raise the hard
> + * VCPU count limit' makes it's way into the mainline.
> + */
> +#ifndef KVM_CAP_MAX_VCPUS
> +#define KVM_CAP_MAX_VCPUS 66
> +#endif
> +
> +int kvm__max_cpus(struct kvm *kvm)
> +{
> + int ret;
> +
> + ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS);
> + if (ret <= 0)
> + ret = kvm__recommended_cpus(kvm);
> +
> + return ret;
> +}
> +
>  /*
>   * Allocating RAM size bigger than 4GB requires us to leave a gap
>   * in the RAM which is used for PCI MMIO, hotplug, and unconfigured
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Sasha.

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


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

2011-12-06 Thread Sasha Levin
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 
> ---
>  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 
>  #include 
>  #include 
> +#define __STDC_FORMAT_MACROS
> +#include 
>  #include 
>  #include 
>  
> @@ -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 
>  #include 
>  #include 
> +#define __STDC_FORMAT_MACROS
> +#include 
>  
>  #include 
>  
> @@ -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 
>  #include 
> +#define __STDC_FORMAT_MACROS
> +#include 
>  
>  #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"%"PRI

Re: [PATCH 02/28] kvm tools: Only build/init i8042 on x86

2011-12-06 Thread Sasha Levin
For some reason I can't explain, I'm getting this error after applying
this patch:

Applying: kvm tools: Only build/init i8042 on x86
fatal: mode change for tools/kvm/bios.c, which is not in current HEAD
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0002 kvm tools: Only build/init i8042 on x86

Any idea why it would happen?

On Tue, 2011-12-06 at 14:37 +1100, Matt Evans wrote:
> Not every architecture has an i8042 kbd controller, so only use this when
> building for x86.
> 
> Signed-off-by: Matt Evans 
> ---
>  tools/kvm/Makefile  |2 +-
>  tools/kvm/builtin-run.c |2 ++
>  2 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile
> index 243886e..f58a1d8 100644
> --- a/tools/kvm/Makefile
> +++ b/tools/kvm/Makefile
> @@ -77,7 +77,6 @@ OBJS+= util/strbuf.o
>  OBJS += virtio/9p.o
>  OBJS += virtio/9p-pdu.o
>  OBJS += hw/vesa.o
> -OBJS += hw/i8042.o
>  OBJS += hw/pci-shmem.o
>  OBJS += kvm-ipc.o
>  
> @@ -153,6 +152,7 @@ ifeq ($(ARCH),x86)
>   OBJS+= x86/kvm.o
>   OBJS+= x86/kvm-cpu.o
>   OBJS+= x86/mptable.o
> + OBJS+= hw/i8042.o
>  # Exclude BIOS object files from header dependencies.
>   OTHEROBJS   += x86/bios.o
>   OTHEROBJS   += x86/bios/bios-rom.o
> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> index 9148d83..e4aa87e 100644
> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -941,7 +941,9 @@ int kvm_cmd_run(int argc, const char **argv, const char 
> *prefix)
>  
>   kvm__init_ram(kvm);
>  
> +#ifdef CONFIG_X86
>   kbd__init(kvm);
> +#endif
>  
>   pci_shmem__init(kvm);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Sasha.

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