From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39080) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTm42-0004fk-DX for qemu-devel@nongnu.org; Thu, 11 Feb 2016 02:53:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTm3x-00029g-Ic for qemu-devel@nongnu.org; Thu, 11 Feb 2016 02:53:22 -0500 References: <1455127752-17293-1-git-send-email-thuth@redhat.com> <1455127752-17293-5-git-send-email-thuth@redhat.com> <20160210234714.GH29288@voom.bne.redhat.com> From: Thomas Huth Message-ID: <56BC3DE3.3010509@redhat.com> Date: Thu, 11 Feb 2016 08:53:07 +0100 MIME-Version: 1.0 In-Reply-To: <20160210234714.GH29288@voom.bne.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="XTD2HCkBCTc8FX90bhX6Lt6P4GDqLmJvE" Subject: Re: [Qemu-devel] [PATCH 4/4] 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, laurent@vivier.eu This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --XTD2HCkBCTc8FX90bhX6Lt6P4GDqLmJvE Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11.02.2016 00:47, David Gibson wrote: > On Wed, Feb 10, 2016 at 07:09:12PM +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 when using H_ICACHE_INVALIDATE or H_ICACHE_SYNCHRONIZE, >> but this is currently only done when running with TCG - assuming >> the cache will be flushed with KVM anyway when switching back to >> kernel / VM context. >=20 > I don't think that's true. dcache and icache aren't usually flushed > by kernel/user or even process context changes in Linux. Cache > control instructions aren't priveleged so, I think to get this right > you'd need a helper which does dcbst and icbi across the page. Oh, ok, didn't know/expect that ... I'll try to come up with a solution..= =2E > I'm pretty sure libc needs to do this at several points, but alas I > don't think there's an exported function to do it. There seems to be at least a __builtin___clear_cache() function for flushing the instruction cache (see ). I haven't found anything similar for the data cache yet ... in the worst case, I guess I need to write a wrapper with some inline assembly, similar to kvmppc_eieio() in kvm_ppc.h ... would that be acceptable? >> The code currently also does not explicitely flush the data cache >> with H_ICACHE_SYNCHRONIZE, since this either also should be done >> when switching back to kernel / VM context (with KVM), or not matter >> anyway (for TCG). >> >> Signed-off-by: Thomas Huth >> --- >> hw/ppc/spapr_hcall.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 42 insertions(+) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index f14f849..91e703d 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -387,6 +387,47 @@ 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]; >> + target_ulong dest =3D args[1]; >> + target_ulong src =3D args[2]; >> + uint8_t buf[TARGET_PAGE_SIZE]; >> + >> + 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); >=20 > This should return H_PARAMETER as well as logging, surely? Yes, that's likely better. >> + } >> + >> + if (!is_ram_address(spapr, dest) || (dest & ~TARGET_PAGE_MASK) !=3D= 0) { >> + return H_PARAMETER; >> + } >> + >> + if (flags & H_COPY_PAGE) { >> + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) = !=3D 0) { >> + return H_PARAMETER; >> + } >> + cpu_physical_memory_read(src, buf, TARGET_PAGE_SIZE); >> + } else if (flags & H_ZERO_PAGE) { >> + memset(buf, 0, TARGET_PAGE_SIZE); >> + } >> + >> + if (flags & (H_COPY_PAGE | H_ZERO_PAGE)) { >> + cpu_physical_memory_write(dest, buf, TARGET_PAGE_SIZE); >> + } >=20 > Hmm, so this does 2 copies for an H_COPY_PAGE and a zero and a copy > for H_ZERO_PAGE, which is going to be substantially slower than the > caller might expect. I guess I'd better use cpu_physical_memory_map and memset/memcpy. I likely need cpu_physical_memory_map now anyway to be able to flush the caches related to that page... >> + if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) { >> + if (tcg_enabled()) { >> + tb_flush(CPU(cpu)); >> + } >> + /* XXX: Flush data cache for H_ICACHE_SYNCHRONIZE? */ >> + } >> + >> + return H_SUCCESS; >> +} Thanks for the review! Thomas --XTD2HCkBCTc8FX90bhX6Lt6P4GDqLmJvE 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) iQIcBAEBAgAGBQJWvD3oAAoJEC7Z13T+cC219u0QAJ1M2ZAdg9l2qyifiab8lIrB 7ZZtJTlQcTylPCOrS/brTb8I4ejW7dREUXD90V3CWOj6LrT6TMuRAydU/PNfYkvW ZKywanduPYqUH5LROXPShWpgP9LVZkEFFS+Ua1JbJnj0T/XmYspVOE6zakYBraeR Pchfv+V76MYML5OsG1dQNp2u/905jOlYXj5On1gwvqIzr+6HanS668SynrJi7DEb eaB7MysJmB38AwwuL548rYYUlcJRD0guSkvj5SptSOSxmeMse+luGgtQRMhbhffM TRGYbAj2pCaHmdidqV5g4RTzWTFMAEXH+AOGZnE37n6zsuDjUaw4YYVpgJneb6ep AXqreo707vQ2IvYMqud2Fw5rXqiUdUCKpeQ9Y4ZKBt6c2MVcTs1hTnkJX98URt1z 7VchYVYV6Nu9CCWteUzHVSmte1579LuW+GrKxW+qAYifKYfMAzaZYFE6r9mrVJNL hnv7sdbamjvpVBZALmC/Z7o6ZidX9Ah/qzELrqAJaaE95lggOUUjvjSoYx7ubw4y fQPDAIXGqKi7BtDeq2+qCBEMMy5yZr0xMgL3ZLML4WtOOD8Cjz65ZVMNC89eprZk IBlVtTHjGjqhiWUvffzU1oEtIKDXsc8GMEE+XWX6be2Q/uSDA3KbGoBQY7DFglPz 5/vRheidLLU4yAwMLgxd =qeXs -----END PGP SIGNATURE----- --XTD2HCkBCTc8FX90bhX6Lt6P4GDqLmJvE--