Re: [Qemu-devel] [PULL v2 00/36] pci, pc, virtio: features, fixes

[Laszlo Ersek]

On 05/22/19 15:06, Igor Mammedov wrote:
> On Tue, 21 May 2019 09:26:16 -0400
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
>> On Tue, May 21, 2019 at 12:49:48PM +0100, Peter Maydell wrote:
>>> On Tue, 21 May 2019 at 00:10, Michael S. Tsirkin <address@hidden> wrote:  
>>>>
>>>> The following changes since commit 
>>>> 2259637b95bef3116cc262459271de08e038cc66:
>>>>
>>>>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into 
>>>> staging (2019-05-20 17:22:05 +0100)
>>>>
>>>> are available in the Git repository at:
>>>>
>>>>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>>>>
>>>> for you to fetch changes up to 0c05ec64c388aea59facbef740651afa78e04f50:
>>>>
>>>>   tests: acpi: print error unable to dump ACPI table during rebuild 
>>>> (2019-05-20 18:40:02 -0400)
>>>>
>>>> ----------------------------------------------------------------
>>>> pci, pc, virtio: features, fixes
>>>>
>>>> reconnect for vhost blk
>>>> tests for UEFI
>>>> misc other stuff
>>>>
>>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>>>>
>>>> ----------------------------------------------------------------  
>>>
>>> Hi -- this failed 'make check' for 32-bit Arm hosts:
>>>
>>> ERROR:/home/peter.maydell/qemu/tests/acpi-utils.c:145:acpi_find_rsdp_address_uefi:
>>> code should not be reached
>>> Aborted
>>> ERROR - too few tests run (expected 1, got 0)
>>> /home/peter.maydell/qemu/tests/Makefile.include:885: recipe for target
>>> 'check-qtest-aarch64' failed
>>>
>>> thanks
>>> -- PMM  
>>
>> Nothing jumps out ... Igor?
> On 32-bit ARM host and it looks like UEFI crashes (CCing Laszlo) with:
> 
> InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B 469E52C0
> ASSERT [DxeCore] 
> /home/lacos/src/upstream/qemu/roms/edk2/MdePkg/Library/BaseLib/String.c(1090):
>  Length < _gPcd_FixedAtBuild_PcdMaximumAsciiStringLength
> 
> CLI to reproduce:
> 
> qemu-system-aarch64  -display none -machine virt,accel=tcg -nodefaults 
> -nographic -drive 
> if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly -drive 
> if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom 
> tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu 
> cortex-a57 -serial stdio

This is very interesting. I had obviously tested booting
"bios-tables-test.aarch64.iso.qcow2" against "edk2-aarch64-code.fd",
using TCG, on my x86_64 laptop. (And, I've run the above exact command
just now, at commit a4f667b67149 -- it works 100% fine.)

However, I've never been *near* a 32-bit ARM host. Therefore my
suspicion is that the AARCH64 UEFI guest code tickles something in the
32-bit ARM code generator. It could be a bug in 32-bit ARM TCG, or it
could be a bug in edk2 that is exposed by 32-bit ARM TCG.

The direct assertion failure is mostly useless. The AsciiStrLen()
function does what you'd expect it to, except it has a kind of "safety
check" where it trips an assertion if the string length (under
measurement) exceeds a pre-set platform constant. It helps with catching
memory corruption errors.

$ git show edk2-stable201903:MdePkg/Library/BaseLib/String.c | less
1090g

UINTN
EFIAPI
AsciiStrLen (
  IN      CONST CHAR8               *String
  )
{
  UINTN                             Length;

  ASSERT (String != NULL);

  for (Length = 0; *String != '\0'; String++, Length++) {
    //
    // If PcdMaximumUnicodeStringLength is not zero,
    // length should not more than PcdMaximumUnicodeStringLength
    //
    if (PcdGet32 (PcdMaximumAsciiStringLength) != 0) {
      ASSERT (Length < PcdGet32 (PcdMaximumAsciiStringLength)); <-- HERE
    }
  }
  return Length;
}

(Never mind that the comment has a typo -- it incorrectly references
"PcdMaximumUnicodeStringLength", but the PCD that's actually checked is
(correctly) "PcdMaximumAsciiStringLength".)

The constant is set to decimal 1,000,000 in ArmVirtQemu builds
(inherited from MdePkg.dec), and that's indeed a quite unlikely length
for real-word strings seen by firmware.

I'll take a closer look once I have access to a 32-bit ARM host, but
I'll definitely need help. Basically we should compare the original
AARCH64 (dis)assembly with the QEMU-generated 32-bit ARM assembly.
Hopefully I can at least get a backtrace myself.

Thanks,
Laszlo