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: 6614 bytes --]

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

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