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

[Alexey Kardashevskiy]

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.


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

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?




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


-- 
Alexey

signature.asc
Description: OpenPGP digital signature