Re: [Qemu-devel] [Bug 1297651] [NEW] KVM create a win7 guest with Qemu, it boots up fail

[Laszlo Ersek]

On 03/26/14 11:31, Michael S. Tsirkin wrote:

> On Wed, Mar 26, 2014 at 06:45:10AM -0000, Robert Hu wrote:

>> Date:   Mon Mar 17 17:05:16 2014 +0100
>>
>>     i386/acpi-build: allow more than 255 elements in CPON
>>
>>     The build_ssdt() function builds a number of AML objects that are related
>>     to CPU hotplug, and whose IDs form a contiguous sequence of APIC IDs.
>>     (APIC IDs are in fact discontiguous, but this is the traditional
>>     interface: build a contiguous sequence from zero up that covers all
>>     possible APIC IDs.) These objects are:
>>
>>     - a Processor() object for each VCPU,
>>     - a NTFY method, with one branch for each VCPU,
>>     - a CPON package with one element (hotplug status byte) for each VCPU.
>>
>>     The build_ssdt() function currently limits the *count* of processor
>>     objects, and NTFY branches, and CPON elements, in 0xFF (see the 
>> assignment
>>     to "acpi_cpus"). This allows for an inclusive APIC ID range of [0..254].
>>     This is incorrect, because the highest APIC ID that we otherwise allow a
>>     VCPU to take is 255.
>>
>>     In order to extend the maximum count to 256, and the traversed APIC ID
>>     range correspondingly to [0..255]:
>>     - the Processor() objects need no change,
>>     - the NTFY method also needs no change,
>>     - the CPON package must be updated, because it is defined with a
>>       DefPackage, and the number of elements in such a package can be at most
>>       255. We pick a DefVarPackage instead.
>>
>>     We replace the Op byte, and the encoding of the number of elements.
>>     Compare:
>>
>>     DefPackage     := PackageOp    PkgLength NumElements    
>> PackageElementList
>>     DefVarPackage  := VarPackageOp PkgLength VarNumElements 
>> PackageElementList
>>
>>     PackageOp      := 0x12
>>     VarPackageOp   := 0x13
> 
> 
> I think I know what's going on here: the specification says:
> 
> The ASL compiler can emit two different AML opcodes for a Package
> declaration, either PackageOp or VarPackageOp. For small, fixed-length
> packages, the PackageOp is used and this
> 
> opcode is compatible with ACPI 1.0. A VarPackageOp will be emitted if
> any of the following conditions are true:
> •
>  The NumElements argument is a TermArg that can only be resolved at
> runtime.
> •
>  At compile time, NumElements resolves to a constant that is larger than
> 255.
> •
>  The PackageList contains more than 255 initializer elements.
> 
> 
> So we clearly violate this rule.

I did see this passage of the spec, but it is not relevant. It is about what 
the ASL compiler does. It comes from:

19      ACPI Source Language (ASL)Reference
19.5    ASL Operator Reference
19.5.98 Package (Declare Package Object)

We do not have an ASL compiler at hand. The specification nowhere restricts 
VarPackageOp to > 255.

However, what I *did* mess up is compatibility with ACPI 1.0. Just below the 
quoted part, there's also this:

  Note: The ability to create variable-sized packages was first
        introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size
        packages with up to 255 elements.

I forgot that the header of the containing table stated the ACPI version:

    /* Copy header and patch values in the S3_ / S4_ / S5_ packages */
    ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml));

and

  DefinitionBlock ("ssdt-misc.aml",  "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1)
                                             ^^^^
                                        ComplianceRevision

So my patch generates ACPI 2.0+ contents for an 1.0 table.

> The following seems to fix the issue - still testing. Can you confirm please?

This patch only restricts the bug to a subset of cases, but it doesn't fix it.

> However the question we should ask is whether
> it's a good idea to allow hotplug ID values that might
> make guests fail to boot.
> 
> How about limiting ACPI_CPU_HOTPLUG_ID_LIMIT to 255?

I think it's not the package size / APIC ID value per se that breaks the boot, 
but the incompatibility between the ACPI revision stated in the SSDT header, 
and the construct in the table.

> 
> We never allowed > 255 in the past, is it worth the
> maintainance headaches?
> 
>  
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index f1054dd..7597517 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1055,9 +1055,21 @@ build_ssdt(GArray *table_data, GArray *linker,
>  
>          {
>              GArray *package = build_alloc_array();
> -            uint8_t op = 0x13; /* VarPackageOp */
> +            uint8_t op;
> +
> +            /*
> +             * Note: The ability to create variable-sized packages was first 
> introduced in ACPI 2.0. ACPI 1.0 only
> +             * allowed fixed-size packages with up to 255 elements.
> +             * Windows guests up to win2k8 fail when VarPackageOp is used.
> +             */
> +            if (acpi_cpus <= 255) {
> +                op = 0x12; /* PackageOp */
> +                build_append_byte(package, acpi_cpus); /* NumElements */
> +            } else {
> +                op = 0x13; /* VarPackageOp */
> +                build_append_int(package, acpi_cpus); /* VarNumElements */
> +            }
>  
> -            build_append_int(package, acpi_cpus); /* VarNumElements */
>              for (i = 0; i < acpi_cpus; i++) {
>                  uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
>                  build_append_byte(package, b);
> 

The patch will mask the problem for most of the cases, but when VarPackageOp is 
used, it will be broken just the same (because the ACPI revision in the SSDT 
header will still mismatch).

Here's my proposal:
- I can post a patch that changes the SSDT DSL files, *and* the DSDT files 
(q35-acpi-dsdt.dsl acpi-dsdt.dsl), to state an ACPI revision of 2.0. (The ACPI 
revision of the DSDT file determines integer sizes for SSDTs too, so we can't 
just bump the SSDTs to 2.0)
- Or I suggest to revert my patches for 2.0.

You probably won't like bumping the ACPI rev in DSDT/SSDT, for various 
compatibility reasons, so I think I suggest to revert these two patches of 
mine. It's now clear to me that this VCPU hotplug limit cannot be lifted 
without much more intrusive (and guest visible) changes. Sorry about missing 
this fact in my original submission.

Thanks,
Laszlo