[SLOF] [PATCH qemu v3] RFC: ppc/spapr: Receive and store device tree blob from SLOF

[David Gibson]

On Tue, Oct 17, 2017 at 04:55:03PM +1100, Alexey Kardashevskiy wrote:
> On 16/10/17 20:36, David Gibson wrote:
> > On Mon, Oct 16, 2017 at 04:20:04PM +1100, Alexey Kardashevskiy
> wrote:
[snip]
> >> +static const VMStateDescription vmstate_spapr_dtb = {
> >> +    .name = "spapr_dtb",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .needed = spapr_dtb_needed,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT32(fdt_initial_size, sPAPRMachineState),
> > 
> > I'm not sure you need to migrate initial size.  The destination can
> > work this out itself.. > unless we skip the reset when we have an
> > incoming migration, I'm not sure.
> 
> 
> Nah, ppc_spapr_reset() is called when incoming migration and does not do
> anything different than normal reset situation so I'll remove this.

Ok.

[snip]
> >> +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;
> >> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> >> +
> >> +    cpu_physical_memory_read(dt, &hdr, sizeof(hdr));
> >> +    cb = fdt32_to_cpu(hdr.totalsize);
> >> +
> >> +    if (fdt_check_header(&hdr) ||
> >> +        fdt32_to_cpu(hdr.boot_cpuid_phys) ||
> > 
> > A nonzero boot cpuid is potentially valid, we shouldn't fail that.
> 
> Already discovered...


Ok :)

> >> +        fdt32_to_cpu(hdr.off_dt_struct) >= cb ||
> >> +        fdt32_to_cpu(hdr.off_dt_strings) >= cb ||
> >> +        fdt32_to_cpu(hdr.off_mem_rsvmap) >= cb ||
> > 
> > Just checking the offset isn't very useful.  You need to check two
> > things for each of the blocks:
> > 	off + size >= off	; this checks there's no overflow
> > 	off + size <= totalsize	; checks the block fits in the blob
> > 
> > With the additional complication that you don't actually have a size
> > for the rsvmap - so you have to step through it to verify that.  For a
> > v16 blob you don't have one for the structure blob either, but you
> > could simply reject v16 blobs.
> 
> Ok, v17 it is.
> 
> 
> > 
> >> +        fdt32_to_cpu(hdr.totalsize) != fdt32_to_cpu(hdr.size_dt_strings) +
> >> +                fdt32_to_cpu(hdr.size_dt_struct) +
> >> +                sizeof(struct fdt_reserve_entry) +
> >> +                sizeof(hdr)
> > 
> > This check is invalid.  It's allowed to have gaps between the blocks
> > in the FDT, which could increase the size.  In some cases it's even
> > required, to meet the alignment requirements for each block.
> 
> 
> Invalid? At the moment what SLOF provides passes this check and it will,
> and we only expect these blobs to come from SLOF - no need to protect from
> who knows what.

Well, "invalid" in the sense of unnecessarily restrictive.  Sure, SLOF
satisfies it at the moment, but it's not something qemu actually
*needs*, so all this accomplishes is making qemu more fragile against
SLOF changes (i.e. evil "coupling" in 1st year Comp Sci terminology).

> > ||
> > 
> > Yeah.. this is all a bit complicated, I'm really thinking about a
> > fdt_fsck() function for libfdt.
> 
> 
> Oh. So what now? Do as below or wait for libdtc update?

So I started hacking on this.  It's a bit fiddlier to get right than I
anticipated.  How about you make a placeholder function to "test" the
tree for now, with a comment that it will be updated once the libfdt
extensions are there.

-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/slof/attachments/20171019/49d9f880/attachment.sig>