Re: git: f1e18f331923 - main - riscv: Exclude OpenSBI memory regions when booting with EFI

On 16 Apr 2025, at 15:30, Jessica Clarke <jrtc27@FreeBSD.org> wrote:
> 
> On 16 Apr 2025, at 15:21, Bojan Novković <bnovkov@FreeBSD.org> wrote:
>> 
>> The branch main has been updated by bnovkov:
>> 
>> URL: https://cgit.FreeBSD.org/src/commit/?id=f1e18f331923041980149fef46cdb2736e61debb
>> 
>> commit f1e18f331923041980149fef46cdb2736e61debb
>> Author:     Bojan Novković <bnovkov@FreeBSD.org>
>> AuthorDate: 2025-04-15 16:28:05 +0000
>> Commit:     Bojan Novković <bnovkov@FreeBSD.org>
>> CommitDate: 2025-04-16 14:20:13 +0000
>> 
>>   riscv: Exclude OpenSBI memory regions when booting with EFI
>> 
>>   OpenSBI uses the first PMP entry to prevent buggy supervisor
>>   software from overwriting the firmware [1]. However, this
>>   region may not be properly marked as reserved in the EFI map, leading
>>   to an access violation exception whenever the kernel
>>   attempts to write to a page from that region.
>> 
>>   Fix this by preemptively excluding first EFI memory map entry
>>   if it is marked as "BootServicesData".
>> 
>>   [1] https://github.com/riscv-non-isa/riscv-sbi-doc/pull/37
>> 
>>   Reported by: tuexen
>>   Tested by: tuexen
>>   Fixes: a2e2178402af
>>   Reviewed by:    imp, jrtc27
> 
> No I didn’t, I left a comment saying I didn’t like the concept.
> 
>>   Differential Revision:  https://reviews.freebsd.org/D49839
>> ---
>> sys/riscv/riscv/machdep.c | 32 ++++++++++++++++++++++++++++++--
>> 1 file changed, 30 insertions(+), 2 deletions(-)
>> 
>> diff --git a/sys/riscv/riscv/machdep.c b/sys/riscv/riscv/machdep.c
>> index 516dbde5ffaa..f253bc9a853b 100644
>> --- a/sys/riscv/riscv/machdep.c
>> +++ b/sys/riscv/riscv/machdep.c
>> @@ -541,6 +541,22 @@ fdt_physmem_exclude_region_cb(const struct mem_region *mr, void *arg __unused)
>> }
>> #endif
>> 
>> +static void
>> +efi_exclude_sbi_pmp_cb(struct efi_md *p, void *argp)
>> +{
>> + bool *first = (bool *)argp;
>> +
>> + if (!*first)
>> + return;
>> +
>> + *first = false;
>> + if (p->md_type == EFI_MD_TYPE_BS_DATA) {
>> + physmem_exclude_region(p->md_phys,
>> +    min(p->md_pages * EFI_PAGE_SIZE, L2_SIZE),
>> +    EXFLAG_NOALLOC);
> 
> Doesn’t this need EXFLAG_NODUMP like the FDT case?

Ping.

Jess