Re: Software breakpoint in kvmppc guest debug
Am 10.11.2010 04:39, Alexander Graf wrote: On 10.11.2010, at 04:31, Yoder Stuart-B08248 wrote: -Original Message- From: kvm-ppc-ow...@vger.kernel.org [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf Sent: Tuesday, November 09, 2010 12:50 PM To: Wood Scott-B07421 Cc: Hollis Blanchard; Liu Yu-B13201; kvm-ppc@vger.kernel.org; Jan Kiszka Subject: Re: Software breakpoint in kvmppc guest debug On 09.11.2010, at 19:43, Scott Wood wrote: On Tue, 9 Nov 2010 19:26:25 +0100 Alexander Graf ag...@suse.de wrote: On 09.11.2010, at 19:17, Scott Wood wrote: On Tue, 9 Nov 2010 18:14:31 +0100 Alexander Graf ag...@suse.de wrote: Now, if we can get away with not using an undefined instruction (be it sc 64 or trap) I don't know. I'm not even sure we can get away with trap. Basically, WARN_ON should also trigger a trap, so you'd end up in gdb for that when having a breakpoint defined. There'd need to be a way for qemu/gdb to have KVM reflect the exception to the guest if it doesn't have a breakpoint on file for that address. Yes, but that piece is missing for every KVM target right now. I'd like to see something generic emerge here that we can reuse across different architectures :). We should try to define something that will make sense on other architectures, to whatever extent is practical -- but that doesn't mean we need to wait for them to act first. :-) Oh, I agree. I'm saying we should have Jan in the discussion :). Trap seems very tricky too though. According to page 1082 of the 2.06 spec, trap can issue a debug or a program interrupt depending on MSR_DE. I don't see any mentioning in the spec that we intercept program or debug interrupts. So I guess we'd have to override the offset vectors and handle _every_ interrupt ourselves. Bleks. Program and debug interrupts always go to the hypervisor. Only the exceptions for which GIVORs are defined (DSI, ISI, external IRQ, syscall, TLB miss) can go directly to the guest, and even those are generally optional based on EPCR. Ah, so it's a positive list. I guess I missed that part :). Thanks for the clarification. So making it a trap instruction for now should be ok. Let's align the code to the same x86 looks like for now. In the meanwhile, let's discuss with Jan on how to get a list of patched IPs into the kernel and implement that on top of the code we'll have aligned with x86 by then :). What about using the ehpriv instruction (defined in 2.06)? We could define a unique qemu software breakpoint, and the hypervisor's ehpriv handler will be able to easily distinguish them. On e500v2, this would be an illegal instruction. We don't need to worry about a conflict with future opcodes and it seems like the whole point of ehpriv being defined-- to let a hypervisor define special opcodes for stuff like this. Yes, it even states so in the little box below the description. Very good catch! If we use trap, we will need the list of patched IPs in the kernel. ehpriv avoids the need for that. Well, the way it's handled on x86 now is that the kernel asks user space if it knows about the breakpoint and if not user space reinjects the debug interrupt into the guest. Right. That's fine for x86 as we have a dedicated soft BP instruction. It's shared with the guest, but as long as the latter does not start using it heavily for whatever purpose, this lengthy loop to find the trap reason is OK. It avoids dispatching tables in the kernel. Still, if it actually turns out that PowerPC (or some other arch) needs this for performance reasons as no proper instructions could be defined, we should probably introduce a generic service to set a soft BP dispatching table in the kernel. That could also be used by x86 then, even though the practical improvement would be minimal there. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- 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 v2] KVM: Clean up vm creation and release
Am 10.11.2010 10:17, Avi Kivity wrote:
On 11/09/2010 11:01 PM, Alexander Graf wrote:
On 09.11.2010, at 17:02, Jan Kiszka wrote:
IA64 support forces us to abstract the allocation of the kvm structure.
But instead of mixing this up with arch-specific initialization and
doing the same on destruction, split both steps. This allows to move
generic destruction calls into generic code.
It also fixes error clean-up on failures of kvm_create_vm for IA64.
Signed-off-by: Jan Kiszkajan.kis...@siemens.com
---
Changes in v2:
- Fixed s390 conversion and added linux/slab.h
as remarked by Christian (thanks!)
arch/ia64/include/asm/kvm_host.h |4
arch/ia64/kvm/kvm-ia64.c | 28 +++-
arch/powerpc/kvm/powerpc.c | 20 +++-
arch/s390/kvm/kvm-s390.c | 23 ++-
arch/x86/kvm/x86.c | 12 ++--
include/linux/kvm_host.h | 15 ++-
virt/kvm/kvm_main.c | 19 +--
7 files changed, 49 insertions(+), 72 deletions(-)
diff --git a/arch/ia64/include/asm/kvm_host.h
b/arch/ia64/include/asm/kvm_host.h
index 2f229e5..2689ee5 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
void kvm_sal_emul(struct kvm_vcpu *vcpu);
+#define __KVM_HAVE_ARCH_VM_ALLOC 1
+struct kvm *kvm_arch_alloc_vm(void);
+void kvm_arch_free_vm(struct kvm *kvm);
+
#endif /* __ASSEMBLY__*/
#endif
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index f56a631..48a48bd 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -749,7 +749,7 @@ out:
return r;
}
-static struct kvm *kvm_alloc_kvm(void)
+struct kvm *kvm_arch_alloc_vm(void)
{
struct kvm *kvm;
@@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
if (!vm_base)
- return ERR_PTR(-ENOMEM);
+ return NULL;
memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
kvm = (struct kvm *)(vm_base +
@@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
#define GUEST_PHYSICAL_RR4 0x2739
#define VMM_INIT_RR0x1660
-static void kvm_init_vm(struct kvm *kvm)
+int kvm_arch_init_vm(struct kvm *kvm)
{
BUG_ON(!kvm);
+ kvm-arch.is_sn2 = ia64_platform_is(sn2);
+
kvm-arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
kvm-arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
kvm-arch.vmm_init_rr = VMM_INIT_RR;
@@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,kvm-arch.irq_sources_bitmap);
-}
-
-struct kvm *kvm_arch_create_vm(void)
-{
- struct kvm *kvm = kvm_alloc_kvm();
-
- if (IS_ERR(kvm))
- return ERR_PTR(-ENOMEM);
-
- kvm-arch.is_sn2 = ia64_platform_is(sn2);
-
- kvm_init_vm(kvm);
-
- return kvm;
+ return 0;
}
static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
@@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct
kvm_vcpu *vcpu,
return -EINVAL;
}
-static void free_kvm(struct kvm *kvm)
+void kvm_arch_free_vm(struct kvm *kvm)
{
unsigned long vm_base = kvm-arch.vm_base;
@@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
#endif
kfree(kvm-arch.vioapic);
kvm_release_vm_pages(kvm);
- kvm_free_physmem(kvm);
- cleanup_srcu_struct(kvm-srcu);
- free_kvm(kvm);
}
void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 38f756f..ce3dd65 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
*(int *)rtn = kvmppc_core_check_processor_compat();
}
-struct kvm *kvm_arch_create_vm(void)
+int kvm_arch_init_vm(struct kvm *)
Eh, no. This doesn't compile :).
What's the problem? lack of a formal argument?
The unnamed argument is referenced as 'kvm' in this function. Will you
fix this up on merge or should I resend?
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
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 v2] KVM: Clean up vm creation and release
On 11/09/2010 11:01 PM, Alexander Graf wrote:
On 09.11.2010, at 17:02, Jan Kiszka wrote:
IA64 support forces us to abstract the allocation of the kvm structure.
But instead of mixing this up with arch-specific initialization and
doing the same on destruction, split both steps. This allows to move
generic destruction calls into generic code.
It also fixes error clean-up on failures of kvm_create_vm for IA64.
Signed-off-by: Jan Kiszkajan.kis...@siemens.com
---
Changes in v2:
- Fixed s390 conversion and added linux/slab.h
as remarked by Christian (thanks!)
arch/ia64/include/asm/kvm_host.h |4
arch/ia64/kvm/kvm-ia64.c | 28 +++-
arch/powerpc/kvm/powerpc.c | 20 +++-
arch/s390/kvm/kvm-s390.c | 23 ++-
arch/x86/kvm/x86.c | 12 ++--
include/linux/kvm_host.h | 15 ++-
virt/kvm/kvm_main.c | 19 +--
7 files changed, 49 insertions(+), 72 deletions(-)
diff --git a/arch/ia64/include/asm/kvm_host.h
b/arch/ia64/include/asm/kvm_host.h
index 2f229e5..2689ee5 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
void kvm_sal_emul(struct kvm_vcpu *vcpu);
+#define __KVM_HAVE_ARCH_VM_ALLOC 1
+struct kvm *kvm_arch_alloc_vm(void);
+void kvm_arch_free_vm(struct kvm *kvm);
+
#endif /* __ASSEMBLY__*/
#endif
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index f56a631..48a48bd 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -749,7 +749,7 @@ out:
return r;
}
-static struct kvm *kvm_alloc_kvm(void)
+struct kvm *kvm_arch_alloc_vm(void)
{
struct kvm *kvm;
@@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
if (!vm_base)
- return ERR_PTR(-ENOMEM);
+ return NULL;
memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
kvm = (struct kvm *)(vm_base +
@@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
#define GUEST_PHYSICAL_RR40x2739
#define VMM_INIT_RR 0x1660
-static void kvm_init_vm(struct kvm *kvm)
+int kvm_arch_init_vm(struct kvm *kvm)
{
BUG_ON(!kvm);
+ kvm-arch.is_sn2 = ia64_platform_is(sn2);
+
kvm-arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
kvm-arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
kvm-arch.vmm_init_rr = VMM_INIT_RR;
@@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,kvm-arch.irq_sources_bitmap);
-}
-
-struct kvm *kvm_arch_create_vm(void)
-{
- struct kvm *kvm = kvm_alloc_kvm();
-
- if (IS_ERR(kvm))
- return ERR_PTR(-ENOMEM);
-
- kvm-arch.is_sn2 = ia64_platform_is(sn2);
-
- kvm_init_vm(kvm);
-
- return kvm;
+ return 0;
}
static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
@@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct
kvm_vcpu *vcpu,
return -EINVAL;
}
-static void free_kvm(struct kvm *kvm)
+void kvm_arch_free_vm(struct kvm *kvm)
{
unsigned long vm_base = kvm-arch.vm_base;
@@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
#endif
kfree(kvm-arch.vioapic);
kvm_release_vm_pages(kvm);
- kvm_free_physmem(kvm);
- cleanup_srcu_struct(kvm-srcu);
- free_kvm(kvm);
}
void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 38f756f..ce3dd65 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
*(int *)rtn = kvmppc_core_check_processor_compat();
}
-struct kvm *kvm_arch_create_vm(void)
+int kvm_arch_init_vm(struct kvm *)
Eh, no. This doesn't compile :).
What's the problem? lack of a formal argument?
Apart from that, the code seems to work on ppc64.
Tested-by: Alexander Grafag...@suse.de
Seems my ppc builder has died, so thanks for the test.
--
error compiling committee.c: too many arguments to function
--
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 v2] KVM: Clean up vm creation and release
On 10.11.2010, at 11:21, Avi Kivity wrote:
On 11/10/2010 12:18 PM, Jan Kiszka wrote:
Am 10.11.2010 10:45, Avi Kivity wrote:
On 11/10/2010 11:30 AM, Jan Kiszka wrote:
Am 10.11.2010 10:17, Avi Kivity wrote:
On 11/09/2010 11:01 PM, Alexander Graf wrote:
On 09.11.2010, at 17:02, Jan Kiszka wrote:
IA64 support forces us to abstract the allocation of the kvm
structure.
But instead of mixing this up with arch-specific initialization and
doing the same on destruction, split both steps. This allows to move
generic destruction calls into generic code.
It also fixes error clean-up on failures of kvm_create_vm for IA64.
Signed-off-by: Jan Kiszkajan.kis...@siemens.com
---
Changes in v2:
- Fixed s390 conversion and added linux/slab.h
as remarked by Christian (thanks!)
arch/ia64/include/asm/kvm_host.h |4
arch/ia64/kvm/kvm-ia64.c | 28 +++-
arch/powerpc/kvm/powerpc.c | 20 +++-
arch/s390/kvm/kvm-s390.c | 23 ++-
arch/x86/kvm/x86.c | 12 ++--
include/linux/kvm_host.h | 15 ++-
virt/kvm/kvm_main.c | 19 +--
7 files changed, 49 insertions(+), 72 deletions(-)
diff --git a/arch/ia64/include/asm/kvm_host.h
b/arch/ia64/include/asm/kvm_host.h
index 2f229e5..2689ee5 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
void kvm_sal_emul(struct kvm_vcpu *vcpu);
+#define __KVM_HAVE_ARCH_VM_ALLOC 1
+struct kvm *kvm_arch_alloc_vm(void);
+void kvm_arch_free_vm(struct kvm *kvm);
+
#endif /* __ASSEMBLY__*/
#endif
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index f56a631..48a48bd 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -749,7 +749,7 @@ out:
return r;
}
-static struct kvm *kvm_alloc_kvm(void)
+struct kvm *kvm_arch_alloc_vm(void)
{
struct kvm *kvm;
@@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
vm_base = __get_free_pages(GFP_KERNEL,
get_order(KVM_VM_DATA_SIZE));
if (!vm_base)
- return ERR_PTR(-ENOMEM);
+ return NULL;
memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
kvm = (struct kvm *)(vm_base +
@@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
#define GUEST_PHYSICAL_RR4 0x2739
#define VMM_INIT_RR 0x1660
-static void kvm_init_vm(struct kvm *kvm)
+int kvm_arch_init_vm(struct kvm *kvm)
{
BUG_ON(!kvm);
+ kvm-arch.is_sn2 = ia64_platform_is(sn2);
+
kvm-arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
kvm-arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
kvm-arch.vmm_init_rr = VMM_INIT_RR;
@@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
/* Reserve bit 0 of irq_sources_bitmap for userspace irq source
*/
set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,kvm-arch.irq_sources_bitmap);
-}
-
-struct kvm *kvm_arch_create_vm(void)
-{
- struct kvm *kvm = kvm_alloc_kvm();
-
- if (IS_ERR(kvm))
- return ERR_PTR(-ENOMEM);
-
- kvm-arch.is_sn2 = ia64_platform_is(sn2);
-
- kvm_init_vm(kvm);
-
- return kvm;
+ return 0;
}
static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
@@ -1357,7 +1346,7 @@ int
kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
return -EINVAL;
}
-static void free_kvm(struct kvm *kvm)
+void kvm_arch_free_vm(struct kvm *kvm)
{
unsigned long vm_base = kvm-arch.vm_base;
@@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
#endif
kfree(kvm-arch.vioapic);
kvm_release_vm_pages(kvm);
- kvm_free_physmem(kvm);
- cleanup_srcu_struct(kvm-srcu);
- free_kvm(kvm);
}
void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 38f756f..ce3dd65 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void
*rtn)
*(int *)rtn = kvmppc_core_check_processor_compat();
}
-struct kvm *kvm_arch_create_vm(void)
+int kvm_arch_init_vm(struct kvm *)
Eh, no. This doesn't compile :).
What's the problem? lack of a formal argument?
The unnamed argument is referenced as 'kvm' in this function.
It isn't referenced:
int kvm_arch_init_vm(struct kvm *)
{
return 0;
}
