From: Igor Mammedov <imammedo@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>,
"Michael S. Tsirkin" <mst@redhat.com>,
Richard Henderson <richard.henderson@linaro.org>,
qemu-devel@nongnu.org,
Alistair Francis <alistair.francis@xilinx.com>,
Shannon Zhao <shannon.zhaosl@gmail.com>,
qemu-arm@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v1] acpi: increase maximum size for "etc/table-loader" blob
Date: Tue, 2 Mar 2021 16:03:44 +0100 [thread overview]
Message-ID: <20210302160344.10d5afe2@MiWiFi-RA69-srv> (raw)
In-Reply-To: <86c59853-b605-2037-5f27-5ad85fdff08c@redhat.com>
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>
next prev parent reply other threads:[~2021-03-02 15:22 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-01 10:48 [PATCH v1] acpi: increase maximum size for "etc/table-loader" blob David Hildenbrand
2021-03-02 9:06 ` Igor Mammedov
2021-03-02 9:43 ` David Hildenbrand
2021-03-02 10:07 ` Michael S. Tsirkin
2021-03-02 10:32 ` David Hildenbrand
2021-03-02 15:03 ` Igor Mammedov [this message]
2021-03-02 16:23 ` Igor Mammedov
2021-03-02 16:33 ` David Hildenbrand
2021-03-02 17:53 ` Laszlo Ersek
2021-03-02 18:43 ` David Hildenbrand
2021-03-03 9:43 ` Michael S. Tsirkin
2021-03-03 9:49 ` David Hildenbrand
2021-03-03 9:49 ` David Hildenbrand
2021-03-03 15:26 ` Igor Mammedov
2021-03-03 16:15 ` Michael S. Tsirkin
2021-03-03 15:03 ` Laszlo Ersek
2021-03-03 16:09 ` Igor Mammedov
2021-03-03 16:21 ` Michael S. Tsirkin
2021-03-03 16:46 ` Michael S. Tsirkin
2021-03-04 9:47 ` David Hildenbrand
2021-03-04 8:15 ` David Hildenbrand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210302160344.10d5afe2@MiWiFi-RA69-srv \
--to=imammedo@redhat.com \
--cc=alistair.francis@xilinx.com \
--cc=david@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=shannon.zhaosl@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).