Re: [PATCH v1] acpi: increase maximum size for "etc/table-loader" blob

[Igor Mammedov]

On Tue, 2 Mar 2021 11:32:09 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 02.03.21 11:07, Michael S. Tsirkin wrote:
> > On Tue, Mar 02, 2021 at 10:43:55AM +0100, David Hildenbrand wrote:  
> >> On 02.03.21 10:06, Igor Mammedov wrote:  
> >>> On Mon,  1 Mar 2021 11:48:33 +0100
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>  
> >>>> The resizeable memory region that is created for the cmd blob has a 
> >>>> maximum
> >>>> size of ACPI_BUILD_ALIGN_SIZE - 4k. This used to be sufficient, however,
> >>>> as we try fitting in additional data (e.g., vmgenid, nvdimm, 
> >>>> intel-iommu),
> >>>> we require more than 4k and can crash QEMU when trying to resize the
> >>>> resizeable memory region beyond its maximum size:
> >>>>     $ build/qemu-system-x86_64 --enable-kvm \
> >>>>         -machine q35,nvdimm=on \
> >>>>         -smp 1 \
> >>>>         -cpu host \
> >>>>         -m size=2G,slots=8,maxmem=4G \
> >>>>         -object 
> >>>> memory-backend-file,id=mem0,mem-path=/tmp/nvdimm,size=256M \
> >>>>         -device nvdimm,label-size=131072,memdev=mem0,id=nvdimm0,slot=1 \
> >>>>         -nodefaults \
> >>>>         -device vmgenid \
> >>>>         -device intel-iommu  
> >>>
> >>> I don't see what's here that would make cmd_blob go above 4k.
> >>> can you try identify what actually fills it up (perhaps we have a hidden 
> >>> bug elsewhere)?  
> >>
> >> VM initialization:
> >>
> >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables'  
> >>   -> new table size: 128  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 64 - 9659  
> >>   -> new table size: 256  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 384  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 512  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 640  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 9659 - 9903  
> >>   -> new table size: 768  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 9903 - 10023  
> >>   -> new table size: 896  
> >> bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid'  
> >>   -> new table size: 1024  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/vmgenid_guid'  
> >>   -> new table size: 1280  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10023 - 10225  
> >>   -> new table size: 1408  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10225 - 10281  
> >>   -> new table size: 1536  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10281 - 10505  
> >>   -> new table size: 1664  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10505 - 10577  
> >>   -> new table size: 1792  
> >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem'  
> >>   -> new table size: 1920  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/nvdimm-mem'  
> >>   -> new table size: 2048  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10577 - 11471  
> >>   -> new table size: 2176  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 11471 - 11695  
> >>   -> new table size: 2304  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 11695 - 11735  
> >>   -> new table size: 2432  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 2560  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 2688  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 2816  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 2944  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3072  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3200  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3328  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3456  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3584  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 11735 - 11807  
> >>   -> new table size: 3712  
> >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp'  
> >>   -> new table size: 3840  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3968  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for 
> >> range 0 - 20  
> >>   -> new table size: 4096  
> >>
> >>
> >> When the bios/guest boots up:
> >>
> >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/tables'  
> >>   -> new table size: 128  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 64 - 9769  
> >>   -> new table size: 256  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 384  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 512  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 640  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 9769 - 10013  
> >>   -> new table size: 768  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10013 - 10133  
> >>   -> new table size: 896  
> >> bios_linker_loader_alloc: allocating memory for 'etc/vmgenid_guid'  
> >>   -> new table size: 1024  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/vmgenid_guid'  
> >>   -> new table size: 1280  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10133 - 10335  
> >>   -> new table size: 1408  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10335 - 10391  
> >>   -> new table size: 1536  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10391 - 10615  
> >>   -> new table size: 1664  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10615 - 10675  
> >>   -> new table size: 1792  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10675 - 10747  
> >>   -> new table size: 1920  
> >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/nvdimm-mem'  
> >>   -> new table size: 2048  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/nvdimm-mem'  
> >>   -> new table size: 2176  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 10747 - 11641  
> >>   -> new table size: 2304  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 11641 - 11865  
> >>   -> new table size: 2432  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 11865 - 11905  
> >>   -> new table size: 2560  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 2688  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 2816  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 2944  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3072  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3200  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3328  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3456  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3584  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3712  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 3840  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' for 
> >> range 11905 - 11981  
> >>   -> new table size: 3968  
> >> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp'  
> >>   -> new table size: 4096  
> >> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 
> >> 'etc/acpi/tables'  
> >>   -> new table size: 4224  
> >> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for 
> >> range 0 - 20  
> >>   -> new table size: 4352  
> > 
> > yea it's because each command has space for 2 file names, so it's size
> > is 128 bytes. So just 32 commands is 4k.
> > 
> > Question is what is the extra table and why isn't it there before boot?
> >   
> > -> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp'
> > +> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/tables' to 
> > 'etc/acpi/tables'  
> >   >  -> new table size: 3840  
> > -> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 
> > 'etc/acpi/tables'
> > +> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/tables' 
> > for range 11905 - 11981  
> >   >  -> new table size: 3968  
> > -> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for 
> > range 0 - 20
> > +> bios_linker_loader_alloc: allocating memory for 'etc/acpi/rsdp'  
> >   >  -> new table size: 4096  
> > +> bios_linker_loader_add_pointer: adding pointer in 'etc/acpi/rsdp' to 
> > 'etc/acpi/tables'  
> > +>  -> new table size: 4224  
> > +> bios_linker_loader_add_checksum: Adding checksum for 'etc/acpi/rsdp' for 
> > range 0 - 20  
> > +>  -> new table size: 4352  
> > 
> > simply put I would expect the command blob size to be the same before
> > and after migration.  
> 
> Seems to be the "MCFG" table. I only see that pop up when the guest boots.
> 
> It depends on acpi_get_mcfg(). When the VM is created, it is not mapped 
> (mcfg->base == PCIE_BASE_ADDR_UNMAPPED), thus we don't create the table. 
> Once the bios configured/mapped it (wild guess), we create the table.
> 
> Anyhow, we seem to end up > 4k in the end.

Patch looks good to me,
maybe add to commit message current number of entries limit and how we reach 
over it
in reproducer above, plus nasty effect of tables that appears at runtime (like 
MCFG).

With that
Reviewed-by: Igor Mammedov <imammedo@redhat.com>