From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42154) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpXeA-0005YN-M7 for qemu-devel@nongnu.org; Tue, 05 May 2015 03:52:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YpXe7-0005CY-LF for qemu-devel@nongnu.org; Tue, 05 May 2015 03:52:06 -0400 Date: Tue, 5 May 2015 17:49:47 +1000 From: David Gibson Message-ID: <20150505074947.GV14090@voom.redhat.com> References: <1430787601-3320-1-git-send-email-david@gibson.dropbear.id.au> <20150505084236.6bc3a900@thh440s> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="HzaOE8X7KzPzAQEl" Content-Disposition: inline In-Reply-To: <20150505084236.6bc3a900@thh440s> Subject: Re: [Qemu-devel] [PATCH] pseries: Enable in-kernel H_LOGICAL_CI_{LOAD, STORE} implementations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: lvivier@redhat.com, qemu-ppc@nongnu.org, agraf@suse.de, stefanha@redhat.com, qemu-devel@nongnu.org --HzaOE8X7KzPzAQEl Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, May 05, 2015 at 08:42:36AM +0200, Thomas Huth wrote: > On Tue, 5 May 2015 11:00:01 +1000 > David Gibson wrote: >=20 > > qemu currently implements the hypercalls H_LOGICAL_CI_LOAD and > > H_LOGICAL_CI_STORE as PAPR extensions. These are used by the SLOF firm= ware > > for IO, because performing cache inhibited MMIO accesses with the MMU o= ff > > (real mode) is very awkward on POWER. > >=20 > > This approach breaks when SLOF needs to access IO devices implemented > > within KVM instead of in qemu. The simplest example would be virtio-blk > > using an iothread, because the iothread / dataplane mechanism relies on > > an in-kernel implementation of the virtio queue notification MMIO. > >=20 > > To fix this, an in-kernel implementation of these hypercalls has been m= ade, > > (kernel commit 99342cf "kvmppc: Implement H_LOGICAL_CI_{LOAD,STORE} in = KVM" > > however, the hypercalls still need to be enabled from qemu. This perfo= rms > > the necessary calls to do so. > >=20 > > It would be nice to provide some warning if we encounter a problematic > > device with a kernel which doesn't support the new calls. Unfortunatel= y, > > I can't see a way to detect this case which won't either warn in far too > > many cases that will probably work, or which is horribly invasive. >=20 > Hmm, is there a function that returns you a list to a given type? > Something like object_resolve_path(TYPE_VIRTIO_BLK, NULL) but not only > returning one matching object but all? ... then you could step through > all the possibly affected devices and check whether they need this > kernel feature. I'm not sure if there's such a function, but even with it, I can't see a way to do this that isn't really fragile. virtio-blk is only affected if using iothread / dataplane - not if fully handled in qemu. There may be other devices which will be affected if using dataplane for the same reasons, and if there aren't now there may well be in future. Likewise any future devices which could have a KVM implementation would be affected, and there's no obvious way to enumerate what those are. >=20 > > Signed-off-by: David Gibson > > --- > > hw/ppc/spapr.c | 5 +++++ > > target-ppc/kvm.c | 18 ++++++++++++++++++ > > target-ppc/kvm_ppc.h | 5 +++++ > > 3 files changed, 28 insertions(+) > >=20 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 644689a..3b5768b 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1504,6 +1504,11 @@ static void ppc_spapr_init(MachineState *machine) > > qemu_register_reset(spapr_cpu_reset, cpu); > > } > > =20 > > + if (kvm_enabled()) { > > + /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices= */ > > + kvmppc_enable_logical_ci_hcalls(); > > + } > > + > > /* allocate RAM */ > > spapr->ram_limit =3D ram_size; > > memory_region_allocate_system_memory(ram, NULL, "ppc_spapr.ram", > > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > > index 12328a4..fde26d0 100644 > > --- a/target-ppc/kvm.c > > +++ b/target-ppc/kvm.c > > @@ -1882,6 +1882,24 @@ int kvmppc_get_hypercall(CPUPPCState *env, uint8= _t *buf, int buf_len) > > return 0; > > } > > =20 > > +static inline int kvmppc_enable_hcall(KVMState *s, target_ulong hcall) > > +{ > > + return kvm_vm_enable_cap(s, KVM_CAP_PPC_ENABLE_HCALL, 0, hcall, 1); > > +} > > + > > +void kvmppc_enable_logical_ci_hcalls(void) > > +{ > > + /* > > + * FIXME: it would be nice if we could detect the cases where > > + * we're using a device which requires the in kernel > > + * implementation of these hcalls, but the kernel lacks them and > > + * produce a warning. So far I haven't seen a practical way to do > > + * that > > + */ >=20 > I'd maybe drop or shorten this comment (at least the last sentence). >=20 > > + kvmppc_enable_hcall(kvm_state, H_LOGICAL_CI_LOAD); > > + kvmppc_enable_hcall(kvm_state, H_LOGICAL_CI_STORE); > > +} > > + > > void kvmppc_set_papr(PowerPCCPU *cpu) > > { > > CPUState *cs =3D CPU(cpu); > > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > > index 2e0224c..4d30e27 100644 > > --- a/target-ppc/kvm_ppc.h > > +++ b/target-ppc/kvm_ppc.h > > @@ -24,6 +24,7 @@ bool kvmppc_get_host_serial(char **buf); > > int kvmppc_get_hasidle(CPUPPCState *env); > > int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len); > > int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level); > > +void kvmppc_enable_logical_ci_hcalls(void); > > void kvmppc_set_papr(PowerPCCPU *cpu); > > int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version); > > void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy); > > @@ -107,6 +108,10 @@ static inline int kvmppc_set_interrupt(PowerPCCPU = *cpu, int irq, int level) > > return -1; > > } > > =20 > > +static inline void kvmppc_enable_logical_ci_hcalls(void) > > +{ > > +} > > + > > static inline void kvmppc_set_papr(PowerPCCPU *cpu) > > { > > } >=20 > Reviewed-by: Thomas Huth >=20 --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --HzaOE8X7KzPzAQEl Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVSHYbAAoJEGw4ysog2bOSr3AQAI9f93lERKJMjwtzgiHB5dp2 vltkqZBszyPexWm+72LFGEZIA1ppNPrHIwOY4Jd/yLB8CtCqKKKHVDNlnu4qoRU2 H0HTB74zYw/AZMTwxavwZmLlRZt24vLv4VJu9quLkVuQ3vRrDgziqNa3LJ0qs7El W94KJbA0sWG3PwXWe3e3U2nKTUz9qnVy8HM1sbAqbRJeq6RHmA0+vreZYaAkKGqL MfUKPn/i4WfYNBaOZYMirpe7X04KDVJ1a0pVW6Ofa1ZPcl4ILVYQr1oexmD9h3qV Og1nZTJELEBudRSDdHJb4MNkt3DP/f085Ou9G887D6LSQACE1YaBuNEdDdcq0gvJ V4U2D5F0KfgxlTxUOjvQhRukYCnpYxpvWYsbG3nsXrrGEKrE3FDMntY1fJQaZYBP eSEYCdGsQib2L3J4DhN5f0m12GePkZZPh1RPPHHRaS+cSvWUlgGiU6cUbme8N7+l iuiA7piSDmYmq2JVCxtg4fheLA/w/XTOUhWb8BO0hVJyJZh1P/OHQJ058Pp9YVD2 0X6JE/+zSp3UpRTPathadPKvRTsxocic36rvU7PX77PCwGBPTSQxHkL4LQ/vkChc 7DOBpsSEuL2MFs5LiL+nLMGxO7r662iLB6/VNGcVCLYAta/mT4w+lpCBocyxMVm4 +XHVdaTxs2LJ4Yq1hoh5 =gGMX -----END PGP SIGNATURE----- --HzaOE8X7KzPzAQEl--