[Qemu-devel] seabios: acpi: add _RMV control method for PCI devices

[Qemu-devel] 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 <mtosatti@redhat.com>

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 */

[Qemu-devel] Re: seabios: acpi: add _RMV control method for PCI devices

[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 <mtosatti@redhat.com>
> 
> 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 */

--
			Gleb.

[Qemu-devel] Re: seabios: acpi: add _RMV control method for PCI devices

[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). 

[Qemu-devel] Re: seabios: acpi: add _RMV control method for PCI devices

[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?

--
			Gleb.

[Qemu-devel] Re: seabios: acpi: add _RMV control method for PCI devices

[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?
> 
> --
> 			Gleb.

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.

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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.

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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 <mtosatti@redhat.com>

Thanks Marcelo,

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

-Kevin

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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 <mtosatti@redhat.com>
> 
> 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.

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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 <mtosatti@redhat.com>
> > 
> > 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

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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).

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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.

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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.

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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.

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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 <mtosatti@redhat.com>
> > > 
> > > 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.     

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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<mtosatti@redhat.com>
> >  >  >
> >  >  >  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

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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<mtosatti@redhat.com>
> >>  >  >
> >>  >  >  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).

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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.

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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.

[Qemu-devel] Re: seabios: acpi: add _RMV control method for PCI devices

[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 <mtosatti@redhat.com>
> 
Acked-by: Gleb Natapov <gleb@redhat.com>

> 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.

[Qemu-devel] Re: [SeaBIOS] seabios: acpi: add _RMV control method for PCI devices

[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 <mtosatti@redhat.com>
> > 
> Acked-by: Gleb Natapov <gleb@redhat.com>

Thanks.  I committed this.

-Kevin