[PATCH 0/2] acpi_read() bit_offset support

[PATCH 0/2] acpi_read() bit_offset support

[Bjorn Helgaas]

We seem to be stuck in a morass of issues surrounding APEI and generic
address structures.  I don't know how to cut that Gordian knot, but
here's a possible tiny step forward.

I think we have general agreement that acpi_read() and friends *should*
pay attention to the generic address structure bit_offset, so these patches
add support for it.  It's done the same way as in the APEI accessors, so I
hope there's consensus that the semantics are correct.

We believe that today, only APEI uses generic address structures with
non-zero bit_offset (based on the fact that Google doesn't find any
"Unsupported register bit offset" warnings).  APEI currently uses its
own accessors, so these patches don't affect it.
---

Bjorn Helgaas (2):
      ACPICA: acpi_read: update return value atomically
      ACPICA: support Generic Address Structure bit_offset in acpi_read/write


 drivers/acpi/acpica/hwregs.c  |   32 ++++++++++++++++++++------------
 drivers/acpi/acpica/hwxface.c |   17 +++++++++++------
 2 files changed, 31 insertions(+), 18 deletions(-)

-- 
Bjorn

[PATCH 1/2] ACPICA: acpi_read: update return value atomically

[Bjorn Helgaas]

Accumulate the entire 64-bit value before updating the return_value.
Previously, it was possible to update the low 32 bits, then return
failure if reading the upper 32 bits failed, leaving a half-updated
return_value.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/acpica/hwxface.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpica/hwxface.c b/drivers/acpi/acpica/hwxface.c
index c2793a8..12b5c57 100644
--- a/drivers/acpi/acpica/hwxface.c
+++ b/drivers/acpi/acpica/hwxface.c
@@ -123,6 +123,7 @@ acpi_status acpi_read(u64 *return_value, struct acpi_generic_address *reg)
 	u32 value;
 	u32 width;
 	u64 address;
+	u64 complete_value;
 	acpi_status status;
 
 	ACPI_FUNCTION_NAME(acpi_read);
@@ -146,6 +147,7 @@ acpi_status acpi_read(u64 *return_value, struct acpi_generic_address *reg)
 	/* Initialize entire 64-bit return value to zero */
 
 	*return_value = 0;
+	complete_value = 0;
 	value = 0;
 
 	/*
@@ -158,7 +160,7 @@ acpi_status acpi_read(u64 *return_value, struct acpi_generic_address *reg)
 		if (ACPI_FAILURE(status)) {
 			return (status);
 		}
-		*return_value = value;
+		complete_value = value;
 
 		if (reg->bit_width == 64) {
 
@@ -169,7 +171,7 @@ acpi_status acpi_read(u64 *return_value, struct acpi_generic_address *reg)
 			if (ACPI_FAILURE(status)) {
 				return (status);
 			}
-			*return_value |= ((u64)value << 32);
+			complete_value |= ((u64)value << 32);
 		}
 	} else {		/* ACPI_ADR_SPACE_SYSTEM_IO, validated earlier */
 
@@ -178,7 +180,7 @@ acpi_status acpi_read(u64 *return_value, struct acpi_generic_address *reg)
 		if (ACPI_FAILURE(status)) {
 			return (status);
 		}
-		*return_value = value;
+		complete_value = value;
 
 		if (reg->bit_width == 64) {
 
@@ -189,10 +191,11 @@ acpi_status acpi_read(u64 *return_value, struct acpi_generic_address *reg)
 			if (ACPI_FAILURE(status)) {
 				return (status);
 			}
-			*return_value |= ((u64)value << 32);
+			complete_value |= ((u64)value << 32);
 		}
 	}
 
+	*return_value = complete_value;
 	ACPI_DEBUG_PRINT((ACPI_DB_IO,
 			  "Read:  %8.8X%8.8X width %2d from %8.8X%8.8X (%s)\n",
 			  ACPI_FORMAT_UINT64(*return_value), reg->bit_width,

[PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Bjorn Helgaas]

acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
ignore the GAS bit_offset field (but they do warn if it is non-zero).

APEI tables are starting to use non-zero bit_offsets.  APEI uses
special-purpose apei_exec_read_register() and apei_exec_write_register()
interfaces that apply the bit_offset.

This patch adds bit_offset support to the generic interfaces, which is
one small step toward using them instead of the special-purpose APEI ones.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/acpi/acpica/hwregs.c  |   32 ++++++++++++++++++++------------
 drivers/acpi/acpica/hwxface.c |    8 +++++---
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
index cc70f3f..2b50800 100644
--- a/drivers/acpi/acpica/hwregs.c
+++ b/drivers/acpi/acpica/hwregs.c
@@ -82,6 +82,7 @@ acpi_status
 acpi_hw_validate_register(struct acpi_generic_address *reg,
 			  u8 max_bit_width, u64 *address)
 {
+	static u8 width[] = {64, 8, 16, 32, 64};
 
 	/* Must have a valid pointer to a GAS structure */
 
@@ -119,12 +120,14 @@ acpi_hw_validate_register(struct acpi_generic_address *reg,
 		return (AE_SUPPORT);
 	}
 
-	/* Validate the bit_offset. Just a warning for now. */
+	/* Validate the bit_offset */
 
-	if (reg->bit_offset != 0) {
-		ACPI_WARNING((AE_INFO,
-			      "Unsupported register bit offset: 0x%X",
-			      reg->bit_offset));
+	if (reg->bit_offset > width[reg->access_width] - 1) {
+		ACPI_ERROR((AE_INFO,
+			   "Unsupported register bit offset: 0x%X access size: 0x%X (%d bits)",
+			   reg->bit_offset, reg->access_width,
+			   width[reg->access_width]));
+		return (AE_BAD_ADDRESS);
 	}
 
 	return (AE_OK);
@@ -146,14 +149,15 @@ acpi_hw_validate_register(struct acpi_generic_address *reg,
  * LIMITATIONS: <These limitations also apply to acpi_hw_write>
  *      bit_width must be exactly 8, 16, or 32.
  *      space_iD must be system_memory or system_iO.
- *      bit_offset and access_width are currently ignored, as there has
- *          not been a need to implement these.
+ *      access_width is currently ignored, as there has
+ *          not been a need to implement it.
  *
  ******************************************************************************/
 
-acpi_status acpi_hw_read(u32 *value, struct acpi_generic_address *reg)
+acpi_status acpi_hw_read(u32 *return_value, struct acpi_generic_address *reg)
 {
 	u64 address;
+	u32 value;
 	acpi_status status;
 
 	ACPI_FUNCTION_NAME(hw_read);
@@ -167,7 +171,7 @@ acpi_status acpi_hw_read(u32 *value, struct acpi_generic_address *reg)
 
 	/* Initialize entire 32-bit return value to zero */
 
-	*value = 0;
+	*return_value = 0;
 
 	/*
 	 * Two address spaces supported: Memory or IO. PCI_Config is
@@ -175,16 +179,18 @@ acpi_status acpi_hw_read(u32 *value, struct acpi_generic_address *reg)
 	 */
 	if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
 		status = acpi_os_read_memory((acpi_physical_address)
-					     address, value, reg->bit_width);
+					     address, &value, reg->bit_width);
 	} else {		/* ACPI_ADR_SPACE_SYSTEM_IO, validated earlier */
 
 		status = acpi_hw_read_port((acpi_io_address)
-					   address, value, reg->bit_width);
+					   address, &value, reg->bit_width);
 	}
 
+	*return_value = value >> reg->bit_offset;
 	ACPI_DEBUG_PRINT((ACPI_DB_IO,
 			  "Read:  %8.8X width %2d from %8.8X%8.8X (%s)\n",
-			  *value, reg->bit_width, ACPI_FORMAT_UINT64(address),
+			  *return_value, reg->bit_width,
+			  ACPI_FORMAT_UINT64(address),
 			  acpi_ut_get_region_name(reg->space_id)));
 
 	return (status);
@@ -219,6 +225,8 @@ acpi_status acpi_hw_write(u32 value, struct acpi_generic_address *reg)
 		return (status);
 	}
 
+	value = value << reg->bit_offset;
+
 	/*
 	 * Two address spaces supported: Memory or IO. PCI_Config is
 	 * not supported here because the GAS structure is insufficient
diff --git a/drivers/acpi/acpica/hwxface.c b/drivers/acpi/acpica/hwxface.c
index 12b5c57..a0526fe 100644
--- a/drivers/acpi/acpica/hwxface.c
+++ b/drivers/acpi/acpica/hwxface.c
@@ -114,8 +114,8 @@ ACPI_EXPORT_SYMBOL(acpi_reset)
  * LIMITATIONS: <These limitations also apply to acpi_write>
  *      bit_width must be exactly 8, 16, 32, or 64.
  *      space_iD must be system_memory or system_iO.
- *      bit_offset and access_width are currently ignored, as there has
- *          not been a need to implement these.
+ *      access_width is currently ignored, as there has
+ *          not been a need to implement it.
  *
  ******************************************************************************/
 acpi_status acpi_read(u64 *return_value, struct acpi_generic_address *reg)
@@ -195,7 +195,7 @@ acpi_status acpi_read(u64 *return_value, struct acpi_generic_address *reg)
 		}
 	}
 
-	*return_value = complete_value;
+	*return_value = complete_value >> reg->bit_offset;
 	ACPI_DEBUG_PRINT((ACPI_DB_IO,
 			  "Read:  %8.8X%8.8X width %2d from %8.8X%8.8X (%s)\n",
 			  ACPI_FORMAT_UINT64(*return_value), reg->bit_width,
@@ -239,6 +239,8 @@ acpi_status acpi_write(u64 value, struct acpi_generic_address *reg)
 		width = 32;	/* Break into two 32-bit transfers */
 	}
 
+	value = value << reg->bit_offset;
+
 	/*
 	 * Two address spaces supported: Memory or IO. PCI_Config is
 	 * not supported here because the GAS structure is insufficient

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Bjorn Helgaas]

On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> ignore the GAS bit_offset field (but they do warn if it is non-zero).
>
> APEI tables are starting to use non-zero bit_offsets.  APEI uses
> special-purpose apei_exec_read_register() and apei_exec_write_register()
> interfaces that apply the bit_offset.
>
> This patch adds bit_offset support to the generic interfaces, which is
> one small step toward using them instead of the special-purpose APEI ones.

Eww, brown paper bag time.  Just pretend you never saw this lame
implementation attempt.

I do think we need to make acpi_read() smart enough to extract a bit
field, but this try doesn't work.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Thomas Renninger]

On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
> >
> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
> > special-purpose apei_exec_read_register() and apei_exec_write_register()
> > interfaces that apply the bit_offset.
> >
> > This patch adds bit_offset support to the generic interfaces, which is
> > one small step toward using them instead of the special-purpose APEI ones.
> 
> Eww, brown paper bag time.  Just pretend you never saw this lame
> implementation attempt.
> 
> I do think we need to make acpi_read() smart enough to extract a bit
> field, but this try doesn't work.

As a first step it would be great if Ying's and Myron's patches which
afaik conflict get serialized and pushed into an "acpi branch".
What the status there?

I'd like to add access width support to the APEI parts on top then.

Thanks,

   Thomas

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Bjorn Helgaas]

On Tue, Nov 15, 2011 at 9:49 AM, Thomas Renninger <trenn@suse.de> wrote:
> On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
>> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
>> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
>> >
>> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
>> > special-purpose apei_exec_read_register() and apei_exec_write_register()
>> > interfaces that apply the bit_offset.
>> >
>> > This patch adds bit_offset support to the generic interfaces, which is
>> > one small step toward using them instead of the special-purpose APEI ones.
>>
>> Eww, brown paper bag time.  Just pretend you never saw this lame
>> implementation attempt.
>>
>> I do think we need to make acpi_read() smart enough to extract a bit
>> field, but this try doesn't work.
>
> As a first step it would be great if Ying's and Myron's patches which
> afaik conflict get serialized and pushed into an "acpi branch".
> What the status there?

Ying's patch ("Add RAM mapping support") fixes a real issue with using
EINJ, so we need to do something with it.  Myron's patches are a nice
refactoring, but as far as I know, they don't fix any current issues,
and there's still a lot of work to hash out how to handle bit_offset,
bit_width, and access_width.

I think we should regard Ying's patches as being first in line, and
Myron's as work in progress.  So I don't think we're ready to try to
combine them and resolve conflicts yet -- Myron just has to update his
work to follow whatever Ying does.

> I'd like to add access width support to the APEI parts on top then.

We should try very hard to treat APEI generic address structures the
same way as all others.  If that means some machines need firmware
upgrades or some sort of quirks to work around BIOS bugs, we might
have to accept that.  I think a single set of GAS accessors plus a few
quirks that fix the GAS structures is far better than having
APEI-specific GAS accessors that are basically tailored to a few
broken machines.

To make acpi_read/acpi_write truly generic, we really need to nail
down the semantics of GAS bit_offset, bit_width, and access_width.

It would be useful to know how Windows deals with those.  I don't
think we have convincing information about it (all I remember is
"here's a GAS that looks wrong, but Windows still works," but I don't
think we know exactly *what* Windows is doing, or even whether it
looks at the broken GAS).  If we could figure out a way to feed a
variety of structures to Windows and observe what happens with
something like qemu, I think we'd learn a lot.

Or maybe we could learn enough from a conversation with BIOS writers
about how they interpret the spec and what they expect to happen with
non-zero bit_offset and bit_width.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Thomas Renninger]

On Wednesday 16 November 2011 16:45:11 Bjorn Helgaas wrote:
> On Tue, Nov 15, 2011 at 9:49 AM, Thomas Renninger <trenn@suse.de> wrote:
> > On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
> >> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> >> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
> >> >
> >> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
> >> > special-purpose apei_exec_read_register() and apei_exec_write_register()
> >> > interfaces that apply the bit_offset.
> >> >
> >> > This patch adds bit_offset support to the generic interfaces, which is
> >> > one small step toward using them instead of the special-purpose APEI ones.
> >>
> >> Eww, brown paper bag time.  Just pretend you never saw this lame
> >> implementation attempt.
> >>
> >> I do think we need to make acpi_read() smart enough to extract a bit
> >> field, but this try doesn't work.
> >
> > As a first step it would be great if Ying's and Myron's patches which
> > afaik conflict get serialized and pushed into an "acpi branch".
> > What the status there?
> 
> Ying's patch ("Add RAM mapping support") fixes a real issue with using
> EINJ, so we need to do something with it.  Myron's patches are a nice
> refactoring, but as far as I know, they don't fix any current issues,
> and there's still a lot of work to hash out how to handle bit_offset,
> bit_width, and access_width.
> 
> I think we should regard Ying's patches as being first in line, and
> Myron's as work in progress.  So I don't think we're ready to try to
> combine them and resolve conflicts yet -- Myron just has to update his
> work to follow whatever Ying does.
> 
> > I'd like to add access width support to the APEI parts on top then.
> 
> We should try very hard to treat APEI generic address structures the
> same way as all others.  If that means some machines need firmware
> upgrades or some sort of quirks to work around BIOS bugs, we might
> have to accept that.  I think a single set of GAS accessors plus a few
> quirks that fix the GAS structures is far better than having
> APEI-specific GAS accessors that are basically tailored to a few
> broken machines.
That does not account that Windows possibly also has duplicated GAS
parsing code in their APEI subsystem/driver.

Most APEI GAS structures have the mask value which makes bit_width
needless/redundant.

It's not unlikely that: Windows only ignores
bit width on APEI GAS structures with a mask value (HEST table only?).
Then quirking specific BIOSes is a bad idea, we want to resolve this
in a generic way and the only possibility (as so often) could be to
do it the same way we expect Windows does it. And if they have
duplicated GAS parsing code, we might need it as well.
Therefore I would not rush with making APEI code using the generic
interfaces. Possibly it's even wrong and will never happen.

I'd like to see an APEI GAS parsing code being able to handle all
(say as much as possible) APEI tables correctly first.

> To make acpi_read/acpi_write truly generic, we really need to nail
> down the semantics of GAS bit_offset, bit_width, and access_width.
> 
> It would be useful to know how Windows deals with those.  I don't
> think we have convincing information about it (all I remember is
> "here's a GAS that looks wrong, but Windows still works," but I don't
> think we know exactly *what* Windows is doing, or even whether it
> looks at the broken GAS).  If we could figure out a way to feed a
> variety of structures to Windows and observe what happens with
> something like qemu, I think we'd learn a lot.
Sure, but we should not wait until someone (possibly never) comes up
with it, but make the APEI GAS parsing code to work with as much BIOSes
as possible in a generic way.
A 64 bit address cut down to 8 bits obviously is wrong. I agree that if
possible, such a condition should get checked and a firmware bug message
is appropriate.

> Or maybe we could learn enough from a conversation with BIOS writers
> about how they interpret the spec and what they expect to happen with
> non-zero bit_offset and bit_width.
You could try at a conference after some beers... This is extra difficult,
because of the NDA coverage of the Whea spec.

   Thomas

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Bjorn Helgaas]

On Wed, Nov 16, 2011 at 12:58 PM, Thomas Renninger <trenn@suse.de> wrote:
> On Wednesday 16 November 2011 16:45:11 Bjorn Helgaas wrote:

>> We should try very hard to treat APEI generic address structures the
>> same way as all others.  If that means some machines need firmware
>> upgrades or some sort of quirks to work around BIOS bugs, we might
>> have to accept that.  I think a single set of GAS accessors plus a few
>> quirks that fix the GAS structures is far better than having
>> APEI-specific GAS accessors that are basically tailored to a few
>> broken machines.
> That does not account that Windows possibly also has duplicated GAS
> parsing code in their APEI subsystem/driver.
>
> Most APEI GAS structures have the mask value which makes bit_width
> needless/redundant.
>
> It's not unlikely that: Windows only ignores
> bit width on APEI GAS structures with a mask value (HEST table only?).
> Then quirking specific BIOSes is a bad idea, we want to resolve this
> in a generic way and the only possibility (as so often) could be to
> do it the same way we expect Windows does it. And if they have
> duplicated GAS parsing code, we might need it as well.
> Therefore I would not rush with making APEI code using the generic
> interfaces. Possibly it's even wrong and will never happen.
>
> I'd like to see an APEI GAS parsing code being able to handle all
> (say as much as possible) APEI tables correctly first.

There's a lot of speculation above about Windows duplicating GAS
parsing, using mask instead of bit_width, etc.  I don't think we have
good evidence for it.  In general, my experience is that Windows has a
pretty high-quality ACPI implementation (though obviously it sometimes
interprets things differently than Linux), so I hesitate to assume
special cases like this in Windows.

APEI support in BIOSes is immature, and I don't want to hard-code
assumptions into Linux based on a few possibly defective BIOSes.
Assumptions like that will constrain us in the future.

>> Or maybe we could learn enough from a conversation with BIOS writers
>> about how they interpret the spec and what they expect to happen with
>> non-zero bit_offset and bit_width.
> You could try at a conference after some beers... This is extra difficult,
> because of the NDA coverage of the Whea spec.

I disagree.  The simple question "how do you expect the OS to
interpret this GAS?" has nothing to do with any Windows NDA.

You seem to have a nice collection of DSDT/APEI/etc tables.  Can you
get any idea of how many of them contain GAS structures that would
break under my proposal
(https://docs.google.com/spreadsheet/ccc?key=0AjvKas55tQpqdElKVXM2TEFVcnI3SjVuZ1pqUENMN1E)?

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Thomas Renninger]

On Thursday, November 17, 2011 01:15:28 AM Bjorn Helgaas wrote:
...
> There's a lot of speculation above about Windows duplicating GAS
> parsing, using mask instead of bit_width, etc.  I don't think we have
> good evidence for it.  In general, my experience is that Windows has a
> pretty high-quality ACPI implementation (though obviously it sometimes
> interprets things differently than Linux), so I hesitate to assume
> special cases like this in Windows.
> 
> APEI support in BIOSes is immature, and I don't want to hard-code
> assumptions into Linux based on a few possibly defective BIOSes.
> Assumptions like that will constrain us in the future.
I agree.
But as long as we do not have more (detailed) info how things work
on Windows or how BIOS writers expect things to work, I recommend
to *not* merge the GAR parsing APEI parts into a generic layer.

Changing, blacklisting or working around in ACPICA is cumbersome and
if extra patches need to go into stable@ it's not nice to handle
if things are in ACPICA.

Also as the code is not much and only used at one single place,
I do not see the urgent need for generalizing it.

...
> You seem to have a nice collection of DSDT/APEI/etc tables.  Can you
> get any idea of how many of them contain GAS structures that would
> break under my proposal
> (https://docs.google.com/spreadsheet/ccc?key=0AjvKas55tQpqdElKVXM2TEFVcnI3SjVuZ1pqUENMN1E)?
Nice!
Imo all finally decalred "Illegal" rows should end up in a
"FIRMWARE BUG" messages, maybe some which could get
workarounded should also be marked as such.

-> ACPICA is still lacking a FW_BUG marker as Linux has.
Would an additional macro like:
include/acpi/acoutput.h:#define ACPI_ERROR(plist)               acpi_error plist
which may look like:
include/acpi/acoutput.h:#define ACPI_FW_BUG(plist)              acpi_fw_bug plist
and which starts with:
FIRMWARE BUG - ACPI...
be acceptable in the ACPICA sources?

It's not that I want to go through all messages and adjust
them. But it would be great to get a starting and convert a message
if it shows up in a kernel log buffer and clearly is a BIOS bug.


Some examples which would break:

Wrong bit width
---------------
bit width - access width (in bits and hex) ...
08        - 40    Bit width not equal Access width - Mask: FFFFFFFFFFFFFFFF
08        - 20    Bit width not equal Access width - Mask: 00000000FFFFFFFF
I've seen these on 2 totally different platforms,
might still be the same source (copy and pasted or whatever).
Above are:
Get Error Address Range
and
Get Error Address Length
which are both cut down to 8 bit right now.
Both should work fine when the mask and/or access width value
is used to determine the relevant bits instead of bit width.
-> This case definitely should issue a message pointing to broken
Firmware.

Not aligned bit width
---------------------
bit width - access width (in bits and hex) ...
03        - 20    Bit width not equal Access width - Mask: 0000000000000007
03        - 20    Bit width not equal Access width - Mask: 0000000000000007
03        - 20    Bit width not equal Access width - Mask: 0000000000000007
03        - 20    Bit width not equal Access width - Mask: 0000000000000007
18        - 20    Bit width not equal Access width - Mask: 0000000000FFFFFF
01        - 20    Bit width not equal Access width - Mask: 0000000000000001
Should get handled correctly with your approach.
I've only seen this on (older?) platforms of a specific vendor.
But on several, and if, clustered all through ERST.dsl.

bit width greater than access width
-----------------------------------
bit width - access width (in bits and hex) ...
40        - 10    Bit width not equal Access width - Mask: 000000000000FFFF
The bit width of 0x40 is weird, no idea what it should be good
for. But your approach should handle this correctly.
Is it worth a firmware warning message?


Other "not that normal" GARs
----------------------------
bit width - access width (in bits and hex) ...
20        - 10    Bit width not equal Access width - Mask: FFFFFFFFFFFFFFFF
Same as above, but with a 64 bit mask.
Might be a good candidate for finding out intended behavior
(if things are correctly (as programmers intended) used on Windows (if at all)).
Your approach would take 0x20 bits...
Hm, better do not count this one, may be an early BIOS (another
bug is in there, an action value marked as "reserved" in the spec
is used...).
I'll update the BIOS and boot a recent kernel if I find the
time.

Everything is zero
------------------
Not mentioned in your table. Only saw it once:
access, bit width, offset and mask value are all zero
-> May come from some code generating the GARs, should just
get ignored without any warning?

Hope that helps.

For now it would be great if the one or other patch
would show up in a acpi-next branch.
Not sure if I miss something, but no ACPI patches for 3.3 were
pushed into any (linux-next) tree yet?
I could imagine it's not that easy to merge the spec 5.0
parts and that's the reason nothing has shown up there yet?

Still, hopefully something shows up there soon, as Myron's
and Ying's patches need to get serialized and before this
does not happen it's not worth writing any code which
potentially conflicts with other pending patches.

   Thomas

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Rafael J. Wysocki]

On Wednesday, November 16, 2011, Bjorn Helgaas wrote:
> On Tue, Nov 15, 2011 at 9:49 AM, Thomas Renninger <trenn@suse.de> wrote:
> > On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
> >> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> >> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
> >> >
> >> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
> >> > special-purpose apei_exec_read_register() and apei_exec_write_register()
> >> > interfaces that apply the bit_offset.
> >> >
> >> > This patch adds bit_offset support to the generic interfaces, which is
> >> > one small step toward using them instead of the special-purpose APEI ones.
> >>
> >> Eww, brown paper bag time.  Just pretend you never saw this lame
> >> implementation attempt.
> >>
> >> I do think we need to make acpi_read() smart enough to extract a bit
> >> field, but this try doesn't work.
> >
> > As a first step it would be great if Ying's and Myron's patches which
> > afaik conflict get serialized and pushed into an "acpi branch".
> > What the status there?
> 
> Ying's patch ("Add RAM mapping support") fixes a real issue with using
> EINJ, so we need to do something with it.

I kind of agree, but I wonder if page_is_ram() is the right check?

> Myron's patches are a nice
> refactoring, but as far as I know, they don't fix any current issues,
> and there's still a lot of work to hash out how to handle bit_offset,
> bit_width, and access_width.
> 
> I think we should regard Ying's patches as being first in line, and
> Myron's as work in progress.  So I don't think we're ready to try to
> combine them and resolve conflicts yet -- Myron just has to update his
> work to follow whatever Ying does.

Agreed.

> > I'd like to add access width support to the APEI parts on top then.
> 
> We should try very hard to treat APEI generic address structures the
> same way as all others.  If that means some machines need firmware
> upgrades or some sort of quirks to work around BIOS bugs, we might
> have to accept that.  I think a single set of GAS accessors plus a few
> quirks that fix the GAS structures is far better than having
> APEI-specific GAS accessors that are basically tailored to a few
> broken machines.

We can use APEI-specific wrappers around generic GAS accessors, though.

> To make acpi_read/acpi_write truly generic, we really need to nail
> down the semantics of GAS bit_offset, bit_width, and access_width.
> 
> It would be useful to know how Windows deals with those.  I don't
> think we have convincing information about it (all I remember is
> "here's a GAS that looks wrong, but Windows still works," but I don't
> think we know exactly *what* Windows is doing, or even whether it
> looks at the broken GAS).  If we could figure out a way to feed a
> variety of structures to Windows and observe what happens with
> something like qemu, I think we'd learn a lot.

Perhaps.

> Or maybe we could learn enough from a conversation with BIOS writers
> about how they interpret the spec and what they expect to happen with
> non-zero bit_offset and bit_width.

I'm a bit skeptic about that, so to speak. ;-)

Thanks,
Rafael

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Bjorn Helgaas]

On Wed, Nov 16, 2011 at 4:27 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, November 16, 2011, Bjorn Helgaas wrote:

>> We should try very hard to treat APEI generic address structures the
>> same way as all others.  If that means some machines need firmware
>> upgrades or some sort of quirks to work around BIOS bugs, we might
>> have to accept that.  I think a single set of GAS accessors plus a few
>> quirks that fix the GAS structures is far better than having
>> APEI-specific GAS accessors that are basically tailored to a few
>> broken machines.
>
> We can use APEI-specific wrappers around generic GAS accessors, though.

Yes, but IMHO, we should only need wrappers to support APEI-specific
functionality, e.g., the mask.

Here's an attempt to lay out how I think acpi_read() and acpi_write()
*should* work:

https://docs.google.com/spreadsheet/ccc?key=0AjvKas55tQpqdElKVXM2TEFVcnI3SjVuZ1pqUENMN1E

I think the proposed behavior is compatible with the APEI assumptions
in the current code.  Please comment :)

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Rafael J. Wysocki]

On Thursday, November 17, 2011, Bjorn Helgaas wrote:
> On Wed, Nov 16, 2011 at 4:27 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, November 16, 2011, Bjorn Helgaas wrote:
> 
> >> We should try very hard to treat APEI generic address structures the
> >> same way as all others.  If that means some machines need firmware
> >> upgrades or some sort of quirks to work around BIOS bugs, we might
> >> have to accept that.  I think a single set of GAS accessors plus a few
> >> quirks that fix the GAS structures is far better than having
> >> APEI-specific GAS accessors that are basically tailored to a few
> >> broken machines.
> >
> > We can use APEI-specific wrappers around generic GAS accessors, though.
> 
> Yes, but IMHO, we should only need wrappers to support APEI-specific
> functionality, e.g., the mask.
> 
> Here's an attempt to lay out how I think acpi_read() and acpi_write()
> *should* work:
> 
> https://docs.google.com/spreadsheet/ccc?key=0AjvKas55tQpqdElKVXM2TEFVcnI3SjVuZ1pqUENMN1E
> 
> I think the proposed behavior is compatible with the APEI assumptions
> in the current code.  Please comment :)

Well, I'd express access_width in multiples of 8.  Or perhaps use a mask?

I'm afraid it may be difficult to get right by the callers, however.

Thanks,
Rafael

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Huang Ying]

On Thu, 2011-11-17 at 07:27 +0800, Rafael J. Wysocki wrote:
> On Wednesday, November 16, 2011, Bjorn Helgaas wrote:
> > On Tue, Nov 15, 2011 at 9:49 AM, Thomas Renninger <trenn@suse.de> wrote:
> > > On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
> > >> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > >> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> > >> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
> > >> >
> > >> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
> > >> > special-purpose apei_exec_read_register() and apei_exec_write_register()
> > >> > interfaces that apply the bit_offset.
> > >> >
> > >> > This patch adds bit_offset support to the generic interfaces, which is
> > >> > one small step toward using them instead of the special-purpose APEI ones.
> > >>
> > >> Eww, brown paper bag time.  Just pretend you never saw this lame
> > >> implementation attempt.
> > >>
> > >> I do think we need to make acpi_read() smart enough to extract a bit
> > >> field, but this try doesn't work.
> > >
> > > As a first step it would be great if Ying's and Myron's patches which
> > > afaik conflict get serialized and pushed into an "acpi branch".
> > > What the status there?
> > 
> > Ying's patch ("Add RAM mapping support") fixes a real issue with using
> > EINJ, so we need to do something with it.
> 
> I kind of agree, but I wonder if page_is_ram() is the right check?

page_is_ram() is used by x86 ioremap implementation to exclude RAM
range.  So I think it can be used here.

Best Regards,
Huang Ying

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Rafael J. Wysocki]

On Thursday, November 17, 2011, Huang Ying wrote:
> On Thu, 2011-11-17 at 07:27 +0800, Rafael J. Wysocki wrote:
> > On Wednesday, November 16, 2011, Bjorn Helgaas wrote:
> > > On Tue, Nov 15, 2011 at 9:49 AM, Thomas Renninger <trenn@suse.de> wrote:
> > > > On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
> > > >> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > > >> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> > > >> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
> > > >> >
> > > >> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
> > > >> > special-purpose apei_exec_read_register() and apei_exec_write_register()
> > > >> > interfaces that apply the bit_offset.
> > > >> >
> > > >> > This patch adds bit_offset support to the generic interfaces, which is
> > > >> > one small step toward using them instead of the special-purpose APEI ones.
> > > >>
> > > >> Eww, brown paper bag time.  Just pretend you never saw this lame
> > > >> implementation attempt.
> > > >>
> > > >> I do think we need to make acpi_read() smart enough to extract a bit
> > > >> field, but this try doesn't work.
> > > >
> > > > As a first step it would be great if Ying's and Myron's patches which
> > > > afaik conflict get serialized and pushed into an "acpi branch".
> > > > What the status there?
> > > 
> > > Ying's patch ("Add RAM mapping support") fixes a real issue with using
> > > EINJ, so we need to do something with it.
> > 
> > I kind of agree, but I wonder if page_is_ram() is the right check?
> 
> page_is_ram() is used by x86 ioremap implementation to exclude RAM
> range.  So I think it can be used here.

Except that ACPI is not going to be x86-specific any more in the (near?)
future.  Have you taken that into consideration?

Rafael

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Thomas Renninger]

On Thursday 17 November 2011 21:27:40 Rafael J. Wysocki wrote:
> On Thursday, November 17, 2011, Huang Ying wrote:
... 
> > page_is_ram() is used by x86 ioremap implementation to exclude RAM
> > range.  So I think it can be used here.
> 
> Except that ACPI is not going to be x86-specific any more in the (near?)
> future.  Have you taken that into consideration?
This is about the NVS ram resource registering/requesting problem?
I had an idea to make the resource management more fine grained.
There already is a possibility to pass "flags" to request_mem_region macro:
include/linux/ioport.h:
#define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
#define request_muxed_region(start,n,name)    __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
...


The idea is to e.g. flag memory with:
  MEM_NVS
  MEM_RESERVED
  (MEM_PCI?)
  other archs may have other memory flags
  ...
once it got detected/initialized at early boot.

and then have something like:
request_specific_mem_region(start, n, name, (MEM_NVS | MEM_RESERVED))
for apei (or others allowed to access NVS mem) which is only successful
if the requested memory region has been marked as NVS/RESERVED memory.

This is nothing I have time for right now, therefore don't get
the mail wrong: I don't object to adding any other working solution
discussed here.
Just an idea how this could be solved in a cleaner way,
also re-usable for others.

What do you think?

       Thomas

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Rafael J. Wysocki]

On Friday, November 18, 2011, Thomas Renninger wrote:
> On Thursday 17 November 2011 21:27:40 Rafael J. Wysocki wrote:
> > On Thursday, November 17, 2011, Huang Ying wrote:
> ... 
> > > page_is_ram() is used by x86 ioremap implementation to exclude RAM
> > > range.  So I think it can be used here.
> > 
> > Except that ACPI is not going to be x86-specific any more in the (near?)
> > future.  Have you taken that into consideration?
> This is about the NVS ram resource registering/requesting problem?
> I had an idea to make the resource management more fine grained.
> There already is a possibility to pass "flags" to request_mem_region macro:
> include/linux/ioport.h:
> #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
> #define request_muxed_region(start,n,name)    __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
> ...
> 
> 
> The idea is to e.g. flag memory with:
>   MEM_NVS
>   MEM_RESERVED
>   (MEM_PCI?)
>   other archs may have other memory flags
>   ...
> once it got detected/initialized at early boot.
> 
> and then have something like:
> request_specific_mem_region(start, n, name, (MEM_NVS | MEM_RESERVED))
> for apei (or others allowed to access NVS mem) which is only successful
> if the requested memory region has been marked as NVS/RESERVED memory.
> 
> This is nothing I have time for right now, therefore don't get
> the mail wrong: I don't object to adding any other working solution
> discussed here.
> Just an idea how this could be solved in a cleaner way,
> also re-usable for others.
> 
> What do you think?

That sounds like a good idea in principle, it would save us from some ugly
hacks here and there I think. :-)

Thanks,
Rafael

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Huang Ying]

On Fri, 2011-11-18 at 04:27 +0800, Rafael J. Wysocki wrote:
> On Thursday, November 17, 2011, Huang Ying wrote:
> > On Thu, 2011-11-17 at 07:27 +0800, Rafael J. Wysocki wrote:
> > > On Wednesday, November 16, 2011, Bjorn Helgaas wrote:
> > > > On Tue, Nov 15, 2011 at 9:49 AM, Thomas Renninger <trenn@suse.de> wrote:
> > > > > On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
> > > > >> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > > > >> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> > > > >> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
> > > > >> >
> > > > >> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
> > > > >> > special-purpose apei_exec_read_register() and apei_exec_write_register()
> > > > >> > interfaces that apply the bit_offset.
> > > > >> >
> > > > >> > This patch adds bit_offset support to the generic interfaces, which is
> > > > >> > one small step toward using them instead of the special-purpose APEI ones.
> > > > >>
> > > > >> Eww, brown paper bag time.  Just pretend you never saw this lame
> > > > >> implementation attempt.
> > > > >>
> > > > >> I do think we need to make acpi_read() smart enough to extract a bit
> > > > >> field, but this try doesn't work.
> > > > >
> > > > > As a first step it would be great if Ying's and Myron's patches which
> > > > > afaik conflict get serialized and pushed into an "acpi branch".
> > > > > What the status there?
> > > > 
> > > > Ying's patch ("Add RAM mapping support") fixes a real issue with using
> > > > EINJ, so we need to do something with it.
> > > 
> > > I kind of agree, but I wonder if page_is_ram() is the right check?
> > 
> > page_is_ram() is used by x86 ioremap implementation to exclude RAM
> > range.  So I think it can be used here.
> 
> Except that ACPI is not going to be x86-specific any more in the (near?)
> future.  Have you taken that into consideration?

Take a look at ARM ioremap implementation.  It appears that RAM can be
ioremaped on ARM.  But this changes should be harmless for ARM too.
Because ioremap implementation is different between different
architecture, maybe we should use #ifdef CONFIG_X86, #endif to enclose
the code?

Best Regards,
Huang Ying

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Rafael J. Wysocki]

On Friday, November 18, 2011, Huang Ying wrote:
> On Fri, 2011-11-18 at 04:27 +0800, Rafael J. Wysocki wrote:
> > On Thursday, November 17, 2011, Huang Ying wrote:
> > > On Thu, 2011-11-17 at 07:27 +0800, Rafael J. Wysocki wrote:
> > > > On Wednesday, November 16, 2011, Bjorn Helgaas wrote:
> > > > > On Tue, Nov 15, 2011 at 9:49 AM, Thomas Renninger <trenn@suse.de> wrote:
> > > > > > On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
> > > > > >> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > > > > >> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> > > > > >> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
> > > > > >> >
> > > > > >> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
> > > > > >> > special-purpose apei_exec_read_register() and apei_exec_write_register()
> > > > > >> > interfaces that apply the bit_offset.
> > > > > >> >
> > > > > >> > This patch adds bit_offset support to the generic interfaces, which is
> > > > > >> > one small step toward using them instead of the special-purpose APEI ones.
> > > > > >>
> > > > > >> Eww, brown paper bag time.  Just pretend you never saw this lame
> > > > > >> implementation attempt.
> > > > > >>
> > > > > >> I do think we need to make acpi_read() smart enough to extract a bit
> > > > > >> field, but this try doesn't work.
> > > > > >
> > > > > > As a first step it would be great if Ying's and Myron's patches which
> > > > > > afaik conflict get serialized and pushed into an "acpi branch".
> > > > > > What the status there?
> > > > > 
> > > > > Ying's patch ("Add RAM mapping support") fixes a real issue with using
> > > > > EINJ, so we need to do something with it.
> > > > 
> > > > I kind of agree, but I wonder if page_is_ram() is the right check?
> > > 
> > > page_is_ram() is used by x86 ioremap implementation to exclude RAM
> > > range.  So I think it can be used here.
> > 
> > Except that ACPI is not going to be x86-specific any more in the (near?)
> > future.  Have you taken that into consideration?
> 
> Take a look at ARM ioremap implementation.  It appears that RAM can be
> ioremaped on ARM.  But this changes should be harmless for ARM too.
> Because ioremap implementation is different between different
> architecture, maybe we should use #ifdef CONFIG_X86, #endif to enclose
> the code?

Well, that would be a bit hackish, wouldn't it?

If the code is going to work on all architectures that _may_ use it in the
future (x86, ARM, ia64 so far), there's no reason to change it.  My question
was whether or not you double checked that.

Rafael

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Huang Ying]

On Fri, 2011-11-18 at 17:25 +0800, Rafael J. Wysocki wrote:
> On Friday, November 18, 2011, Huang Ying wrote:
> > On Fri, 2011-11-18 at 04:27 +0800, Rafael J. Wysocki wrote:
> > > On Thursday, November 17, 2011, Huang Ying wrote:
> > > > On Thu, 2011-11-17 at 07:27 +0800, Rafael J. Wysocki wrote:
> > > > > On Wednesday, November 16, 2011, Bjorn Helgaas wrote:
> > > > > > On Tue, Nov 15, 2011 at 9:49 AM, Thomas Renninger <trenn@suse.de> wrote:
> > > > > > > On Tuesday, November 15, 2011 04:13:15 PM Bjorn Helgaas wrote:
> > > > > > >> On Fri, Nov 11, 2011 at 4:05 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > > > > > >> > acpi_read(), acpi_write(), acpi_hw_read(), and acpi_hw_write() currently
> > > > > > >> > ignore the GAS bit_offset field (but they do warn if it is non-zero).
> > > > > > >> >
> > > > > > >> > APEI tables are starting to use non-zero bit_offsets.  APEI uses
> > > > > > >> > special-purpose apei_exec_read_register() and apei_exec_write_register()
> > > > > > >> > interfaces that apply the bit_offset.
> > > > > > >> >
> > > > > > >> > This patch adds bit_offset support to the generic interfaces, which is
> > > > > > >> > one small step toward using them instead of the special-purpose APEI ones.
> > > > > > >>
> > > > > > >> Eww, brown paper bag time.  Just pretend you never saw this lame
> > > > > > >> implementation attempt.
> > > > > > >>
> > > > > > >> I do think we need to make acpi_read() smart enough to extract a bit
> > > > > > >> field, but this try doesn't work.
> > > > > > >
> > > > > > > As a first step it would be great if Ying's and Myron's patches which
> > > > > > > afaik conflict get serialized and pushed into an "acpi branch".
> > > > > > > What the status there?
> > > > > > 
> > > > > > Ying's patch ("Add RAM mapping support") fixes a real issue with using
> > > > > > EINJ, so we need to do something with it.
> > > > > 
> > > > > I kind of agree, but I wonder if page_is_ram() is the right check?
> > > > 
> > > > page_is_ram() is used by x86 ioremap implementation to exclude RAM
> > > > range.  So I think it can be used here.
> > > 
> > > Except that ACPI is not going to be x86-specific any more in the (near?)
> > > future.  Have you taken that into consideration?
> > 
> > Take a look at ARM ioremap implementation.  It appears that RAM can be
> > ioremaped on ARM.  But this changes should be harmless for ARM too.
> > Because ioremap implementation is different between different
> > architecture, maybe we should use #ifdef CONFIG_X86, #endif to enclose
> > the code?
> 
> Well, that would be a bit hackish, wouldn't it?
> 
> If the code is going to work on all architectures that _may_ use it in the
> future (x86, ARM, ia64 so far), there's no reason to change it.  My question
> was whether or not you double checked that.

Take a look at the page_is_ram() implementation and ioremap()
implementation for ARM and ia64.  It appears that the patch should work
on these architecture.  Add Tony and Russell to loop to confirm it.

Hi, Tony and Russel,

Do you have time to take a look at the following patch:

https://lkml.org/lkml/2011/11/7/567

to check whether acpi_map/unmap implemented in patch works on ia64 and
ARM?

Thanks,
Huang Ying

Re: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Russell King - ARM Linux]

On Mon, Nov 21, 2011 at 03:51:20PM +0800, Huang Ying wrote:
> On Fri, 2011-11-18 at 17:25 +0800, Rafael J. Wysocki wrote:
> > On Friday, November 18, 2011, Huang Ying wrote:
> > > Take a look at ARM ioremap implementation.  It appears that RAM can be
> > > ioremaped on ARM.  But this changes should be harmless for ARM too.
> > > Because ioremap implementation is different between different
> > > architecture, maybe we should use #ifdef CONFIG_X86, #endif to enclose
> > > the code?

I think you're wrong there.

void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
        unsigned long offset, size_t size, unsigned int mtype, void *caller)
{
...
        /*
         * Don't allow RAM to be mapped - this causes problems with ARMv6+
         */
        if (WARN_ON(pfn_valid(pfn)))
                return NULL;

While we don't exhaustively check, this catches most offenders trying to
use ioremap() on system RAM.

> > Well, that would be a bit hackish, wouldn't it?
> > 
> > If the code is going to work on all architectures that _may_ use it in the
> > future (x86, ARM, ia64 so far), there's no reason to change it.  My question
> > was whether or not you double checked that.
> 
> Take a look at the page_is_ram() implementation and ioremap()
> implementation for ARM and ia64.  It appears that the patch should work
> on these architecture.  Add Tony and Russell to loop to confirm it.

Even though I've not heard of anyone wanting to use ACPI on ARM, I'm not
worried at all about this.  However, it does look harmless for ARM even
if someone wanted to use ACPI (and in any case kmapping RAM is much better
than ioremapping it.)

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Luck, Tony]

> Do you have time to take a look at the following patch:
>
> https://lkml.org/lkml/2011/11/7/567
>
> to check whether acpi_map/unmap implemented in patch works on ia64 and
> ARM?

Ia64 won't care - it doesn't use (and won't ever use) any of these ACPI methods.
It has some PAL functions for error injection.

-Tony

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Huang Ying]

On Tue, 2011-11-29 at 07:03 +0800, Luck, Tony wrote:
> > Do you have time to take a look at the following patch:
> >
> > https://lkml.org/lkml/2011/11/7/567
> >
> > to check whether acpi_map/unmap implemented in patch works on ia64 and
> > ARM?
> 
> Ia64 won't care - it doesn't use (and won't ever use) any of these ACPI methods.
> It has some PAL functions for error injection.

Thanks.

But as this code may be used in general acpi_read/write path too in the
future, can you verify whether it is harmless on ia64?

Best Regards,
Huang Ying

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Luck, Tony]

> But as this code may be used in general acpi_read/write path too in the
> future, can you verify whether it is harmless on ia64?

This might be a problem on ia64 - it is s/w responsibility to make sure
that we don't map the same underlying physical address using different
cache attributes - e.g. we must not map memory both cacheable and uncacheable
at the same time.  Accessing such a mis-attributed page will result in a
machine check.

So I'd worry that if the memory in question was being used as uncacheable,
this code might result in a cached access, which would crash the machine.

-Tony

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Huang Ying]

On Thu, 2011-12-01 at 05:54 +0800, Luck, Tony wrote:
> > But as this code may be used in general acpi_read/write path too in the
> > future, can you verify whether it is harmless on ia64?
> 
> This might be a problem on ia64 - it is s/w responsibility to make sure
> that we don't map the same underlying physical address using different
> cache attributes - e.g. we must not map memory both cacheable and uncacheable
> at the same time.  Accessing such a mis-attributed page will result in a
> machine check.
> 
> So I'd worry that if the memory in question was being used as uncacheable,
> this code might result in a cached access, which would crash the machine.

+static void __iomem *acpi_map(phys_addr_t pg_off, unsigned long pg_sz)
+{
+	unsigned long pfn;
+
+	pfn = pg_off >> PAGE_SHIFT;
+	if (page_is_ram(pfn)) {
+		if (pg_sz > PAGE_SIZE)
+			return NULL;
+		return (void __iomem __force *)kmap(pfn_to_page(pfn));
+	} else
+		return ioremap(pg_off, pg_sz);
+}

Is it possible to use the page_is_ram() and kamp() path in the patch to avoid the situation you mentioned?

Best Regards,
Huang Ying

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Luck, Tony]

> Is it possible to use the page_is_ram() and kamp() path in the patch to avoid the situation you mentioned?

Maybe - it could certainly check the current attributes and match them.

Not sure whether there might be a gap if the acpi mapping is long-lived,
and the other kernel mappings change ... though this is very rare to
switch attributes

-Tony

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Huang Ying]

On Thu, 2011-12-01 at 08:53 +0800, Luck, Tony wrote:
> > Is it possible to use the page_is_ram() and kamp() path in the patch to avoid the situation you mentioned?
> 
> Maybe - it could certainly check the current attributes and match them.
> 
> Not sure whether there might be a gap if the acpi mapping is long-lived,
> and the other kernel mappings change ... though this is very rare to
> switch attributes

In the original acpi_read/write, we just call ioremap blindly, so the
code in the below patch will not introduce new issue for ia64.  Do you
agree?

https://lkml.org/lkml/2011/11/7/567

Best Regards,
Huang Ying

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Luck, Tony]

> In the original acpi_read/write, we just call ioremap blindly, so the
> code in the below patch will not introduce new issue for ia64.  Do you
> agree?

See arch/ia64/mm/ioremap.c ... ioremap() on ia64 is smart and looks
at attributes and makes new mapping match existing ones

That doesn't deal with long-lived mappings and changes that I just
mentioned in last e-mail - but it does mean that callers of ioremap()
don't need to worry ... they can call "blindly".

-Tony

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Huang Ying]

On Thu, 2011-12-01 at 09:03 +0800, Luck, Tony wrote:
> > In the original acpi_read/write, we just call ioremap blindly, so the
> > code in the below patch will not introduce new issue for ia64.  Do you
> > agree?
> 
> See arch/ia64/mm/ioremap.c ... ioremap() on ia64 is smart and looks
> at attributes and makes new mapping match existing ones
> 
> That doesn't deal with long-lived mappings and changes that I just
> mentioned in last e-mail - but it does mean that callers of ioremap()
> don't need to worry ... they can call "blindly".

I see.  So you think it is better to enclose page_is_ram() with #ifndef
CONFIG_IA64 in the patch, so that only ioremap() is used?

Best Regards,
Huang Ying

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Luck, Tony]

>> That doesn't deal with long-lived mappings and changes that I just
>> mentioned in last e-mail - but it does mean that callers of ioremap()
>> don't need to worry ... they can call "blindly".
>
> I see.  So you think it is better to enclose page_is_ram() with #ifndef
> CONFIG_IA64 in the patch, so that only ioremap() is used?

Functionally yes - though the aversion to #ifdef in the middle of code
would imply some extra hoops.  Perhaps

#ifndef CONFIG_IA64
#define can_use_kmap(pfn) page_is_ram(pfn)
#else
#define can_use_kmap(pfn) (0) /* ioremap() will take care of cache attributes */
#endif


...

		if (can_use_kmap(pfn)) {


-Tony

RE: [PATCH 2/2] ACPICA: support Generic Address Structure bit_offset in acpi_read/write

[Huang Ying]

On Fri, 2011-12-02 at 01:35 +0800, Luck, Tony wrote:
> >> That doesn't deal with long-lived mappings and changes that I just
> >> mentioned in last e-mail - but it does mean that callers of ioremap()
> >> don't need to worry ... they can call "blindly".
> >
> > I see.  So you think it is better to enclose page_is_ram() with #ifndef
> > CONFIG_IA64 in the patch, so that only ioremap() is used?
> 
> Functionally yes - though the aversion to #ifdef in the middle of code
> would imply some extra hoops.  Perhaps
> 
> #ifndef CONFIG_IA64
> #define can_use_kmap(pfn) page_is_ram(pfn)
> #else
> #define can_use_kmap(pfn) (0) /* ioremap() will take care of cache attributes */
> #endif
> 
> 
> ...
> 
> 		if (can_use_kmap(pfn)) {

Yes.  That is better.  I will do that.

Best Regards,
Huang Ying