[PATCH 1/3] ACPI: Ignore the upper bits of SystemIO addresses

[PATCH 1/3] ACPI: Ignore the upper bits of SystemIO addresses

[Matthew Garrett]

Various machines (https://bugzilla.redhat.com/show_bug.cgi?id=585756 for
instance) contain SystemIO spaces with addresses > 16 bits. acpica throws
an error for this, while Windows silently ignores the upper 16 bits and
carries on happily. Provide support for the latter behaviour for bug
compatibility.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/acpica/acglobal.h |    6 ++++++
 drivers/acpi/acpica/hwvalid.c  |   10 ++++++++++
 include/acpi/acpixf.h          |    1 +
 3 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h
index f8dd8f2..f302168 100644
--- a/drivers/acpi/acpica/acglobal.h
+++ b/drivers/acpi/acpica/acglobal.h
@@ -112,6 +112,12 @@ u8 ACPI_INIT_GLOBAL(acpi_gbl_leave_wake_gpes_disabled, TRUE);
  */
 u8 ACPI_INIT_GLOBAL(acpi_gbl_use_default_register_widths, TRUE);
 
+/*
+ * Optionally provide compatibility with Windows by only using the lower
+ * 16 bits of system io addresses.
+ */
+u8 ACPI_INIT_GLOBAL(acpi_gbl_ignore_high_ioport_bits, FALSE);
+
 /* acpi_gbl_FADT is a local copy of the FADT, converted to a common format. */
 
 struct acpi_table_fadt acpi_gbl_FADT;
diff --git a/drivers/acpi/acpica/hwvalid.c b/drivers/acpi/acpica/hwvalid.c
index e26c17d..c151a15 100644
--- a/drivers/acpi/acpica/hwvalid.c
+++ b/drivers/acpi/acpica/hwvalid.c
@@ -222,6 +222,11 @@ acpi_status acpi_hw_read_port(acpi_io_address address, u32 *value, u32 width)
 	u32 one_byte;
 	u32 i;
 
+	if (acpi_gbl_ignore_high_ioport_bits)
+		/* Windows only uses the lower 16 bits of an address.
+		   Emulate that */
+		address &= 0xffff;
+
 	/* Validate the entire request and perform the I/O */
 
 	status = acpi_hw_validate_io_request(address, width);
@@ -279,6 +284,11 @@ acpi_status acpi_hw_write_port(acpi_io_address address, u32 value, u32 width)
 	acpi_status status;
 	u32 i;
 
+	if (acpi_gbl_ignore_high_ioport_bits)
+		/* Windows only uses the lower 16 bits of an address.
+		   Emulate that */
+		address &= 0xffff;
+
 	/* Validate the entire request and perform the I/O */
 
 	status = acpi_hw_validate_io_request(address, width);
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 4447a04..847d262 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -65,6 +65,7 @@ extern u8 acpi_gbl_all_methods_serialized;
 extern u8 acpi_gbl_create_osi_method;
 extern u8 acpi_gbl_leave_wake_gpes_disabled;
 extern u8 acpi_gbl_use_default_register_widths;
+extern u8 acpi_gbl_ignore_high_ioport_bits;
 extern acpi_name acpi_gbl_trace_method_name;
 extern u32 acpi_gbl_trace_flags;
 
-- 
1.7.0.1

[PATCH 2/3] ACPI: Add acpi_gbl_osi_data to OS headers

[Matthew Garrett]

Different operating systems may wish to enable different workaround quirks
depending on the _OSI methods called by the firmware. Add the declaration
of acpi_gbl_osi_data to the OS headers and move the associated #defines in
order to make this possible.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/acpica/aclocal.h |   13 -------------
 include/acpi/acpixf.h         |   14 ++++++++++++++
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index 24b8faa..66f58f0 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -891,19 +891,6 @@ struct acpi_bit_register_info {
 
 #define ACPI_BITPOSITION_ARB_DISABLE            0x00
 
-/* Structs and definitions for _OSI support and I/O port validation */
-
-#define ACPI_OSI_WIN_2000               0x01
-#define ACPI_OSI_WIN_XP                 0x02
-#define ACPI_OSI_WIN_XP_SP1             0x03
-#define ACPI_OSI_WINSRV_2003            0x04
-#define ACPI_OSI_WIN_XP_SP2             0x05
-#define ACPI_OSI_WINSRV_2003_SP1        0x06
-#define ACPI_OSI_WIN_VISTA              0x07
-#define ACPI_OSI_WINSRV_2008            0x08
-#define ACPI_OSI_WIN_VISTA_SP1          0x09
-#define ACPI_OSI_WIN_7                  0x0A
-
 #define ACPI_ALWAYS_ILLEGAL             0x00
 
 struct acpi_interface_info {
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 847d262..fa0a8ef 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -66,9 +66,23 @@ extern u8 acpi_gbl_create_osi_method;
 extern u8 acpi_gbl_leave_wake_gpes_disabled;
 extern u8 acpi_gbl_use_default_register_widths;
 extern u8 acpi_gbl_ignore_high_ioport_bits;
+extern u8 acpi_gbl_osi_data;
 extern acpi_name acpi_gbl_trace_method_name;
 extern u32 acpi_gbl_trace_flags;
 
+/* Structs and definitions for _OSI support and I/O port validation */
+
+#define ACPI_OSI_WIN_2000               0x01
+#define ACPI_OSI_WIN_XP                 0x02
+#define ACPI_OSI_WIN_XP_SP1             0x03
+#define ACPI_OSI_WINSRV_2003            0x04
+#define ACPI_OSI_WIN_XP_SP2             0x05
+#define ACPI_OSI_WINSRV_2003_SP1        0x06
+#define ACPI_OSI_WIN_VISTA              0x07
+#define ACPI_OSI_WINSRV_2008            0x08
+#define ACPI_OSI_WIN_VISTA_SP1          0x09
+#define ACPI_OSI_WIN_7                  0x0A
+
 extern u32 acpi_current_gpe_count;
 extern struct acpi_table_fadt acpi_gbl_FADT;
 
-- 
1.7.0.1

Re: [PATCH 2/3] ACPI: Add acpi_gbl_osi_data to OS headers

[Len Brown]

> Different operating systems may wish to enable different workaround quirks
> depending on the _OSI methods called by the firmware.

I think that if this compatibility puzzle is solved,
then there is no reason that, say, BSD, would want to do something
different than Linux.

So I think it makes more sent to implement this workaround
inside ACPICA, and not in the Linux-specific code.

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH 3/3] ACPI: Enable Windows ioport access compatibility on Windows-compatible systems

[Matthew Garrett]

Windows ignores everything but the lower 16 bits of system io accesses.
Enable compatibility with it if the firmware indicates Windows compatibility
by requesting a version of Windows via the _OSI method.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/bus.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 743576b..a10144b 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -904,6 +904,14 @@ static int __init acpi_bus_init(void)
 		goto error1;
 	}
 
+	/*
+	 * _SB_._INI has been called, so any _OSI requests should now have
+	 * been completed - enable any OS-specific workarounds
+	 */
+
+	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_2000)
+		acpi_gbl_ignore_high_ioport_bits = TRUE;
+
 	acpi_early_processor_set_pdc();
 
 	/*
-- 
1.7.0.1

Re: [PATCH 3/3] ACPI: Enable Windows ioport access compatibility on Windows-compatible systems

[Bjorn Helgaas]

On Wednesday, May 19, 2010 09:43:11 am Matthew Garrett wrote:
> Windows ignores everything but the lower 16 bits of system io accesses.
> Enable compatibility with it if the firmware indicates Windows compatibility
> by requesting a version of Windows via the _OSI method.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/acpi/bus.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 743576b..a10144b 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -904,6 +904,14 @@ static int __init acpi_bus_init(void)
>  		goto error1;
>  	}
>  
> +	/*
> +	 * _SB_._INI has been called, so any _OSI requests should now have
> +	 * been completed - enable any OS-specific workarounds
> +	 */
> +
> +	if (acpi_gbl_osi_data >= ACPI_OSI_WIN_2000)
> +		acpi_gbl_ignore_high_ioport_bits = TRUE;

What's the basis for the Win 2000 check?  Is the intent that we
do this for all Windows versions?  Wikipedia claims Windows 98 had
ACPI support, but there's no ACPI_OSI_WIN_98 definition.

Is there a reason why we wouldn't just set ignore_high_ioport_bits = TRUE
always?

I'm sure you're doing this right; I'm just hoping for enough details
that if we ever *do* have a valid IO address that doesn't fit in 16
bits, we'll be able to accomodate that without breaking old boxes
again.

Bjorn

Re: [PATCH 3/3] ACPI: Enable Windows ioport access compatibility on Windows-compatible systems

[Matthew Garrett]

On Wed, May 19, 2010 at 10:25:58AM -0600, Bjorn Helgaas wrote:

> What's the basis for the Win 2000 check?  Is the intent that we
> do this for all Windows versions?  Wikipedia claims Windows 98 had
> ACPI support, but there's no ACPI_OSI_WIN_98 definition.

The aim is to do this for all Windows versions, but Windows < 2000 
didn't provide an _OSI method - instead there's a _OS string that the 
firmware can strcmp. It's not really practical to figure out what the 
firmware's looking for in that case, and given that nobody else has 
complained about this with luck we'll be fine.

> Is there a reason why we wouldn't just set ignore_high_ioport_bits = TRUE
> always?

My only concern is that there may be a machine that was never intended 
for use with Windows and which has an incorrect io port declared. In 
that case we'd /potentially/ break an otherwise working machine. I think 
the probability of this being the case is astronomically small, but it 
probably makes sense to check. If x86 gains more ioports in future and 
Windows supports that, we can limit the check to systems that don't 
claim support for that newer version of Windows.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/3] ACPI: Ignore the upper bits of SystemIO addresses

[Bjorn Helgaas]

On Wednesday, May 19, 2010 09:43:09 am Matthew Garrett wrote:
> Various machines (https://bugzilla.redhat.com/show_bug.cgi?id=585756 for
> instance) contain SystemIO spaces with addresses > 16 bits. acpica throws
> an error for this, while Windows silently ignores the upper 16 bits and
> carries on happily. Provide support for the latter behaviour for bug
> compatibility.
> 

> +	if (acpi_gbl_ignore_high_ioport_bits)
> +		/* Windows only uses the lower 16 bits of an address.
> +		   Emulate that */
> +		address &= 0xffff;

I think this is a good idea.  But it makes me a little bit nervous
to change addresses supplied by the firmware without any user-visible
indication at all.  Is it worth doing a WARN_ONCE() sort of thing
when we truncate?

I know you experimented quite a bit to confirm that Windows does this
sort of masking.  Do you have any notes about that experimentation
that would be useful to add to the bugzilla?

Bjorn

Re: [PATCH 1/3] ACPI: Ignore the upper bits of SystemIO addresses

[Matthew Garrett]

On Wed, May 19, 2010 at 10:18:46AM -0600, Bjorn Helgaas wrote:

> I think this is a good idea.  But it makes me a little bit nervous
> to change addresses supplied by the firmware without any user-visible
> indication at all.  Is it worth doing a WARN_ONCE() sort of thing
> when we truncate?

That doesn't seem unreasonable, but do we have anything equivalent to 
that in the acpica code right now?

> I know you experimented quite a bit to confirm that Windows does this
> sort of masking.  Do you have any notes about that experimentation
> that would be useful to add to the bugzilla?

Sure. I'll do that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [PATCH 1/3] ACPI: Ignore the upper bits of SystemIO addresses

[Moore, Robert]

No, ACPICA does not have a WARN_ONCE. In some places, we implement something similar by marking a namespace node, but that's all.

>-----Original Message-----
>From: Matthew Garrett [mailto:mjg@redhat.com]
>Sent: Wednesday, May 19, 2010 9:27 AM
>To: Bjorn Helgaas
>Cc: linux-acpi@vger.kernel.org; Moore, Robert; lenb@kernel.org
>Subject: Re: [PATCH 1/3] ACPI: Ignore the upper bits of SystemIO addresses
>
>On Wed, May 19, 2010 at 10:18:46AM -0600, Bjorn Helgaas wrote:
>
>> I think this is a good idea.  But it makes me a little bit nervous
>> to change addresses supplied by the firmware without any user-visible
>> indication at all.  Is it worth doing a WARN_ONCE() sort of thing
>> when we truncate?
>
>That doesn't seem unreasonable, but do we have anything equivalent to
>that in the acpica code right now?
>
>> I know you experimented quite a bit to confirm that Windows does this
>> sort of masking.  Do you have any notes about that experimentation
>> that would be useful to add to the bugzilla?
>
>Sure. I'll do that.
>
>--
>Matthew Garrett | mjg59@srcf.ucam.org