Re: [PATCH v5 1/3] hw/i386/pc: Refactor logic to set SMBIOS defaults

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 7/6/23 10:11, Daniel P. Berrangé wrote:
> On Tue, Jun 06, 2023 at 09:49:37PM -0500, Suravee Suthikulpanit wrote:
>> Into a helper function pc_machine_init_smbios() in preparation for
>> subsequent code to upgrade default SMBIOS entry point type.
>>
>> Then, call the helper function from the pc_machine_initfn() to eliminate
>> duplicate code in pc_q35.c and pc_pixx.c. However, this changes the
>> ordering of when the smbios_set_defaults() is called to before
>> pc_machine_set_smbios_ep() (i.e. before handling the user specified
>> QEMU option "-M ...,smbios-entry-point-type=[32|64]" to override
>> the default type.)
>>
>> Therefore, also call the helper function in pc_machine_set_smbios_ep()
>> to update the defaults.
> 
> This is unsafe - smbios_set_defaults is only intended to be called
> once. Calling it twice leads to a SEGV due to double-free
> 
> $  ./build/qemu-system-x86_64 -machine pc,smbios-entry-point-type=64 -smbios file=/tmp/smbios_entry_point
> Segmentation fault (core dumped)

Doh, good catch.

> IMHO we should just not do this refactoring. The existing duplicated
> code is not a significant burden, and thus is better than having to
> workaround calling pc_machine_set_smbios_ep too early in startup.
> 
>>
>> There is no functional change.

I was too confident because of this line ...

>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   hw/i386/pc.c      | 24 +++++++++++++++++++++++-
>>   hw/i386/pc_piix.c |  9 ---------
>>   hw/i386/pc_q35.c  |  8 --------
>>   3 files changed, 23 insertions(+), 18 deletions(-)