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

[Alexey Kardashevskiy]

On 03/10/17 17:40, David Gibson wrote:
> On Tue, Oct 03, 2017 at 04:17:01PM +1100, Alexey Kardashevskiy wrote:
>> SLOF receives a device tree and updates it with various properties
>> before switching to the guest kernel and QEMU is not aware of any changes
>> made by SLOF. Since there is no real RTAS and QEMU implements it,
>> it makes sense to pass the SLOF device tree to QEMU so the latter could
>> implement RTAS related tasks better.
>>
>> Specifially, now QEMU can find out the actual XICS phandle (for PHB
>> hotplug) and the RTAS linux,rtas-entry/base properties (for firmware
>> assisted NMI - FWNMI).
>>
>> This stores the initial DT blob in the sPAPR machine and replaces it
>> in the KVMPPC_H_UPDATE_DT (new private hypercall) handler.
>>
>> This implements a very basic validity check of the new blob - magic and
>> size are checked; the new blob size should not increase more than twice.
>>
>> This requires SLOF update: "fdt: Pass the resulting device tree to QEMU".
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
>>
>> I could store just a size of the QEMU's blob, or a tree, not sure
>> which one makes more sense here.
>>
>> This allows up to 2 times blob increase. Not 1.5 just to avoid
>> float/double, just looks a bit ugly imho.
>> ---
>>  include/hw/ppc/spapr.h |  4 +++-
>>  hw/ppc/spapr.c         |  4 +++-
>>  hw/ppc/spapr_hcall.c   | 33 +++++++++++++++++++++++++++++++++
>>  hw/ppc/trace-events    |  2 ++
>>  4 files changed, 41 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index a805b817a5..09f3a54dc2 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -92,6 +92,7 @@ struct sPAPRMachineState {
>>      int vrma_adjust;
>>      ssize_t rtas_size;
>>      void *rtas_blob;
>> +    void *fdt_blob;
>>      long kernel_size;
>>      bool kernel_le;
>>      uint32_t initrd_base;
>> @@ -400,7 +401,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 + 0x3)
>> +#define KVMPPC_HCALL_MAX        KVMPPC_H_UPDATE_DT
>>  
>>  typedef struct sPAPRDeviceTreeUpdateHeader {
>>      uint32_t version_id;
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 17ea77618c..b471f7e1ff 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1453,7 +1453,9 @@ static void ppc_spapr_reset(void)
>>      /* Load the fdt */
>>      qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt));
>>      cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
>> -    g_free(fdt);
>> +    g_free(spapr->fdt_blob);
>> +    spapr->fdt_blob = fdt;
>> +    spapr->fdt_size = fdt_totalsize(fdt);
>>  
>>      /* Set up the entry state */
>>      first_ppc_cpu = POWERPC_CPU(first_cpu);
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 57bb411394..a11831d3b2 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1635,6 +1635,37 @@ 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 = { 0 };
>> +    unsigned cb, magic, old_cb = fdt_totalsize(spapr->fdt_blob);
>> +
>> +    cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
>> +    cb = fdt32_to_cpu(hdr.totalsize);
>> +    magic = fdt32_to_cpu(hdr.magic);
>> +    if (magic != FDT_MAGIC || cb / old_cb > 2) {
> 
> Uh.. no.  This prevents the guest from gobbling arbitrary amounts of
> qemu RAM _in one go_, but it can still call h_update_dt arbitrarily
> often, doubling the amount of memory consumed each time.
> 
> You need to compare the updated DT size with the _original_ DT size,
> not just the last DT size.


Ah, right. Does it still make sense to store the original DT in the machine?

> 
>> +        trace_spapr_update_dt_failed(old_cb, cb, magic);
>> +        return H_PARAMETER;
>> +    }
> 
> Still needs more sanity checks here.  At least check version and that
> each of the sub-blocks fits correctly within totalsize.
> 
> Maybe I should add an fdt_fsck() function to libfdt, hmm...


ookay, I just assumed libfdt checks for subblock sizes.





-- 
Alexey

signature.asc
Description: OpenPGP digital signature