From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59086) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWK3y-0001ns-4L for qemu-devel@nongnu.org; Thu, 18 Feb 2016 03:35:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWK3u-000697-PQ for qemu-devel@nongnu.org; Thu, 18 Feb 2016 03:35:49 -0500 References: <1455727542-1448-1-git-send-email-thuth@redhat.com> <20160218004341.GA15224@voom.fritz.box> From: Thomas Huth Message-ID: <56C58258.6020407@redhat.com> Date: Thu, 18 Feb 2016 09:35:36 +0100 MIME-Version: 1.0 In-Reply-To: <20160218004341.GA15224@voom.fritz.box> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="EbNPW8Ol5vBapTetu834vWIHSLV4Jux7R" Subject: Re: [Qemu-devel] [PATCH v3] hw/ppc/spapr: Implement the h_page_init hypercall List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --EbNPW8Ol5vBapTetu834vWIHSLV4Jux7R Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 18.02.2016 01:43, David Gibson wrote: > On Wed, Feb 17, 2016 at 05:45:42PM +0100, Thomas Huth wrote: >> This hypercall either initializes a page with zeros, or copies >> another page. >> According to LoPAPR, the i-cache of the page should also be >> flushed if using H_ICACHE_INVALIDATE or H_ICACHE_SYNCHRONIZE, >> and the d-cache should be synchronized to the RAM if the >> H_ICACHE_SYNCHRONIZE flag is used. For this, two new functions >> are introduced, kvmppc_dcbst_range() and kvmppc_icbi()_range, which >> use the corresponding assembler instructions to flush the caches >> if running with KVM on Power. If the code runs with TCG instead, >> the code only uses tb_flush(), assuming that this will be >> enough for synchronization. >> >> Signed-off-by: Thomas Huth >=20 > Ugh, sorry to nitpick, but I've hit one more little issue here. >=20 >> --- >> v3: >> - Change H_HARDWARE return value into H_PARAMETER (which should >> be the right one according to the LoPAPR spec) >> - The dcbst and icbi helpers now contain the for-loop, too >> >> PS: I'll have a look at the missing entries in the ibm,hypertas >> property later, once this got merged. >> >> hw/ppc/spapr_hcall.c | 64 +++++++++++++++++++++++++++++++++++++++++++= +++++++++ >> target-ppc/kvm_ppc.h | 36 +++++++++++++++++++++++++++-- >> 2 files changed, 98 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 6e9b6be..6343caa 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -386,6 +386,69 @@ static target_ulong h_set_xdabr(PowerPCCPU *cpu, = sPAPRMachineState *spapr, >> return H_SUCCESS; >> } >> =20 >> +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *s= papr, >> + target_ulong opcode, target_ulong *ar= gs) >> +{ >> + target_ulong flags =3D args[0]; >> + hwaddr dst =3D args[1]; >> + hwaddr src =3D args[2]; >> + hwaddr len =3D TARGET_PAGE_SIZE; >> + uint8_t *pdst, *psrc; >> + >> + if (flags & ~(H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE >> + | H_COPY_PAGE | H_ZERO_PAGE)) { >> + qemu_log_mask(LOG_UNIMP, "h_page_init: Bad flags (" TARGET_FM= T_lx "\n", >> + flags); >> + return H_PARAMETER; >> + } >> + >> + if (!is_ram_address(spapr, dst) || (dst & ~TARGET_PAGE_MASK) !=3D= 0) { >> + return H_PARAMETER; >> + } >> + >> + /* Map-in source */ >> + if (flags & H_COPY_PAGE) { >> + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) = !=3D 0) { >> + return H_PARAMETER; >> + } >> + psrc =3D cpu_physical_memory_map(src, &len, 0); >> + if (!psrc || len !=3D TARGET_PAGE_SIZE) { >> + return H_PARAMETER; >> + } >> + } >> + >> + /* Map-in destination */ >> + pdst =3D cpu_physical_memory_map(dst, &len, 1); >> + if (!pdst || len !=3D TARGET_PAGE_SIZE) { >> + if (flags & H_COPY_PAGE) { >> + cpu_physical_memory_unmap(psrc, len, 0, 0); >> + } >> + return H_PARAMETER; >> + } >> + >> + if (flags & H_ZERO_PAGE) { >> + memset(pdst, 0, len); >> + } >> + if (flags & H_COPY_PAGE) { >> + memcpy(pdst, psrc, len); >> + cpu_physical_memory_unmap(psrc, len, 0, len); >=20 > So, at least on my compiler version (Fedora 23) I get one of those > irritating "variable may be used uninitialized" warnings here for > psrc. > > The compiler is wrong, of course, but you could both prevent its > confusion and make the code a little straightforward if you remove the > multiple tests on flags. I think you should be able to do that if you > restructure as: >=20 > map in dest > if H_COPY_PAGE > map in src > memcpy > unmap src > else if H_ZERO_PAGE > memset > cache sync > unmap dest I did not get that compiler warning here, but you're right, restructuring the code also makes sense for readabilty, , so I'll change my patch accordingly. >> + } >> + >> + if (kvm_enabled() && (flags & H_ICACHE_SYNCHRONIZE) !=3D 0) { >> + kvmppc_dcbst_range(cpu, pdst, len); >> + } >> + if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) { >> + if (kvm_enabled()) { >> + kvmppc_icbi_range(cpu, pdst, len); >> + } else { >> + tb_flush(CPU(cpu)); >> + } >> + } >> + >> + cpu_physical_memory_unmap(pdst, len, 1, len); >> + return H_SUCCESS; >> +} Thomas --EbNPW8Ol5vBapTetu834vWIHSLV4Jux7R Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJWxYJdAAoJEC7Z13T+cC21y4gP+weHTZTWRPrBX9pXiKD5csHv Qh0k70bkV41y266APvVXc22hF59Ci6ISg4/q9y/CxJbDr3ifqdv8R/JsDRwY5CIb LbSY3eclzBa+wP3zIos0w3KKyGEq5wlRjAjgc/l8z5NCBCnuh+qE2cLwtHFy0Qe+ YMZqnNZ2ubg8+BO7Azn0MjdqfXvdxlDx1rge9vvn69isW/lqlXH2yd+LuF8p65yi 2ZLgsR8u/zFKYV0s2eYso40O+yA+pzlkVXI68LnwznFQEl5OQZrIitcJshcFfwRu fsfSBhQY+Jy0TKCISnViYBP2TKQXTesUCc/Ajv/Tzr1o/QpDY2zliibV/NHj4yn6 stYHLE6JPK+fCooFc7v/QMBu/WrqpWxURsv0VmRcCTDz3wlgwV9tqWpxGT4ap5ap nMh2JQqq6VOr/BXd7O04Tisrpk8WdoCGmbkKNdhpZJSlhKjxIerrC7TWBfQr7PcS 2edc+M8IeUUcuJ29obkcfFR3Dfedsex/86ajMPJ8sReJm7GVTSEBt4lTBD7V1TgV lvOjRmnn9itp53XBsPcB5fYaePZF5EMxFP7mLn+skA2Y/o2gialSZj1ljxpp64Vc YtKJ4FdH1eF1uQ/jR6S5/0M46em9D0OSOJC6+myhSSV4iM9Yv8qz17DXSu1WUfOv cyrmAgkSMPAodAPS297R =8duj -----END PGP SIGNATURE----- --EbNPW8Ol5vBapTetu834vWIHSLV4Jux7R--