From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Mackerras Date: Sat, 14 Sep 2013 02:21:09 +0000 Subject: Re: [PATCH 04/11] KVM: PPC: Book3S HV: Add GET/SET_ONE_REG interface for LPCR Message-Id: <20130914022109.GC5872@iris.ozlabs.ibm.com> List-Id: References: <20130906031003.GA29710@iris.ozlabs.ibm.com> <20130906032107.GE29710@iris.ozlabs.ibm.com> <2614E750-19D5-4A30-A1AC-B63DDC327721@suse.de> In-Reply-To: <2614E750-19D5-4A30-A1AC-B63DDC327721@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alexander Graf Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org On Fri, Sep 13, 2013 at 01:36:25PM -0500, Alexander Graf wrote: > > On 05.09.2013, at 22:21, Paul Mackerras wrote: > > > This adds the ability for userspace to read and write the LPCR > > (Logical Partitioning Control Register) value relating to a guest > > via the GET/SET_ONE_REG interface. There is only one LPCR value > > for the guest, which can be accessed through any vcpu. Userspace > > can only modify the following fields of the LPCR value: > > > > DPFD Default prefetch depth > > ILE Interrupt little-endian > > TC Translation control (secondary HPT hash group search disable) > > > > Signed-off-by: Paul Mackerras > > There are 3 things I dislike about this patch :) > > 1) A vcpu one_reg should only change the state of the vcpu it's targeting. You want a vm wide thing. In hardware there is in fact an LPCR per hardware CPU thread, though the architecture says that on threaded processors many of the fields have to be the same on each thread, including the three fields mentioned here. > 2) If anyone gets crazy enough to implement HV emulation in PR KVM this would overlap with the guest's guest LPCR, so we need to name it differently. It's really VM configuration, not a register. Maybe ENABLE_CAP is a better fit? If we were doing HV emulation in PR KVM then there would still only be one LPCR per guest vcpu. If there was a hypervisor running as the PR guest, it would be its job to context switch the LPCR between its guests. So I don't see why there would be any need for the top-level KVM to know about the guest's guest LPCR. In that situation we would need a one_reg to get and set the guest's LPCR, which would be per vcpu, and there would be no restriction on which bits could be set by the host. It sounds to me like the best option would be to keep an LPCR per vcpu and use the one_reg interface to let the host get and set it. The actual LPCR applied when the guest runs would use some bits from the per-vcpu value plus some bits set by the KVM host code (for the host's protection). How does that sound? Alternatively I could define a new per-VM ioctl. ENABLE_CAP doesn't really help since it also is per-vcpu, not per-VM. > 3) Checkpatch fails: > > WARNING: please, no space before tabs > #59: FILE: arch/powerpc/include/asm/reg.h:295: > +#define LPCR_TC ^I0x00000200^I/* Translation control */$ Oops, will fix. Paul. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Mackerras Subject: Re: [PATCH 04/11] KVM: PPC: Book3S HV: Add GET/SET_ONE_REG interface for LPCR Date: Sat, 14 Sep 2013 12:21:09 +1000 Message-ID: <20130914022109.GC5872@iris.ozlabs.ibm.com> References: <20130906031003.GA29710@iris.ozlabs.ibm.com> <20130906032107.GE29710@iris.ozlabs.ibm.com> <2614E750-19D5-4A30-A1AC-B63DDC327721@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org To: Alexander Graf Return-path: Received: from ozlabs.org ([203.10.76.45]:60910 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756335Ab3INCVQ (ORCPT ); Fri, 13 Sep 2013 22:21:16 -0400 Content-Disposition: inline In-Reply-To: <2614E750-19D5-4A30-A1AC-B63DDC327721@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Sep 13, 2013 at 01:36:25PM -0500, Alexander Graf wrote: > > On 05.09.2013, at 22:21, Paul Mackerras wrote: > > > This adds the ability for userspace to read and write the LPCR > > (Logical Partitioning Control Register) value relating to a guest > > via the GET/SET_ONE_REG interface. There is only one LPCR value > > for the guest, which can be accessed through any vcpu. Userspace > > can only modify the following fields of the LPCR value: > > > > DPFD Default prefetch depth > > ILE Interrupt little-endian > > TC Translation control (secondary HPT hash group search disable) > > > > Signed-off-by: Paul Mackerras > > There are 3 things I dislike about this patch :) > > 1) A vcpu one_reg should only change the state of the vcpu it's targeting. You want a vm wide thing. In hardware there is in fact an LPCR per hardware CPU thread, though the architecture says that on threaded processors many of the fields have to be the same on each thread, including the three fields mentioned here. > 2) If anyone gets crazy enough to implement HV emulation in PR KVM this would overlap with the guest's guest LPCR, so we need to name it differently. It's really VM configuration, not a register. Maybe ENABLE_CAP is a better fit? If we were doing HV emulation in PR KVM then there would still only be one LPCR per guest vcpu. If there was a hypervisor running as the PR guest, it would be its job to context switch the LPCR between its guests. So I don't see why there would be any need for the top-level KVM to know about the guest's guest LPCR. In that situation we would need a one_reg to get and set the guest's LPCR, which would be per vcpu, and there would be no restriction on which bits could be set by the host. It sounds to me like the best option would be to keep an LPCR per vcpu and use the one_reg interface to let the host get and set it. The actual LPCR applied when the guest runs would use some bits from the per-vcpu value plus some bits set by the KVM host code (for the host's protection). How does that sound? Alternatively I could define a new per-VM ioctl. ENABLE_CAP doesn't really help since it also is per-vcpu, not per-VM. > 3) Checkpatch fails: > > WARNING: please, no space before tabs > #59: FILE: arch/powerpc/include/asm/reg.h:295: > +#define LPCR_TC ^I0x00000200^I/* Translation control */$ Oops, will fix. Paul.