Re: [Qemu-devel] [PATCH v3] hw/ppc/spapr: Implement the h_page_init hypercall

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4748 bytes --]

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.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]