From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWKf5-0005Ux-U9 for qemu-devel@nongnu.org; Thu, 18 Feb 2016 04:14:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWKf1-0005uy-LW for qemu-devel@nongnu.org; Thu, 18 Feb 2016 04:14:11 -0500 Date: Thu, 18 Feb 2016 20:15:00 +1100 From: David Gibson Message-ID: <20160218091500.GL15224@voom.fritz.box> References: <1455727542-1448-1-git-send-email-thuth@redhat.com> <20160218004341.GA15224@voom.fritz.box> <56C58258.6020407@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="NZiXfHLGvOGtDZMn" Content-Disposition: inline In-Reply-To: <56C58258.6020407@redhat.com> 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: Thomas Huth Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org --NZiXfHLGvOGtDZMn Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 18, 2016 at 09:35:36AM +0100, Thomas Huth wrote: > 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 >=20 > 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. Thanks. The compiler warning seems to kick in both on my machine and on Travis builds, so it doesn't look like it's that rare. And with -Werror it's a real pain. --=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 --NZiXfHLGvOGtDZMn Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWxYuUAAoJEGw4ysog2bOSCJEP/1G0+tktSbVIgSxgpo2C8R83 mqNxbpS1GB7eXo6TR/70sgH0CGMtb3rA+XZBeHZcGqhH7rgkfAYQHdUzMnavSxSr nH+xQCo5LXraWaos7/MLbB3kyoKwinqyzt7Cu/4NQxMBd8u8Rzay9yxpBVtoSYI4 0Z8q8AX/xSr4kshKTfXlXTrFk+I8tluAWAQc0IeMJGS+dwAs2RMKCtI1GUmv9Jbv yUK6eUxby1A86ahfa9C5BE8swME/MEYDXBNhwYdBeW8YhQXxp5xvZQB7CJCiWXvO 8RKNyyk48Ie6PsMSQFyxkw3CvoAtkzNrfeWeErwgmWao2zGDTwjfmwED4apIzp70 1VqFPCYQ1Rgc/lqmhvW3W8BLW8z1vRyDbEAy1PYO4EcMrKKNDWydJzWNn2MBZS4q aWLHAsWHuT1XmtVU1qv3tRz7JLPuZ3KkFPtsj+ZMg3s8kF8olucIPkk/bQIE1Kus pJr+3WZstVekQIiFIEUddZw0SE//pC6qWbZAAXLXVHZ7VSiWAyPilqDIBtdlNhIl GhKM+aCSahjhN1IqT2HZdRmu2HDFWgljkm0Xkdd2Ix+EaArdRnpDGagzYE3l5dMC Wyjsa1ZF0Swj5cm8TojZ8y8OYL9vcxrX4B7VYEETpG9Nyvsl/6tYbHEQJCGz6GKg nUpxkT+BPBWx6gvZNunk =2ZkU -----END PGP SIGNATURE----- --NZiXfHLGvOGtDZMn--