On Wed, Sep 21, 2011 at 01:39:22AM -0400, Amos Kong wrote:
> ----- Original Message -----
> > On Tue, Sep 20, 2011 at 06:45:57AM -0400, Amos Kong wrote:
> > > From 48ea1c9188334b89a60b4f9e853e86fc04fda4a5 Mon Sep 17 00:00:00
> > > 2001
> > > From: Amos Kong <akong(a)redhat.com>
> > > Date: Tue, 20 Sep 2011 15:38:43 +0800
> > > Subject: [SeaBIOS PATCH v2] hotplug: Add device per func in ACPI
> > > DSDT tables
> > >
> > > Only func 0 is registered to guest driver (we can
> > > only found func 0 in slot->funcs list of driver),
> > > the other functions could not be cleaned when
> > > hot-removing the whole slot. This patch adds
> > > device per function in ACPI DSDT tables.
> > >
> > > Have tested with linux/winxp/win7, hot-adding/hot-remving,
> > > single/multiple function device, they are all fine.
> > > ---
> > > Changes from v1:
> > > - cleanup the macros, bios.bin gets back to 128K
> > > - notify only when func0 is added and removed
> >
> > How about moving code into functions so that it isn't duplicated for
> > each PCI device. See the patch below as an example (100% untested).
Hmm, I sent patches that did a similar thing but
in a slightly more compact way.
Message ids:
20110919092932.GB4501(a)redhat.com
20110919093644.GC4501(a)redhat.com
20110919100434.GA6764(a)redhat.com
Did they not reach you or something's wrong with them?
>
> Tested, it works as my original patch-v2.
>
> > The CPU hotplug stuff works this way, except it run-time generates
> > the
> > equivalent of "Device(S???)" and "PCNT". (It may make sense to
> > run-time generate the PCI hotplug as well - it consumes about 10K of
> > space to statically generate the 31 devices.)
>
> I'm ok with the new version.
>
> Acked-by: Amos Kong <akong(a)redhat.com>
>
> > -Kevin
> >
> >
> > diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
> > index 08412e2..31ac5eb 100644
> > --- a/src/acpi-dsdt.dsl
> > +++ b/src/acpi-dsdt.dsl
> > @@ -128,48 +128,6 @@ DefinitionBlock (
> > PCRM, 32,
> > }
> >
> > -#define hotplug_slot(name, nr) \
> > - Device (S##name) { \
> > - Name (_ADR, nr##0000) \
> > - Method (_EJ0,1) { \
> > - Store(ShiftLeft(1, nr), B0EJ) \
> > - Return (0x0) \
> > - } \
> > - Name (_SUN, name) \
> > - }
> > -
> > - hotplug_slot(1, 0x0001)
> > - hotplug_slot(2, 0x0002)
> > - hotplug_slot(3, 0x0003)
> > - hotplug_slot(4, 0x0004)
> > - hotplug_slot(5, 0x0005)
> > - hotplug_slot(6, 0x0006)
> > - hotplug_slot(7, 0x0007)
> > - hotplug_slot(8, 0x0008)
> > - hotplug_slot(9, 0x0009)
> > - hotplug_slot(10, 0x000a)
> > - hotplug_slot(11, 0x000b)
> > - hotplug_slot(12, 0x000c)
> > - hotplug_slot(13, 0x000d)
> > - hotplug_slot(14, 0x000e)
> > - hotplug_slot(15, 0x000f)
> > - hotplug_slot(16, 0x0010)
> > - hotplug_slot(17, 0x0011)
> > - hotplug_slot(18, 0x0012)
> > - hotplug_slot(19, 0x0013)
> > - hotplug_slot(20, 0x0014)
> > - hotplug_slot(21, 0x0015)
> > - hotplug_slot(22, 0x0016)
> > - hotplug_slot(23, 0x0017)
> > - hotplug_slot(24, 0x0018)
> > - hotplug_slot(25, 0x0019)
> > - hotplug_slot(26, 0x001a)
> > - hotplug_slot(27, 0x001b)
> > - hotplug_slot(28, 0x001c)
> > - hotplug_slot(29, 0x001d)
> > - hotplug_slot(30, 0x001e)
> > - hotplug_slot(31, 0x001f)
> > -
> > Name (_CRS, ResourceTemplate ()
> > {
> > WordBusNumber (ResourceProducer, MinFixed, MaxFixed,
> > PosDecode,
> > @@ -762,6 +720,119 @@ DefinitionBlock (
> > Zero /* reserved */
> > })
> >
> > + /* PCI hotplug */
> > + Scope(\_SB.PCI0) {
> > + /* Methods called by bulk generated PCI devices below */
> > + Method (PCEJ, 1, NotSerialized) {
> > + // _EJ0 method - eject callback
> > + Store(ShiftLeft(1, Arg0), B0EJ)
> > + Return (0x0)
> > + }
> > +
> > + /* Bulk generated PCI hotplug devices */
> > +#define hotplug_func(nr, fn) \
> > + Device (S##nr##fn) { \
> > + Name (_ADR, 0x##nr##000##fn) \
> > + Method (_EJ0, 1) { Return(PCEJ(0x##nr)) } \
> > + Name (_SUN, 0x##nr) \
> > + }
The fundamental question is still why would
we have _EJ0 methods in functions >0 when they are
not individually hotpluggable.
I think only function 0 should have _EJ0.
> > +
> > +#define hotplug_slot(nr) \
> > + hotplug_func(nr, 0) \
> > + hotplug_func(nr, 1) \
> > + hotplug_func(nr, 2) \
> > + hotplug_func(nr, 3) \
> > + hotplug_func(nr, 4) \
> > + hotplug_func(nr, 5) \
> > + hotplug_func(nr, 6) \
> > + hotplug_func(nr, 7)
> > +
> > + hotplug_slot(01)
> > + hotplug_slot(02)
> > + hotplug_slot(03)
> > + hotplug_slot(04)
> > + hotplug_slot(05)
> > + hotplug_slot(06)
> > + hotplug_slot(07)
> > + hotplug_slot(08)
> > + hotplug_slot(09)
> > + hotplug_slot(0a)
> > + hotplug_slot(0b)
> > + hotplug_slot(0c)
> > + hotplug_slot(0d)
> > + hotplug_slot(0e)
> > + hotplug_slot(0f)
> > + hotplug_slot(10)
> > + hotplug_slot(11)
> > + hotplug_slot(12)
> > + hotplug_slot(13)
> > + hotplug_slot(14)
> > + hotplug_slot(15)
> > + hotplug_slot(16)
> > + hotplug_slot(17)
> > + hotplug_slot(18)
> > + hotplug_slot(19)
> > + hotplug_slot(1a)
> > + hotplug_slot(1b)
> > + hotplug_slot(1c)
> > + hotplug_slot(1d)
> > + hotplug_slot(1e)
> > + hotplug_slot(1f)
> > +
> > + /* PCI hotplug notify method */
> > + Method(PCNF, 0) {
> > + // Local0 = iterator
> > + Store (Zero, Local0)
> > + While (LLess(Local0, 31)) {
> > + Increment(Local0)
> > + If (And(PCIU, ShiftLeft(1, Local0))) {
> > + PCNT(Local0, 1)
> > + }
> > + If (And(PCID, ShiftLeft(1, Local0))) {
> > + PCNT(Local0, 3)
> > + }
> > + }
> > + Return(One)
> > + }
> > +
> > +#define hotplug_notify(nr) \
> > + If (LEqual(Arg0, 0x##nr)) {Notify(S##nr##0, Arg1)}
> > +
> > + Method(PCNT, 2) {
> > + hotplug_notify(01)
> > + hotplug_notify(02)
> > + hotplug_notify(03)
> > + hotplug_notify(04)
> > + hotplug_notify(05)
> > + hotplug_notify(06)
> > + hotplug_notify(07)
> > + hotplug_notify(08)
> > + hotplug_notify(09)
> > + hotplug_notify(0a)
> > + hotplug_notify(0b)
> > + hotplug_notify(0c)
> > + hotplug_notify(0d)
> > + hotplug_notify(0e)
> > + hotplug_notify(0f)
> > + hotplug_notify(10)
> > + hotplug_notify(11)
> > + hotplug_notify(12)
> > + hotplug_notify(13)
> > + hotplug_notify(14)
> > + hotplug_notify(15)
> > + hotplug_notify(16)
> > + hotplug_notify(17)
> > + hotplug_notify(18)
> > + hotplug_notify(19)
> > + hotplug_notify(1a)
> > + hotplug_notify(1b)
> > + hotplug_notify(1c)
> > + hotplug_notify(1d)
> > + hotplug_notify(1e)
> > + hotplug_notify(1f)
> > + }
> > + }
> > +
> > /* CPU hotplug */
> > Scope(\_SB) {
> > /* Objects filled in by run-time generated SSDT */
> > @@ -842,49 +913,9 @@ DefinitionBlock (
> > Return(0x01)
> > }
> >
> > -#define gen_pci_hotplug(nr) \
> > - If (And(\_SB.PCI0.PCIU, ShiftLeft(1, nr))) { \
> > - Notify(\_SB.PCI0.S##nr, 1) \
> > - } \
> > - If (And(\_SB.PCI0.PCID, ShiftLeft(1, nr))) { \
> > - Notify(\_SB.PCI0.S##nr, 3) \
> > - }
> > -
> > Method(_L01) {
> > - gen_pci_hotplug(1)
> > - gen_pci_hotplug(2)
> > - gen_pci_hotplug(3)
> > - gen_pci_hotplug(4)
> > - gen_pci_hotplug(5)
> > - gen_pci_hotplug(6)
> > - gen_pci_hotplug(7)
> > - gen_pci_hotplug(8)
> > - gen_pci_hotplug(9)
> > - gen_pci_hotplug(10)
> > - gen_pci_hotplug(11)
> > - gen_pci_hotplug(12)
> > - gen_pci_hotplug(13)
> > - gen_pci_hotplug(14)
> > - gen_pci_hotplug(15)
> > - gen_pci_hotplug(16)
> > - gen_pci_hotplug(17)
> > - gen_pci_hotplug(18)
> > - gen_pci_hotplug(19)
> > - gen_pci_hotplug(20)
> > - gen_pci_hotplug(21)
> > - gen_pci_hotplug(22)
> > - gen_pci_hotplug(23)
> > - gen_pci_hotplug(24)
> > - gen_pci_hotplug(25)
> > - gen_pci_hotplug(26)
> > - gen_pci_hotplug(27)
> > - gen_pci_hotplug(28)
> > - gen_pci_hotplug(29)
> > - gen_pci_hotplug(30)
> > - gen_pci_hotplug(31)
> > -
> > - Return (0x01)
> > -
> > + // PCI hotplug event
> > + Return(\_SB.PCI0.PCNF())
> > }
> > Method(_L02) {
> > // CPU hotplug event
> >