Re: Build failed in Jenkins: FreeBSD_HEAD #220

On Friday, April 29, 2016 05:01:46 PM Jung-uk Kim wrote:
> On 04/29/16 04:02 PM, John Baldwin wrote:
> > On Friday, April 29, 2016 06:58:41 PM jenkins-admin_at_FreeBSD.org wrote:
> >> See <https://jenkins.FreeBSD.org/job/FreeBSD_HEAD/220/>
> ...
> >> Unhandled inl 0x0402 at 0xffffffff8039c2bd
> > 
> > This is a read of the PM1 enable register.  In bhyve this is treated as a 16-bit
> > register, so only inw/outw is permitted.
> > 
> > Hmm, PM1a is defined as 16-bits in the spec, so this seems to be an error in
> > the recent ACPICA import.  I note that the bitwidth calculations changed in
> > hwregs.c in the most recent import.  Perhaps those are incorrect?
> > 
> > I suspect that this might also explain the issue Kurt Lidl reported of a VM
> > shutting down after spewing errors related to FixedEvents and GPEs.
> 
> Unfortunately, there were too many recent upstream commits to this file. :-(
> 
> At least, we should take a closer look at the following commits:
> 
> https://github.com/acpica/acpica/commit/6cb97888
> https://github.com/acpica/acpica/commit/3d8583a0
> https://github.com/acpica/acpica/commit/48eea5e7
> https://github.com/acpica/acpica/commit/41f6aefa
> https://github.com/acpica/acpica/commit/c23034a3
> https://github.com/acpica/acpica/commit/c49a751b

I bet it is getting confused because PM1_STATUS, etc. are 32-bit
"psuedo" registers split across 2 16-bit registers and it's probably
using the psuedo register size somewhere.  Either that or the
hardcoded '32' bits for I/O ports when AccessWidth isn't specified.

Ah, that looks likely!

In tbfadt.c the code creates psuedo GAS entries from the FADT data:

    /*
     * Calculate separate GAS structs for the PM1x (A/B) Status and Enable
     * registers. These addresses do not appear (directly) in the FADT, so it
     * is useful to pre-calculate them from the PM1 Event Block definitions.
     *
     * The PM event blocks are split into two register blocks, first is the
     * PM Status Register block, followed immediately by the PM Enable
     * Register block. Each is of length (Pm1EventLength/2)
     *
     * Note: The PM1A event block is required by the ACPI specification.
     * However, the PM1B event block is optional and is rarely, if ever,
     * used.
     */

    for (i = 0; i < ACPI_FADT_PM_INFO_ENTRIES; i++)
    {
        Source64 = ACPI_ADD_PTR (ACPI_GENERIC_ADDRESS, &AcpiGbl_FADT,
            FadtPmInfoTable[i].Source);

        if (Source64->Address)
        {
            AcpiTbInitGenericAddress (FadtPmInfoTable[i].Target,
                Source64->SpaceId, Pm1RegisterByteWidth,
                Source64->Address +
                    (FadtPmInfoTable[i].RegisterNum * Pm1RegisterByteWidth),
                "PmRegisters", 0);
        }
    }

The helper function used always uses an AccessWidth of 0:

static void
AcpiTbInitGenericAddress (
    ACPI_GENERIC_ADDRESS    *GenericAddress,
	...
    GenericAddress->SpaceId = SpaceId;
    GenericAddress->BitWidth = BitWidth;
    GenericAddress->BitOffset = 0;
    GenericAddress->AccessWidth = 0; /* Access width ANY */
}

That AccessWidth of 0 for the PM1 blocks means that the function that
computes AccessWidth falls into the case of always using 32 bits for I/O
access.  (For memory it uses MaxBitWidth.)

You'll have to talk to the Intel guy who broke this to find out how he'd
like to fix it (not hardcode 32, or fix the AccessWidth).

Apparently they didn't bother to actually test these changes on any
real machines with PM1 blocks though. :(

(And/or the hardware they tested against isn't strict and permits 32-bit
accesses to these 16-bit registers.)

-- 
John Baldwin