Message ID | 1502387883-16667-1-git-send-email-Yazen.Ghannam@amd.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
+EDAC, Boris and Tony for RAS comments. Any comments from ACPI folks? Thanks, Yazen > -----Original Message----- > From: Ghannam, Yazen > Sent: Thursday, August 10, 2017 1:58 PM > To: linux-acpi@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; rjw@rjwysocki.net; lenb@kernel.org; > Ghannam, Yazen <Yazen.Ghannam@amd.com> > Subject: [PATCH] ACPI, APEI, EINJ: Subtract any matching Register Region from > Trigger resources > > From: Yazen Ghannam <yazen.ghannam@amd.com> > > ACPI defines a number of instructions to use for triggering errors. However > we are currently removing the address resources from the trigger resources > for only the WRITE_REGISTER_VALUE instruction. This leads to a resource > conflict for any other valid instruction. > > Remove the WRITE_REGISTER_VALUE instruction check to fix the resource > conflict with other instructions. > > Fixes: b4e008dc53a3 ("ACPI, APEI, EINJ, Refine the fix of resource conflict") > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> > --- > drivers/acpi/apei/einj.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c > index ec50c32ea3da..e0f201ea161d 100644 > --- a/drivers/acpi/apei/einj.c > +++ b/drivers/acpi/apei/einj.c > @@ -281,7 +281,6 @@ static struct acpi_generic_address > *einj_get_trigger_parameter_region( > ((char *)trigger_tab + sizeof(struct acpi_einj_trigger)); > for (i = 0; i < trigger_tab->entry_count; i++) { > if (entry->action == ACPI_EINJ_TRIGGER_ERROR && > - entry->instruction == ACPI_EINJ_WRITE_REGISTER_VALUE && > entry->register_region.space_id == > ACPI_ADR_SPACE_SYSTEM_MEMORY && > (entry->register_region.address & param2) == (param1 & > param2)) > -- > 2.7.4 -- 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
On Tue, Aug 22, 2017 at 02:04:27PM +0000, Ghannam, Yazen wrote: > > *einj_get_trigger_parameter_region( > > ((char *)trigger_tab + sizeof(struct acpi_einj_trigger)); > > for (i = 0; i < trigger_tab->entry_count; i++) { > > if (entry->action == ACPI_EINJ_TRIGGER_ERROR && > > - entry->instruction == ACPI_EINJ_WRITE_REGISTER_VALUE && > > entry->register_region.space_id == > > ACPI_ADR_SPACE_SYSTEM_MEMORY && > > (entry->register_region.address & param2) == (param1 & > > param2)) Maybe you should add the other instruction types that make sense here (ACPI_EINJ_READ_REGISTER, ACPI_EINJ_READ_REGISTER_VALUE and ACPI_EINJ_WRITE_REGISTER) as a sanity check against the BIOS giving you an action in the trigger table that doesn't involve a memory access? -Tony -- 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
> -----Original Message----- > From: Luck, Tony [mailto:tony.luck@intel.com] > Sent: Tuesday, August 22, 2017 12:03 PM > To: Ghannam, Yazen <Yazen.Ghannam@amd.com> > Cc: linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org; > rjw@rjwysocki.net; lenb@kernel.org; linux-edac@vger.kernel.org; Borislav > Petkov <bp@suse.de> > Subject: Re: [PATCH] ACPI, APEI, EINJ: Subtract any matching Register Region > from Trigger resources > > On Tue, Aug 22, 2017 at 02:04:27PM +0000, Ghannam, Yazen wrote: > > > *einj_get_trigger_parameter_region( > > > ((char *)trigger_tab + sizeof(struct acpi_einj_trigger)); > > > for (i = 0; i < trigger_tab->entry_count; i++) { > > > if (entry->action == ACPI_EINJ_TRIGGER_ERROR && > > > - entry->instruction == ACPI_EINJ_WRITE_REGISTER_VALUE && > > > entry->register_region.space_id == > > > ACPI_ADR_SPACE_SYSTEM_MEMORY && > > > (entry->register_region.address & param2) == (param1 & > > > param2)) > > Maybe you should add the other instruction types that make sense here > (ACPI_EINJ_READ_REGISTER, ACPI_EINJ_READ_REGISTER_VALUE and > ACPI_EINJ_WRITE_REGISTER) as a sanity check against the BIOS giving you an > action in the trigger table that doesn't involve a memory access? > Yes, in that case we can do: entry->instruction <= ACPI_EINJ_WRITE_REGISTER_VALUE That would cover all the memory access instructions and protect against invalid instructions. How does that sound? Thanks, Yazen -- 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
On Wed, Aug 23, 2017 at 04:37:43PM +0000, Ghannam, Yazen wrote: > > -----Original Message----- > > From: Luck, Tony [mailto:tony.luck@intel.com] > > Maybe you should add the other instruction types that make sense here > > (ACPI_EINJ_READ_REGISTER, ACPI_EINJ_READ_REGISTER_VALUE and > > ACPI_EINJ_WRITE_REGISTER) as a sanity check against the BIOS giving you an > > action in the trigger table that doesn't involve a memory access? > > > > Yes, in that case we can do: > entry->instruction <= ACPI_EINJ_WRITE_REGISTER_VALUE > > That would cover all the memory access instructions and protect against invalid > instructions. > > How does that sound? Perfect. Acked-by: Tony Luck <tony.luck@intel.com> -Tony -- 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
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c index ec50c32ea3da..e0f201ea161d 100644 --- a/drivers/acpi/apei/einj.c +++ b/drivers/acpi/apei/einj.c @@ -281,7 +281,6 @@ static struct acpi_generic_address *einj_get_trigger_parameter_region( ((char *)trigger_tab + sizeof(struct acpi_einj_trigger)); for (i = 0; i < trigger_tab->entry_count; i++) { if (entry->action == ACPI_EINJ_TRIGGER_ERROR && - entry->instruction == ACPI_EINJ_WRITE_REGISTER_VALUE && entry->register_region.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY && (entry->register_region.address & param2) == (param1 & param2))