| Message ID | 1455727542-1448-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <thuth@redhat.com> Ugh, sorry to nitpick, but I've hit one more little issue here. > --- > 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; > } > > +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr, > + target_ulong opcode, target_ulong *args) > +{ > + target_ulong flags = args[0]; > + hwaddr dst = args[1]; > + hwaddr src = args[2]; > + hwaddr len = 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_FMT_lx "\n", > + flags); > + return H_PARAMETER; > + } > + > + if (!is_ram_address(spapr, dst) || (dst & ~TARGET_PAGE_MASK) != 0) { > + return H_PARAMETER; > + } > + > + /* Map-in source */ > + if (flags & H_COPY_PAGE) { > + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 0) { > + return H_PARAMETER; > + } > + psrc = cpu_physical_memory_map(src, &len, 0); > + if (!psrc || len != TARGET_PAGE_SIZE) { > + return H_PARAMETER; > + } > + } > + > + /* Map-in destination */ > + pdst = cpu_physical_memory_map(dst, &len, 1); > + if (!pdst || len != 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); 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: map in dest if H_COPY_PAGE map in src memcpy unmap src else if H_ZERO_PAGE memset cache sync unmap dest > + } > + > + if (kvm_enabled() && (flags & H_ICACHE_SYNCHRONIZE) != 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; > +} > + > #define FLAGS_REGISTER_VPA 0x0000200000000000ULL > #define FLAGS_REGISTER_DTL 0x0000400000000000ULL > #define FLAGS_REGISTER_SLBSHADOW 0x0000600000000000ULL > @@ -1045,6 +1108,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 > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > index aaa828c..fd64c44 100644 > --- a/target-ppc/kvm_ppc.h > +++ b/target-ppc/kvm_ppc.h > @@ -249,15 +249,47 @@ static inline int kvmppc_enable_hwrng(void) > #endif > > #ifndef CONFIG_KVM > + > #define kvmppc_eieio() do { } while (0) > -#else > + > +static inline void kvmppc_dcbst_range(PowerPCCPU *cpu, uint8_t *addr, int len) > +{ > +} > + > +static inline void kvmppc_icbi_range(PowerPCCPU *cpu, uint8_t *addr, int len) > +{ > +} > + > +#else /* CONFIG_KVM */ > + > #define kvmppc_eieio() \ > do { \ > if (kvm_enabled()) { \ > asm volatile("eieio" : : : "memory"); \ > } \ > } while (0) > -#endif > + > +/* Store data cache blocks back to memory */ > +static inline void kvmppc_dcbst_range(PowerPCCPU *cpu, uint8_t *addr, int len) > +{ > + uint8_t *p; > + > + for (p = addr; p < addr + len; p += cpu->env.dcache_line_size) { > + asm volatile("dcbst 0,%0" : : "r"(p) : "memory"); > + } > +} > + > +/* Invalidate instruction cache blocks */ > +static inline void kvmppc_icbi_range(PowerPCCPU *cpu, uint8_t *addr, int len) > +{ > + uint8_t *p; > + > + for (p = addr; p < addr + len; p += cpu->env.icache_line_size) { > + asm volatile("icbi 0,%0" : : "r"(p)); > + } > +} > + > +#endif /* CONFIG_KVM */ > > #ifndef KVM_INTERRUPT_SET > #define KVM_INTERRUPT_SET -1
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 <thuth@redhat.com> > > Ugh, sorry to nitpick, but I've hit one more little issue here. > >> --- >> 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; >> } >> >> +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr, >> + target_ulong opcode, target_ulong *args) >> +{ >> + target_ulong flags = args[0]; >> + hwaddr dst = args[1]; >> + hwaddr src = args[2]; >> + hwaddr len = 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_FMT_lx "\n", >> + flags); >> + return H_PARAMETER; >> + } >> + >> + if (!is_ram_address(spapr, dst) || (dst & ~TARGET_PAGE_MASK) != 0) { >> + return H_PARAMETER; >> + } >> + >> + /* Map-in source */ >> + if (flags & H_COPY_PAGE) { >> + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 0) { >> + return H_PARAMETER; >> + } >> + psrc = cpu_physical_memory_map(src, &len, 0); >> + if (!psrc || len != TARGET_PAGE_SIZE) { >> + return H_PARAMETER; >> + } >> + } >> + >> + /* Map-in destination */ >> + pdst = cpu_physical_memory_map(dst, &len, 1); >> + if (!pdst || len != 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); > > 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: > > 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) != 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
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 <thuth@redhat.com> > > > > Ugh, sorry to nitpick, but I've hit one more little issue here. > > > >> --- > >> 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; > >> } > >> > >> +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr, > >> + target_ulong opcode, target_ulong *args) > >> +{ > >> + target_ulong flags = args[0]; > >> + hwaddr dst = args[1]; > >> + hwaddr src = args[2]; > >> + hwaddr len = 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_FMT_lx "\n", > >> + flags); > >> + return H_PARAMETER; > >> + } > >> + > >> + if (!is_ram_address(spapr, dst) || (dst & ~TARGET_PAGE_MASK) != 0) { > >> + return H_PARAMETER; > >> + } > >> + > >> + /* Map-in source */ > >> + if (flags & H_COPY_PAGE) { > >> + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 0) { > >> + return H_PARAMETER; > >> + } > >> + psrc = cpu_physical_memory_map(src, &len, 0); > >> + if (!psrc || len != TARGET_PAGE_SIZE) { > >> + return H_PARAMETER; > >> + } > >> + } > >> + > >> + /* Map-in destination */ > >> + pdst = cpu_physical_memory_map(dst, &len, 1); > >> + if (!pdst || len != 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); > > > > 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: > > > > 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. 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.
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; } +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr, + target_ulong opcode, target_ulong *args) +{ + target_ulong flags = args[0]; + hwaddr dst = args[1]; + hwaddr src = args[2]; + hwaddr len = 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_FMT_lx "\n", + flags); + return H_PARAMETER; + } + + if (!is_ram_address(spapr, dst) || (dst & ~TARGET_PAGE_MASK) != 0) { + return H_PARAMETER; + } + + /* Map-in source */ + if (flags & H_COPY_PAGE) { + if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 0) { + return H_PARAMETER; + } + psrc = cpu_physical_memory_map(src, &len, 0); + if (!psrc || len != TARGET_PAGE_SIZE) { + return H_PARAMETER; + } + } + + /* Map-in destination */ + pdst = cpu_physical_memory_map(dst, &len, 1); + if (!pdst || len != 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); + } + + if (kvm_enabled() && (flags & H_ICACHE_SYNCHRONIZE) != 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; +} + #define FLAGS_REGISTER_VPA 0x0000200000000000ULL #define FLAGS_REGISTER_DTL 0x0000400000000000ULL #define FLAGS_REGISTER_SLBSHADOW 0x0000600000000000ULL @@ -1045,6 +1108,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 diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h index aaa828c..fd64c44 100644 --- a/target-ppc/kvm_ppc.h +++ b/target-ppc/kvm_ppc.h @@ -249,15 +249,47 @@ static inline int kvmppc_enable_hwrng(void) #endif #ifndef CONFIG_KVM + #define kvmppc_eieio() do { } while (0) -#else + +static inline void kvmppc_dcbst_range(PowerPCCPU *cpu, uint8_t *addr, int len) +{ +} + +static inline void kvmppc_icbi_range(PowerPCCPU *cpu, uint8_t *addr, int len) +{ +} + +#else /* CONFIG_KVM */ + #define kvmppc_eieio() \ do { \ if (kvm_enabled()) { \ asm volatile("eieio" : : : "memory"); \ } \ } while (0) -#endif + +/* Store data cache blocks back to memory */ +static inline void kvmppc_dcbst_range(PowerPCCPU *cpu, uint8_t *addr, int len) +{ + uint8_t *p; + + for (p = addr; p < addr + len; p += cpu->env.dcache_line_size) { + asm volatile("dcbst 0,%0" : : "r"(p) : "memory"); + } +} + +/* Invalidate instruction cache blocks */ +static inline void kvmppc_icbi_range(PowerPCCPU *cpu, uint8_t *addr, int len) +{ + uint8_t *p; + + for (p = addr; p < addr + len; p += cpu->env.icache_line_size) { + asm volatile("icbi 0,%0" : : "r"(p)); + } +} + +#endif /* CONFIG_KVM */ #ifndef KVM_INTERRUPT_SET #define KVM_INTERRUPT_SET -1
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 <thuth@redhat.com> --- 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(-)