[PATCH 1/2] ARM/efi: Remove duplicate permission settings

[PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Qiang Ma]

In the efi_virtmap_init(), permission settings have been applied:

static bool __init efi_virtmap_init(void)
{
	...
	for_each_efi_memory_desc(md)
		...
                efi_create_mapping(&efi_mm, md);
	...
	efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
	...
}

Therefore, there is no need to apply it again in the efi_create_mapping().

Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")

Signed-off-by: Qiang Ma <maqianga@uniontech.com>
---
 arch/arm/kernel/efi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
index 6f9ec7d28a71..d2fca20d912e 100644
--- a/arch/arm/kernel/efi.c
+++ b/arch/arm/kernel/efi.c
@@ -70,11 +70,6 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 
 	create_mapping_late(mm, &desc, true);
 
-	/*
-	 * If stricter permissions were specified, apply them now.
-	 */
-	if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
-		return efi_set_mapping_permissions(mm, md, false);
 	return 0;
 }
 
-- 
2.20.1

[PATCH 2/2] efi/arm*: Remove the useless failure return message print

[Qiang Ma]

In the efi_create_mapping() in arch/arm*/kernel/efi.c,
the return value is always 0, and this debug message
is unnecessary. So, remove it.

Signed-off-by: Qiang Ma <maqianga@uniontech.com>
---
 drivers/firmware/efi/arm-runtime.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 83092d93f36a..ca27fbfaff5c 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -66,12 +66,7 @@ static bool __init efi_virtmap_init(void)
 		if (md->virt_addr == U64_MAX)
 			return false;
 
-		ret = efi_create_mapping(&efi_mm, md);
-		if (ret) {
-			pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
-				&phys, ret);
-			return false;
-		}
+		efi_create_mapping(&efi_mm, md);
 	}
 
 	if (efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions))
-- 
2.20.1

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Ard Biesheuvel]

On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
>
> In the efi_virtmap_init(), permission settings have been applied:
>
> static bool __init efi_virtmap_init(void)
> {
>         ...
>         for_each_efi_memory_desc(md)
>                 ...
>                 efi_create_mapping(&efi_mm, md);
>         ...
>         efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
>         ...
> }
>
> Therefore, there is no need to apply it again in the efi_create_mapping().
>
> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
>
> Signed-off-by: Qiang Ma <maqianga@uniontech.com>

No, efi_memattr_apply_permissions() uses the /optional/ memory
attributes table, whereas efi_create_mapping() uses the permission
attributes in the EFI memory map. The memory attributes table is
optional, in which case any RO/XP attributes from the memory map
should be used.


> ---
>  arch/arm/kernel/efi.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
> index 6f9ec7d28a71..d2fca20d912e 100644
> --- a/arch/arm/kernel/efi.c
> +++ b/arch/arm/kernel/efi.c
> @@ -70,11 +70,6 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
>
>         create_mapping_late(mm, &desc, true);
>
> -       /*
> -        * If stricter permissions were specified, apply them now.
> -        */
> -       if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
> -               return efi_set_mapping_permissions(mm, md, false);
>         return 0;
>  }
>
> --
> 2.20.1
>

Re: [PATCH 2/2] efi/arm*: Remove the useless failure return message print

[kernel test robot]

Hi Qiang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on efi/next]
[also build test WARNING on linus/master v6.18-rc2 next-20251023]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Qiang-Ma/efi-arm-Remove-the-useless-failure-return-message-print/20251023-162558
base:   https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
patch link:    https://lore.kernel.org/r/20251023082129.75612-2-maqianga%40uniontech.com
patch subject: [PATCH 2/2] efi/arm*: Remove the useless failure return message print
config: arm64-randconfig-001-20251024 (https://download.01.org/0day-ci/archive/20251024/202510240949.NKb4ca96-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251024/202510240949.NKb4ca96-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510240949.NKb4ca96-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/firmware/efi/arm-runtime.c: In function 'efi_virtmap_init':
>> drivers/firmware/efi/arm-runtime.c:62:7: warning: unused variable 'ret' [-Wunused-variable]
      int ret;
          ^~~
>> drivers/firmware/efi/arm-runtime.c:61:15: warning: unused variable 'phys' [-Wunused-variable]
      phys_addr_t phys = md->phys_addr;
                  ^~~~


vim +/ret +62 drivers/firmware/efi/arm-runtime.c

9d80448ac92b720 Ard Biesheuvel 2016-08-16  51  
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  52  static bool __init efi_virtmap_init(void)
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  53  {
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  54  	efi_memory_desc_t *md;
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  55  
f7d924894265794 Ard Biesheuvel 2015-11-30  56  	efi_mm.pgd = pgd_alloc(&efi_mm);
d1eb98143c56f24 Ard Biesheuvel 2017-03-01  57  	mm_init_cpumask(&efi_mm);
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  58  	init_new_context(NULL, &efi_mm);
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  59  
78ce248faa3c46e Matt Fleming   2016-04-25  60  	for_each_efi_memory_desc(md) {
f7d924894265794 Ard Biesheuvel 2015-11-30 @61  		phys_addr_t phys = md->phys_addr;
f7d924894265794 Ard Biesheuvel 2015-11-30 @62  		int ret;
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  63  
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  64  		if (!(md->attribute & EFI_MEMORY_RUNTIME))
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  65  			continue;
37926f96302d8b6 Ard Biesheuvel 2022-10-20  66  		if (md->virt_addr == U64_MAX)
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  67  			return false;
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  68  
73fefd7f81521c6 Qiang Ma       2025-10-23  69  		efi_create_mapping(&efi_mm, md);
789957ef72f976c Ard Biesheuvel 2016-04-25  70  	}
789957ef72f976c Ard Biesheuvel 2016-04-25  71  
789957ef72f976c Ard Biesheuvel 2016-04-25  72  	if (efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions))
789957ef72f976c Ard Biesheuvel 2016-04-25  73  		return false;
789957ef72f976c Ard Biesheuvel 2016-04-25  74  
789957ef72f976c Ard Biesheuvel 2016-04-25  75  	return true;
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  76  }
e5bc22a42e4d46c Ard Biesheuvel 2015-11-30  77  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Qiang Ma]

在 2025/10/23 16:30, Ard Biesheuvel 写道:
> On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
>> In the efi_virtmap_init(), permission settings have been applied:
>>
>> static bool __init efi_virtmap_init(void)
>> {
>>          ...
>>          for_each_efi_memory_desc(md)
>>                  ...
>>                  efi_create_mapping(&efi_mm, md);
>>          ...
>>          efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
>>          ...
>> }
>>
>> Therefore, there is no need to apply it again in the efi_create_mapping().
>>
>> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
>>
>> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> No, efi_memattr_apply_permissions() uses the /optional/ memory
> attributes table, whereas efi_create_mapping() uses the permission
> attributes in the EFI memory map. The memory attributes table is
> optional, in which case any RO/XP attributes from the memory map
> should be used.
>
I see.

Then, can it be modified like this?
--- a/arch/arm/kernel/efi.c
+++ b/arch/arm/kernel/efi.c
@@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm, 
efi_memory_desc_t *md)
                 desc.type = MT_MEMORY_RWX_NONCACHED;
         else if (md->attribute & EFI_MEMORY_WC)
                 desc.type = MT_DEVICE_WC;
+       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
+               desc.type = MT_MEMORY_RO;
         else
                 desc.type = MT_DEVICE;

         create_mapping_late(mm, &desc, true);

-       /*
-        * If stricter permissions were specified, apply them now.
-        */
-       if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
-               return efi_set_mapping_permissions(mm, md, false);
         return 0;
  }

The create_mapping_late() finds the corresponding page table attribute 
from mem_types through type.
Here it is MT_MEMORY_RO, and its corresponding prot_pte is:
       ...
         [MT_MEMORY_RO] = {
                 .prot_pte  = L_PTE_PRESENT | L_PTE_YOUNG | L_PTE_DIRTY |
                              L_PTE_XN | L_PTE_RDONLY,
       ...

Finally, the page table is also set through the set_pte_ext().


Thanks.

>> ---
>>   arch/arm/kernel/efi.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
>> index 6f9ec7d28a71..d2fca20d912e 100644
>> --- a/arch/arm/kernel/efi.c
>> +++ b/arch/arm/kernel/efi.c
>> @@ -70,11 +70,6 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
>>
>>          create_mapping_late(mm, &desc, true);
>>
>> -       /*
>> -        * If stricter permissions were specified, apply them now.
>> -        */
>> -       if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
>> -               return efi_set_mapping_permissions(mm, md, false);
>>          return 0;
>>   }
>>
>> --
>> 2.20.1
>>

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Ard Biesheuvel]

On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@uniontech.com> wrote:
>
>
> 在 2025/10/23 16:30, Ard Biesheuvel 写道:
> > On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
> >> In the efi_virtmap_init(), permission settings have been applied:
> >>
> >> static bool __init efi_virtmap_init(void)
> >> {
> >>          ...
> >>          for_each_efi_memory_desc(md)
> >>                  ...
> >>                  efi_create_mapping(&efi_mm, md);
> >>          ...
> >>          efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
> >>          ...
> >> }
> >>
> >> Therefore, there is no need to apply it again in the efi_create_mapping().
> >>
> >> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
> >>
> >> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> > No, efi_memattr_apply_permissions() uses the /optional/ memory
> > attributes table, whereas efi_create_mapping() uses the permission
> > attributes in the EFI memory map. The memory attributes table is
> > optional, in which case any RO/XP attributes from the memory map
> > should be used.
> >
> I see.
>
> Then, can it be modified like this?

No

> --- a/arch/arm/kernel/efi.c
> +++ b/arch/arm/kernel/efi.c
> @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
> efi_memory_desc_t *md)
>                  desc.type = MT_MEMORY_RWX_NONCACHED;
>          else if (md->attribute & EFI_MEMORY_WC)
>                  desc.type = MT_DEVICE_WC;
> +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))

This will be true for RO, XP or RO+XP.

> +               desc.type = MT_MEMORY_RO;

This will apply RO permissions even to XP regions, which need to be writable.

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Qiang Ma]

在 2025/10/28 21:42, Ard Biesheuvel 写道:
> On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@uniontech.com> wrote:
>>
>> 在 2025/10/23 16:30, Ard Biesheuvel 写道:
>>> On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
>>>> In the efi_virtmap_init(), permission settings have been applied:
>>>>
>>>> static bool __init efi_virtmap_init(void)
>>>> {
>>>>           ...
>>>>           for_each_efi_memory_desc(md)
>>>>                   ...
>>>>                   efi_create_mapping(&efi_mm, md);
>>>>           ...
>>>>           efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
>>>>           ...
>>>> }
>>>>
>>>> Therefore, there is no need to apply it again in the efi_create_mapping().
>>>>
>>>> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
>>>>
>>>> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
>>> No, efi_memattr_apply_permissions() uses the /optional/ memory
>>> attributes table, whereas efi_create_mapping() uses the permission
>>> attributes in the EFI memory map. The memory attributes table is
>>> optional, in which case any RO/XP attributes from the memory map
>>> should be used.
>>>
>> I see.
>>
>> Then, can it be modified like this?
> No
>
>> --- a/arch/arm/kernel/efi.c
>> +++ b/arch/arm/kernel/efi.c
>> @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
>> efi_memory_desc_t *md)
>>                   desc.type = MT_MEMORY_RWX_NONCACHED;
>>           else if (md->attribute & EFI_MEMORY_WC)
>>                   desc.type = MT_DEVICE_WC;
>> +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
> This will be true for RO, XP or RO+XP.
>
>> +               desc.type = MT_MEMORY_RO;
> This will apply RO permissions even to XP regions, which need to be writable.
>
Thanks for your review.
I see.

I can introduce a new type MT_MEMORY_RO_XP, to describe RO+XP,
and then we can use the RO+XP attribute to implement memory mapping.

--- a/arch/arm/kernel/efi.c
+++ b/arch/arm/kernel/efi.c
@@ -68,13 +68,16 @@ int __init efi_create_mapping(struct mm_struct *mm, 
efi_memory_desc_t *md)
         else
                 desc.type = MT_DEVICE;

+       if ((md->attribute & EFI_MEMORY_RO) &&
+           (md->attribute & EFI_MEMORY_XP))
+               desc.type = MT_MEMORY_RO_XP;
+       else if ((md->attribute & EFI_MEMORY_RO)
+               desc.type = MT_MEMORY_RO;
+       else if (md->attribute & EFI_MEMORY_XP)
+               desc.type = MT_MEMORY_RW;
+
         create_mapping_late(mm, &desc, true);

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Ard Biesheuvel]

On Wed, 29 Oct 2025 at 10:55, Qiang Ma <maqianga@uniontech.com> wrote:
>
>
> 在 2025/10/28 21:42, Ard Biesheuvel 写道:
> > On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@uniontech.com> wrote:
> >>
> >> 在 2025/10/23 16:30, Ard Biesheuvel 写道:
> >>> On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
> >>>> In the efi_virtmap_init(), permission settings have been applied:
> >>>>
> >>>> static bool __init efi_virtmap_init(void)
> >>>> {
> >>>>           ...
> >>>>           for_each_efi_memory_desc(md)
> >>>>                   ...
> >>>>                   efi_create_mapping(&efi_mm, md);
> >>>>           ...
> >>>>           efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
> >>>>           ...
> >>>> }
> >>>>
> >>>> Therefore, there is no need to apply it again in the efi_create_mapping().
> >>>>
> >>>> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
> >>>>
> >>>> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> >>> No, efi_memattr_apply_permissions() uses the /optional/ memory
> >>> attributes table, whereas efi_create_mapping() uses the permission
> >>> attributes in the EFI memory map. The memory attributes table is
> >>> optional, in which case any RO/XP attributes from the memory map
> >>> should be used.
> >>>
> >> I see.
> >>
> >> Then, can it be modified like this?
> > No
> >
> >> --- a/arch/arm/kernel/efi.c
> >> +++ b/arch/arm/kernel/efi.c
> >> @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
> >> efi_memory_desc_t *md)
> >>                   desc.type = MT_MEMORY_RWX_NONCACHED;
> >>           else if (md->attribute & EFI_MEMORY_WC)
> >>                   desc.type = MT_DEVICE_WC;
> >> +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
> > This will be true for RO, XP or RO+XP.
> >
> >> +               desc.type = MT_MEMORY_RO;
> > This will apply RO permissions even to XP regions, which need to be writable.
> >
> Thanks for your review.
> I see.
>
> I can introduce a new type MT_MEMORY_RO_XP, to describe RO+XP,
> and then we can use the RO+XP attribute to implement memory mapping.
>

Why? The current code is working fine, no?

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Qiang Ma]

在 2025/10/29 22:15, Ard Biesheuvel 写道:
> On Wed, 29 Oct 2025 at 10:55, Qiang Ma <maqianga@uniontech.com> wrote:
>>
>> 在 2025/10/28 21:42, Ard Biesheuvel 写道:
>>> On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@uniontech.com> wrote:
>>>> 在 2025/10/23 16:30, Ard Biesheuvel 写道:
>>>>> On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
>>>>>> In the efi_virtmap_init(), permission settings have been applied:
>>>>>>
>>>>>> static bool __init efi_virtmap_init(void)
>>>>>> {
>>>>>>            ...
>>>>>>            for_each_efi_memory_desc(md)
>>>>>>                    ...
>>>>>>                    efi_create_mapping(&efi_mm, md);
>>>>>>            ...
>>>>>>            efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
>>>>>>            ...
>>>>>> }
>>>>>>
>>>>>> Therefore, there is no need to apply it again in the efi_create_mapping().
>>>>>>
>>>>>> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
>>>>>>
>>>>>> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
>>>>> No, efi_memattr_apply_permissions() uses the /optional/ memory
>>>>> attributes table, whereas efi_create_mapping() uses the permission
>>>>> attributes in the EFI memory map. The memory attributes table is
>>>>> optional, in which case any RO/XP attributes from the memory map
>>>>> should be used.
>>>>>
>>>> I see.
>>>>
>>>> Then, can it be modified like this?
>>> No
>>>
>>>> --- a/arch/arm/kernel/efi.c
>>>> +++ b/arch/arm/kernel/efi.c
>>>> @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
>>>> efi_memory_desc_t *md)
>>>>                    desc.type = MT_MEMORY_RWX_NONCACHED;
>>>>            else if (md->attribute & EFI_MEMORY_WC)
>>>>                    desc.type = MT_DEVICE_WC;
>>>> +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
>>> This will be true for RO, XP or RO+XP.
>>>
>>>> +               desc.type = MT_MEMORY_RO;
>>> This will apply RO permissions even to XP regions, which need to be writable.
>>>
>> Thanks for your review.
>> I see.
>>
>> I can introduce a new type MT_MEMORY_RO_XP, to describe RO+XP,
>> and then we can use the RO+XP attribute to implement memory mapping.
>>
> Why? The current code is working fine, no?
>
Yes, the current code is running normally.

The reasons for the modification are as follows:
I noticed that the arm64/RISC-V efi_create_mapping() always return 0,
but in the code where efi_virtmap_init() calls it, it is as follows:

ret = efi_create_mapping(&efi_mm, md);
if (ret) {
     pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
         &phys, ret);
     return false;
}

This return error print is unnecessary, so I want to remove it.
But, the return value of efi_create_mapping() in the arm doesn't
always return 0, so I'm trying to modify it.

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Ard Biesheuvel]

On Thu, 30 Oct 2025 at 08:37, Qiang Ma <maqianga@uniontech.com> wrote:
>
>
> 在 2025/10/29 22:15, Ard Biesheuvel 写道:
> > On Wed, 29 Oct 2025 at 10:55, Qiang Ma <maqianga@uniontech.com> wrote:
> >>
> >> 在 2025/10/28 21:42, Ard Biesheuvel 写道:
> >>> On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@uniontech.com> wrote:
> >>>> 在 2025/10/23 16:30, Ard Biesheuvel 写道:
> >>>>> On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
> >>>>>> In the efi_virtmap_init(), permission settings have been applied:
> >>>>>>
> >>>>>> static bool __init efi_virtmap_init(void)
> >>>>>> {
> >>>>>>            ...
> >>>>>>            for_each_efi_memory_desc(md)
> >>>>>>                    ...
> >>>>>>                    efi_create_mapping(&efi_mm, md);
> >>>>>>            ...
> >>>>>>            efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
> >>>>>>            ...
> >>>>>> }
> >>>>>>
> >>>>>> Therefore, there is no need to apply it again in the efi_create_mapping().
> >>>>>>
> >>>>>> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
> >>>>>>
> >>>>>> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> >>>>> No, efi_memattr_apply_permissions() uses the /optional/ memory
> >>>>> attributes table, whereas efi_create_mapping() uses the permission
> >>>>> attributes in the EFI memory map. The memory attributes table is
> >>>>> optional, in which case any RO/XP attributes from the memory map
> >>>>> should be used.
> >>>>>
> >>>> I see.
> >>>>
> >>>> Then, can it be modified like this?
> >>> No
> >>>
> >>>> --- a/arch/arm/kernel/efi.c
> >>>> +++ b/arch/arm/kernel/efi.c
> >>>> @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
> >>>> efi_memory_desc_t *md)
> >>>>                    desc.type = MT_MEMORY_RWX_NONCACHED;
> >>>>            else if (md->attribute & EFI_MEMORY_WC)
> >>>>                    desc.type = MT_DEVICE_WC;
> >>>> +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
> >>> This will be true for RO, XP or RO+XP.
> >>>
> >>>> +               desc.type = MT_MEMORY_RO;
> >>> This will apply RO permissions even to XP regions, which need to be writable.
> >>>
> >> Thanks for your review.
> >> I see.
> >>
> >> I can introduce a new type MT_MEMORY_RO_XP, to describe RO+XP,
> >> and then we can use the RO+XP attribute to implement memory mapping.
> >>
> > Why? The current code is working fine, no?
> >
> Yes, the current code is running normally.
>
> The reasons for the modification are as follows:
> I noticed that the arm64/RISC-V efi_create_mapping() always return 0,
> but in the code where efi_virtmap_init() calls it, it is as follows:
>
> ret = efi_create_mapping(&efi_mm, md);
> if (ret) {
>      pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
>          &phys, ret);
>      return false;
> }
>
> This return error print is unnecessary, so I want to remove it.

So what is preventing you from removing this from the RISC-V version?

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Qiang Ma]

在 2025/10/30 18:02, Ard Biesheuvel 写道:
> On Thu, 30 Oct 2025 at 08:37, Qiang Ma <maqianga@uniontech.com> wrote:
>>
>> 在 2025/10/29 22:15, Ard Biesheuvel 写道:
>>> On Wed, 29 Oct 2025 at 10:55, Qiang Ma <maqianga@uniontech.com> wrote:
>>>> 在 2025/10/28 21:42, Ard Biesheuvel 写道:
>>>>> On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@uniontech.com> wrote:
>>>>>> 在 2025/10/23 16:30, Ard Biesheuvel 写道:
>>>>>>> On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
>>>>>>>> In the efi_virtmap_init(), permission settings have been applied:
>>>>>>>>
>>>>>>>> static bool __init efi_virtmap_init(void)
>>>>>>>> {
>>>>>>>>             ...
>>>>>>>>             for_each_efi_memory_desc(md)
>>>>>>>>                     ...
>>>>>>>>                     efi_create_mapping(&efi_mm, md);
>>>>>>>>             ...
>>>>>>>>             efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
>>>>>>>>             ...
>>>>>>>> }
>>>>>>>>
>>>>>>>> Therefore, there is no need to apply it again in the efi_create_mapping().
>>>>>>>>
>>>>>>>> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
>>>>>>>>
>>>>>>>> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
>>>>>>> No, efi_memattr_apply_permissions() uses the /optional/ memory
>>>>>>> attributes table, whereas efi_create_mapping() uses the permission
>>>>>>> attributes in the EFI memory map. The memory attributes table is
>>>>>>> optional, in which case any RO/XP attributes from the memory map
>>>>>>> should be used.
>>>>>>>
>>>>>> I see.
>>>>>>
>>>>>> Then, can it be modified like this?
>>>>> No
>>>>>
>>>>>> --- a/arch/arm/kernel/efi.c
>>>>>> +++ b/arch/arm/kernel/efi.c
>>>>>> @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
>>>>>> efi_memory_desc_t *md)
>>>>>>                     desc.type = MT_MEMORY_RWX_NONCACHED;
>>>>>>             else if (md->attribute & EFI_MEMORY_WC)
>>>>>>                     desc.type = MT_DEVICE_WC;
>>>>>> +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
>>>>> This will be true for RO, XP or RO+XP.
>>>>>
>>>>>> +               desc.type = MT_MEMORY_RO;
>>>>> This will apply RO permissions even to XP regions, which need to be writable.
>>>>>
>>>> Thanks for your review.
>>>> I see.
>>>>
>>>> I can introduce a new type MT_MEMORY_RO_XP, to describe RO+XP,
>>>> and then we can use the RO+XP attribute to implement memory mapping.
>>>>
>>> Why? The current code is working fine, no?
>>>
>> Yes, the current code is running normally.
>>
>> The reasons for the modification are as follows:
>> I noticed that the arm64/RISC-V efi_create_mapping() always return 0,
>> but in the code where efi_virtmap_init() calls it, it is as follows:
>>
>> ret = efi_create_mapping(&efi_mm, md);
>> if (ret) {
>>       pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
>>           &phys, ret);
>>       return false;
>> }
>>
>> This return error print is unnecessary, so I want to remove it.
> So what is preventing you from removing this from the RISC-V version?
>
The current idea is to first remove the unnecessary return print from 
arm/arm64,
and then remove RISC-V later, as this RISC-V code is also adapted based 
on arm64.

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Ard Biesheuvel]

On Thu, 30 Oct 2025 at 11:25, Qiang Ma <maqianga@uniontech.com> wrote:
>
>
> 在 2025/10/30 18:02, Ard Biesheuvel 写道:
> > On Thu, 30 Oct 2025 at 08:37, Qiang Ma <maqianga@uniontech.com> wrote:
> >>
> >> 在 2025/10/29 22:15, Ard Biesheuvel 写道:
> >>> On Wed, 29 Oct 2025 at 10:55, Qiang Ma <maqianga@uniontech.com> wrote:
> >>>> 在 2025/10/28 21:42, Ard Biesheuvel 写道:
> >>>>> On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@uniontech.com> wrote:
> >>>>>> 在 2025/10/23 16:30, Ard Biesheuvel 写道:
> >>>>>>> On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
> >>>>>>>> In the efi_virtmap_init(), permission settings have been applied:
> >>>>>>>>
> >>>>>>>> static bool __init efi_virtmap_init(void)
> >>>>>>>> {
> >>>>>>>>             ...
> >>>>>>>>             for_each_efi_memory_desc(md)
> >>>>>>>>                     ...
> >>>>>>>>                     efi_create_mapping(&efi_mm, md);
> >>>>>>>>             ...
> >>>>>>>>             efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
> >>>>>>>>             ...
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> Therefore, there is no need to apply it again in the efi_create_mapping().
> >>>>>>>>
> >>>>>>>> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
> >>>>>>>>
> >>>>>>>> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
> >>>>>>> No, efi_memattr_apply_permissions() uses the /optional/ memory
> >>>>>>> attributes table, whereas efi_create_mapping() uses the permission
> >>>>>>> attributes in the EFI memory map. The memory attributes table is
> >>>>>>> optional, in which case any RO/XP attributes from the memory map
> >>>>>>> should be used.
> >>>>>>>
> >>>>>> I see.
> >>>>>>
> >>>>>> Then, can it be modified like this?
> >>>>> No
> >>>>>
> >>>>>> --- a/arch/arm/kernel/efi.c
> >>>>>> +++ b/arch/arm/kernel/efi.c
> >>>>>> @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
> >>>>>> efi_memory_desc_t *md)
> >>>>>>                     desc.type = MT_MEMORY_RWX_NONCACHED;
> >>>>>>             else if (md->attribute & EFI_MEMORY_WC)
> >>>>>>                     desc.type = MT_DEVICE_WC;
> >>>>>> +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
> >>>>> This will be true for RO, XP or RO+XP.
> >>>>>
> >>>>>> +               desc.type = MT_MEMORY_RO;
> >>>>> This will apply RO permissions even to XP regions, which need to be writable.
> >>>>>
> >>>> Thanks for your review.
> >>>> I see.
> >>>>
> >>>> I can introduce a new type MT_MEMORY_RO_XP, to describe RO+XP,
> >>>> and then we can use the RO+XP attribute to implement memory mapping.
> >>>>
> >>> Why? The current code is working fine, no?
> >>>
> >> Yes, the current code is running normally.
> >>
> >> The reasons for the modification are as follows:
> >> I noticed that the arm64/RISC-V efi_create_mapping() always return 0,
> >> but in the code where efi_virtmap_init() calls it, it is as follows:
> >>
> >> ret = efi_create_mapping(&efi_mm, md);
> >> if (ret) {
> >>       pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
> >>           &phys, ret);
> >>       return false;
> >> }
> >>
> >> This return error print is unnecessary, so I want to remove it.
> > So what is preventing you from removing this from the RISC-V version?
> >
> The current idea is to first remove the unnecessary return print from
> arm/arm64,

Please leave the ARM code alone.

> and then remove RISC-V later, as this RISC-V code is also adapted based
> on arm64.
>

RISC-V copied the ARM code and used it as a starting point. That does
not mean it has to remain that way.

Re: [PATCH 1/2] ARM/efi: Remove duplicate permission settings

[Qiang Ma]

在 2025/10/30 18:36, Ard Biesheuvel 写道:
> On Thu, 30 Oct 2025 at 11:25, Qiang Ma <maqianga@uniontech.com> wrote:
>>
>> 在 2025/10/30 18:02, Ard Biesheuvel 写道:
>>> On Thu, 30 Oct 2025 at 08:37, Qiang Ma <maqianga@uniontech.com> wrote:
>>>> 在 2025/10/29 22:15, Ard Biesheuvel 写道:
>>>>> On Wed, 29 Oct 2025 at 10:55, Qiang Ma <maqianga@uniontech.com> wrote:
>>>>>> 在 2025/10/28 21:42, Ard Biesheuvel 写道:
>>>>>>> On Mon, 27 Oct 2025 at 04:46, Qiang Ma <maqianga@uniontech.com> wrote:
>>>>>>>> 在 2025/10/23 16:30, Ard Biesheuvel 写道:
>>>>>>>>> On Thu, 23 Oct 2025 at 10:22, Qiang Ma <maqianga@uniontech.com> wrote:
>>>>>>>>>> In the efi_virtmap_init(), permission settings have been applied:
>>>>>>>>>>
>>>>>>>>>> static bool __init efi_virtmap_init(void)
>>>>>>>>>> {
>>>>>>>>>>              ...
>>>>>>>>>>              for_each_efi_memory_desc(md)
>>>>>>>>>>                      ...
>>>>>>>>>>                      efi_create_mapping(&efi_mm, md);
>>>>>>>>>>              ...
>>>>>>>>>>              efi_memattr_apply_permissions(&efi_mm, efi_set_mapping_permissions);
>>>>>>>>>>              ...
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> Therefore, there is no need to apply it again in the efi_create_mapping().
>>>>>>>>>>
>>>>>>>>>> Fixes: 9fc68b717c24 ("ARM/efi: Apply strict permissions for UEFI Runtime Services regions")
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Qiang Ma <maqianga@uniontech.com>
>>>>>>>>> No, efi_memattr_apply_permissions() uses the /optional/ memory
>>>>>>>>> attributes table, whereas efi_create_mapping() uses the permission
>>>>>>>>> attributes in the EFI memory map. The memory attributes table is
>>>>>>>>> optional, in which case any RO/XP attributes from the memory map
>>>>>>>>> should be used.
>>>>>>>>>
>>>>>>>> I see.
>>>>>>>>
>>>>>>>> Then, can it be modified like this?
>>>>>>> No
>>>>>>>
>>>>>>>> --- a/arch/arm/kernel/efi.c
>>>>>>>> +++ b/arch/arm/kernel/efi.c
>>>>>>>> @@ -65,16 +65,13 @@ int __init efi_create_mapping(struct mm_struct *mm,
>>>>>>>> efi_memory_desc_t *md)
>>>>>>>>                      desc.type = MT_MEMORY_RWX_NONCACHED;
>>>>>>>>              else if (md->attribute & EFI_MEMORY_WC)
>>>>>>>>                      desc.type = MT_DEVICE_WC;
>>>>>>>> +       else if (md->attribute & (EFI_MEMORY_RO | EFI_MEMORY_XP))
>>>>>>> This will be true for RO, XP or RO+XP.
>>>>>>>
>>>>>>>> +               desc.type = MT_MEMORY_RO;
>>>>>>> This will apply RO permissions even to XP regions, which need to be writable.
>>>>>>>
>>>>>> Thanks for your review.
>>>>>> I see.
>>>>>>
>>>>>> I can introduce a new type MT_MEMORY_RO_XP, to describe RO+XP,
>>>>>> and then we can use the RO+XP attribute to implement memory mapping.
>>>>>>
>>>>> Why? The current code is working fine, no?
>>>>>
>>>> Yes, the current code is running normally.
>>>>
>>>> The reasons for the modification are as follows:
>>>> I noticed that the arm64/RISC-V efi_create_mapping() always return 0,
>>>> but in the code where efi_virtmap_init() calls it, it is as follows:
>>>>
>>>> ret = efi_create_mapping(&efi_mm, md);
>>>> if (ret) {
>>>>        pr_warn("  EFI remap %pa: failed to create mapping (%d)\n",
>>>>            &phys, ret);
>>>>        return false;
>>>> }
>>>>
>>>> This return error print is unnecessary, so I want to remove it.
>>> So what is preventing you from removing this from the RISC-V version?
>>>
>> The current idea is to first remove the unnecessary return print from
>> arm/arm64,
> Please leave the ARM code alone.
I see.
>> and then remove RISC-V later, as this RISC-V code is also adapted based
>> on arm64.
>>
> RISC-V copied the ARM code and used it as a starting point. That does
> not mean it has to remain that way.
I can propose a patch specifically for RISC-V.