Re: [PULL 06/13] pnv/mpipl: Set thread entry size to be allocated by firmware
Hi Peter, On Fri, May 08, 2026 at 10:15:10AM +0100, Peter Maydell wrote: > On Wed, 29 Apr 2026 at 19:35, Harsh Prateek Bora > wrote: > > > > From: Aditya Gupta > > > > Set the "Thread Register State Entry Size" that is required by firmware > > (OPAL), to know size of memory to allocate to capture CPU state, in the > > event of a crash > > > > Reviewed-by: Hari Bathini > > Reviewed-by: Sourabh Jain > > Signed-off-by: Aditya Gupta > > Tested-by: Shivang Upadhyay > > Link: > > https://lore.kernel.org/qemu-devel/[email protected] > > Signed-off-by: Harsh Prateek Bora > > Hi; Coverity points out an issue with this change (CID 1658041): Thanks for reporting, I'll look into this. > > > static void pnv_reset(MachineState *machine, ResetType type) > > { > > +PnvMachineState *pnv = PNV_MACHINE(machine); > > void *fdt; > > > > qemu_devices_reset(type); > > > > +if (!pnv->mpipl_state.is_next_boot_mpipl) { > > +/* > > + * Set the "Thread Register State Entry Size", so that firmware can > > + * allocate enough memory to capture CPU state in the event of a > > + * crash > > + */ > > + > > +MpiplProcDumpArea proc_area; > > Here we don't initialize the struct... > > > + > > +proc_area.version = PROC_DUMP_AREA_VERSION_P9; > > +proc_area.thread_size = > > cpu_to_be32(sizeof(MpiplPreservedCPUState)); > > + > > +/* These are to be allocated & assigned by the firmware */ > > +proc_area.alloc_addr = 0; > > +proc_area.alloc_size = 0; > > + > > +/* These get assigned after crash, when QEMU preserves the > > registers */ > > +proc_area.dest_addr = 0; > > +proc_area.act_size = 0; > > ...and here we don't fill in all the fields; we don't set > the reserved, reserved2 or reserved3 fields to anything. > This means that we will write data to the guest which is > potentially random host data from the stack. > > I think I'd suggest fixing this by initializing the struct in > one go, like this: > > MpiplProcDumpArea proc_area = { >.version = PROC_DUMP_AREA_VERSION_P9, >.thread_size = cpu_to_be32(sizeof(MpiplPreservedCPUState)), >[set alloc_addr = 0 etc here if you like, or rely on > the "fields not listed are zero-inited"] > }; > > (In fact because we use -ftrivial-auto-var-init=zero the compiler > will zero init this for us anyway, but we can make Coverity > happy anyway.) Thanks for suggesting the fix too. ~Shivang.
Re: [PULL 06/13] pnv/mpipl: Set thread entry size to be allocated by firmware
On Wed, 29 Apr 2026 at 19:35, Harsh Prateek Bora wrote: > > From: Aditya Gupta > > Set the "Thread Register State Entry Size" that is required by firmware > (OPAL), to know size of memory to allocate to capture CPU state, in the > event of a crash > > Reviewed-by: Hari Bathini > Reviewed-by: Sourabh Jain > Signed-off-by: Aditya Gupta > Tested-by: Shivang Upadhyay > Link: > https://lore.kernel.org/qemu-devel/[email protected] > Signed-off-by: Harsh Prateek Bora Hi; Coverity points out an issue with this change (CID 1658041): > static void pnv_reset(MachineState *machine, ResetType type) > { > +PnvMachineState *pnv = PNV_MACHINE(machine); > void *fdt; > > qemu_devices_reset(type); > > +if (!pnv->mpipl_state.is_next_boot_mpipl) { > +/* > + * Set the "Thread Register State Entry Size", so that firmware can > + * allocate enough memory to capture CPU state in the event of a > + * crash > + */ > + > +MpiplProcDumpArea proc_area; Here we don't initialize the struct... > + > +proc_area.version = PROC_DUMP_AREA_VERSION_P9; > +proc_area.thread_size = cpu_to_be32(sizeof(MpiplPreservedCPUState)); > + > +/* These are to be allocated & assigned by the firmware */ > +proc_area.alloc_addr = 0; > +proc_area.alloc_size = 0; > + > +/* These get assigned after crash, when QEMU preserves the registers > */ > +proc_area.dest_addr = 0; > +proc_area.act_size = 0; ...and here we don't fill in all the fields; we don't set the reserved, reserved2 or reserved3 fields to anything. This means that we will write data to the guest which is potentially random host data from the stack. I think I'd suggest fixing this by initializing the struct in one go, like this: MpiplProcDumpArea proc_area = { .version = PROC_DUMP_AREA_VERSION_P9, .thread_size = cpu_to_be32(sizeof(MpiplPreservedCPUState)), [set alloc_addr = 0 etc here if you like, or rely on the "fields not listed are zero-inited"] }; (In fact because we use -ftrivial-auto-var-init=zero the compiler will zero init this for us anyway, but we can make Coverity happy anyway.) > + > +cpu_physical_memory_write(PROC_DUMP_AREA_OFF, &proc_area, > +sizeof(proc_area)); > +} > + > fdt = machine->fdt; > cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt)); > } thanks -- PMM
