| Message ID | 1455127752-17293-5-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. 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. > 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 <thuth@redhat.com> > --- > 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; > } > > +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr, > + target_ulong opcode, target_ulong *args) > +{ > + target_ulong flags = args[0]; > + target_ulong dest = args[1]; > + target_ulong src = 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_FMT_lx "\n", > + flags); This should return H_PARAMETER as well as logging, surely? > + } > + > + if (!is_ram_address(spapr, dest) || (dest & ~TARGET_PAGE_MASK) != 0) { > + return H_PARAMETER; > + } > + > + if (flags & H_COPY_PAGE) { > + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 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); > + } 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. > + 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; > +} > + > #define FLAGS_REGISTER_VPA 0x0000200000000000ULL > #define FLAGS_REGISTER_DTL 0x0000400000000000ULL > #define FLAGS_REGISTER_SLBSHADOW 0x0000600000000000ULL > @@ -1046,6 +1087,7 @@ static void hypercall_register_types(void) > spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0); > spapr_register_hypercall(H_SET_DABR, h_set_dabr); > spapr_register_hypercall(H_SET_XDABR, h_set_xdabr); > + spapr_register_hypercall(H_PAGE_INIT, h_page_init); > spapr_register_hypercall(H_SET_MODE, h_set_mode); > > /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
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. > > 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... > 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 <https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html>). 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 <thuth@redhat.com> >> --- >> 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; >> } >> >> +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr, >> + target_ulong opcode, target_ulong *args) >> +{ >> + target_ulong flags = args[0]; >> + target_ulong dest = args[1]; >> + target_ulong src = 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_FMT_lx "\n", >> + flags); > > 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) != 0) { >> + return H_PARAMETER; >> + } >> + >> + if (flags & H_COPY_PAGE) { >> + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 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); >> + } > > 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
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; } +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr, + target_ulong opcode, target_ulong *args) +{ + target_ulong flags = args[0]; + target_ulong dest = args[1]; + target_ulong src = 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_FMT_lx "\n", + flags); + } + + if (!is_ram_address(spapr, dest) || (dest & ~TARGET_PAGE_MASK) != 0) { + return H_PARAMETER; + } + + if (flags & H_COPY_PAGE) { + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 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); + } + + 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; +} + #define FLAGS_REGISTER_VPA 0x0000200000000000ULL #define FLAGS_REGISTER_DTL 0x0000400000000000ULL #define FLAGS_REGISTER_SLBSHADOW 0x0000600000000000ULL @@ -1046,6 +1087,7 @@ static void hypercall_register_types(void) spapr_register_hypercall(H_SET_SPRG0, h_set_sprg0); spapr_register_hypercall(H_SET_DABR, h_set_dabr); spapr_register_hypercall(H_SET_XDABR, h_set_xdabr); + spapr_register_hypercall(H_PAGE_INIT, h_page_init); spapr_register_hypercall(H_SET_MODE, h_set_mode); /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate
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. 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 <thuth@redhat.com> --- hw/ppc/spapr_hcall.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)