[RFC PATCH v2 0/3] Add support for Vendor Defined Error Types in Einj Module

[RFC PATCH v2 0/3] Add support for Vendor Defined Error Types in Einj Module

[Avadhut Naik]

This patchset adds support for Vendor Defined Error types in the einj
module by exporting a binary blob file in module's debugfs directory.
Userspace tools can write OEM Defined Structures into the blob file as
part of injecting Vendor defined errors.

The first patch refactors available_error_type_show() function to ensure
all errors supported by the platform are output through einj module's
available_error_type file in debugfs.

The second patch adds a write callback for binary blobs created through
debugfs_create_blob() API.

The third adds the required support i.e. establishing the memory mapping
and exporting it through debugfs blob file for Vendor-defined Error types.

Changes in V2:
 - Split the v1 patch, as was recommended, to have a seperate patch for
changes in debugfs.
 - Refactored available_error_type_show() function into a seperate patch.
 - Changed file permissions to octal format to remove checkpatch warnings.

Avadhut Naik (3):
  ACPI: APEI: EINJ: Refactor available_error_type_show()
  fs: debugfs: Add write functionality to debugfs blobs
  ACPI: APEI: EINJ: Add support for vendor defined error types

 drivers/acpi/apei/einj.c | 62 ++++++++++++++++++++++++++--------------
 fs/debugfs/file.c        | 28 ++++++++++++++----
 2 files changed, 64 insertions(+), 26 deletions(-)

-- 
2.34.1

[RFC PATCH v2 1/3] ACPI: APEI: EINJ: Refactor available_error_type_show()

[Avadhut Naik]

OSPM can discover the error injection capabilities of the platform by
executing GET_ERROR_TYPE error injection action.[1] The action returns
a DWORD representing a bitmap of platform supported error injections.[2]

The available_error_type_show() function determines the bits set within
this DWORD and provides a verbose output, from einj_error_type_string
array, through /sys/kernel/debug/apei/einj/available_error_type file.

The function however, assumes one to one correspondence between an error's
position in the bitmap and its array entry offset. Consequently, some
errors like Vendor Defined Error Type fail this assumption and will
incorrectly be shown as not supported, even if their corresponding bit is
set in the bitmap and they have an entry in the array.

Navigate around the issue by converting einj_error_type_string into an
array of structures with a predetermined mask for all error types
corresponding to their bit position in the DWORD returned by GET_ERROR_TYPE
action. The same breaks the aforementioned assumption resulting in all
supported error types by a platform being outputted through the above
available_error_type file.

[1] ACPI specification 6.5, Table 18.25
[2] ACPI specification 6.5, Table 18.30

Suggested-by: Alexey Kardashevskiy <alexey.kardashevskiy@amd.com>
Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
---
 drivers/acpi/apei/einj.c | 43 ++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 013eb621dc92..d5f8dc4df7a5 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -577,25 +577,25 @@ static u64 error_param2;
 static u64 error_param3;
 static u64 error_param4;
 static struct dentry *einj_debug_dir;
-static const char * const einj_error_type_string[] = {
-	"0x00000001\tProcessor Correctable\n",
-	"0x00000002\tProcessor Uncorrectable non-fatal\n",
-	"0x00000004\tProcessor Uncorrectable fatal\n",
-	"0x00000008\tMemory Correctable\n",
-	"0x00000010\tMemory Uncorrectable non-fatal\n",
-	"0x00000020\tMemory Uncorrectable fatal\n",
-	"0x00000040\tPCI Express Correctable\n",
-	"0x00000080\tPCI Express Uncorrectable non-fatal\n",
-	"0x00000100\tPCI Express Uncorrectable fatal\n",
-	"0x00000200\tPlatform Correctable\n",
-	"0x00000400\tPlatform Uncorrectable non-fatal\n",
-	"0x00000800\tPlatform Uncorrectable fatal\n",
-	"0x00001000\tCXL.cache Protocol Correctable\n",
-	"0x00002000\tCXL.cache Protocol Uncorrectable non-fatal\n",
-	"0x00004000\tCXL.cache Protocol Uncorrectable fatal\n",
-	"0x00008000\tCXL.mem Protocol Correctable\n",
-	"0x00010000\tCXL.mem Protocol Uncorrectable non-fatal\n",
-	"0x00020000\tCXL.mem Protocol Uncorrectable fatal\n",
+static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
+	{0x00000001, "Processor Correctable"},
+	{0x00000002, "Processor Uncorrectable non-fatal"},
+	{0x00000004, "Processor Uncorrectable fatal"},
+	{0x00000008, "Memory Correctable"},
+	{0x00000010, "Memory Uncorrectable non-fatal"},
+	{0x00000020, "Memory Uncorrectable fatal"},
+	{0x00000040, "PCI Express Correctable"},
+	{0x00000080, "PCI Express Uncorrectable non-fatal"},
+	{0x00000100, "PCI Express Uncorrectable fatal"},
+	{0x00000200, "Platform Correctable"},
+	{0x00000400, "Platform Uncorrectable non-fatal"},
+	{0x00000800, "Platform Uncorrectable fatal"},
+	{0x00001000, "CXL.cache Protocol Correctable"},
+	{0x00002000, "CXL.cache Protocol Uncorrectable non-fatal"},
+	{0x00004000, "CXL.cache Protocol Uncorrectable fatal"},
+	{0x00008000, "CXL.mem Protocol Correctable"},
+	{0x00010000, "CXL.mem Protocol Uncorrectable non-fatal"},
+	{0x00020000, "CXL.mem Protocol Uncorrectable fatal"},
 };
 
 static int available_error_type_show(struct seq_file *m, void *v)
@@ -607,8 +607,9 @@ static int available_error_type_show(struct seq_file *m, void *v)
 	if (rc)
 		return rc;
 	for (int pos = 0; pos < ARRAY_SIZE(einj_error_type_string); pos++)
-		if (available_error_type & BIT(pos))
-			seq_puts(m, einj_error_type_string[pos]);
+		if (available_error_type & einj_error_type_string[pos].mask)
+			seq_printf(m, "0x%08x\t%s\n", einj_error_type_string[pos].mask,
+				   einj_error_type_string[pos].str);
 
 	return 0;
 }
-- 
2.34.1

Re: [RFC PATCH v2 1/3] ACPI: APEI: EINJ: Refactor available_error_type_show()

[Yazen Ghannam]

On 5/25/23 4:44 PM, Avadhut Naik wrote:
> OSPM can discover the error injection capabilities of the platform by
> executing GET_ERROR_TYPE error injection action.[1] The action returns
> a DWORD representing a bitmap of platform supported error injections.[2]
> 
> The available_error_type_show() function determines the bits set within
> this DWORD and provides a verbose output, from einj_error_type_string
> array, through /sys/kernel/debug/apei/einj/available_error_type file.
> 
> The function however, assumes one to one correspondence between an error's
> position in the bitmap and its array entry offset. Consequently, some
> errors like Vendor Defined Error Type fail this assumption and will
> incorrectly be shown as not supported, even if their corresponding bit is
> set in the bitmap and they have an entry in the array.
> 
> Navigate around the issue by converting einj_error_type_string into an
> array of structures with a predetermined mask for all error types
> corresponding to their bit position in the DWORD returned by GET_ERROR_TYPE
> action. The same breaks the aforementioned assumption resulting in all
> supported error types by a platform being outputted through the above
> available_error_type file.
> 
> [1] ACPI specification 6.5, Table 18.25
> [2] ACPI specification 6.5, Table 18.30
> 
> Suggested-by: Alexey Kardashevskiy <alexey.kardashevskiy@amd.com>
> Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
> ---
>  drivers/acpi/apei/einj.c | 43 ++++++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 013eb621dc92..d5f8dc4df7a5 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -577,25 +577,25 @@ static u64 error_param2;
>  static u64 error_param3;
>  static u64 error_param4;
>  static struct dentry *einj_debug_dir;
> -static const char * const einj_error_type_string[] = {
> -	"0x00000001\tProcessor Correctable\n",
> -	"0x00000002\tProcessor Uncorrectable non-fatal\n",
> -	"0x00000004\tProcessor Uncorrectable fatal\n",
> -	"0x00000008\tMemory Correctable\n",
> -	"0x00000010\tMemory Uncorrectable non-fatal\n",
> -	"0x00000020\tMemory Uncorrectable fatal\n",
> -	"0x00000040\tPCI Express Correctable\n",
> -	"0x00000080\tPCI Express Uncorrectable non-fatal\n",
> -	"0x00000100\tPCI Express Uncorrectable fatal\n",
> -	"0x00000200\tPlatform Correctable\n",
> -	"0x00000400\tPlatform Uncorrectable non-fatal\n",
> -	"0x00000800\tPlatform Uncorrectable fatal\n",
> -	"0x00001000\tCXL.cache Protocol Correctable\n",
> -	"0x00002000\tCXL.cache Protocol Uncorrectable non-fatal\n",
> -	"0x00004000\tCXL.cache Protocol Uncorrectable fatal\n",
> -	"0x00008000\tCXL.mem Protocol Correctable\n",
> -	"0x00010000\tCXL.mem Protocol Uncorrectable non-fatal\n",
> -	"0x00020000\tCXL.mem Protocol Uncorrectable fatal\n",
> +static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
> +	{0x00000001, "Processor Correctable"},
> +	{0x00000002, "Processor Uncorrectable non-fatal"},
> +	{0x00000004, "Processor Uncorrectable fatal"},
> +	{0x00000008, "Memory Correctable"},
> +	{0x00000010, "Memory Uncorrectable non-fatal"},
> +	{0x00000020, "Memory Uncorrectable fatal"},
> +	{0x00000040, "PCI Express Correctable"},
> +	{0x00000080, "PCI Express Uncorrectable non-fatal"},
> +	{0x00000100, "PCI Express Uncorrectable fatal"},
> +	{0x00000200, "Platform Correctable"},
> +	{0x00000400, "Platform Uncorrectable non-fatal"},
> +	{0x00000800, "Platform Uncorrectable fatal"},
> +	{0x00001000, "CXL.cache Protocol Correctable"},
> +	{0x00002000, "CXL.cache Protocol Uncorrectable non-fatal"},
> +	{0x00004000, "CXL.cache Protocol Uncorrectable fatal"},
> +	{0x00008000, "CXL.mem Protocol Correctable"},
> +	{0x00010000, "CXL.mem Protocol Uncorrectable non-fatal"},
> +	{0x00020000, "CXL.mem Protocol Uncorrectable fatal"},
>  };
>

I think it'd be easier to read if the masks used the BIT() macro rather
than a hex value.

Thanks,
Yazen

Re: [RFC PATCH v2 1/3] ACPI: APEI: EINJ: Refactor available_error_type_show()

[Alexey Kardashevskiy]

On 8/6/23 00:20, Yazen Ghannam wrote:
> On 5/25/23 4:44 PM, Avadhut Naik wrote:
>> OSPM can discover the error injection capabilities of the platform by
>> executing GET_ERROR_TYPE error injection action.[1] The action returns
>> a DWORD representing a bitmap of platform supported error injections.[2]
>>
>> The available_error_type_show() function determines the bits set within
>> this DWORD and provides a verbose output, from einj_error_type_string
>> array, through /sys/kernel/debug/apei/einj/available_error_type file.
>>
>> The function however, assumes one to one correspondence between an error's
>> position in the bitmap and its array entry offset. Consequently, some
>> errors like Vendor Defined Error Type fail this assumption and will
>> incorrectly be shown as not supported, even if their corresponding bit is
>> set in the bitmap and they have an entry in the array.
>>
>> Navigate around the issue by converting einj_error_type_string into an
>> array of structures with a predetermined mask for all error types
>> corresponding to their bit position in the DWORD returned by GET_ERROR_TYPE
>> action. The same breaks the aforementioned assumption resulting in all
>> supported error types by a platform being outputted through the above
>> available_error_type file.
>>
>> [1] ACPI specification 6.5, Table 18.25
>> [2] ACPI specification 6.5, Table 18.30
>>
>> Suggested-by: Alexey Kardashevskiy <alexey.kardashevskiy@amd.com>
>> Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
>> ---
>>   drivers/acpi/apei/einj.c | 43 ++++++++++++++++++++--------------------
>>   1 file changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
>> index 013eb621dc92..d5f8dc4df7a5 100644
>> --- a/drivers/acpi/apei/einj.c
>> +++ b/drivers/acpi/apei/einj.c
>> @@ -577,25 +577,25 @@ static u64 error_param2;
>>   static u64 error_param3;
>>   static u64 error_param4;
>>   static struct dentry *einj_debug_dir;
>> -static const char * const einj_error_type_string[] = {
>> -	"0x00000001\tProcessor Correctable\n",
>> -	"0x00000002\tProcessor Uncorrectable non-fatal\n",
>> -	"0x00000004\tProcessor Uncorrectable fatal\n",
>> -	"0x00000008\tMemory Correctable\n",
>> -	"0x00000010\tMemory Uncorrectable non-fatal\n",
>> -	"0x00000020\tMemory Uncorrectable fatal\n",
>> -	"0x00000040\tPCI Express Correctable\n",
>> -	"0x00000080\tPCI Express Uncorrectable non-fatal\n",
>> -	"0x00000100\tPCI Express Uncorrectable fatal\n",
>> -	"0x00000200\tPlatform Correctable\n",
>> -	"0x00000400\tPlatform Uncorrectable non-fatal\n",
>> -	"0x00000800\tPlatform Uncorrectable fatal\n",
>> -	"0x00001000\tCXL.cache Protocol Correctable\n",
>> -	"0x00002000\tCXL.cache Protocol Uncorrectable non-fatal\n",
>> -	"0x00004000\tCXL.cache Protocol Uncorrectable fatal\n",
>> -	"0x00008000\tCXL.mem Protocol Correctable\n",
>> -	"0x00010000\tCXL.mem Protocol Uncorrectable non-fatal\n",
>> -	"0x00020000\tCXL.mem Protocol Uncorrectable fatal\n",
>> +static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>> +	{0x00000001, "Processor Correctable"},
>> +	{0x00000002, "Processor Uncorrectable non-fatal"},
>> +	{0x00000004, "Processor Uncorrectable fatal"},
>> +	{0x00000008, "Memory Correctable"},
>> +	{0x00000010, "Memory Uncorrectable non-fatal"},
>> +	{0x00000020, "Memory Uncorrectable fatal"},
>> +	{0x00000040, "PCI Express Correctable"},
>> +	{0x00000080, "PCI Express Uncorrectable non-fatal"},
>> +	{0x00000100, "PCI Express Uncorrectable fatal"},
>> +	{0x00000200, "Platform Correctable"},
>> +	{0x00000400, "Platform Uncorrectable non-fatal"},
>> +	{0x00000800, "Platform Uncorrectable fatal"},
>> +	{0x00001000, "CXL.cache Protocol Correctable"},
>> +	{0x00002000, "CXL.cache Protocol Uncorrectable non-fatal"},
>> +	{0x00004000, "CXL.cache Protocol Uncorrectable fatal"},
>> +	{0x00008000, "CXL.mem Protocol Correctable"},
>> +	{0x00010000, "CXL.mem Protocol Uncorrectable non-fatal"},
>> +	{0x00020000, "CXL.mem Protocol Uncorrectable fatal"},
>>   };
>>
> 
> I think it'd be easier to read if the masks used the BIT() macro rather
> than a hex value.

Makes sense but I'd say because it is easier to match the APCI spec 
which uses the bit numbers, not easier to read (which is arguable).


> 
> Thanks,
> Yazen

-- 
Alexey

Re: [RFC PATCH v2 1/3] ACPI: APEI: EINJ: Refactor available_error_type_show()

[Avadhut Naik]

Hi,
	Thanks for reviewing.

On 6/7/2023 22:48, Alexey Kardashevskiy wrote:
> 
> 
> On 8/6/23 00:20, Yazen Ghannam wrote:
>> On 5/25/23 4:44 PM, Avadhut Naik wrote:
>>> OSPM can discover the error injection capabilities of the platform by
>>> executing GET_ERROR_TYPE error injection action.[1] The action returns
>>> a DWORD representing a bitmap of platform supported error injections.[2]
>>>
>>> The available_error_type_show() function determines the bits set within
>>> this DWORD and provides a verbose output, from einj_error_type_string
>>> array, through /sys/kernel/debug/apei/einj/available_error_type file.
>>>
>>> The function however, assumes one to one correspondence between an error's
>>> position in the bitmap and its array entry offset. Consequently, some
>>> errors like Vendor Defined Error Type fail this assumption and will
>>> incorrectly be shown as not supported, even if their corresponding bit is
>>> set in the bitmap and they have an entry in the array.
>>>
>>> Navigate around the issue by converting einj_error_type_string into an
>>> array of structures with a predetermined mask for all error types
>>> corresponding to their bit position in the DWORD returned by GET_ERROR_TYPE
>>> action. The same breaks the aforementioned assumption resulting in all
>>> supported error types by a platform being outputted through the above
>>> available_error_type file.
>>>
>>> [1] ACPI specification 6.5, Table 18.25
>>> [2] ACPI specification 6.5, Table 18.30
>>>
>>> Suggested-by: Alexey Kardashevskiy <alexey.kardashevskiy@amd.com>
>>> Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
>>> ---
>>>   drivers/acpi/apei/einj.c | 43 ++++++++++++++++++++--------------------
>>>   1 file changed, 22 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
>>> index 013eb621dc92..d5f8dc4df7a5 100644
>>> --- a/drivers/acpi/apei/einj.c
>>> +++ b/drivers/acpi/apei/einj.c
>>> @@ -577,25 +577,25 @@ static u64 error_param2;
>>>   static u64 error_param3;
>>>   static u64 error_param4;
>>>   static struct dentry *einj_debug_dir;
>>> -static const char * const einj_error_type_string[] = {
>>> -    "0x00000001\tProcessor Correctable\n",
>>> -    "0x00000002\tProcessor Uncorrectable non-fatal\n",
>>> -    "0x00000004\tProcessor Uncorrectable fatal\n",
>>> -    "0x00000008\tMemory Correctable\n",
>>> -    "0x00000010\tMemory Uncorrectable non-fatal\n",
>>> -    "0x00000020\tMemory Uncorrectable fatal\n",
>>> -    "0x00000040\tPCI Express Correctable\n",
>>> -    "0x00000080\tPCI Express Uncorrectable non-fatal\n",
>>> -    "0x00000100\tPCI Express Uncorrectable fatal\n",
>>> -    "0x00000200\tPlatform Correctable\n",
>>> -    "0x00000400\tPlatform Uncorrectable non-fatal\n",
>>> -    "0x00000800\tPlatform Uncorrectable fatal\n",
>>> -    "0x00001000\tCXL.cache Protocol Correctable\n",
>>> -    "0x00002000\tCXL.cache Protocol Uncorrectable non-fatal\n",
>>> -    "0x00004000\tCXL.cache Protocol Uncorrectable fatal\n",
>>> -    "0x00008000\tCXL.mem Protocol Correctable\n",
>>> -    "0x00010000\tCXL.mem Protocol Uncorrectable non-fatal\n",
>>> -    "0x00020000\tCXL.mem Protocol Uncorrectable fatal\n",
>>> +static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>>> +    {0x00000001, "Processor Correctable"},
>>> +    {0x00000002, "Processor Uncorrectable non-fatal"},
>>> +    {0x00000004, "Processor Uncorrectable fatal"},
>>> +    {0x00000008, "Memory Correctable"},
>>> +    {0x00000010, "Memory Uncorrectable non-fatal"},
>>> +    {0x00000020, "Memory Uncorrectable fatal"},
>>> +    {0x00000040, "PCI Express Correctable"},
>>> +    {0x00000080, "PCI Express Uncorrectable non-fatal"},
>>> +    {0x00000100, "PCI Express Uncorrectable fatal"},
>>> +    {0x00000200, "Platform Correctable"},
>>> +    {0x00000400, "Platform Uncorrectable non-fatal"},
>>> +    {0x00000800, "Platform Uncorrectable fatal"},
>>> +    {0x00001000, "CXL.cache Protocol Correctable"},
>>> +    {0x00002000, "CXL.cache Protocol Uncorrectable non-fatal"},
>>> +    {0x00004000, "CXL.cache Protocol Uncorrectable fatal"},
>>> +    {0x00008000, "CXL.mem Protocol Correctable"},
>>> +    {0x00010000, "CXL.mem Protocol Uncorrectable non-fatal"},
>>> +    {0x00020000, "CXL.mem Protocol Uncorrectable fatal"},
>>>   };
>>>
>>
>> I think it'd be easier to read if the masks used the BIT() macro rather
>> than a hex value.
> 
> Makes sense but I'd say because it is easier to match the APCI spec which uses the bit numbers, not easier to read (which is arguable).
> 
	Agreed, will replace the hex values with BIT() macro.

Thanks,
Avadhut Naik
> 
>>
>> Thanks,
>> Yazen
> 

-- 

[RFC PATCH v2 2/3] fs: debugfs: Add write functionality to debugfs blobs

[Avadhut Naik]

Currently, debugfs_create_blob() creates read-only debugfs binary blob
files.

In some cases, however, userspace tools need to write variable length
data structures into predetermined memory addresses. An example is when
injecting Vendor-defined error types through the einj module. In such
cases, the functionality to write to these blob files in debugfs would
be desired since the mapping aspect can be handled within the modules
with userspace tools only needing to write into the blob files.

Implement a write callback to enable writing to these blob files in
debugfs.

Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
---
 fs/debugfs/file.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 1f971c880dde..fab5a562b57c 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -973,17 +973,35 @@ static ssize_t read_file_blob(struct file *file, char __user *user_buf,
 	return r;
 }
 
+static ssize_t write_file_blob(struct file *file, const char __user *user_buf,
+			       size_t count, loff_t *ppos)
+{
+	struct debugfs_blob_wrapper *blob = file->private_data;
+	struct dentry *dentry = F_DENTRY(file);
+	ssize_t r;
+
+	r = debugfs_file_get(dentry);
+	if (unlikely(r))
+		return r;
+	r = simple_write_to_buffer(blob->data, blob->size, ppos, user_buf,
+				   count);
+
+	debugfs_file_put(dentry);
+	return r;
+}
+
 static const struct file_operations fops_blob = {
 	.read =		read_file_blob,
+	.write =	write_file_blob,
 	.open =		simple_open,
 	.llseek =	default_llseek,
 };
 
 /**
- * debugfs_create_blob - create a debugfs file that is used to read a binary blob
+ * debugfs_create_blob - create a debugfs file that is used to read and write
+ * a binary blob
  * @name: a pointer to a string containing the name of the file to create.
- * @mode: the read permission that the file should have (other permissions are
- *	  masked out)
+ * @mode: the permission that the file should have
  * @parent: a pointer to the parent dentry for this file.  This should be a
  *          directory dentry if set.  If this parameter is %NULL, then the
  *          file will be created in the root of the debugfs filesystem.
@@ -992,7 +1010,7 @@ static const struct file_operations fops_blob = {
  *
  * This function creates a file in debugfs with the given name that exports
  * @blob->data as a binary blob. If the @mode variable is so set it can be
- * read from. Writing is not supported.
+ * read from and written to.
  *
  * This function will return a pointer to a dentry if it succeeds.  This
  * pointer must be passed to the debugfs_remove() function when the file is
@@ -1007,7 +1025,7 @@ struct dentry *debugfs_create_blob(const char *name, umode_t mode,
 				   struct dentry *parent,
 				   struct debugfs_blob_wrapper *blob)
 {
-	return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
+	return debugfs_create_file_unsafe(name, mode, parent, blob, &fops_blob);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_blob);
 
-- 
2.34.1

Re: [RFC PATCH v2 2/3] fs: debugfs: Add write functionality to debugfs blobs

[Alexey Kardashevskiy]

On 26/5/23 06:44, Avadhut Naik wrote:
> Currently, debugfs_create_blob() creates read-only debugfs binary blob
> files.
> 
> In some cases, however, userspace tools need to write variable length
> data structures into predetermined memory addresses. An example is when
> injecting Vendor-defined error types through the einj module. In such
> cases, the functionality to write to these blob files in debugfs would
> be desired since the mapping aspect can be handled within the modules
> with userspace tools only needing to write into the blob files.
> 
> Implement a write callback to enable writing to these blob files in
> debugfs.
> 
> Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>

Reviewed-by: Alexey Kardashevskiy <aik@amd.com>

> ---
>   fs/debugfs/file.c | 28 +++++++++++++++++++++++-----
>   1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 1f971c880dde..fab5a562b57c 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -973,17 +973,35 @@ static ssize_t read_file_blob(struct file *file, char __user *user_buf,
>   	return r;
>   }
>   
> +static ssize_t write_file_blob(struct file *file, const char __user *user_buf,
> +			       size_t count, loff_t *ppos)
> +{
> +	struct debugfs_blob_wrapper *blob = file->private_data;
> +	struct dentry *dentry = F_DENTRY(file);
> +	ssize_t r;
> +
> +	r = debugfs_file_get(dentry);
> +	if (unlikely(r))
> +		return r;
> +	r = simple_write_to_buffer(blob->data, blob->size, ppos, user_buf,
> +				   count);
> +
> +	debugfs_file_put(dentry);
> +	return r;
> +}
> +
>   static const struct file_operations fops_blob = {
>   	.read =		read_file_blob,
> +	.write =	write_file_blob,
>   	.open =		simple_open,
>   	.llseek =	default_llseek,
>   };
>   
>   /**
> - * debugfs_create_blob - create a debugfs file that is used to read a binary blob
> + * debugfs_create_blob - create a debugfs file that is used to read and write
> + * a binary blob
>    * @name: a pointer to a string containing the name of the file to create.
> - * @mode: the read permission that the file should have (other permissions are
> - *	  masked out)
> + * @mode: the permission that the file should have
>    * @parent: a pointer to the parent dentry for this file.  This should be a
>    *          directory dentry if set.  If this parameter is %NULL, then the
>    *          file will be created in the root of the debugfs filesystem.
> @@ -992,7 +1010,7 @@ static const struct file_operations fops_blob = {
>    *
>    * This function creates a file in debugfs with the given name that exports
>    * @blob->data as a binary blob. If the @mode variable is so set it can be
> - * read from. Writing is not supported.
> + * read from and written to.
>    *
>    * This function will return a pointer to a dentry if it succeeds.  This
>    * pointer must be passed to the debugfs_remove() function when the file is
> @@ -1007,7 +1025,7 @@ struct dentry *debugfs_create_blob(const char *name, umode_t mode,
>   				   struct dentry *parent,
>   				   struct debugfs_blob_wrapper *blob)
>   {
> -	return debugfs_create_file_unsafe(name, mode & 0444, parent, blob, &fops_blob);
> +	return debugfs_create_file_unsafe(name, mode, parent, blob, &fops_blob);
>   }
>   EXPORT_SYMBOL_GPL(debugfs_create_blob);
>   

-- 
Alexey

[RFC PATCH v2 3/3] ACPI: APEI: EINJ: Add support for vendor defined error types

[Avadhut Naik]

Vendor-Defined Error types are supported by the platform apart from
standard error types if bit 31 is set in the output of GET_ERROR_TYPE
Error Injection Action.[1] While the errors themselves and the length
of their associated "OEM Defined data structure" might vary between
vendors, the physical address of this structure can be computed through
vendor_extension and length fields of "SET_ERROR_TYPE_WITH_ADDRESS" and
"Vendor Error Type Extension" Structures respectively.[2][3]

Currently, however, the einj module only computes the physical address of
Vendor Error Type Extension Structure. Neither does it compute the physical
address of OEM Defined structure nor does it establish the memory mapping
required for injecting Vendor-defined errors. Consequently, userspace
tools have to establish the very mapping through /dev/mem, nopat kernel
parameter and system calls like mmap/munmap initially before injecting
Vendor-defined errors.

Circumvent the issue by computing the physical address of OEM Defined data
structure and establishing the required mapping with the structure. Create
a new file "oem_error", if the system supports Vendor-defined errors, to
export this mapping, through debugfs_create_blob(). Userspace tools can
then populate their respective OEM Defined structure instances and just
write to the file as part of injecting Vendor-defined Errors.

[1] ACPI specification 6.5, section 18.6.4
[2] ACPI specification 6.5, Table 18.31
[3] ACPI specification 6.5, Table 18.32

Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
---
 drivers/acpi/apei/einj.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index d5f8dc4df7a5..9f23b6955cf0 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -73,6 +73,7 @@ static u32 notrigger;
 
 static u32 vendor_flags;
 static struct debugfs_blob_wrapper vendor_blob;
+static struct debugfs_blob_wrapper vendor_errors;
 static char vendor_dev[64];
 
 /*
@@ -182,6 +183,16 @@ static int einj_timedout(u64 *t)
 	return 0;
 }
 
+static void get_oem_vendor_struct(u64 paddr, int offset,
+				  struct vendor_error_type_extension *v)
+{
+	u64 target_pa = paddr + offset + sizeof(struct vendor_error_type_extension);
+
+	vendor_errors.size = v->length - sizeof(struct vendor_error_type_extension);
+	if (vendor_errors.size)
+		vendor_errors.data = acpi_os_map_iomem(target_pa, vendor_errors.size);
+}
+
 static void check_vendor_extension(u64 paddr,
 				   struct set_error_type_with_address *v5param)
 {
@@ -194,6 +205,7 @@ static void check_vendor_extension(u64 paddr,
 	v = acpi_os_map_iomem(paddr + offset, sizeof(*v));
 	if (!v)
 		return;
+	get_oem_vendor_struct(paddr, offset, v);
 	sbdf = v->pcie_sbdf;
 	sprintf(vendor_dev, "%x:%x:%x.%x vendor_id=%x device_id=%x rev_id=%x\n",
 		sbdf >> 24, (sbdf >> 16) & 0xff,
@@ -596,6 +608,7 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
 	{0x00008000, "CXL.mem Protocol Correctable"},
 	{0x00010000, "CXL.mem Protocol Uncorrectable non-fatal"},
 	{0x00020000, "CXL.mem Protocol Uncorrectable fatal"},
+	{0x80000000, "Vendor Defined Error Types"},
 };
 
 static int available_error_type_show(struct seq_file *m, void *v)
@@ -768,6 +781,10 @@ static int __init einj_init(void)
 				   einj_debug_dir, &vendor_flags);
 	}
 
+	if (vendor_errors.size)
+		debugfs_create_blob("oem_error", 0200, einj_debug_dir,
+				    &vendor_errors);
+
 	pr_info("Error INJection is initialized.\n");
 
 	return 0;
@@ -793,6 +810,8 @@ static void __exit einj_exit(void)
 			sizeof(struct einj_parameter);
 
 		acpi_os_unmap_iomem(einj_param, size);
+		if (vendor_errors.size)
+			acpi_os_unmap_iomem(vendor_errors.data, vendor_errors.size);
 	}
 	einj_exec_ctx_init(&ctx);
 	apei_exec_post_unmap_gars(&ctx);
-- 
2.34.1

Re: [RFC PATCH v2 3/3] ACPI: APEI: EINJ: Add support for vendor defined error types

[Alexey Kardashevskiy]

On 26/5/23 06:44, Avadhut Naik wrote:
> Vendor-Defined Error types are supported by the platform apart from
> standard error types if bit 31 is set in the output of GET_ERROR_TYPE
> Error Injection Action.[1] While the errors themselves and the length
> of their associated "OEM Defined data structure" might vary between
> vendors, the physical address of this structure can be computed through
> vendor_extension and length fields of "SET_ERROR_TYPE_WITH_ADDRESS" and
> "Vendor Error Type Extension" Structures respectively.[2][3]
> 
> Currently, however, the einj module only computes the physical address of
> Vendor Error Type Extension Structure. Neither does it compute the physical
> address of OEM Defined structure nor does it establish the memory mapping
> required for injecting Vendor-defined errors. Consequently, userspace
> tools have to establish the very mapping through /dev/mem, nopat kernel
> parameter and system calls like mmap/munmap initially before injecting
> Vendor-defined errors.
> 
> Circumvent the issue by computing the physical address of OEM Defined data
> structure and establishing the required mapping with the structure. Create
> a new file "oem_error", if the system supports Vendor-defined errors, to
> export this mapping, through debugfs_create_blob(). Userspace tools can
> then populate their respective OEM Defined structure instances and just
> write to the file as part of injecting Vendor-defined Errors.
> 
> [1] ACPI specification 6.5, section 18.6.4
> [2] ACPI specification 6.5, Table 18.31
> [3] ACPI specification 6.5, Table 18.32
> 
> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
> ---
>   drivers/acpi/apei/einj.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index d5f8dc4df7a5..9f23b6955cf0 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -73,6 +73,7 @@ static u32 notrigger;
>   
>   static u32 vendor_flags;
>   static struct debugfs_blob_wrapper vendor_blob;
> +static struct debugfs_blob_wrapper vendor_errors;
>   static char vendor_dev[64];
>   
>   /*
> @@ -182,6 +183,16 @@ static int einj_timedout(u64 *t)
>   	return 0;
>   }
>   
> +static void get_oem_vendor_struct(u64 paddr, int offset,
> +				  struct vendor_error_type_extension *v)
> +{
> +	u64 target_pa = paddr + offset + sizeof(struct vendor_error_type_extension);
> +
> +	vendor_errors.size = v->length - sizeof(struct vendor_error_type_extension);
> +	if (vendor_errors.size)
> +		vendor_errors.data = acpi_os_map_iomem(target_pa, vendor_errors.size);


acpi_os_map_iomem() can return NULL but you check for the size (see 
below comments).

> +}
> +
>   static void check_vendor_extension(u64 paddr,
>   				   struct set_error_type_with_address *v5param)
>   {
> @@ -194,6 +205,7 @@ static void check_vendor_extension(u64 paddr,
>   	v = acpi_os_map_iomem(paddr + offset, sizeof(*v));
>   	if (!v)
>   		return;
> +	get_oem_vendor_struct(paddr, offset, v);
>   	sbdf = v->pcie_sbdf;
>   	sprintf(vendor_dev, "%x:%x:%x.%x vendor_id=%x device_id=%x rev_id=%x\n",
>   		sbdf >> 24, (sbdf >> 16) & 0xff,
> @@ -596,6 +608,7 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>   	{0x00008000, "CXL.mem Protocol Correctable"},
>   	{0x00010000, "CXL.mem Protocol Uncorrectable non-fatal"},
>   	{0x00020000, "CXL.mem Protocol Uncorrectable fatal"},
> +	{0x80000000, "Vendor Defined Error Types"},
>   };
>   
>   static int available_error_type_show(struct seq_file *m, void *v)
> @@ -768,6 +781,10 @@ static int __init einj_init(void)
>   				   einj_debug_dir, &vendor_flags);
>   	}
>   
> +	if (vendor_errors.size)
> +		debugfs_create_blob("oem_error", 0200, einj_debug_dir,
> +				    &vendor_errors);

Here writing to "oem_error" will crash.


> +
>   	pr_info("Error INJection is initialized.\n");
>   
>   	return 0;
> @@ -793,6 +810,8 @@ static void __exit einj_exit(void)
>   			sizeof(struct einj_parameter);
>   
>   		acpi_os_unmap_iomem(einj_param, size);
> +		if (vendor_errors.size)
> +			acpi_os_unmap_iomem(vendor_errors.data, vendor_errors.size);

And here is will produce an error message I suppose.

Just change get_oem_vendor_struct() to store the size in a local 
variable and only copy it to vendor_errors.size if vendor_errors.data!=NULL.

With that bit fixed,

Reviewed-by: Alexey Kardashevskiy <aik@amd.com>



>   	}
>   	einj_exec_ctx_init(&ctx);
>   	apei_exec_post_unmap_gars(&ctx);

-- 
Alexey

[RFC PATCH v2 3/3] ACPI: APEI: EINJ: Add support for vendor defined error types

[Avadhut Naik]

Hi,
	Thanks for reviewing!

On 6/7/2023 23:44, Alexey Kardashevskiy wrote:
> 
> 
> On 26/5/23 06:44, Avadhut Naik wrote:
>> Vendor-Defined Error types are supported by the platform apart from
>> standard error types if bit 31 is set in the output of GET_ERROR_TYPE
>> Error Injection Action.[1] While the errors themselves and the length
>> of their associated "OEM Defined data structure" might vary between
>> vendors, the physical address of this structure can be computed through
>> vendor_extension and length fields of "SET_ERROR_TYPE_WITH_ADDRESS" and
>> "Vendor Error Type Extension" Structures respectively.[2][3]
>>
>> Currently, however, the einj module only computes the physical address of
>> Vendor Error Type Extension Structure. Neither does it compute the physical
>> address of OEM Defined structure nor does it establish the memory mapping
>> required for injecting Vendor-defined errors. Consequently, userspace
>> tools have to establish the very mapping through /dev/mem, nopat kernel
>> parameter and system calls like mmap/munmap initially before injecting
>> Vendor-defined errors.
>>
>> Circumvent the issue by computing the physical address of OEM Defined data
>> structure and establishing the required mapping with the structure. Create
>> a new file "oem_error", if the system supports Vendor-defined errors, to
>> export this mapping, through debugfs_create_blob(). Userspace tools can
>> then populate their respective OEM Defined structure instances and just
>> write to the file as part of injecting Vendor-defined Errors.
>>
>> [1] ACPI specification 6.5, section 18.6.4
>> [2] ACPI specification 6.5, Table 18.31
>> [3] ACPI specification 6.5, Table 18.32
>>
>> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
>> Signed-off-by: Avadhut Naik <Avadhut.Naik@amd.com>
>> ---
>>   drivers/acpi/apei/einj.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
>> index d5f8dc4df7a5..9f23b6955cf0 100644
>> --- a/drivers/acpi/apei/einj.c
>> +++ b/drivers/acpi/apei/einj.c
>> @@ -73,6 +73,7 @@ static u32 notrigger;
>>     static u32 vendor_flags;
>>   static struct debugfs_blob_wrapper vendor_blob;
>> +static struct debugfs_blob_wrapper vendor_errors;
>>   static char vendor_dev[64];
>>     /*
>> @@ -182,6 +183,16 @@ static int einj_timedout(u64 *t)
>>       return 0;
>>   }
>>   +static void get_oem_vendor_struct(u64 paddr, int offset,
>> +                  struct vendor_error_type_extension *v)
>> +{
>> +    u64 target_pa = paddr + offset + sizeof(struct vendor_error_type_extension);
>> +
>> +    vendor_errors.size = v->length - sizeof(struct vendor_error_type_extension);
>> +    if (vendor_errors.size)
>> +        vendor_errors.data = acpi_os_map_iomem(target_pa, vendor_errors.size);
> 
> 
> acpi_os_map_iomem() can return NULL but you check for the size (see below comments).
> 
>> +}
>> +
>>   static void check_vendor_extension(u64 paddr,
>>                      struct set_error_type_with_address *v5param)
>>   {
>> @@ -194,6 +205,7 @@ static void check_vendor_extension(u64 paddr,
>>       v = acpi_os_map_iomem(paddr + offset, sizeof(*v));
>>       if (!v)
>>           return;
>> +    get_oem_vendor_struct(paddr, offset, v);
>>       sbdf = v->pcie_sbdf;
>>       sprintf(vendor_dev, "%x:%x:%x.%x vendor_id=%x device_id=%x rev_id=%x\n",
>>           sbdf >> 24, (sbdf >> 16) & 0xff,
>> @@ -596,6 +608,7 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>>       {0x00008000, "CXL.mem Protocol Correctable"},
>>       {0x00010000, "CXL.mem Protocol Uncorrectable non-fatal"},
>>       {0x00020000, "CXL.mem Protocol Uncorrectable fatal"},
>> +    {0x80000000, "Vendor Defined Error Types"},
>>   };
>>     static int available_error_type_show(struct seq_file *m, void *v)
>> @@ -768,6 +781,10 @@ static int __init einj_init(void)
>>                      einj_debug_dir, &vendor_flags);
>>       }
>>   +    if (vendor_errors.size)
>> +        debugfs_create_blob("oem_error", 0200, einj_debug_dir,
>> +                    &vendor_errors);
> 
> Here writing to "oem_error" will crash.
> 
> 
>> +
>>       pr_info("Error INJection is initialized.\n");
>>         return 0;
>> @@ -793,6 +810,8 @@ static void __exit einj_exit(void)
>>               sizeof(struct einj_parameter);
>>             acpi_os_unmap_iomem(einj_param, size);
>> +        if (vendor_errors.size)
>> +            acpi_os_unmap_iomem(vendor_errors.data, vendor_errors.size);
> 
> And here is will produce an error message I suppose.
> 
> Just change get_oem_vendor_struct() to store the size in a local variable and only copy it to vendor_errors.size if vendor_errors.data!=NULL.
> 
> With that bit fixed,
> 
> Reviewed-by: Alexey Kardashevskiy <aik@amd.com>
> 
	Agreed. Will change the function to something like below:

static void get_oem_vendor_struct(u64 paddr, int offset,
                  struct vendor_error_type_extension *v)
{
    unsigned long vendor_size;  
    u64 target_pa = paddr + offset + sizeof(struct vendor_error_type_extension);

    vendor_size = v->length - sizeof(struct vendor_error_type_extension);

    if (vendor_size)
        vendor_errors.data = acpi_os_map_iomem(target_pa, vendor_size);

    if (vendor_errors.data)
        vendor_errors.size = vendor_size;
}

Thanks,
Avadhut Naik
> 
> 
>>       }
>>       einj_exec_ctx_init(&ctx);
>>       apei_exec_post_unmap_gars(&ctx);
> 

--