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

[Qiang Ma <maqianga@uniontech.com>]

在 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.