Re: [PATCH 1/6] KVM: PPC: booke: use vcpu reference from thread_struct
On 21.08.2012, at 15:51, Bharat Bhushan wrote: > Like other places, use thread_struct to get vcpu reference. Please remove the definition of SPRN_SPRG_R/WVCPU as well. Alex > > Signed-off-by: Bharat Bhushan > --- > arch/powerpc/kernel/asm-offsets.c |2 +- > arch/powerpc/kvm/booke_interrupts.S |6 ++ > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/kernel/asm-offsets.c > b/arch/powerpc/kernel/asm-offsets.c > index 85b05c4..fbb999c 100644 > --- a/arch/powerpc/kernel/asm-offsets.c > +++ b/arch/powerpc/kernel/asm-offsets.c > @@ -116,7 +116,7 @@ int main(void) > #ifdef CONFIG_KVM_BOOK3S_32_HANDLER > DEFINE(THREAD_KVM_SVCPU, offsetof(struct thread_struct, > kvm_shadow_vcpu)); > #endif > -#ifdef CONFIG_KVM_BOOKE_HV > +#if defined(CONFIG_KVM) && defined(CONFIG_BOOKE) > DEFINE(THREAD_KVM_VCPU, offsetof(struct thread_struct, kvm_vcpu)); > #endif > > diff --git a/arch/powerpc/kvm/booke_interrupts.S > b/arch/powerpc/kvm/booke_interrupts.S > index bb46b32..ca16d57 100644 > --- a/arch/powerpc/kvm/booke_interrupts.S > +++ b/arch/powerpc/kvm/booke_interrupts.S > @@ -56,7 +56,8 @@ > _GLOBAL(kvmppc_handler_\ivor_nr) > /* Get pointer to vcpu and record exit number. */ > mtspr \scratch , r4 > - mfspr r4, SPRN_SPRG_RVCPU > + mfspr r4, SPRN_SPRG_THREAD > + lwz r4, THREAD_KVM_VCPU(r4) > stw r3, VCPU_GPR(R3)(r4) > stw r5, VCPU_GPR(R5)(r4) > stw r6, VCPU_GPR(R6)(r4) > @@ -402,9 +403,6 @@ lightweight_exit: > lwz r8, kvmppc_booke_handlers@l(r8) > mtspr SPRN_IVPR, r8 > > - /* Save vcpu pointer for the exception handlers. */ > - mtspr SPRN_SPRG_WVCPU, r4 > - > lwz r5, VCPU_SHARED(r4) > > /* Can't switch the stack pointer until after IVPR is switched, > -- > 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 6/6] KVM: booke/bookehv: Add debug stub support
On 21.08.2012, at 15:52, Bharat Bhushan wrote:
> This patch adds the debug stub support on booke/bookehv.
> Now QEMU debug stub can use hw breakpoint, watchpoint and
> software breakpoint to debug guest.
>
> Signed-off-by: Bharat Bhushan
> ---
> arch/powerpc/include/asm/kvm.h| 29 ++-
> arch/powerpc/include/asm/kvm_host.h |5 +
> arch/powerpc/kernel/asm-offsets.c | 26 ++
> arch/powerpc/kvm/booke.c | 144 +
> arch/powerpc/kvm/booke_interrupts.S | 110 +
> arch/powerpc/kvm/bookehv_interrupts.S | 141 +++-
> arch/powerpc/kvm/e500mc.c |3 +-
> 7 files changed, 435 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
> index 61b197e..53479ea 100644
> --- a/arch/powerpc/include/asm/kvm.h
> +++ b/arch/powerpc/include/asm/kvm.h
> @@ -25,6 +25,7 @@
> /* Select powerpc specific features in */
> #define __KVM_HAVE_SPAPR_TCE
> #define __KVM_HAVE_PPC_SMT
> +#define __KVM_HAVE_GUEST_DEBUG
>
> struct kvm_regs {
> __u64 pc;
> @@ -264,7 +265,31 @@ struct kvm_fpu {
> __u64 fpr[32];
> };
>
> +
> +/*
> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
> + * software breakpoint.
> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
> + * for KVM_DEBUG_EXIT.
> + */
> +#define KVMPPC_DEBUG_NONE0x0
> +#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
> +#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
> +#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
> struct kvm_debug_exit_arch {
> + __u64 pc;
> + /*
> + * exception -> returns the exception number. If the KVM_DEBUG_EXIT
> + * exit is not handled (say not h/w breakpoint or software breakpoint
> + * set for this address) by qemu then it is supposed to inject this
> + * exception to guest.
> + */
> + __u32 exception;
> + /*
> + * exiting to userspace because of h/w breakpoint, watchpoint
> + * (read, write or both) and software breakpoint.
> + */
> + __u32 status;
> };
>
> /* for KVM_SET_GUEST_DEBUG */
> @@ -276,10 +301,6 @@ struct kvm_guest_debug_arch {
>* Type denotes h/w breakpoint, read watchpoint, write
>* watchpoint or watchpoint (both read and write).
>*/
> -#define KVMPPC_DEBUG_NOTYPE 0x0
> -#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
> -#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
> -#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
> __u32 type;
> __u32 pad1;
> __u64 pad2;
> diff --git a/arch/powerpc/include/asm/kvm_host.h
> b/arch/powerpc/include/asm/kvm_host.h
> index c7219c1..3ba465a 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -496,7 +496,12 @@ struct kvm_vcpu_arch {
> u32 mmucfg;
> u32 epr;
> u32 crit_save;
> + /* guest debug registers*/
> struct kvmppc_booke_debug_reg dbg_reg;
> + /* shadow debug registers */
> + struct kvmppc_booke_debug_reg shadow_dbg_reg;
> + /* host debug registers*/
> + struct kvmppc_booke_debug_reg host_dbg_reg;
> #endif
> gpa_t paddr_accessed;
> gva_t vaddr_accessed;
> diff --git a/arch/powerpc/kernel/asm-offsets.c
> b/arch/powerpc/kernel/asm-offsets.c
> index 555448e..6987821 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -564,6 +564,32 @@ int main(void)
> DEFINE(VCPU_FAULT_DEAR, offsetof(struct kvm_vcpu, arch.fault_dear));
> DEFINE(VCPU_FAULT_ESR, offsetof(struct kvm_vcpu, arch.fault_esr));
> DEFINE(VCPU_CRIT_SAVE, offsetof(struct kvm_vcpu, arch.crit_save));
> + DEFINE(VCPU_DBSR, offsetof(struct kvm_vcpu, arch.dbsr));
> + DEFINE(VCPU_SHADOW_DBG, offsetof(struct kvm_vcpu, arch.shadow_dbg_reg));
> + DEFINE(VCPU_HOST_DBG, offsetof(struct kvm_vcpu, arch.host_dbg_reg));
> + DEFINE(KVMPPC_DBG_DBCR0, offsetof(struct kvmppc_booke_debug_reg,
> + dbcr0));
> + DEFINE(KVMPPC_DBG_DBCR1, offsetof(struct kvmppc_booke_debug_reg,
> + dbcr1));
> + DEFINE(KVMPPC_DBG_DBCR2, offsetof(struct kvmppc_booke_debug_reg,
> + dbcr2));
> +#ifdef CONFIG_KVM_E500MC
> + DEFINE(KVMPPC_DBG_DBCR4, offsetof(struct kvmppc_booke_debug_reg,
> + dbcr4));
> +#endif
> + DEFINE(KVMPPC_DBG_IAC1, offsetof(struct kvmppc_booke_debug_reg,
> + iac[0]));
> + DEFINE(KVMPPC_DBG_IAC2, offsetof(struct kvmppc_booke_debug_reg,
> + iac[1]));
> + DEFINE(KVMPPC_DBG_IAC3, offsetof(struct kvmppc_booke_debug_reg,
> + iac[2]));
> + DEFINE(KVMPPC_DBG_IAC4, offsetof(stru
Re: [PATCH 4/6] KVM: PPC: debug stub interface parameter defined
On 21.08.2012, at 15:51, Bharat Bhushan wrote:
> This patch defines the interface parameter for KVM_SET_GUEST_DEBUG
> ioctl support. Follow up patches will use this for setting up
> hardware breakpoints, watchpoints and software breakpoints.
>
> Signed-off-by: Bharat Bhushan
> ---
> arch/powerpc/include/asm/kvm.h | 33 +
> arch/powerpc/kvm/book3s.c |6 ++
> arch/powerpc/kvm/booke.c |6 ++
> arch/powerpc/kvm/powerpc.c |6 --
> 4 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
> index 3c14202..61b197e 100644
> --- a/arch/powerpc/include/asm/kvm.h
> +++ b/arch/powerpc/include/asm/kvm.h
> @@ -269,8 +269,41 @@ struct kvm_debug_exit_arch {
>
> /* for KVM_SET_GUEST_DEBUG */
> struct kvm_guest_debug_arch {
> + struct {
> + /* H/W breakpoint/watchpoint address */
> + __u64 addr;
> + /*
> + * Type denotes h/w breakpoint, read watchpoint, write
> + * watchpoint or watchpoint (both read and write).
> + */
> +#define KVMPPC_DEBUG_NOTYPE 0x0
> +#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
> +#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
> +#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
> + __u32 type;
> + __u32 pad1;
Why the padding?
> + __u64 pad2;
> + } bp[16];
Why 16?
> };
>
> +/* Debug related defines */
> +/*
> + * kvm_guest_debug->control is a 32 bit field. The lower 16 bits are generic
> + * and upper 16 bits are architecture specific. Architecture specific defines
> + * that ioctl is for setting hardware breakpoint or software breakpoint.
> + */
> +#define KVM_GUESTDBG_USE_SW_BP 0x0001
> +#define KVM_GUESTDBG_USE_HW_BP 0x0002
> +
> +/* When setting software breakpoint, Change the software breakpoint
> + * instruction to special trap instruction and set KVM_GUESTDBG_USE_SW_BP
> + * flag in kvm_guest_debug->control. KVM does keep track of software
> + * breakpoints. So when KVM_GUESTDBG_USE_SW_BP flag is set and special trap
> + * instruction is executed by guest then exit to userspace.
> + * NOTE: A Nice interface can be added to get the special trap instruction.
> + */
> +#define KVMPPC_INST_GUEST_GDB0x7C00021C /* ehpriv OC=0
> */
This definitely has to be passed to user space (which writes that instruction
into guest phys memory). Other PPC subarchs will use different instructions.
Just model it as a read-only ONE_REG.
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 6/6] KVM: booke/bookehv: Add debug stub support
On 07.09.2012, at 00:56, Scott Wood wrote:
> On 09/06/2012 09:56 AM, Bhushan Bharat-R65777 wrote:
>>
>>
>>> -Original Message-
>>> From: Wood Scott-B07421
>>> Sent: Thursday, September 06, 2012 4:57 AM
>>> To: Bhushan Bharat-R65777
>>> Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; ag...@suse.de; Bhushan
>>> Bharat-
>>> R65777
>>> Subject: Re: [PATCH 6/6] KVM: booke/bookehv: Add debug stub support
>>>
>>> On 09/05/2012 06:23 PM, Scott Wood wrote:
On 08/21/2012 08:52 AM, Bharat Bhushan wrote:
> This patch adds the debug stub support on booke/bookehv.
> Now QEMU debug stub can use hw breakpoint, watchpoint and software
> breakpoint to debug guest.
>
> Signed-off-by: Bharat Bhushan
> ---
> arch/powerpc/include/asm/kvm.h| 29 ++-
> arch/powerpc/include/asm/kvm_host.h |5 +
> arch/powerpc/kernel/asm-offsets.c | 26 ++
> arch/powerpc/kvm/booke.c | 144
> +--
>>> --
> arch/powerpc/kvm/booke_interrupts.S | 110 +
> arch/powerpc/kvm/bookehv_interrupts.S | 141
>>> +++-
> arch/powerpc/kvm/e500mc.c |3 +-
> 7 files changed, 435 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm.h
> b/arch/powerpc/include/asm/kvm.h index 61b197e..53479ea 100644
> --- a/arch/powerpc/include/asm/kvm.h
> +++ b/arch/powerpc/include/asm/kvm.h
> @@ -25,6 +25,7 @@
> /* Select powerpc specific features in */ #define
> __KVM_HAVE_SPAPR_TCE #define __KVM_HAVE_PPC_SMT
> +#define __KVM_HAVE_GUEST_DEBUG
>
> struct kvm_regs {
> __u64 pc;
> @@ -264,7 +265,31 @@ struct kvm_fpu {
> __u64 fpr[32];
> };
>
> +
> +/*
> + * Defines for h/w breakpoint, watchpoint (read, write or both) and
> + * software breakpoint.
> + * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
> + * for KVM_DEBUG_EXIT.
> + */
> +#define KVMPPC_DEBUG_NONE0x0
> +#define KVMPPC_DEBUG_BREAKPOINT (1UL << 1)
> +#define KVMPPC_DEBUG_WATCH_WRITE (1UL << 2)
> +#define KVMPPC_DEBUG_WATCH_READ (1UL << 3)
> struct kvm_debug_exit_arch {
That says "arch", but it's not in an arch-specific file.
>>>
>>> Sigh, I can't read today apparently.
>>>
> + __u64 pc;
> + /*
> + * exception -> returns the exception number. If the KVM_DEBUG_EXIT
> + * exit is not handled (say not h/w breakpoint or software breakpoint
> + * set for this address) by qemu then it is supposed to inject this
> + * exception to guest.
> + */
> + __u32 exception;
> + /*
> + * exiting to userspace because of h/w breakpoint, watchpoint
> + * (read, write or both) and software breakpoint.
> + */
> + __u32 status;
> };
What does "exception number" mean in a generic API?
>>>
>>> Still, "exception number" is not a well-defined concept powerpc-wide.
>>
>> Just for background why we added is that, on x86 this exception number is
>> used to inject the exception to guest if QEMU is not able to handle the
>> debug exception.
>>
>> Should we just through a print with clearing the exception condition? Or
>> something else you would like to suggest?
>
> We can pass up the exception type; it just needs more documentation
> about what exactly you're referring to, and probably some enumeration
> that says which exception numberspace it is.
>
> For booke the exception number should probably be related to the fixed
> offsets rather than the IVOR number, as IVORs are phased out.
Jan, I would like to get your comment on this one.
Since we don't have standardized exception vectors like x86 does, we need to
convert things between different semantics in user space if we want to make use
of the exception type. Do we actually need to know about it in user space or do
we only need to store it in case we get a migration at that point?
If it's the latter, can we maybe keep the reinjection logic internal to KVM and
make DEBUG exits non-migratable similar to how we already handle MMIO/PIO exits
today?
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 10/10] KVM: PPC: Book3S HV: Fix calculation of guest phys address for MMIO emulation
On 21.09.2012, at 07:39, Paul Mackerras wrote: > In the case where the host kernel is using a 64kB base page size and > the guest uses a 4k HPTE (hashed page table entry) to map an emulated > MMIO device, we were calculating the guest physical address wrongly. > We were calculating a gfn as the guest physical address shifted right > 16 bits (PAGE_SHIFT) but then only adding back in 12 bits from the > effective address, since the HPTE had a 4k page size. Thus the gpa > reported to userspace was missing 4 bits. > > Instead, we now compute the guest physical address from the HPTE > without reference to the host page size, and then compute the gfn > by shifting the gpa right PAGE_SHIFT bits. > > Reported-by: Alexey Kardashevskiy > Signed-off-by: Paul Mackerras Thanks, applied to kvm-ppc-next. 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 04/10] KVM: PPC: Book3S HV: Remove bogus update of physical thread IDs
On 21.09.2012, at 07:36, Paul Mackerras wrote: > When making a vcpu non-runnable we incorrectly changed the > thread IDs of all other threads on the core, just remove that > code. > > Signed-off-by: Benjamin Herrenschmidt > Signed-off-by: Paul Mackerras Thanks, applied to kvm-ppc-next. 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 03/10] KVM: PPC: Book3S HV: Fix updates of vcpu->cpu
On 21.09.2012, at 07:35, Paul Mackerras wrote: > This removes the powerpc "generic" updates of vcpu->cpu in load and > put, and moves them to the various backends. > > The reason is that "HV" KVM does its own sauce with that field > and the generic updates might corrupt it. The field contains the > CPU# of the -first- HW CPU of the core always for all the VCPU > threads of a core (the one that's online from a host Linux > perspective). > > However, the preempt notifiers are going to be called on the > threads VCPUs when they are running (due to them sleeping on our > private waitqueue) causing unload to be called, potentially > clobbering the value. > > Signed-off-by: Benjamin Herrenschmidt > Signed-off-by: Paul Mackerras Thanks, applied to kvm-ppc-next. 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 06/10] KVM: PPC: Book3s HV: Don't access runnable threads list without vcore lock
On 21.09.2012, at 07:37, Paul Mackerras wrote: > There were a few places where we were traversing the list of runnable > threads in a virtual core, i.e. vc->runnable_threads, without holding > the vcore spinlock. This extends the places where we hold the vcore > spinlock to cover everywhere that we traverse that list. > > Since we possibly need to sleep inside kvmppc_book3s_hv_page_fault, > this moves the call of it from kvmppc_handle_exit out to > kvmppc_vcpu_run, where we don't hold the vcore lock. > > In kvmppc_vcore_blocked, we don't actually need to check whether > all vcpus are ceded and don't have any pending exceptions, since the > caller has already done that. The caller (kvmppc_run_vcpu) wasn't > actually checking for pending exceptions, so we add that. > > The change of if to while in kvmppc_run_vcpu is to make sure that we > never call kvmppc_remove_runnable() when the vcore state is RUNNING or > EXITING. > > Signed-off-by: Paul Mackerras > --- > arch/powerpc/include/asm/kvm_asm.h |1 + > arch/powerpc/kvm/book3s_hv.c | 64 +--- > 2 files changed, 31 insertions(+), 34 deletions(-) > > diff --git a/arch/powerpc/include/asm/kvm_asm.h > b/arch/powerpc/include/asm/kvm_asm.h > index 76fdcfe..fb99a21 100644 > --- a/arch/powerpc/include/asm/kvm_asm.h > +++ b/arch/powerpc/include/asm/kvm_asm.h > @@ -123,6 +123,7 @@ > #define RESUME_GUEST_NV RESUME_FLAG_NV > #define RESUME_HOST RESUME_FLAG_HOST > #define RESUME_HOST_NV (RESUME_FLAG_HOST|RESUME_FLAG_NV) > +#define RESUME_PAGE_FAULT(1<<2) I would actually prefer if you could move this to core specific code. How about #define RESUME_ARCH1(1 << 2) and then in book3s_hv.c: #define RESUME_PAGE_FAULT (RESUME_GUEST | RESUME_ARCH1) 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 v3 1/2] KVM: PPC: Book3S: Get/set guest SPRs using the GET/SET_ONE_REG interface
On 24.09.2012, at 14:16, Paul Mackerras wrote:
> On Fri, Sep 21, 2012 at 11:15:51AM +0200, Alexander Graf wrote:
>
>> So how about something like
>>
>> #define kvmppc_set_reg(id, val, reg) { \
>> switch (one_reg_size(id)) { \
>> case 4: val.wval = reg; break; \
>> case 8: val.dval = reg; break; \
>> default: BUG(); \
>> } \
>> }
>>
>> case KVM_REG_PPC_DAR:
>> kvmppc_set_reg(reg->id, &val, vcpu->arch.shared->dar);
>> break;
>
> I tried that and it was fine for the wval vs. dval thing, and gcc even
> compiled it to the same number of instructions as what I had before.
> But it breaks down when you get to VMX and VSX -- firstly because
> there are two different types that are both 16 bytes, and secondly
> because when you do this:
>
> #define kvmppc_get_reg(id, val, reg) { \
> switch (one_reg_size(id)) { \
> case 4: val.wval = reg; break; \
> case 8: val.dval = reg; break; \
> case 16: val.vval = reg; break; \
> default: BUG(); \
> } \
> }
>
> you get compile errors on things like:
>
> kvmppc_set_reg(reg->id, val, vcpu->arch.shared->dar);
>
> because val.vval is a vector128, and vcpu->arch.shared->dar is a u64,
> and you can't assign a u64 to a vector128 since vector128 is a struct.
> In other words, all of the cases of the switch have to satisfy the
> type rules even though only one of them will ever be used. Basically
> that trick will only work for integer types, and we don't have 128 bit
> integer support in gcc.
What a shame. Could you please repost a version that only handles 32/64 setting
with the above helper then and leaves 128-bit the way they are implemented now?
> Given all that, I would like to see my patches go in while we continue
> to search for a way to avoid the potential mistakes you're talking
> about.
... that way we can at least make the "common case" of 32-bit and 64-bit
registers error proof and only have to worry more about double-checking the
128-bit ones, which are a lot less.
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 02/10] KVM: PPC: Book3S HV: Allow KVM guests to stop secondary threads coming online
On 21.09.2012, at 07:35, Paul Mackerras wrote:
> When a Book3S HV KVM guest is running, we need the host to be in
> single-thread mode, that is, all of the cores (or at least all of
> the cores where the KVM guest could run) to be running only one
> active hardware thread. This is because of the hardware restriction
> in POWER processors that all of the hardware threads in the core
> must be in the same logical partition. Complying with this restriction
> is much easier if, from the host kernel's point of view, only one
> hardware thread is active.
>
> This adds two hooks in the SMP hotplug code to allow the KVM code to
> make sure that secondary threads (i.e. hardware threads other than
> thread 0) cannot come online while any KVM guest exists. The KVM
> code still has to check that any core where it runs a guest has the
> secondary threads offline, but having done that check it can now be
> sure that they will not come online while the guest is running.
>
> Signed-off-by: Paul Mackerras
Ben, since this touches generic ppc code, could you please ack?
Alex
> ---
> arch/powerpc/include/asm/smp.h |8 +++
> arch/powerpc/kernel/smp.c | 46
> arch/powerpc/kvm/book3s_hv.c | 12 +--
> 3 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> index ebc24dc..b625a1a 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -66,6 +66,14 @@ void generic_cpu_die(unsigned int cpu);
> void generic_mach_cpu_die(void);
> void generic_set_cpu_dead(unsigned int cpu);
> int generic_check_cpu_restart(unsigned int cpu);
> +
> +extern void inhibit_secondary_onlining(void);
> +extern void uninhibit_secondary_onlining(void);
> +
> +#else /* HOTPLUG_CPU */
> +static inline void inhibit_secondary_onlining(void) {}
> +static inline void uninhibit_secondary_onlining(void) {}
> +
> #endif
>
> #ifdef CONFIG_PPC64
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 0321007..c45f51d 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -410,6 +410,45 @@ int generic_check_cpu_restart(unsigned int cpu)
> {
> return per_cpu(cpu_state, cpu) == CPU_UP_PREPARE;
> }
> +
> +static atomic_t secondary_inhibit_count;
> +
> +/*
> + * Don't allow secondary CPU threads to come online
> + */
> +void inhibit_secondary_onlining(void)
> +{
> + /*
> + * This makes secondary_inhibit_count stable during cpu
> + * online/offline operations.
> + */
> + get_online_cpus();
> +
> + atomic_inc(&secondary_inhibit_count);
> + put_online_cpus();
> +}
> +EXPORT_SYMBOL_GPL(inhibit_secondary_onlining);
> +
> +/*
> + * Allow secondary CPU threads to come online again
> + */
> +void uninhibit_secondary_onlining(void)
> +{
> + get_online_cpus();
> + atomic_dec(&secondary_inhibit_count);
> + put_online_cpus();
> +}
> +EXPORT_SYMBOL_GPL(uninhibit_secondary_onlining);
> +
> +static int secondaries_inhibited(void)
> +{
> + return atomic_read(&secondary_inhibit_count);
> +}
> +
> +#else /* HOTPLUG_CPU */
> +
> +#define secondaries_inhibited() 0
> +
> #endif
>
> static void cpu_idle_thread_init(unsigned int cpu, struct task_struct *idle)
> @@ -428,6 +467,13 @@ int __cpuinit __cpu_up(unsigned int cpu, struct
> task_struct *tidle)
> {
> int rc, c;
>
> + /*
> + * Don't allow secondary threads to come online if inhibited
> + */
> + if (threads_per_core > 1 && secondaries_inhibited() &&
> + cpu % threads_per_core != 0)
> + return -EBUSY;
> +
> if (smp_ops == NULL ||
> (smp_ops->cpu_bootable && !smp_ops->cpu_bootable(cpu)))
> return -EINVAL;
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index bebf9cb..6fe1410 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -47,6 +47,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -918,8 +919,6 @@ static int kvmppc_run_core(struct kvmppc_vcore *vc)
> /*
>* Make sure we are running on thread 0, and that
>* secondary threads are offline.
> - * XXX we should also block attempts to bring any
> - * secondary threads online.
>*/
> if (threads_per_core > 1 && !on_primary_thread()) {
> list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list)
> @@ -1632,11 +1631,20 @@ int kvmppc_core_init_vm(struct kvm *kvm)
>
> kvm->arch.using_mmu_notifiers = !!cpu_has_feature(CPU_FTR_ARCH_206);
> spin_lock_init(&kvm->arch.slot_phys_lock);
> +
> + /*
> + * Don't allow secondary CPU threads to come online
> + * while any KVM VMs exist.
> + */
> + inhibit_secondary_onlining();
> +
> return 0;
> }
>
> void kvmppc_core_destroy_vm(struct kvm *kvm)
> {
> + uninhibit_secondary_onlining(
Re: [PATCH 01/10] KVM: PPC: Book3S HV: Provide a way for userspace to get/set per-vCPU areas
On 21.09.2012, at 07:33, Paul Mackerras wrote: > The PAPR paravirtualization interface lets guests register three > different types of per-vCPU buffer areas in its memory for communication > with the hypervisor. These are called virtual processor areas (VPAs). > Currently the hypercalls to register and unregister VPAs are handled > by KVM in the kernel, and userspace has no way to know about or save > and restore these registrations across a migration. > > This adds get and set ioctls to allow userspace to see what addresses > have been registered, and to register or unregister them. This will > be needed for guest hibernation and migration, and is also needed so > that userspace can unregister them on reset (otherwise we corrupt > guest memory after reboot by writing to the VPAs registered by the > previous kernel). We also add a capability to indicate that the > ioctls are supported. > > This also fixes a bug where we were calling init_vpa unconditionally, > leading to an oops when unregistering the VPA. > > Signed-off-by: Paul Mackerras Do you think it'd be possible to map these onto ONE_REG as well? I'm slightly wary to add an interface that is restricted to only a limited amount of entries. What if some future PAPR spec wants to add another VPA to the game? We'd have to do a completely new ioctl for that one then. However, if instead we could have 3 REGs 64-bit VPA_ADDR 128-bit VPA_SLB 128-bit VPA_DTL where you'd have to set VPA_ADDR first, then the other two. It gives us nice extensibility for the future too, right? We could just add another REG and everyone's happy. 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 v3 1/2] KVM: PPC: Book3S: Get/set guest SPRs using the GET/SET_ONE_REG interface
On Fri, Sep 21, 2012 at 11:15:51AM +0200, Alexander Graf wrote:
> So how about something like
>
> #define kvmppc_set_reg(id, val, reg) { \
> switch (one_reg_size(id)) { \
> case 4: val.wval = reg; break; \
> case 8: val.dval = reg; break; \
> default: BUG(); \
> } \
> }
>
> case KVM_REG_PPC_DAR:
> kvmppc_set_reg(reg->id, &val, vcpu->arch.shared->dar);
> break;
I tried that and it was fine for the wval vs. dval thing, and gcc even
compiled it to the same number of instructions as what I had before.
But it breaks down when you get to VMX and VSX -- firstly because
there are two different types that are both 16 bytes, and secondly
because when you do this:
#define kvmppc_get_reg(id, val, reg) { \
switch (one_reg_size(id)) { \
case 4: val.wval = reg; break; \
case 8: val.dval = reg; break; \
case 16: val.vval = reg; break; \
default: BUG(); \
} \
}
you get compile errors on things like:
kvmppc_set_reg(reg->id, val, vcpu->arch.shared->dar);
because val.vval is a vector128, and vcpu->arch.shared->dar is a u64,
and you can't assign a u64 to a vector128 since vector128 is a struct.
In other words, all of the cases of the switch have to satisfy the
type rules even though only one of them will ever be used. Basically
that trick will only work for integer types, and we don't have 128 bit
integer support in gcc.
Given all that, I would like to see my patches go in while we continue
to search for a way to avoid the potential mistakes you're talking
about.
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
