Re: [PATCH v6 3/5] KVM: PPC: clean up redundant kvm_run parameters in assembly
On Tue, Jun 23, 2020 at 09:14:16PM +0800, Tianjia Zhang wrote: > In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' > structure. For historical reasons, many kvm-related function parameters > retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This > patch does a unified cleanup of these remaining redundant parameters. Thanks, patch applied to my kvm-ppc-next branch, with fixes. Paul. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 3/7] KVM: PPC: Remove redundant kvm_run from vcpu_arch
On Mon, Apr 27, 2020 at 12:35:10PM +0800, Tianjia Zhang wrote: > The 'kvm_run' field already exists in the 'vcpu' structure, which > is the same structure as the 'kvm_run' in the 'vcpu_arch' and > should be deleted. > > Signed-off-by: Tianjia Zhang Thanks, patches 3 and 4 of this series applied to my kvm-ppc-next branch. Paul. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 4/7] KVM: PPC: clean up redundant 'kvm_run' parameters
On Mon, Apr 27, 2020 at 12:35:11PM +0800, Tianjia Zhang wrote: > In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' > structure. For historical reasons, many kvm-related function parameters > retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This > patch does a unified cleanup of these remaining redundant parameters. > > Signed-off-by: Tianjia Zhang This looks OK, though possibly a little larger than it needs to be because of variable name changes (kvm_run -> run) that aren't strictly necessary. Reviewed-by: Paul Mackerras ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 5/7] KVM: PPC: clean up redundant kvm_run parameters in assembly
On Mon, Apr 27, 2020 at 12:35:12PM +0800, Tianjia Zhang wrote: > In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' > structure. For historical reasons, many kvm-related function parameters > retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This > patch does a unified cleanup of these remaining redundant parameters. Some of these changes don't look completely correct to me, see below. If you're expecting these patches to go through my tree, I can fix up the patch and commit it (with you as author), noting the changes I made in the commit message. Do you want me to do that? > diff --git a/arch/powerpc/kvm/book3s_interrupts.S > b/arch/powerpc/kvm/book3s_interrupts.S > index f7ad99d972ce..0eff749d8027 100644 > --- a/arch/powerpc/kvm/book3s_interrupts.S > +++ b/arch/powerpc/kvm/book3s_interrupts.S > @@ -55,8 +55,7 @@ > > / > > /* Registers: > - * r3: kvm_run pointer > - * r4: vcpu pointer > + * r3: vcpu pointer > */ > _GLOBAL(__kvmppc_vcpu_run) > > @@ -68,8 +67,8 @@ kvm_start_entry: > /* Save host state to the stack */ > PPC_STLU r1, -SWITCH_FRAME_SIZE(r1) > > - /* Save r3 (kvm_run) and r4 (vcpu) */ > - SAVE_2GPRS(3, r1) > + /* Save r3 (vcpu) */ > + SAVE_GPR(3, r1) > > /* Save non-volatile registers (r14 - r31) */ > SAVE_NVGPRS(r1) > @@ -82,11 +81,11 @@ kvm_start_entry: > PPC_STL r0, _LINK(r1) > > /* Load non-volatile guest state from the vcpu */ > - VCPU_LOAD_NVGPRS(r4) > + VCPU_LOAD_NVGPRS(r3) > > kvm_start_lightweight: > /* Copy registers into shadow vcpu so we can access them in real mode */ > - mr r3, r4 > + mr r4, r3 This mr doesn't seem necessary. > bl FUNC(kvmppc_copy_to_svcpu) > nop > REST_GPR(4, r1) This should be loading r4 from GPR3(r1), not GPR4(r1) - which is what REST_GPR(4, r1) will do. Then, in the file but not in the patch context, there is this line: PPC_LL r3, GPR4(r1)/* vcpu pointer */ where once again GPR4 needs to be GPR3. > @@ -191,10 +190,10 @@ after_sprg3_load: > PPC_STL r31, VCPU_GPR(R31)(r7) > > /* Pass the exit number as 3rd argument to kvmppc_handle_exit */ The comment should be modified to say "2nd" instead of "3rd", otherwise it is confusing. The rest of the patch looks OK. Paul. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 3/7] KVM: PPC: Remove redundant kvm_run from vcpu_arch
On Mon, Apr 27, 2020 at 12:35:10PM +0800, Tianjia Zhang wrote: > The 'kvm_run' field already exists in the 'vcpu' structure, which > is the same structure as the 'kvm_run' in the 'vcpu_arch' and > should be deleted. > > Signed-off-by: Tianjia Zhang This looks fine. I assume each architecture sub-maintainer is taking the relevant patches from this series via their tree - is that right? Reviewed-by: Paul Mackerras ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 1/9] KVM: Pass kvm_init()'s opaque param to additional arch funcs
On Sat, Mar 21, 2020 at 01:25:55PM -0700, Sean Christopherson wrote: > Pass @opaque to kvm_arch_hardware_setup() and > kvm_arch_check_processor_compat() to allow architecture specific code to > reference @opaque without having to stash it away in a temporary global > variable. This will enable x86 to separate its vendor specific callback > ops, which are passed via @opaque, into "init" and "runtime" ops without > having to stash away the "init" ops. > > No functional change intended. > > Reviewed-by: Cornelia Huck > Tested-by: Cornelia Huck #s390 > Acked-by: Marc Zyngier > Signed-off-by: Sean Christopherson Acked-by: Paul Mackerras ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 12/45] KVM: PPC: Allocate vcpu struct in common PPC code
On Wed, Dec 18, 2019 at 01:54:57PM -0800, Sean Christopherson wrote: > Move allocation of all flavors of PPC vCPUs to common PPC code. All > variants either allocate 'struct kvm_vcpu' directly, or require that > the embedded 'struct kvm_vcpu' member be located at offset 0, i.e. > guarantee that the allocation can be directly interpreted as a 'struct > kvm_vcpu' object. > > Remove the message from the build-time assertion regarding placement of > the struct, as compatibility with the arch usercopy region is no longer > the sole dependent on 'struct kvm_vcpu' being at offset zero. > > Signed-off-by: Sean Christopherson This fails to compile for Book E configs: CC arch/powerpc/kvm/e500.o /home/paulus/kernel/kvm/arch/powerpc/kvm/e500.c: In function ‘kvmppc_core_vcpu_create_e500’: /home/paulus/kernel/kvm/arch/powerpc/kvm/e500.c:464:9: error: return makes integer from pointer without a cast [-Werror=int-conversion] return vcpu; ^ cc1: all warnings being treated as errors make[3]: *** [/home/paulus/kernel/kvm/scripts/Makefile.build:266: arch/powerpc/kvm/e500.o] Error 1 There is a "return vcpu" statement in kvmppc_core_vcpu_create_e500(), and another in kvmppc_core_vcpu_create_e500mc(), which both need to be changed to "return 0". (By the way, I do appreciate you fixing the PPC code, even if there are some errors.) Paul. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 15/45] KVM: PPC: Move kvm_vcpu_init() invocation to common code
On Wed, Dec 18, 2019 at 01:55:00PM -0800, Sean Christopherson wrote:
> Move the kvm_cpu_{un}init() calls to common PPC code as an intermediate
> step towards removing kvm_cpu_{un}init() altogether.
>
> No functional change intended.
Another error to fix:
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 047c9f707704..dd7440e50c7a 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -2114,10 +2114,9 @@ int kvmppc_core_init_vm(struct kvm *kvm)
> return kvm->arch.kvm_ops->init_vm(kvm);
> }
>
> -int kvmppc_core_vcpu_create(struct kvm *kvm, struct kvm_vcpu *vcpu,
> - unsigned int id)
> +int kvmppc_core_vcpu_create(struct kvm_vcpu *vcpu)
> {
> - return kvm->arch.kvm_ops->vcpu_create(kvm, vcpu, id);
> + return kvm->arch.kvm_ops->vcpu_create(vcpu);
This also needs s/kvm/vcpu->kvm/.
Paul.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 41/45] KVM: PPC: Move all vcpu init code into kvm_arch_vcpu_create()
On Wed, Dec 18, 2019 at 01:55:26PM -0800, Sean Christopherson wrote:
> Fold init() into create() now that the two are called back-to-back by
> common KVM code (kvm_vcpu_init() calls kvm_arch_vcpu_init() as its last
> action, and kvm_vm_ioctl_create_vcpu() calls kvm_arch_vcpu_create()
> immediately thereafter). Rinse and repeat for kvm_arch_vcpu_uninit()
> and kvm_arch_vcpu_destroy(). This paves the way for removing
> kvm_arch_vcpu_{un}init() entirely.
>
> Note, calling kvmppc_mmu_destroy() if kvmppc_core_vcpu_create() fails
> may or may not be necessary. Move it along with the more obvious call
> to kvmppc_subarch_vcpu_uninit() so as not to inadvertantly introduce a
> functional change and/or bug.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson
This doesn't compile. I get:
CC [M] arch/powerpc/kvm/powerpc.o
/home/paulus/kernel/kvm/arch/powerpc/kvm/powerpc.c: In function
‘kvm_arch_vcpu_create’:
/home/paulus/kernel/kvm/arch/powerpc/kvm/powerpc.c:733:34: error:
‘kvmppc_decrementer_wakeup’ undeclared (first use in this function)
vcpu->arch.dec_timer.function = kvmppc_decrementer_wakeup;
^
/home/paulus/kernel/kvm/arch/powerpc/kvm/powerpc.c:733:34: note: each
undeclared identifier is reported only once for each function it appears in
/home/paulus/kernel/kvm/arch/powerpc/kvm/powerpc.c: At top level:
/home/paulus/kernel/kvm/arch/powerpc/kvm/powerpc.c:794:29: warning:
‘kvmppc_decrementer_wakeup’ defined but not used [-Wunused-function]
static enum hrtimer_restart kvmppc_decrementer_wakeup(struct hrtimer *timer)
^
make[3]: *** [/home/paulus/kernel/kvm/scripts/Makefile.build:266:
arch/powerpc/kvm/powerpc.o] Error 1
The problem is that kvmppc_decrementer_wakeup() is a static function
defined in this file (arch/powerpc/kvm/powerpc.c) after
kvm_arch_vcpu_create() but before kvm_arch_vcpu_init(). You need a
forward static declaration of kvmppc_decrementer_wakeup() before
kvm_arch_vcpu_create(), or else move one or other function.
Paul.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 15/45] KVM: PPC: Move kvm_vcpu_init() invocation to common code
On Wed, Dec 18, 2019 at 01:55:00PM -0800, Sean Christopherson wrote:
> Move the kvm_cpu_{un}init() calls to common PPC code as an intermediate
> step towards removing kvm_cpu_{un}init() altogether.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson
This doesn't compile:
CC [M] arch/powerpc/kvm/book3s.o
/home/paulus/kernel/kvm/arch/powerpc/kvm/book3s.c: In function
‘kvmppc_core_vcpu_create’:
/home/paulus/kernel/kvm/arch/powerpc/kvm/book3s.c:794:9: error: ‘kvm’
undeclared (first use in this function)
return kvm->arch.kvm_ops->vcpu_create(vcpu);
^
/home/paulus/kernel/kvm/arch/powerpc/kvm/book3s.c:794:9: note: each undeclared
identifier is reported only once for each function it appears in
/home/paulus/kernel/kvm/arch/powerpc/kvm/book3s.c:795:1: warning: control
reaches end of non-void function [-Wreturn-type]
}
^
make[3]: *** [/home/paulus/kernel/kvm/scripts/Makefile.build:266:
arch/powerpc/kvm/book3s.o] Error 1
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 13385656b90d..5ad20fc0c6a1 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -789,10 +789,9 @@ void kvmppc_decrementer_func(struct kvm_vcpu *vcpu)
> kvm_vcpu_kick(vcpu);
> }
>
> -int kvmppc_core_vcpu_create(struct kvm *kvm, struct kvm_vcpu *vcpu,
> - unsigned int id)
> +int kvmppc_core_vcpu_create(struct kvm_vcpu *vcpu)
> {
> - return kvm->arch.kvm_ops->vcpu_create(kvm, vcpu, id);
> + return kvm->arch.kvm_ops->vcpu_create(vcpu);
Needs s/kvm/vcpu->kvm/ here.
You also need to change the declaration of the vcpu_create function
pointer in the kvmppc_ops struct in kvm_ppc.h to have just the vcpu
parameter instead of 3 parameters.
Paul.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 02/45] KVM: PPC: Book3S PR: Free shared page if mmu initialization fails
On Wed, Dec 18, 2019 at 01:54:47PM -0800, Sean Christopherson wrote:
> Explicitly free the shared page if kvmppc_mmu_init() fails during
> kvmppc_core_vcpu_create(), as the page is freed only in
> kvmppc_core_vcpu_free(), which is not reached via kvm_vcpu_uninit().
>
> Fixes: 96bc451a15329 ("KVM: PPC: Introduce shared page")
> Cc: sta...@vger.kernel.org
> Reviewed-by: Greg Kurz
> Signed-off-by: Sean Christopherson
> ---
> arch/powerpc/kvm/book3s_pr.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index ce4fcf76e53e..26ca62b6d773 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -1806,10 +1806,12 @@ static struct kvm_vcpu
> *kvmppc_core_vcpu_create_pr(struct kvm *kvm,
>
> err = kvmppc_mmu_init(vcpu);
> if (err < 0)
> - goto uninit_vcpu;
> + goto free_shared_page;
>
> return vcpu;
>
> +free_shared_page:
> + free_page((unsigned long)vcpu->arch.shared);
> uninit_vcpu:
> kvm_vcpu_uninit(vcpu);
> free_shadow_vcpu:
> --
> 2.24.1
Looks correct.
Acked-by: Paul Mackerras
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 01/45] KVM: PPC: Book3S HV: Uninit vCPU if vcore creation fails
On Wed, Dec 18, 2019 at 01:54:46PM -0800, Sean Christopherson wrote:
> Call kvm_vcpu_uninit() if vcore creation fails to avoid leaking any
> resources allocated by kvm_vcpu_init(), i.e. the vcpu->run page.
>
> Fixes: 371fefd6f2dc4 ("KVM: PPC: Allow book3s_hv guests to use SMT processor
> modes")
> Cc: sta...@vger.kernel.org
> Reviewed-by: Greg Kurz
> Signed-off-by: Sean Christopherson
> ---
> arch/powerpc/kvm/book3s_hv.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index dc53578193ee..d07d2f5273e5 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -2368,7 +2368,7 @@ static struct kvm_vcpu
> *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
> mutex_unlock(>lock);
>
> if (!vcore)
> - goto free_vcpu;
> + goto uninit_vcpu;
>
> spin_lock(>lock);
> ++vcore->num_threads;
> @@ -2385,6 +2385,8 @@ static struct kvm_vcpu
> *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
>
> return vcpu;
>
> +uninit_vcpu:
> + kvm_vcpu_uninit(vcpu);
> free_vcpu:
> kmem_cache_free(kvm_vcpu_cache, vcpu);
> out:
> --
> 2.24.1
Looks correct.
Acked-by: Paul Mackerras
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 10/13] KVM: Provide common implementation for generic dirty log functions
On Wed, Sep 11, 2019 at 11:50:35AM -0700, Sean Christopherson wrote: > Move the implementations of KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG > for CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT into common KVM code. > The arch specific implemenations are extremely similar, differing > only in whether the dirty log needs to be sync'd from hardware (x86) > and how the TLBs are flushed. Add new arch hooks to handle sync > and TLB flush; the sync will also be used for non-generic dirty log > support in a future patch (s390). > > The ulterior motive for providing a common implementation is to > eliminate the dependency between arch and common code with respect to > the memslot referenced by the dirty log, i.e. to make it obvious in the > code that the validity of the memslot is guaranteed, as a future patch > will rework memslot handling such that id_to_memslot() can return NULL. I notice you add empty definitions of kvm_arch_sync_dirty_log() for PPC, both Book E and Book 3S. Given that PPC doesn't select CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT, why is this necessary? Paul. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 02/13] KVM: PPC: Move memslot memory allocation into prepare_memory_region()
On Wed, Sep 11, 2019 at 11:50:27AM -0700, Sean Christopherson wrote: > Allocate the rmap array during kvm_arch_prepare_memory_region() to pave > the way for removing kvm_arch_create_memslot() altogether. Moving PPC's > memory allocation only changes the order of kernel memory allocations > between PPC and common KVM code. > > No functional change intended. > > Signed-off-by: Sean Christopherson Seems OK. Acked-by: Paul Mackerras
Re: [Resend PATCH V5 7/10] KVM: Make kvm_set_spte_hva() return int
On Thu, Dec 06, 2018 at 09:21:10PM +0800, lantianyu1...@gmail.com wrote: > From: Lan Tianyu > > The patch is to make kvm_set_spte_hva() return int and caller can > check return value to determine flush tlb or not. It would be helpful if the patch description told the reader which return value(s) mean that the caller should flush the tlb. I would guess that non-zero means to do the flush, but you should make that explicit. > Signed-off-by: Lan Tianyu For the powerpc bits: Acked-by: Paul Mackerras ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
