Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Mon, Sep 26, 2011 at 08:04:24PM -0400, Kevin O'Connor wrote: On Mon, Sep 26, 2011 at 10:04:13AM +0300, Michael S. Tsirkin wrote: On Mon, Sep 26, 2011 at 12:40:18AM -0400, Kevin O'Connor wrote: On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote: On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote: The code to generate basic SSDT code isn't that difficult (see build_ssdt and src/ssdt-proc.dsl). Is there a compelling reason to patch the DSDT versus just generating the necessary blocks in an SSDT? I don't really care whether the code is in DSDT or SSDT, IMO there isn't much difference between build_ssdt and patching: main reason is build_ssdt uses offsets hardcoded to a specific binary (ssdt_proc and SD_OFFSET_* ) while I used a script to extract offsets. Yes - your script to extract the offsets is nice. If you still have doubts, it might make sense to merge just patch 1 - acpi: generate and parse mixed asl/aml listing - as the first step. With the infrastructure in place it will be easier to discuss the best way to use it. I'm okay with your first patch. BTW, any more comments with the rest of the patchset? If you just need to think about it, I understand. However, I wish to tag a release before committing ACPI changes. Sure. So you'll take this patchset from here or want me to ping you later? There was a concern raised with two-pass PCI initialization that I need to follow up on before tagging. The isa bridge? I thought that got fixed ... -Kevin -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On 09/27/2011 03:04 PM, Michael S. Tsirkin wrote: There was a concern raised with two-pass PCI initialization that I need to follow up on before tagging. The isa bridge? I thought that got fixed ... Daniel Berrange reported a regression with virtio-9p. Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Mon, Sep 26, 2011 at 12:40:18AM -0400, Kevin O'Connor wrote:
On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote:
On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
The code to generate basic SSDT code isn't that difficult (see
build_ssdt and src/ssdt-proc.dsl). Is there a compelling reason to
patch the DSDT versus just generating the necessary blocks in an SSDT?
I don't really care whether the code is in DSDT or SSDT,
IMO there isn't much difference between build_ssdt and patching:
main reason is build_ssdt uses offsets hardcoded to a specific binary
(ssdt_proc and SD_OFFSET_* ) while I used
a script to extract offsets.
Yes - your script to extract the offsets is nice.
If you still have doubts,
it might make sense to merge just patch 1 -
acpi: generate and parse mixed asl/aml listing
- as the first step.
With the infrastructure in place it will be
easier to discuss the best way to use it.
I think we should avoid relying on copy-pasted binary
because I see the related ASL code changing in the near future
(with multifunction and bridge support among others).
Can you expand on this? Would multi-function and bridge support make
patching easier than dynamic SSDT generation?
Just generic concerns:
1. We are going to have to modify DSL, so binary bytecode
would be hard to maintain. Specifically IMO the more
work can be done compile-time, the better, both iasl
and my script do compile-time checks to keep runtime simple.
2. There are going to be a lot of devices so one new table
with all of them would be ok, a table per device would be
expensive.
I'm a little leary of patching the DSDT - doubly so when the DSDT is
creating dummy devices that are then dynamically patched out.
A small correction, my specific code only patches out methods, not whole
devices.
(For
example, even with your patch series there are still two devices
defined with _ADR 1.)
So this is a bug, I think.
I am not sure whether we just want ISA and VGA always
non-removable - is there some reason that
we have hotplug_slot macros for these slots?
If no we should just remove hotplug
macros for these two slots. Gleb, Marcelo, any preferences?
If we want to keep the option of making these slots hotpluggable
from qemu, it is still easy to fix the duplication. For example,
I could put ACPI_EXTRACT tags in VGA/ISA devices without
renaming them. But the easiest way is probably by using Scope.
See patch below.
It seems more straight-forward to just create
the devices that are needed.
-Kevin
FWIW the acpi spec does mention that it's ok to describe
unoccupied slots.
Multiple device with the same _ADR relies on unspecified
behavior of ACPI. Fix DSDT so we always have a single device per slot.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
-
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 055202b..305d2e5 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -260,8 +260,8 @@ DefinitionBlock (
}
Scope(\_SB.PCI0) {
-Device (VGA) {
- Name (_ADR, 0x0002)
+ /* VGA device */
+Scope (S2) {
OperationRegion(PCIC, PCI_Config, Zero, 0x4)
Field(PCIC, DWordAcc, NoLock, Preserve) {
VEND, 32
@@ -282,13 +282,10 @@ DefinitionBlock (
Return (0x00)
}
}
- Method(_RMV) { Return (0x00) }
}
/* PIIX3 ISA bridge */
-Device (ISA) {
-Name (_ADR, 0x0001)
-Method(_RMV) { Return (0x00) }
+Scope (S1) {
/* PIIX PCI to ISA irq remapping */
@@ -486,7 +483,7 @@ DefinitionBlock (
/* PCI IRQs */
Scope(\_SB) {
- Field (\_SB.PCI0.ISA.P40C, ByteAcc, NoLock, Preserve)
+ Field (\_SB.PCI0.S1.P40C, ByteAcc, NoLock, Preserve)
{
PRQ0, 8,
PRQ1, 8,
--
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [SeaBIOS] [PATCH 0/4] acpi: fix up EJ0 in DSDT
Hi all, defined with _ADR 1.) It seems more straight-forward to just create the devices that are needed. Yes something like this: http://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/arch/x86/boot/acpigen.c;hb=HEAD Thanks Rudolf -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Mon, Sep 26, 2011 at 10:04:13AM +0300, Michael S. Tsirkin wrote:
On Mon, Sep 26, 2011 at 12:40:18AM -0400, Kevin O'Connor wrote:
On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote:
On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
The code to generate basic SSDT code isn't that difficult (see
build_ssdt and src/ssdt-proc.dsl). Is there a compelling reason to
patch the DSDT versus just generating the necessary blocks in an SSDT?
I don't really care whether the code is in DSDT or SSDT,
IMO there isn't much difference between build_ssdt and patching:
main reason is build_ssdt uses offsets hardcoded to a specific binary
(ssdt_proc and SD_OFFSET_* ) while I used
a script to extract offsets.
Yes - your script to extract the offsets is nice.
If you still have doubts,
it might make sense to merge just patch 1 -
acpi: generate and parse mixed asl/aml listing
- as the first step.
With the infrastructure in place it will be
easier to discuss the best way to use it.
I think we should avoid relying on copy-pasted binary
because I see the related ASL code changing in the near future
(with multifunction and bridge support among others).
Can you expand on this? Would multi-function and bridge support make
patching easier than dynamic SSDT generation?
Just generic concerns:
1. We are going to have to modify DSL, so binary bytecode
would be hard to maintain. Specifically IMO the more
work can be done compile-time, the better, both iasl
and my script do compile-time checks to keep runtime simple.
2. There are going to be a lot of devices so one new table
with all of them would be ok, a table per device would be
expensive.
I'm a little leary of patching the DSDT - doubly so when the DSDT is
creating dummy devices that are then dynamically patched out.
A small correction, my specific code only patches out methods, not whole
devices.
And note only the name of the method is changed to something which the
guests do not identify, its not as if the entire method is added or
removed (although IMO it would be interesting to patch out entirely with
NOPs).
(For
example, even with your patch series there are still two devices
defined with _ADR 1.)
So this is a bug, I think.
I am not sure whether we just want ISA and VGA always
non-removable - is there some reason that
we have hotplug_slot macros for these slots?
Just so its a nice linear range.
If no we should just remove hotplug
macros for these two slots. Gleb, Marcelo, any preferences?
Currently they are hardcoded as not hotpluggable (as can be noted by
the RMV macros), but its nice to retrieve that information from QEMU
instead.
If we want to keep the option of making these slots hotpluggable
from qemu, it is still easy to fix the duplication. For example,
I could put ACPI_EXTRACT tags in VGA/ISA devices without
renaming them. But the easiest way is probably by using Scope.
See patch below.
It seems more straight-forward to just create
the devices that are needed.
-Kevin
FWIW the acpi spec does mention that it's ok to describe
unoccupied slots.
Multiple device with the same _ADR relies on unspecified
behavior of ACPI. Fix DSDT so we always have a single device per slot.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
-
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 055202b..305d2e5 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -260,8 +260,8 @@ DefinitionBlock (
}
Scope(\_SB.PCI0) {
-Device (VGA) {
- Name (_ADR, 0x0002)
+ /* VGA device */
+Scope (S2) {
OperationRegion(PCIC, PCI_Config, Zero, 0x4)
Field(PCIC, DWordAcc, NoLock, Preserve) {
VEND, 32
@@ -282,13 +282,10 @@ DefinitionBlock (
Return (0x00)
}
}
- Method(_RMV) { Return (0x00) }
}
/* PIIX3 ISA bridge */
-Device (ISA) {
-Name (_ADR, 0x0001)
-Method(_RMV) { Return (0x00) }
+Scope (S1) {
/* PIIX PCI to ISA irq remapping */
@@ -486,7 +483,7 @@ DefinitionBlock (
/* PCI IRQs */
Scope(\_SB) {
- Field (\_SB.PCI0.ISA.P40C, ByteAcc, NoLock, Preserve)
+ Field (\_SB.PCI0.S1.P40C, ByteAcc, NoLock, Preserve)
{
PRQ0, 8,
PRQ1, 8,
--
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Mon, Sep 26, 2011 at 08:36:41AM -0300, Marcelo Tosatti wrote:
If no we should just remove hotplug
macros for these two slots. Gleb, Marcelo, any preferences?
Currently they are hardcoded as not hotpluggable (as can be noted by
the RMV macros), but its nice to retrieve that information from QEMU
instead.
OK, so you ACK the patch below?
If we want to keep the option of making these slots hotpluggable
from qemu, it is still easy to fix the duplication. For example,
I could put ACPI_EXTRACT tags in VGA/ISA devices without
renaming them. But the easiest way is probably by using Scope.
See patch below.
It seems more straight-forward to just create
the devices that are needed.
-Kevin
FWIW the acpi spec does mention that it's ok to describe
unoccupied slots.
Multiple device with the same _ADR relies on unspecified
behavior of ACPI. Fix DSDT so we always have a single device per slot.
Signed-off-by: Michael S. Tsirkin m...@redhat.com
-
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 055202b..305d2e5 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -260,8 +260,8 @@ DefinitionBlock (
}
Scope(\_SB.PCI0) {
-Device (VGA) {
- Name (_ADR, 0x0002)
+ /* VGA device */
+Scope (S2) {
OperationRegion(PCIC, PCI_Config, Zero, 0x4)
Field(PCIC, DWordAcc, NoLock, Preserve) {
VEND, 32
@@ -282,13 +282,10 @@ DefinitionBlock (
Return (0x00)
}
}
- Method(_RMV) { Return (0x00) }
}
/* PIIX3 ISA bridge */
-Device (ISA) {
-Name (_ADR, 0x0001)
-Method(_RMV) { Return (0x00) }
+Scope (S1) {
/* PIIX PCI to ISA irq remapping */
@@ -486,7 +483,7 @@ DefinitionBlock (
/* PCI IRQs */
Scope(\_SB) {
- Field (\_SB.PCI0.ISA.P40C, ByteAcc, NoLock, Preserve)
+ Field (\_SB.PCI0.S1.P40C, ByteAcc, NoLock, Preserve)
{
PRQ0, 8,
PRQ1, 8,
--
MST
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Mon, Sep 26, 2011 at 10:04:13AM +0300, Michael S. Tsirkin wrote: On Mon, Sep 26, 2011 at 12:40:18AM -0400, Kevin O'Connor wrote: On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote: On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote: The code to generate basic SSDT code isn't that difficult (see build_ssdt and src/ssdt-proc.dsl). Is there a compelling reason to patch the DSDT versus just generating the necessary blocks in an SSDT? I don't really care whether the code is in DSDT or SSDT, IMO there isn't much difference between build_ssdt and patching: main reason is build_ssdt uses offsets hardcoded to a specific binary (ssdt_proc and SD_OFFSET_* ) while I used a script to extract offsets. Yes - your script to extract the offsets is nice. If you still have doubts, it might make sense to merge just patch 1 - acpi: generate and parse mixed asl/aml listing - as the first step. With the infrastructure in place it will be easier to discuss the best way to use it. I'm okay with your first patch. However, I wish to tag a release before committing ACPI changes. There was a concern raised with two-pass PCI initialization that I need to follow up on before tagging. -Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote: On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote: The code to generate basic SSDT code isn't that difficult (see build_ssdt and src/ssdt-proc.dsl). Is there a compelling reason to patch the DSDT versus just generating the necessary blocks in an SSDT? I don't really care whether the code is in DSDT or SSDT, IMO there isn't much difference between build_ssdt and patching: main reason is build_ssdt uses offsets hardcoded to a specific binary (ssdt_proc and SD_OFFSET_* ) while I used a script to extract offsets. Yes - your script to extract the offsets is nice. I think we should avoid relying on copy-pasted binary because I see the related ASL code changing in the near future (with multifunction and bridge support among others). Can you expand on this? Would multi-function and bridge support make patching easier than dynamic SSDT generation? I'm a little leary of patching the DSDT - doubly so when the DSDT is creating dummy devices that are then dynamically patched out. (For example, even with your patch series there are still two devices defined with _ADR 1.) It seems more straight-forward to just create the devices that are needed. -Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote: On Wed, Sep 21, 2011 at 03:44:13PM +0300, Michael S. Tsirkin wrote: The reason is that our acpi tables declare both _RMV with value 0, and _EJ0 method for these slots. What happens in this case is undocumented by ACPI spec, so linux ignores _RMV, and windows seems to ignore _EJ0. Could the DSDT just not define _EJ0 for device 1 2 instead of dynamically patching them? (Would there ever be a case where we wouldn't know at compile time which devices need _EJ0?) Yes. in qemu we can make any slot non hotpluggable on command line by requesting a non hotpluggable device be put there. The correct way to suppress hotplug is not to have _EJ0, so this is what this patch does: it probes PIIX and modifies DSDT to match. The code to generate basic SSDT code isn't that difficult (see build_ssdt and src/ssdt-proc.dsl). Is there a compelling reason to patch the DSDT versus just generating the necessary blocks in an SSDT? -Kevin I don't really care whether the code is in DSDT or SSDT, IMO there isn't much difference between build_ssdt and patching: main reason is build_ssdt uses offsets hardcoded to a specific binary (ssdt_proc and SD_OFFSET_* ) while I used a script to extract offsets. I think we should avoid relying on copy-pasted binary because I see the related ASL code changing in the near future (with multifunction and bridge support among others). I can generalize the approach though, so that it can work for finding arbitrary names without writing more scripts, hopefully with the potential to address the hard-coded offsets in acpi.c as well. Does that sound interesting? -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote: On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote: On Wed, Sep 21, 2011 at 03:44:13PM +0300, Michael S. Tsirkin wrote: The correct way to suppress hotplug is not to have _EJ0, so this is what this patch does: it probes PIIX and modifies DSDT to match. The code to generate basic SSDT code isn't that difficult (see build_ssdt and src/ssdt-proc.dsl). Is there a compelling reason to patch the DSDT versus just generating the necessary blocks in an SSDT? I don't really care whether the code is in DSDT or SSDT, IMO there isn't much difference between build_ssdt and patching: main reason is build_ssdt uses offsets hardcoded to a specific binary (ssdt_proc and SD_OFFSET_* ) while I used a script to extract offsets. I think we should avoid relying on copy-pasted binary because I see the related ASL code changing in the near future (with multifunction and bridge support among others). I can generalize the approach though, so that it can work for finding arbitrary names without writing more scripts, hopefully with the potential to address the hard-coded offsets in acpi.c as well. Does that sound interesting? Replacing the hardcoding of offsets in src/ssdt-proc.dsl would be nice. I'll take a look at your new patches tonight. -Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] acpi: fix up EJ0 in DSDT
Here's a bug: guest thinks it can eject VGA device and ISA bridge. [root@dhcp74-172 ~]#lspci 00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03) 00:02.0 VGA compatible controller: Cirrus Logic GD 5446 00:03.0 PCI bridge: Red Hat, Inc. Device 0001 00:04.0 Ethernet controller: Qumranet, Inc. Virtio network device 00:05.0 SCSI storage controller: Qumranet, Inc. Virtio block device 01:00.0 Ethernet controller: Intel Corporation 82540EM Gigabit Ethernet Controller (rev 03) [root@dhcp74-172 ~]# ls /sys/bus/pci/slots/1/ adapter address attention latch module power [root@dhcp74-172 ~]# ls /sys/bus/pci/slots/2/ adapter address attention latch module power [root@dhcp74-172 ~]# echo 0 /sys/bus/pci/slots/2/power [root@dhcp74-172 ~]# lspci 00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02) 00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II] 00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II] 00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03) 00:03.0 PCI bridge: Red Hat, Inc. Device 0001 00:04.0 Ethernet controller: Qumranet, Inc. Virtio network device 00:05.0 SCSI storage controller: Qumranet, Inc. Virtio block device 01:00.0 Ethernet controller: Intel Corporation 82540EM Gigabit Ethernet Controller (rev 03) This is wrong because slots 1 and 2 are marked as not hotpluggable in qemu. The reason is that our acpi tables declare both _RMV with value 0, and _EJ0 method for these slots. What happens in this case is undocumented by ACPI spec, so linux ignores _RMV, and windows seems to ignore _EJ0. The correct way to suppress hotplug is not to have _EJ0, so this is what this patch does: it probes PIIX and modifies DSDT to match. With these patches applied, we get: [root@dhcp74-172 ~]# ls /sys/bus/pci/slots/1/ address [root@dhcp74-172 ~]# ls /sys/bus/pci/slots/2/ address I had to add a bit of compile-time infrastructure to handle the runtime patching in a simple way. I expect that it will be possible to reuse it if we need to patch other methods in the future. Michael S. Tsirkin (4): acpi: generate mixed asl/aml listing acpi: add aml/asl parsing script acpi: EJ0 method name patching acpi: remove _RMV Makefile | 10 ++-- src/acpi-dsdt.dsl | 58 +++ src/acpi.c| 11 src/find_ej0.pl | 136 + src/splitdsl.pl | 16 ++ 5 files changed, 176 insertions(+), 55 deletions(-) create mode 100755 src/find_ej0.pl create mode 100755 src/splitdsl.pl -- 1.7.5.53.gc233e -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT
On Wed, Sep 21, 2011 at 03:44:13PM +0300, Michael S. Tsirkin wrote: The reason is that our acpi tables declare both _RMV with value 0, and _EJ0 method for these slots. What happens in this case is undocumented by ACPI spec, so linux ignores _RMV, and windows seems to ignore _EJ0. Could the DSDT just not define _EJ0 for device 1 2 instead of dynamically patching them? (Would there ever be a case where we wouldn't know at compile time which devices need _EJ0?) The correct way to suppress hotplug is not to have _EJ0, so this is what this patch does: it probes PIIX and modifies DSDT to match. The code to generate basic SSDT code isn't that difficult (see build_ssdt and src/ssdt-proc.dsl). Is there a compelling reason to patch the DSDT versus just generating the necessary blocks in an SSDT? -Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
