On May 2, 2008, at 7:35 PM, Marcelo Tosatti wrote:
> Add 3 PCI bridges to the ACPI table:
> - Move IRQ routing, slot device and GPE processing to separate files
> which can be included from acpi-dsdt.dsl.
> - Add _SUN methods to every slot device so as to avoid collisions
> in OS handling.
> - Fix copy&paste typo in slot devices 8/9 and 24/25.
>
> This table breaks PCI hotplug for older userspace, hopefully not an
> issue (trivial enough to upgrade the BIOS).
>
> Signed-off-by: Marcelo Tosatti <[EMAIL PROTECTED]>
>
> Index: kvm-userspace.pci3/bios/acpi-dsdt.dsl
> ===================================================================
> --- kvm-userspace.pci3.orig/bios/acpi-dsdt.dsl
> +++ kvm-userspace.pci3/bios/acpi-dsdt.dsl
> @@ -208,218 +208,29 @@ DefinitionBlock (
> Name (_HID, EisaId ("PNP0A03"))
> Name (_ADR, 0x00)
> Name (_UID, 1)
> - Name(_PRT, Package() {
> - /* PCI IRQ routing table, example from ACPI 2.0a
> specification,
> - section 6.2.8.1 */
> - /* Note: we provide the same info as the PCI routing
> - table of the Bochs BIOS */
> -
> - // PCI Slot 0
> - Package() {0x0000ffff, 0, LNKD, 0},
> - Package() {0x0000ffff, 1, LNKA, 0},
> - Package() {0x0000ffff, 2, LNKB, 0},
> - Package() {0x0000ffff, 3, LNKC, 0},
[ ... snip ... ]
> - // PCI Slot 31
> - Package() {0x001fffff, 0, LNKC, 0},
> - Package() {0x001fffff, 1, LNKD, 0},
> - Package() {0x001fffff, 2, LNKA, 0},
> - Package() {0x001fffff, 3, LNKB, 0},
> - })
> +
> + Include ("acpi-irq-routing.dsl")
>
> OperationRegion(PCST, SystemIO, 0xae00, 0x08)
> Field (PCST, DWordAcc, NoLock, WriteAsZeros)
> - {
> + {
> PCIU, 32,
> PCID, 32,
> - }
> -
> + }
Are these whitespace patches supposed to be here?
>
> OperationRegion(SEJ, SystemIO, 0xae08, 0x04)
> Field (SEJ, DWordAcc, NoLock, WriteAsZeros)
> {
> B0EJ, 32,
> }
>
> + Device (S0) { // Slot 0
> + Name (_ADR, 0x00000000)
> + Method (_EJ0,1) {
> + Store(0x1, B0EJ)
> + Return (0x0)
> + }
> + }
> +
I'm having trouble understanding the semantic of the Sx devices here.
What is this S0, S1 and S2 device? Maybe different names would make
everything more understandable.
>
> Device (S1) { // Slot 1
> Name (_ADR, 0x00010000)
> Method (_EJ0,1) {
> @@ -436,28 +247,70 @@ DefinitionBlock (
> }
> }
>
> - Device (S3) { // Slot 3
> + Device (S3) { // Slot 3, PCI-to-PCI bridge
This device could be called BRI1 for example. That would make reading
the DSDT a lot easier.
>
> Name (_ADR, 0x00030000)
> - Method (_EJ0,1) {
> - Store (0x8, B0EJ)
> - Return (0x0)
> + Include ("acpi-irq-routing.dsl")
> +
> + OperationRegion(PCST, SystemIO, 0xae0c, 0x08)
> + Field (PCST, DWordAcc, NoLock, WriteAsZeros)
> + {
> + PCIU, 32,
> + PCID, 32,
> }
> +
> + OperationRegion(SEJ, SystemIO, 0xae14, 0x04)
> + Field (SEJ, DWordAcc, NoLock, WriteAsZeros)
> + {
> + B1EJ, 32,
> + }
> +
> + Name (SUN1, 30)
> + Alias (\_SB.PCI0.S3.B1EJ, BEJ)
> + Include ("acpi-pci-slots.dsl")
[ ... snip ... ]
>
> Method(_L05) {
> Return(0x01)
> Index: kvm-userspace.pci3/bios/acpi-hotplug-gpe.dsl
> ===================================================================
> --- /dev/null
> +++ kvm-userspace.pci3/bios/acpi-hotplug-gpe.dsl
> @@ -0,0 +1,257 @@
> + /* Up status */
> + If (And(UP, 0x1)) {
> + Notify(S0, 0x1)
> + }
While this is proper syntax I prefer the way Fabrice wrote the tables.
Most of his entries were one-lined, even though they wouldn't end up
like that when getting decompiled. In this case I'd vote for something
like:
If (And(UP, 0x1)) { Notify(S0, 0x1) }
Which makes things easier to read again. The same goes for a lot of
code below that chunk.
>
> +
> + If (And(UP, 0x2)) {
> + Notify(S1, 0x1)
> + }
> +
[ ... snip ... ]
> Index: kvm-userspace.pci3/bios/acpi-pci-slots.dsl
> ===================================================================
> --- /dev/null
> +++ kvm-userspace.pci3/bios/acpi-pci-slots.dsl
> @@ -0,0 +1,385 @@
> + Device (S0) { // Slot 0
> + Name (_ADR, 0x00000000)
> + Method (_EJ0,1) {
Hmm ... I never assumed anything could be wrong here, but doesn't that
1 mean there is one argument to the method?
From the ACPI Specification:
Method(_EJ0, 1){ //Hot docking support
//Arg0: 0=insert, 1=eject
So we aren't using this information? What else do we use? Sorry if I
missed something.
>
> + Store(0x1, BEJ)
> + Return (0x0)
> + }
> + Method(_SUN) {
> + Add (SUN1, 0, Local0)
> + Return (Local0)
> + }
> + }
Same comment here. I don't like copy&paste code that goes over a lot
of lines. Can't you simply do some helper methods that do what _EJ0
and _SUN do in a generic manner and Return that? I'd imagine something
like:
Device (S0) { // Slot 0
Name (_ADR, 0x00000000)
Method (_EJ0,1) { Return( GEJ0(0x1) }
Method(_SUN) { Return( GSUN(0) }
}
This looks way easier to read to me and keeps generic things generic
and not copy&pasted.
Nevertheless this is a nice approach, which will definitely show that
we need to think about interrupt routing properly ;-).
Alex
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don't miss this year's exciting event. There's still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
