Re: [Qemu-ppc] [RFC PATCH qemu] spapr: Receive device tree blob from SLOF

[David Gibson]

On Fri, Sep 29, 2017 at 07:11:10PM +1000, Alexey Kardashevskiy wrote:
> This is a debug patch for those who want to test:
> "[PATCH slof] fdt: Pass the resulting device tree to QEMU"

So, obviously this is fine as just a debug patch.  A few comments for
things that will be necessary when/if we do this for real.

> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
>  include/hw/ppc/spapr.h |  3 ++-
>  hw/ppc/spapr_hcall.c   | 25 +++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index a805b817a5..15e865be38 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -400,7 +400,8 @@ struct sPAPRMachineState {
>  #define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
>  /* Client Architecture support */
>  #define KVMPPC_H_CAS            (KVMPPC_HCALL_BASE + 0x2)
> -#define KVMPPC_HCALL_MAX        KVMPPC_H_CAS
> +#define KVMPPC_H_UPDATE_DT      (KVMPPC_HCALL_BASE + 0x5)
> +#define KVMPPC_HCALL_MAX        KVMPPC_H_UPDATE_DT
>  
>  typedef struct sPAPRDeviceTreeUpdateHeader {
>      uint32_t version_id;
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 57bb411394..599cbb99f7 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1635,6 +1635,29 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>      return H_SUCCESS;
>  }
>  
> +static target_ulong h_update_dt(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> +                                target_ulong opcode, target_ulong *args)
> +{
> +    target_ulong dt = ppc64_phys_to_real(args[0]);
> +    struct fdt_header hdr;
> +    void *dtb;
> +    FILE *f;
> +    uint32_t cb;
> +
> +    cpu_physical_memory_read(dt, &hdr, sizeof(hdr));

I'm pretty sure the physical mem access functions won't do anything
dangerous if given an address outside the guest's memory.  However, it
would be good to detect and report that situation (and, obviously,
ignore any partial data we pulled in).

> +    cb =  be32_to_cpu(hdr.totalsize);

We'll need to sanity check the size here, so the guest can't allocate
arbitrary amounts of memory outside its address space.  Not sure if we
should do that with a fixed limit for the DT size, or compare the size
here to the size of the dtb we passed in, or as a fraction of guest
ram size, or what.

> +    dtb = g_malloc0(cb);
> +    cpu_physical_memory_read(dt, dtb, cb);

After reading it in we'll also want some degree of sanity checking.
libfdt _should_ be safe when we read this later on, even if it's any
kind of random junk, but best to be safe.

Not immediately clear to me how thorough we should be with this.  I
think we want to at least verify that the magic number and version are
correct, and the 3 data blocks do lie within the given total size.

> +    f = fopen("dbg.dtb", "wb");
> +    fwrite(dtb, cb, 1, f);
> +    fclose(f);
> +    printf("+++Q+++ (%u) %s %u: DT at %lx (%lx) %d bytes\n", getpid(), 
> __func__, __LINE__,
> +                dt, args[0], cb);
> +
> +    return H_SUCCESS;
> +}
> +
>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - 
> KVMPPC_HCALL_BASE + 1];
>  
> @@ -1732,6 +1755,8 @@ static void hypercall_register_types(void)
>  
>      /* ibm,client-architecture-support support */
>      spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
> +
> +    spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
>  }
>  
>  type_init(hypercall_register_types)

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