| Message ID | 20170929091110.20313-1-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,qemu] spapr: Receive device tree blob from SLOF | expand |
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 <aik@ozlabs.ru> > --- > 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)
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 <aik@ozlabs.ru> >> --- >> 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) >
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 <aik@ozlabs.ru> > >> --- > >> 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.
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)); + cb = be32_to_cpu(hdr.totalsize); + dtb = g_malloc0(cb); + cpu_physical_memory_read(dt, dtb, cb); + + 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)
This is a debug patch for those who want to test: "[PATCH slof] fdt: Pass the resulting device tree to QEMU" Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- include/hw/ppc/spapr.h | 3 ++- hw/ppc/spapr_hcall.c | 25 +++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-)