Re: [SeaBIOS PATCH v2] hotplug: Add device per func in ACPI DSDT tables

["Michael S. Tsirkin" <mst@redhat.com>]

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@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@redhat.com
20110919093644.GC4501@redhat.com
20110919100434.GA6764@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@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
> >