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

[David Gibson]

On Sat, Sep 30, 2017 at 03:48:42PM +1000, Alexey Kardashevskiy wrote:
> On 30/09/17 14:06, David Gibson wrote:
> > 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).
> 
> afaik it should be directed to unassigned memory access ops and do nothing
> silently. If I simply zero the @hdr, then it should remain empty after
> _read() and proposed sanity check will do the job.
> 
> 
> > 
> >> +    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.
> 
> Who decides? :) assert(Final_DT_size/Source_DT_size > 1.5) should do it imho.

Ultimately, I do, I guess, but I'm hoping to take some advice first
:).

I assume you meant <, not > there.  And not technically assert(), but
return an error to the guest.  Apart from that, it sounds like a
pretty reasonable idea.

> >> +    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.
> 
> btw what version should SLOF use?

v17 (0x11), with last_comp_version == 16.  It's been the current
version for a very long time now.

> Also, in my RFC patch, SLOF allocates 2 buffers, fills them up, then
> allocates a final chunk, adds a header and merges the structs/strings.
> Instead I could simply pass 3 pointers in the hypercall - the header, the
> structs, the strings - will this make sense?

No, keep it as a single pointer.  Although, as a rule, I prefer to
remove complexity from the code executed in the guest, inventing new
encodings that are sort of FDT, but in pieces is just not a good idea
I think.

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