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 <[email protected]> >> --- >> >> 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
