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
signature.asc
Description: PGP signature
