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 <th...@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.
--
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
signature.asc
Description: PGP signature
