Re: [Qemu-devel] [PULL 19/21] tests: Add unit tests for the VM Generation ID feature

[Michael S. Tsirkin]

On Thu, Jul 13, 2017 at 06:34:33AM -0700, Ben Warren wrote:
> Hi,
> 
>     On Jul 13, 2017, at 4:51 AM, Marc-André Lureau <address@hidden>
>     wrote:
> 
>     Hi
> 
>     On Thu, Jul 13, 2017 at 1:32 PM Laszlo Ersek <address@hidden> wrote:
> 
>         On 07/13/17 12:47, Peter Maydell wrote:
>         > On 12 July 2017 at 00:43, Ben Warren <address@hidden> wrote:
>         >> Yes, it’s definitely a setup time problem.  With the values that 
> are
>         checked
>         >> in, I can’t get it to fail on my setup, but if I wind the numbers
>         down I see
>         >> the same failure as Peter.  So now we have the ages-old problem of
>         “what new
>         >> arbitrary value should I use that will not cause false failures but
>         will
>         >> eventually time out”.
>         >
>         > Empirically, we already have an answer to this, in the form
>         > of the existing code in tests/boot-sector.c, which is used
>         > by both that test and the bios-tables-test.c code to wait
>         > for the BIOS initialization to complete, and which doesn't
>         > cause false test timeouts in practice.
>         >
>         > Can we make this test just use that existing function to
>         > wait for the BIOS to be done, rather than having its own
>         > timeout loop?
> 
>         This is incredibly cool. Now that I've looked at "tests/boot-sector.c"
>         (again), I recall having seen it earlier, but I couldn't have
>         remembered
>         it off-hand.
> 
>         Perhaps this boot sector code should be factored out and moved to
>         "tests/acpi-utils".
> 
>         Marc-André, do you think it would be feasible for the vmcoreinfo unit
>         test as well? (Which is derived from the vmgenid unit test.)
> 
> 
>     It seems likely.
> 
>     Ben, are you going to work on the fix?
> 
> 
> Yes, I will take care of this.  I remember seeing the boot-sector
> synchronization code before, but didn’t really grok it.  This time I’ll dig
> deeper.

It's already a library, I don't think you need changes or refactoring.
You can just link boot_sector.c and call boot_sector_test like the pxe
test does. If this passes you know all acpi tables are in memory.

I'll post a patch.

There's no clever logic around timeout there though - right now your
timeout is 10 seconds while boot_sector_test has a timeout of 90
seconds.

So it is still worth improving to check the per-thread clock imho,
but I agree it's best to rework vm gen id test.
While at it, I think it's best to drop the assumption that vmgenid
is the 1st table in the ssdt.

>     Thanks
>     -- 
>     Marc-André Lureau
> 
> 
> —Ben
>