seabios: acpi: add _RMV control method for PCI devices

[Marcelo Tosatti]

Use _RMV method to indicate whether device can be removed.

Data is retrieved from QEMU via I/O port 0xae0c.

Signed-off-by: Marcelo Tosatti <[e-mail]>

Index: seabios/src/acpi-dsdt.dsl
===================================================================
--- seabios.orig/src/acpi-dsdt.dsl
+++ seabios/src/acpi-dsdt.dsl
@@ -122,6 +122,12 @@ DefinitionBlock (
                 B0EJ, 32,
             }
 
+            OperationRegion(RMVC, SystemIO, 0xae0c, 0x04)
+            Field(RMVC, DWordAcc, NoLock, WriteAsZeros)
+            {
+                PCRM, 32,
+            }
+
 #define hotplug_slot(name, nr) \
             Device (S##name) {                    \
                Name (_ADR, nr##0000)              \
@@ -245,11 +251,14 @@ DefinitionBlock (
                  {
                          Return (0x00)
                  }
+                 Method(_RMV) { Return (0x00) }
         }
 
 	/* PIIX3 ISA bridge */
         Device (ISA) {
             Name (_ADR, 0x00010000)
+            Method(_RMV) { Return (0x00) }
+
 
             /* PIIX PCI to ISA irq remapping */
             OperationRegion (P40C, PCI_Config, 0x60, 0x04)
@@ -442,6 +451,49 @@ DefinitionBlock (
 		DRSJ, 32
 	    }
 	}
+
+#define gen_pci_device(name, nr)                                \
+        Device(SL##name) {                                      \
+            Name (_ADR, nr##0000)                               \
+            Method (_RMV) {                                     \
+                If (And(\_SB.PCI0.PCRM, ShiftLeft(1, nr))) {    \
+                    Return (0x1)                                \
+                }                                               \
+                Return (0x0)                                    \
+            }                                                   \
+            Name (_SUN, name)                                   \
+        }
+
+        /* VGA (slot 1) and ISA bus (slot 2) defined above */
+	gen_pci_device(3, 0x0003)
+	gen_pci_device(4, 0x0004)
+	gen_pci_device(5, 0x0005)
+	gen_pci_device(6, 0x0006)
+	gen_pci_device(7, 0x0007)
+	gen_pci_device(8, 0x0008)
+	gen_pci_device(9, 0x0009)
+	gen_pci_device(10, 0x000a)
+	gen_pci_device(11, 0x000b)
+	gen_pci_device(12, 0x000c)
+	gen_pci_device(13, 0x000d)
+	gen_pci_device(14, 0x000e)
+	gen_pci_device(15, 0x000f)
+	gen_pci_device(16, 0x0010)
+	gen_pci_device(17, 0x0011)
+	gen_pci_device(18, 0x0012)
+	gen_pci_device(19, 0x0013)
+	gen_pci_device(20, 0x0014)
+	gen_pci_device(21, 0x0015)
+	gen_pci_device(22, 0x0016)
+	gen_pci_device(23, 0x0017)
+	gen_pci_device(24, 0x0018)
+	gen_pci_device(25, 0x0019)
+	gen_pci_device(26, 0x001a)
+	gen_pci_device(27, 0x001b)
+	gen_pci_device(28, 0x001c)
+	gen_pci_device(29, 0x001d)
+	gen_pci_device(30, 0x001e)
+	gen_pci_device(31, 0x001f)
     }
 
     /* PCI IRQs */

[Gleb Natapov]

On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> Use _RMV method to indicate whether device can be removed.

But Windows still shows device as removable in the gui and allows to
remove it, correct?
> Data is retrieved from QEMU via I/O port 0xae0c.
>
> Signed-off-by: Marcelo Tosatti <[e-mail]>
>
> Index: seabios/src/acpi-dsdt.dsl
> ===================================================================
> --- seabios.orig/src/acpi-dsdt.dsl
> +++ seabios/src/acpi-dsdt.dsl
> @@ -122,6 +122,12 @@ DefinitionBlock (
>                  B0EJ, 32,
>              }
>
> +            OperationRegion(RMVC, SystemIO, 0xae0c, 0x04)
> +            Field(RMVC, DWordAcc, NoLock, WriteAsZeros)
> +            {
> +                PCRM, 32,
> +            }
> +
>  #define hotplug_slot(name, nr) \
>              Device (S##name) {                    \
>                 Name (_ADR, nr##0000)              \
> @@ -245,11 +251,14 @@ DefinitionBlock (
>                   {
>                           Return (0x00)
>                   }
> +                 Method(_RMV) { Return (0x00) }
>          }
>
> /* PIIX3 ISA bridge */
>          Device (ISA) {
>              Name (_ADR, 0x00010000)
> +            Method(_RMV) { Return (0x00) }
> +
>
> /* PIIX PCI to ISA irq remapping */
>              OperationRegion (P40C, PCI_Config, 0x60, 0x04)
> @@ -442,6 +451,49 @@ DefinitionBlock (
>       DRSJ, 32
>       }
>   }
> +
> +#define gen_pci_device(name, nr)                                \
> +        Device(SL##name) {                                      \
> +            Name (_ADR, nr##0000)                               \
> +            Method (_RMV) {                                     \
> +                If (And(\_SB.PCI0.PCRM, ShiftLeft(1, nr))) {    \
> +                    Return (0x1)                                \
> +                }                                               \
> +                Return (0x0)                                    \
> +            }                                                   \
> +            Name (_SUN, name)                                   \
> +        }
> Why not add this to hotplug_slot() macro?
> +
> +        /* VGA (slot 1) and ISA bus (slot 2) defined above */
> +	gen_pci_device(3, 0x0003)
> +	gen_pci_device(4, 0x0004)
> +	gen_pci_device(5, 0x0005)
> +	gen_pci_device(6, 0x0006)
> +	gen_pci_device(7, 0x0007)
> +	gen_pci_device(8, 0x0008)
> +	gen_pci_device(9, 0x0009)
> +	gen_pci_device(10, 0x000a)
> +	gen_pci_device(11, 0x000b)
> +	gen_pci_device(12, 0x000c)
> +	gen_pci_device(13, 0x000d)
> +	gen_pci_device(14, 0x000e)
> +	gen_pci_device(15, 0x000f)
> +	gen_pci_device(16, 0x0010)
> +	gen_pci_device(17, 0x0011)
> +	gen_pci_device(18, 0x0012)
> +	gen_pci_device(19, 0x0013)
> +	gen_pci_device(20, 0x0014)
> +	gen_pci_device(21, 0x0015)
> +	gen_pci_device(22, 0x0016)
> +	gen_pci_device(23, 0x0017)
> +	gen_pci_device(24, 0x0018)
> +	gen_pci_device(25, 0x0019)
> +	gen_pci_device(26, 0x001a)
> +	gen_pci_device(27, 0x001b)
> +	gen_pci_device(28, 0x001c)
> +	gen_pci_device(29, 0x001d)
> +	gen_pci_device(30, 0x001e)
> +	gen_pci_device(31, 0x001f)
>      }
>
> /* PCI IRQs */

[Marcelo Tosatti]

On Wed, Dec 08, 2010 at 07:34:42PM +0200, Gleb Natapov wrote:
> On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > Use _RMV method to indicate whether device can be removed.
>
> But Windows still shows device as removable in the gui and allows to
> remove it, correct?

No. From "Designing Hardware for Surprise Removal under Windows XP"
document:

"An ACPI BIOS can override the Removable capability by using the _RMV
method ..."
> > +#define gen_pci_device(name, nr)                                \
> > +        Device(SL##name) {                                      \
> > +            Name (_ADR, nr##0000)                               \
> > +            Method (_RMV) {                                     \
> > +                If (And(\_SB.PCI0.PCRM, ShiftLeft(1, nr))) {    \
> > +                    Return (0x1)                                \
> > +                }                                               \
> > +                Return (0x0)                                    \
> > +            }                                                   \
> > +            Name (_SUN, name)                                   \
> > +        }
> > Why not add this to hotplug_slot() macro?

Because its ignored if declared in the device object thats a child
of SB.PCI0 (hotplug_slot).

[Gleb Natapov]

On Wed, Dec 08, 2010 at 04:01:18PM -0200, Marcelo Tosatti wrote:
> On Wed, Dec 08, 2010 at 07:34:42PM +0200, Gleb Natapov wrote:
> > On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > > Use _RMV method to indicate whether device can be removed.
> >
> > But Windows still shows device as removable in the gui and allows to
> > remove it, correct?
>
> No. From "Designing Hardware for Surprise Removal under Windows XP"
> document:
>
> "An ACPI BIOS can override the Removable capability by using the _RMV
> method ..."

Cool. I wonder how it co-exists with _EJ0 method for the same device.
> > > +#define gen_pci_device(name, nr)                                \
> > > +        Device(SL##name) {                                      \
> > > +            Name (_ADR, nr##0000)                               \
> > > +            Method (_RMV) {                                     \
> > > +                If (And(\_SB.PCI0.PCRM, ShiftLeft(1, nr))) {    \
> > > +                    Return (0x1)                                \
> > > +                }                                               \
> > > +                Return (0x0)                                    \
> > > +            }                                                   \
> > > +            Name (_SUN, name)                                   \
> > > +        }
> > > Why not add this to hotplug_slot() macro?
>
> Because its ignored if declared in the device object thats a child
> of SB.PCI0 (hotplug_slot). 
> Any idea why?

[Marcelo Tosatti]

On Wed, Dec 08, 2010 at 09:58:35PM +0200, Gleb Natapov wrote:
> On Wed, Dec 08, 2010 at 04:01:18PM -0200, Marcelo Tosatti wrote:
> > On Wed, Dec 08, 2010 at 07:34:42PM +0200, Gleb Natapov wrote:
> > > On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > > > Use _RMV method to indicate whether device can be removed.
> > >
> > > But Windows still shows device as removable in the gui and allows to
> > > remove it, correct?
> >
> > No. From "Designing Hardware for Surprise Removal under Windows XP"
> > document:
> >
> > "An ACPI BIOS can override the Removable capability by using the _RMV
> > method ..."
>
> Cool. I wonder how it co-exists with _EJ0 method for the same device.

The Linux driver, at least, will use the _EJ0 method of the first device
object. I guess Windows does the same.
> > > > +#define gen_pci_device(name, nr)                                \
> > > > +        Device(SL##name) {                                      \
> > > > +            Name (_ADR, nr##0000)                               \
> > > > +            Method (_RMV) {                                     \
> > > > +                If (And(\_SB.PCI0.PCRM, ShiftLeft(1, nr))) {    \
> > > > +                    Return (0x1)                                \
> > > > +                }                                               \
> > > > +                Return (0x0)                                    \
> > > > +            }                                                   \
> > > > +            Name (_SUN, name)                                   \
> > > > +        }
> > > > Why not add this to hotplug_slot() macro?
> >
> > Because its ignored if declared in the device object thats a child
> > of SB.PCI0 (hotplug_slot). 
> > Any idea why?

Because _EJ0 overrides _RMV when deciding removability, inside a
device object (just checked). So the above "if declared in a child of
SB.PCI0..." is wrong.

[Avi Kivity]

On 12/08/2010 07:08 PM, Marcelo Tosatti wrote:
> Use _RMV method to indicate whether device can be removed.
>
> Data is retrieved from QEMU via I/O port 0xae0c.

Where did this port come from?  What's the protocol?

Maybe we should do this via fw_cfg.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Marcelo Tosatti]

On Sat, Dec 11, 2010 at 09:39:30AM +0200, Avi Kivity wrote:
> On 12/08/2010 07:08 PM, Marcelo Tosatti wrote:
> > Use _RMV method to indicate whether device can be removed.
> >
> > Data is retrieved from QEMU via I/O port 0xae0c.
>
> Where did this port come from?

Its the next available address after "PCI EJ base", used
for QEMU<->ACPI hotplug communication.
> What's the protocol?

ACPI reads the 32-bit field indicating the return value of the _RMV
method (which is used by Windows to decide removability). 1-bit per
slot.

More ports have to be registered if more buses are added.
> Maybe we should do this via fw_cfg.

I don't see a need for it? (yes, it might be possible, but i'm not
familiar enough with AML).

[Avi Kivity]

On 12/13/2010 02:00 AM, Marcelo Tosatti wrote:
> On Sat, Dec 11, 2010 at 09:39:30AM +0200, Avi Kivity wrote:
> > On 12/08/2010 07:08 PM, Marcelo Tosatti wrote:
> > > Use _RMV method to indicate whether device can be removed.
> > >
> > > Data is retrieved from QEMU via I/O port 0xae0c.
> >
> > Where did this port come from?
>
> Its the next available address after "PCI EJ base", used
> for QEMU<->ACPI hotplug communication.
> > What's the protocol?
>
> ACPI reads the 32-bit field indicating the return value of the _RMV
> method (which is used by Windows to decide removability). 1-bit per
> slot.
>
> More ports have to be registered if more buses are added.
> > Maybe we should do this via fw_cfg.
>
> I don't see a need for it? (yes, it might be possible, but i'm not
> familiar enough with AML).

To avoid adding tons of undocumented I/O ports, and to allow 
discoverability (what happens with a new seabios on old qemu)?

We could do this in two ways: by adding a fwcfg client to the DSDT, or 
by copying the information to system memory, and referencing system 
memory from the DSDT.

-- 
error compiling committee.c: too many arguments to function

[Gleb Natapov]

On Mon, Dec 13, 2010 at 10:41:25AM +0200, Avi Kivity wrote:
> On 12/13/2010 02:00 AM, Marcelo Tosatti wrote:
> > On Sat, Dec 11, 2010 at 09:39:30AM +0200, Avi Kivity wrote:
> > > On 12/08/2010 07:08 PM, Marcelo Tosatti wrote:
> > > > Use _RMV method to indicate whether device can be removed.
> > > >
> > > > Data is retrieved from QEMU via I/O port 0xae0c.
> > >
> > > Where did this port come from?
> >
> > Its the next available address after "PCI EJ base", used
> > for QEMU<->ACPI hotplug communication.
> > > What's the protocol?
> >
> > ACPI reads the 32-bit field indicating the return value of the _RMV
> > method (which is used by Windows to decide removability). 1-bit per
> > slot.
> >
> > More ports have to be registered if more buses are added.
> > > Maybe we should do this via fw_cfg.
> >
> > I don't see a need for it? (yes, it might be possible, but i'm not
> > familiar enough with AML).
>
> To avoid adding tons of undocumented I/O ports, and to allow
> discoverability (what happens with a new seabios on old qemu)?

We already have out own mini pci hot-plug controller at io port 0xae00.
The patch just extends its functionality a bit. Logically this
functionality belongs there.
> We could do this in two ways: by adding a fwcfg client to the DSDT,
> or by copying the information to system memory, and referencing
> system memory from the DSDT.

This is even worse. It requires some fixed address to be shared between
DSDT and Seabios (or alternatively Seabios will have to generate this
part of DSDT dynamically).

--
			Gleb.

[Avi Kivity]

On 12/13/2010 10:49 AM, Gleb Natapov wrote:
> On Mon, Dec 13, 2010 at 10:41:25AM +0200, Avi Kivity wrote:
> > On 12/13/2010 02:00 AM, Marcelo Tosatti wrote:
> > > On Sat, Dec 11, 2010 at 09:39:30AM +0200, Avi Kivity wrote:
> > > > On 12/08/2010 07:08 PM, Marcelo Tosatti wrote:
> > > > > Use _RMV method to indicate whether device can be removed.
> > > > >
> > > > > Data is retrieved from QEMU via I/O port 0xae0c.
> > > >
> > > > Where did this port come from?
> > >
> > > Its the next available address after "PCI EJ base", used
> > > for QEMU<->ACPI hotplug communication.
> > > > What's the protocol?
> > >
> > > ACPI reads the 32-bit field indicating the return value of the _RMV
> > > method (which is used by Windows to decide removability). 1-bit per
> > > slot.
> > >
> > > More ports have to be registered if more buses are added.
> > > > Maybe we should do this via fw_cfg.
> > >
> > > I don't see a need for it? (yes, it might be possible, but i'm not
> > > familiar enough with AML).
> >
> > To avoid adding tons of undocumented I/O ports, and to allow
> >  discoverability (what happens with a new seabios on old qemu)?
>
> We already have out own mini pci hot-plug controller at io port 0xae00.
> The patch just extends its functionality a bit. Logically this
> functionality belongs there.

Well, at least it should be documented.

We could also deprecate the old port and use fwcfg for everything (try 
fwcfg, fall back to ae00).
> > We could do this in two ways: by adding a fwcfg client to the DSDT,
> >  or by copying the information to system memory, and referencing
> >  system memory from the DSDT.
>
> This is even worse. It requires some fixed address to be shared between
> DSDT and Seabios (or alternatively Seabios will have to generate this
> part of DSDT dynamically).

Could easily be something in the F segment.

-- 
error compiling committee.c: too many arguments to function

[Gleb Natapov]

On Mon, Dec 13, 2010 at 10:53:07AM +0200, Avi Kivity wrote:
> On 12/13/2010 10:49 AM, Gleb Natapov wrote:
> > On Mon, Dec 13, 2010 at 10:41:25AM +0200, Avi Kivity wrote:
> > > On 12/13/2010 02:00 AM, Marcelo Tosatti wrote:
> > > > On Sat, Dec 11, 2010 at 09:39:30AM +0200, Avi Kivity wrote:
> > > > > On 12/08/2010 07:08 PM, Marcelo Tosatti wrote:
> > > > > > Use _RMV method to indicate whether device can be removed.
> > > > > >
> > > > > > Data is retrieved from QEMU via I/O port 0xae0c.
> > > > >
> > > > > Where did this port come from?
> > > >
> > > > Its the next available address after "PCI EJ base", used
> > > > for QEMU<->ACPI hotplug communication.
> > > > > What's the protocol?
> > > >
> > > > ACPI reads the 32-bit field indicating the return value of the _RMV
> > > > method (which is used by Windows to decide removability). 1-bit per
> > > > slot.
> > > >
> > > > More ports have to be registered if more buses are added.
> > > > > Maybe we should do this via fw_cfg.
> > > >
> > > > I don't see a need for it? (yes, it might be possible, but i'm not
> > > > familiar enough with AML).
> > >
> > > To avoid adding tons of undocumented I/O ports, and to allow
> > >  discoverability (what happens with a new seabios on old qemu)?
> >
> > We already have out own mini pci hot-plug controller at io port 0xae00.
> > The patch just extends its functionality a bit. Logically this
> > functionality belongs there.
>
> Well, at least it should be documented.

Agree.
> We could also deprecate the old port and use fwcfg for everything
> (try fwcfg, fall back to ae00).

fwcfg designed to be simple for easy use by firmware. It has two port
one for index another for value, so its use is racy in multi-threaded SMP
environment. DSDT code is executed in such environment. There is lock
facility in AML, but why complicate things.
> > > We could do this in two ways: by adding a fwcfg client to the DSDT,
> > >  or by copying the information to system memory, and referencing
> > >  system memory from the DSDT.
> >
> > This is even worse. It requires some fixed address to be shared between
> > DSDT and Seabios (or alternatively Seabios will have to generate this
> > part of DSDT dynamically).
>
> Could easily be something in the F segment.

Yes, but then we will have two magic values (fwcfg index + address
in F segment) instead of one (address of pci hot-plug controller).

--
			Gleb.

[Avi Kivity]

On 12/13/2010 11:03 AM, Gleb Natapov wrote:
> > We could also deprecate the old port and use fwcfg for everything
> >  (try fwcfg, fall back to ae00).
>
> fwcfg designed to be simple for easy use by firmware. It has two port
> one for index another for value, so its use is racy in multi-threaded SMP
> environment. DSDT code is executed in such environment. There is lock
> facility in AML, but why complicate things.

I prefer to remove complexity from interfaces and have it in the 
implementation instead.
> > > > We could do this in two ways: by adding a fwcfg client to the DSDT,
> > > >   or by copying the information to system memory, and referencing
> > > >   system memory from the DSDT.
> > >
> > > This is even worse. It requires some fixed address to be shared between
> > > DSDT and Seabios (or alternatively Seabios will have to generate this
> > > part of DSDT dynamically).
> >
> > Could easily be something in the F segment.
>
> Yes, but then we will have two magic values (fwcfg index + address
> in F segment) instead of one (address of pci hot-plug controller).

The F segment address is internal to SeaBIOS; it isn't an external 
interface.

-- 
error compiling committee.c: too many arguments to function

[Gleb Natapov]

On Mon, Dec 13, 2010 at 11:10:38AM +0200, Avi Kivity wrote:
> On 12/13/2010 11:03 AM, Gleb Natapov wrote:
> > > We could also deprecate the old port and use fwcfg for everything
> > >  (try fwcfg, fall back to ae00).
> >
> > fwcfg designed to be simple for easy use by firmware. It has two port
> > one for index another for value, so its use is racy in multi-threaded SMP
> > environment. DSDT code is executed in such environment. There is lock
> > facility in AML, but why complicate things.
>
> I prefer to remove complexity from interfaces and have it in the
> implementation instead.
> I prefer whatever is simpler :) simpler == less bugs. And it is not like
> we discuss new interface here. You want to deprecate existing interface
> in favor of something that was not designed to handle the task.
> > > > > We could do this in two ways: by adding a fwcfg client to the DSDT,
> > > > >   or by copying the information to system memory, and referencing
> > > > >   system memory from the DSDT.
> > > >
> > > > This is even worse. It requires some fixed address to be shared between
> > > > DSDT and Seabios (or alternatively Seabios will have to generate this
> > > > part of DSDT dynamically).
> > >
> > > Could easily be something in the F segment.
> >
> > Yes, but then we will have two magic values (fwcfg index + address
> > in F segment) instead of one (address of pci hot-plug controller).
>
> The F segment address is internal to SeaBIOS; it isn't an external
> interface.

Depends on how you define external interface. It can be considered as
interface between OSPM and firmware. Next time layout of F segment
changes in SeaBIOS will you remember fixing DSDT too?

--
			Gleb.

[Kevin O'Connor]

On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> Use _RMV method to indicate whether device can be removed.
>
> Data is retrieved from QEMU via I/O port 0xae0c.
>
> Signed-off-by: Marcelo Tosatti <[e-mail]>

Thanks Marcelo,

Can you add acked-bys from the qemu/kvm maintaners?

-Kevin

[Gleb Natapov]

On Sun, Dec 12, 2010 at 02:11:29PM -0500, Kevin O'Connor wrote:
> On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > Use _RMV method to indicate whether device can be removed.
> >
> > Data is retrieved from QEMU via I/O port 0xae0c.
> >
> > Signed-off-by: Marcelo Tosatti <[e-mail]>
>
> Thanks Marcelo,
>
> Can you add acked-bys from the qemu/kvm maintaners?

Marcelo is kvm maintainer ;) FWIW I tested this with Windows 7 & XP
and it works as expected. The patch relies on patch not yes excepted
to qemu though.

--
			Gleb.

[Kevin O'Connor]

On Sun, Dec 12, 2010 at 09:49:16PM +0200, Gleb Natapov wrote:
> On Sun, Dec 12, 2010 at 02:11:29PM -0500, Kevin O'Connor wrote:
> > On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > > Use _RMV method to indicate whether device can be removed.
> > >
> > > Data is retrieved from QEMU via I/O port 0xae0c.
> > >
> > > Signed-off-by: Marcelo Tosatti <[e-mail]>
> >
> > Thanks Marcelo,
> >
> > Can you add acked-bys from the qemu/kvm maintaners?
>
> Marcelo is kvm maintainer ;) FWIW I tested this with Windows 7 & XP
> and it works as expected. The patch relies on patch not yes excepted
> to qemu though.

Yeah - I know - but I figured I'd ask for concensus before committing.
:-)

The committing of seabios patches that have a dependency on qemu/kvm
is an area that I think could be better clarified.  I'm thinking that
if the seabios part depends on something in qemu/kvm then we should
have the corresponding kvm/qemu maintaners "ack" it.

-Kevin

[Marcelo Tosatti]

On Sun, Dec 12, 2010 at 02:57:45PM -0500, Kevin O'Connor wrote:
> On Sun, Dec 12, 2010 at 09:49:16PM +0200, Gleb Natapov wrote:
> > On Sun, Dec 12, 2010 at 02:11:29PM -0500, Kevin O'Connor wrote:
> > > On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > > > Use _RMV method to indicate whether device can be removed.
> > > >
> > > > Data is retrieved from QEMU via I/O port 0xae0c.
> > > >
> > > > Signed-off-by: Marcelo Tosatti <[e-mail]>
> > >
> > > Thanks Marcelo,
> > >
> > > Can you add acked-bys from the qemu/kvm maintaners?
> >
> > Marcelo is kvm maintainer ;) FWIW I tested this with Windows 7 & XP
> > and it works as expected. The patch relies on patch not yes excepted
> > to qemu though.
>
> Yeah - I know - but I figured I'd ask for concensus before committing.
> :-)
>
> The committing of seabios patches that have a dependency on qemu/kvm
> is an area that I think could be better clarified.  I'm thinking that
> if the seabios part depends on something in qemu/kvm then we should
> have the corresponding kvm/qemu maintaners "ack" it.
>
> -Kevin

Avi, are you OK with this patch ?

Yes, perhaps it all (including PCI hotplug controller) should be using  
something else than hardcoded IO ports, but thats what we have now.

[Avi Kivity]

On 12/20/2010 10:49 AM, Marcelo Tosatti wrote:
> On Sun, Dec 12, 2010 at 02:57:45PM -0500, Kevin O'Connor wrote:
> > On Sun, Dec 12, 2010 at 09:49:16PM +0200, Gleb Natapov wrote:
> > > On Sun, Dec 12, 2010 at 02:11:29PM -0500, Kevin O'Connor wrote:
> > > > On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > > > > Use _RMV method to indicate whether device can be removed.
> > > > >
> > > > > Data is retrieved from QEMU via I/O port 0xae0c.
> > > > >
> > > > > Signed-off-by: Marcelo Tosatti<[e-mail]>
> > > >
> > > > Thanks Marcelo,
> > > >
> > > > Can you add acked-bys from the qemu/kvm maintaners?
> > >
> > > Marcelo is kvm maintainer ;) FWIW I tested this with Windows 7&  XP
> > >  and it works as expected. The patch relies on patch not yes excepted
> > >  to qemu though.
> >
> > Yeah - I know - but I figured I'd ask for concensus before committing.
> >  :-)
> >
> > The committing of seabios patches that have a dependency on qemu/kvm
> >  is an area that I think could be better clarified.  I'm thinking that
> >  if the seabios part depends on something in qemu/kvm then we should
> >  have the corresponding kvm/qemu maintaners "ack" it.
> >
> > -Kevin
>
> Avi, are you OK with this patch ?
>
> Yes, perhaps it all (including PCI hotplug controller) should be using
> something else than hardcoded IO ports, but thats what we have now.

At least it should be documented.

What's the behaviour with a qemu that doesn't support the new port?  We 
don't strictly support it, but let's do so if we can.

-- 
error compiling committee.c: too many arguments to function

[Marcelo Tosatti]

On Mon, Dec 20, 2010 at 11:15:40AM +0200, Avi Kivity wrote:
> On 12/20/2010 10:49 AM, Marcelo Tosatti wrote:
> > On Sun, Dec 12, 2010 at 02:57:45PM -0500, Kevin O'Connor wrote:
> > > On Sun, Dec 12, 2010 at 09:49:16PM +0200, Gleb Natapov wrote:
> > > > On Sun, Dec 12, 2010 at 02:11:29PM -0500, Kevin O'Connor wrote:
> > > > > On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > > > > > Use _RMV method to indicate whether device can be removed.
> > > > > >
> > > > > > Data is retrieved from QEMU via I/O port 0xae0c.
> > > > > >
> > > > > > Signed-off-by: Marcelo Tosatti<[e-mail]>
> > > > >
> > > > > Thanks Marcelo,
> > > > >
> > > > > Can you add acked-bys from the qemu/kvm maintaners?
> > > >
> > > > Marcelo is kvm maintainer ;) FWIW I tested this with Windows 7&  XP
> > > >  and it works as expected. The patch relies on patch not yes excepted
> > > >  to qemu though.
> > >
> > > Yeah - I know - but I figured I'd ask for concensus before committing.
> > >  :-)
> > >
> > > The committing of seabios patches that have a dependency on qemu/kvm
> > >  is an area that I think could be better clarified.  I'm thinking that
> > >  if the seabios part depends on something in qemu/kvm then we should
> > >  have the corresponding kvm/qemu maintaners "ack" it.
> > >
> > > -Kevin
> >
> > Avi, are you OK with this patch ?
> >
> > Yes, perhaps it all (including PCI hotplug controller) should be using
> > something else than hardcoded IO ports, but thats what we have now.
>
> At least it should be documented.
>
> What's the behaviour with a qemu that doesn't support the new port?
> We don't strictly support it, but let's do so if we can.

All slots will be marked as hotpluggable (since the "removability" IO port will not be
registered and return all 1's).

[Avi Kivity]

On 12/20/2010 07:05 PM, Marcelo Tosatti wrote:
> > > Yes, perhaps it all (including PCI hotplug controller) should be using
> > > something else than hardcoded IO ports, but thats what we have now.
> >
> > At least it should be documented.
> >
> > What's the behaviour with a qemu that doesn't support the new port?
> >  We don't strictly support it, but let's do so if we can.
>
> All slots will be marked as hotpluggable (since the "removability" IO port will not be
> registered and return all 1's).

Good, so compatibility is retained.  seabios will know not to allocate 
this address to pci bars?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Marcelo Tosatti]

On Mon, Dec 20, 2010 at 07:44:12PM +0200, Avi Kivity wrote:
> On 12/20/2010 07:05 PM, Marcelo Tosatti wrote:
> > > > Yes, perhaps it all (including PCI hotplug controller) should be using
> > > > something else than hardcoded IO ports, but thats what we have now.
> > >
> > > At least it should be documented.
> > >
> > > What's the behaviour with a qemu that doesn't support the new port?
> > >  We don't strictly support it, but let's do so if we can.
> >
> > All slots will be marked as hotpluggable (since the "removability" IO port will not be
> > registered and return all 1's).
>
> Good, so compatibility is retained.  seabios will know not to
> allocate this address to pci bars?

seabios starts allocating at 0xc000, which is past the hardcoded
addresses.

In any case, qemu will abort on registration if there are conflicts.

[Gleb Natapov]

On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> Use _RMV method to indicate whether device can be removed.
>
> Data is retrieved from QEMU via I/O port 0xae0c.
>
> Signed-off-by: Marcelo Tosatti <[e-mail]>

Acked-by: Gleb Natapov <[e-mail]>
> Index: seabios/src/acpi-dsdt.dsl
> ===================================================================
> --- seabios.orig/src/acpi-dsdt.dsl
> +++ seabios/src/acpi-dsdt.dsl
> @@ -122,6 +122,12 @@ DefinitionBlock (
>                  B0EJ, 32,
>              }
>
> +            OperationRegion(RMVC, SystemIO, 0xae0c, 0x04)
> +            Field(RMVC, DWordAcc, NoLock, WriteAsZeros)
> +            {
> +                PCRM, 32,
> +            }
> +
>  #define hotplug_slot(name, nr) \
>              Device (S##name) {                    \
>                 Name (_ADR, nr##0000)              \
> @@ -245,11 +251,14 @@ DefinitionBlock (
>                   {
>                           Return (0x00)
>                   }
> +                 Method(_RMV) { Return (0x00) }
>          }
>
> /* PIIX3 ISA bridge */
>          Device (ISA) {
>              Name (_ADR, 0x00010000)
> +            Method(_RMV) { Return (0x00) }
> +
>
> /* PIIX PCI to ISA irq remapping */
>              OperationRegion (P40C, PCI_Config, 0x60, 0x04)
> @@ -442,6 +451,49 @@ DefinitionBlock (
>       DRSJ, 32
>       }
>   }
> +
> +#define gen_pci_device(name, nr)                                \
> +        Device(SL##name) {                                      \
> +            Name (_ADR, nr##0000)                               \
> +            Method (_RMV) {                                     \
> +                If (And(\_SB.PCI0.PCRM, ShiftLeft(1, nr))) {    \
> +                    Return (0x1)                                \
> +                }                                               \
> +                Return (0x0)                                    \
> +            }                                                   \
> +            Name (_SUN, name)                                   \
> +        }
> +
> +        /* VGA (slot 1) and ISA bus (slot 2) defined above */
> +	gen_pci_device(3, 0x0003)
> +	gen_pci_device(4, 0x0004)
> +	gen_pci_device(5, 0x0005)
> +	gen_pci_device(6, 0x0006)
> +	gen_pci_device(7, 0x0007)
> +	gen_pci_device(8, 0x0008)
> +	gen_pci_device(9, 0x0009)
> +	gen_pci_device(10, 0x000a)
> +	gen_pci_device(11, 0x000b)
> +	gen_pci_device(12, 0x000c)
> +	gen_pci_device(13, 0x000d)
> +	gen_pci_device(14, 0x000e)
> +	gen_pci_device(15, 0x000f)
> +	gen_pci_device(16, 0x0010)
> +	gen_pci_device(17, 0x0011)
> +	gen_pci_device(18, 0x0012)
> +	gen_pci_device(19, 0x0013)
> +	gen_pci_device(20, 0x0014)
> +	gen_pci_device(21, 0x0015)
> +	gen_pci_device(22, 0x0016)
> +	gen_pci_device(23, 0x0017)
> +	gen_pci_device(24, 0x0018)
> +	gen_pci_device(25, 0x0019)
> +	gen_pci_device(26, 0x001a)
> +	gen_pci_device(27, 0x001b)
> +	gen_pci_device(28, 0x001c)
> +	gen_pci_device(29, 0x001d)
> +	gen_pci_device(30, 0x001e)
> +	gen_pci_device(31, 0x001f)
>      }
>
> /* PCI IRQs */

[Kevin O'Connor]

On Wed, Jan 05, 2011 at 01:10:08PM +0200, Gleb Natapov wrote:
> On Wed, Dec 08, 2010 at 03:08:59PM -0200, Marcelo Tosatti wrote:
> > Use _RMV method to indicate whether device can be removed.
> >
> > Data is retrieved from QEMU via I/O port 0xae0c.
> >
> > Signed-off-by: Marcelo Tosatti <[e-mail]>
>
> Acked-by: Gleb Natapov <[e-mail]>

Thanks.  I committed this.

-Kevin