Page MenuHomeFreeBSD

efi: Update efi_phys_to_kva to accept the size of the object being mapped.
AcceptedPublic

Authored by alfredo.mazzinghi_cl.cam.ac.uk on May 23 2023, 5:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 27, 7:28 AM
Unknown Object (File)
Fri, Sep 27, 12:08 AM
Unknown Object (File)
Mon, Sep 23, 6:02 PM
Unknown Object (File)
Thu, Sep 19, 6:24 PM
Unknown Object (File)
Tue, Sep 17, 11:42 AM
Unknown Object (File)
Tue, Sep 17, 5:25 AM
Unknown Object (File)
Thu, Sep 12, 2:41 PM
Unknown Object (File)
Wed, Sep 11, 9:55 AM

Details

Summary

This allows to check that the entire object falls into the direct map.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 51834
Build 48725: arc lint + arc unit

Event Timeline

I like it. But let's get Andy and kib to sign off on it too...

This revision is now accepted and ready to land.May 23 2023, 6:02 PM

I think it would be cleaner to have macro like PHYS_SZ_IN_DMAP(pa, sz) which would check for [pa, pa+sz) belonging to the DMAPed region.

Added the PHYS_SZ_IN_DMAP macro as requested.

Currently this is only added for arm64 and amd64, which are the platforms that use it.
I'm not sure what the policy is in this case, should I add the same macro for other archs to keep the interface consistent?

This revision now requires review to proceed.Jun 1 2023, 2:48 PM

Only add the macro where needed.

sys/amd64/include/vmparam.h
247

No, this is not what I mean. PHYS_SZ_IN_DMAP(pa, sz) should verify that [pa, ps+sz) is mapped by DMAP.

Should now be fixed. I misinterpreted the desired behaviour for PHYS_SZ_IN_DMAP.

sys/amd64/include/vmparam.h
248

Should the check verify pa+sz-1 instead of pa+sz? Consider the last page in the DMAP, at the address x. If you want to check if the whole page belongs to DMAP-mapped region, you would check the x+PAGE_SIZE address, which lives in the next page not covered by DMAP.

alfredo.mazzinghi_cl.cam.ac.uk added inline comments.
sys/amd64/include/vmparam.h
248

Uh, yes I think it should, the check in PHYS_IN_DMAP uses strict inequality for (pa) < dmaplimit.

Fixed off-by-one check for the DMAP limit.

This revision is now accepted and ready to land.Jun 2 2023, 7:22 PM
sys/amd64/include/vmparam.h
247

FYI, downstream in CheriBSD we have found uses for this on RISC-V as well, so I had suggested to Alfredo to move this to <vm/vm_param.h>. While PHYS_IN_DMAP is inherently MD, this new macro is not.

One final suggestion on naming might be to call the new macro DMAP_CONTAINS to match the pattern from the recently added kstack_contains function.

In D40244#923550, @jhb wrote:

One final suggestion on naming might be to call the new macro DMAP_CONTAINS to match the pattern from the recently added kstack_contains function.

That could be confused with testing whether a virtual address is within the DMAP mapping.

alfredo.mazzinghi_cl.cam.ac.uk marked an inline comment as done.

Move PHYS_SZ_IN_DMAP to the machine-independent vm/vm_param.h

This revision now requires review to proceed.Jun 19 2023, 8:01 PM
sys/vm/vm_param.h
136 ↗(On Diff #123446)

You do not need this guard for #define.
But, wouldn't it be better to place the new macro in vm/pmap.h instead? Then, I think, you do not need to add vm/vm_param.h includes to efirt_machdep.c's.

sys/vm/vm_param.h
136 ↗(On Diff #123446)

I'll drop the guard if it isn't necessary. I think vm_param.h seems a more natural place since the original definitions are in machine/vmparam.h instead of machine/pmap.h, but I can move it if avoiding extra includes is a priority.

sys/vm/vm_param.h
136 ↗(On Diff #123446)

About vm_param.h vs pmap.h, I do not insist, it was only a suggestion.

sys/vm/vm_param.h
136 ↗(On Diff #123446)

I had guessed at vm/vm_param.h when suggesting to move this downstream due to the machine/vmparam.h for the original location, but vm/pmap.h might indeed be the better header? I can never remember which headers to actually include for the DMAP macros and tend to just start adding more vm/*.h until it compiles. :( We could really use a DMAP.9 or the like that described the API and gave the suggested #includes to use, etc.

Dropped #ifdef PHYS_HAS_DMAP for the PHYS_SZ_IN_DMAP macro definition.

This revision is now accepted and ready to land.Jun 27 2023, 11:22 PM