Re: [Qemu-devel] [Xen-devel] [PATCH v4] Hvmloader: Modify ACPI to only supply _EJ0 methods for PCIslots that support hotplug by runtime patching

[Ross Philipson <ross.philipson@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 4086 bytes --]

On 05/12/2014 05:05 AM, Ian Campbell wrote:
> On Fri, 2014-05-09 at 13:32 -0400, Ross Philipson wrote:
>> On 05/09/2014 12:34 PM, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Ian Campbell
>>>> Sent: 09 May 2014 17:12
>>>> To: Konrad Rzeszutek Wilk
>>>> Cc: Ross Philipson; kevin@koconnor.net; Huangweidong (C); Hanweidong
>>>> (Randy); mst@redhat.com; qemu-devel@nongnu.org; xen-
>>>> devel@lists.xen.org; fabio.fantoni@m2r.biz;
>>>> johannes.krampf@googlemail.com; Gonglei (Arei); Stefano Stabellini;
>>>> Gaowei (UVP); Jan Beulich; Anthony Perard; Paul Durrant
>>>> Subject: Re: [Xen-devel] [PATCH v4] Hvmloader: Modify ACPI to only supply
>>>> _EJ0 methods for PCIslots that support hotplug by runtime patching
>>>>
>>>> On Fri, 2014-05-09 at 12:00 -0400, Konrad Rzeszutek Wilk wrote:
>>>>
>>>>> So we could just then gat the _EJ0 functionality based on values that
>>>>> are present (or not) in the SSDT ?
>>>>
>>>> AIUI the very presence of _EJ0 is what marks the device as being
>>>> ejectable (e.g. in the Windows device manager).
>>>>
>>>> It would be possible to make _EJ0 conditionally turn itself into a NOP
>>>> without resorting to an SSDT, but I don't think that solves the issue
>>>> they are trying to solve, which is that the user can even try to eject
>>>> an non-hotplug device. (grep for UAR1 in our dsdt.asl and
>>>> acpi_info->com1_present in hvmloader/acpi/build.c for an example of this
>>>> sort of conditional thing)
>>>>
>>
>> Going back to the SSDT idea. A little poking around and what not and I
>> came up with something like this that I build into an SSDT:
>>
>> DefinitionBlock ("SSDTX.aml", "SSDT", 2, "Xen", "HVM", 0)
>> {
>>       /* S00 device is defined in DSDT, this allows me to
>>        * refrence it in this SSDT
>>        */
>>       External (\_SB.PCI0.S00, DeviceObj)
>>
>>       ...
>>
>>       /* Extend the functionality of S00 */
>>       Scope ( \_SB.PCI0.S00 ) {
>>           Method(_EJ0, 1, NotSerialized)
>>           {
>>               /* Do stuffs here */
>>           }
>>       }
>> }
>
> Thanks, this looks like the sort of thing I was naively imagining would
> be possible.
>
>> So I did find some examples of this after all in my pile of ACPI
>> firmware snapshots from all our supported platforms.
>
> Thanks (none of the machines I looked at had PCI hotplug apparently). I
> was curious to know how Real Firmware Engineers(tm) dealt with this sort
> of issue.
>
> I was worried how real life OSPMs might interpret this method being in
> an SSDT instead of the DSDT. In theory it shouldn't matter, and the fact
> that real firmware does this seem to suggest that at least Windows
> treats it that way (which is a relief).

I did actually find SSDTs that were specifically adding an _EJ0 to a 
device scope for a device defined externally. I attached an example from 
a Fujitsu system I have. The PRT1 device on SAT0 is external:

External (\_SB_.PCI0.SAT0.PRT1, DeviceObj)

And _EJ0 is added to the scope.

>
>>   I think this would
>> work allowing you to just add or not add _EJ0 methods to the PCI devices
>> you want by either using different SSDTs or doing something to generate
>> or munge the SSDT at runtime (which would be simpler than messing with
>> the DSDT I think.
>
> Without filling out the body of _EJ0 (which I tried but failed to do)
> your stub compiles to 60 bytes of AML, I suppose that even having filled
> in _EJ0 in the result would be less than, say, 128 bytes.
>
> Given that there are 32 PCI slots we would be talking about a total of
> 4k of space in hvmloader to provide a precompiled SSDT for each slot,
> which can be inserted at runtime depending on each slots configuration.
>
> I wouldn't be especially surprised if the code to generate a suitable
> SSDT dynamically was a reasonable proportion of that size, so unless
> there is the possibility of needing other variants it seems like just
> generating each of them would be the say to go.
>
>>   I did not try it (actually I did but ran into other
>> problems on our platform :).
>
> ;-)
>
> Ian.
>


-- 
Ross Philipson

[-- Attachment #2: SSDT2.dsl --]
[-- Type: text/x-dsl, Size: 4883 bytes --]

/*
 * Intel ACPI Component Architecture
 * AML Disassembler version 20100528
 *
 * Disassembly of ./SSDT2.bin, Mon May 12 09:48:37 2014
 *
 *
 * Original Table Header:
 *     Signature        "SSDT"
 *     Length           0x000002C3 (707)
 *     Revision         0x01
 *     Checksum         0x2C
 *     OEM ID           "FUJ   "
 *     OEM Table ID     "FJNB210 "
 *     OEM Revision     0x01200000 (18874368)
 *     Compiler ID      "FUJ "
 *     Compiler Version 0x00000100 (256)
 */
DefinitionBlock ("./SSDT2.aml", "SSDT", 1, "FUJ   ", "FJNB210 ", 0x01200000)
{
    External (PIID, IntObj)
    External (SMIC)
    External (DID_)
    External (BCMD)
    External (SSTA)
    External (PRDE)
    External (PRDS)
    External (BAYF)
    External (DPCS, MethodObj)    // 1 Arguments
    External (D1EN, IntObj)
    External (BY1O)
    External (BY1C)
    External (\_SB_.PCI0.SAT0.PMMN)
    External (\_SB_.PCI0.SAT0.PRT1, DeviceObj)

    Name (BYIS, 0xFF)
    Scope (\_SB.PCI0.SAT0.PRT1)
    {
        Method (_STA, 0, NotSerialized)
        {
            If (LAnd (LEqual (BY1C, Zero), BY1O))
            {
                Return (0x0F)
            }
            Else
            {
                Return (0x00)
            }
        }

        Method (_EJ0, 1, NotSerialized)
        {
            BOFF ()
            Return (Zero)
        }

        Method (BON, 0, NotSerialized)
        {
            If (D1EN)
            {
                If (LEqual (BY1C, Zero))
                {
                    Store (One, BY1O)
                    Sleep (0x46)
                }
            }
        }

        Method (BOFF, 0, NotSerialized)
        {
            Store (Zero, BY1O)
            Sleep (0x46)
        }

        Method (EON, 0, NotSerialized)
        {
            \_SB.PCI0.SAT0.PRT1.BON ()
            If (LAnd (LEqual (BY1C, Zero), BY1O))
            {
                Notify (\_SB.PCI0.SAT0.PRT1, One)
            }
        }

        Method (EOFF, 0, NotSerialized)
        {
            \_SB.PCI0.SAT0.PRT1.BOFF ()
            Notify (\_SB.PCI0.SAT0.PRT1, One)
        }

        Method (EPTS, 0, NotSerialized)
        {
            If (\_SB.PCI0.SAT0.PRT1._STA ())
            {
                Store (Zero, BYIS)
            }
            Else
            {
                Store (0xFF, BYIS)
            }
        }

        Method (EWAK, 0, NotSerialized)
        {
            If (LEqual (BYIS, 0xFF))
            {
                If (LEqual (BY1C, Zero))
                {
                    If (DPCS (\_SB.PCI0.SAT0.PRT1.BOFF ())) {}
                    Else
                    {
                        If (LNot (\_SB.PCI0.SAT0.PRT1._STA ()))
                        {
                            Store (0x01, BAYF)
                            Store (0x01, PRDS)
                            Store (0x01, PRDE)
                        }
                        Else
                        {
                            Store (0x01, BAYF)
                            Store (0x01, PRDS)
                            Store (0x01, PRDE)
                        }
                    }
                }
                Else
                {
                    Noop
                }
            }
            Else
            {
                If (LNot (\_SB.PCI0.SAT0.PRT1._STA ()))
                {
                    Notify (\_SB.PCI0.SAT0.PRT1, One)
                }
                Else
                {
                    If (LEqual (SSTA, 0x04))
                    {
                        Store (0x87, BCMD)
                        Store (0x01, DID)
                        Store (Zero, SMIC)
                        Name (BFMN, Buffer (0x28) {})
                        Store (PIID, BFMN)
                        Store (Zero, Local0)
                        Store (Zero, Local1)
                        While (LAnd (LNot (Local0), LLess (Local1, 0x28)))
                        {
                            If (LEqual (DerefOf (Index (\_SB.PCI0.SAT0.PMMN, Local1)), DerefOf (Index (BFMN, 
                                Local1))))
                            {
                                Increment (Local1)
                            }
                            Else
                            {
                                Store (0x01, Local0)
                            }
                        }

                        If (Local0)
                        {
                            \_SB.PCI0.SAT0.PRT1.BOFF ()
                            Notify (\_SB.PCI0.SAT0.PRT1, One)
                        }
                    }
                }

                If (LAnd (LEqual (BY1C, Zero), LNot (\_SB.PCI0.SAT0.PRT1._STA ())))
                {
                    Store (0x01, BAYF)
                    Store (0x01, PRDS)
                    Store (0x01, PRDE)
                }
            }
        }
    }
}