arch/*: Align virtual/physical address validation on x86 and arm64

[kha-dinh]

Prerequisite checklist

• Read the contribution guidelines regarding submitting new changes to the project;
• Tested your changes against relevant architectures and platforms;
• Ran the checkpatch.uk on your commit series before opening this PR;
• Updated relevant documentation.

Base target

• Architecture(s): x86, arm64
• Platform(s): kvm
• Application(s): helloworld

Additional configuration

Description of changes

This PR fixes the signature of the address range validation functions ukarch_paddr_range_isvalid, and ukarch_vaddr_range_isvalid to take the length of the range, instead of the start and end addresses.
It also make the logic consistent across x86 and arm64.

[mogasergiu]

Hi! Thank you a lot for the contribution 🛩️ ! I remember this resulted following an offline discussion we had. But now that I had to review it and take a second look, I believe this should be made more generic instead.

static __paddr_t x86_pg_maxphysaddr;

-#define X86_PG_VALID_PADDR(paddr)	((paddr) < x86_pg_maxphysaddr)
+#define X86_PG_VALID_PADDR(paddr)	((paddr) <= x86_pg_maxphysaddr)

int ukarch_paddr_range_isvalid(__paddr_t start, __paddr_t end)
{
	UK_ASSERT(start <= end);
	return (X86_PG_VALID_PADDR(start) && X86_PG_VALID_PADDR(end));
}

This is what I have now in mind. What do you think 😄?

[kha-dinh]

I had some thoughts about this.

The value assigned to x86_pg_maxphysaddr is (1UL << max_addr_bit);, which is not the maximum address. E.g., the maximum possible address that can be indexed by 16-bit is 0xFFFF which is (1UL << max_addr_bit) - 1.
X86_PG_VALID_PADDR makes up for this by using <. Your proposed fix would make sense, and make things more clear if we also fix x86_pg_maxphysaddr.

On the other hand, code that uses ukarch_paddr_range_isvalid seems to implicitly agree that the range for checking is [start, end) (e.g., ukarch_paddr_range_isvalid(start, start + len)) so the current PR also not wrong.

In the end, what do you think of the following fixes:

• Fix the address assignment to x86_pg_maxphysaddr
• My fix to ukarch_paddr_range_isvalid, and update its comment to make it more clear.
• Your fix to X86_PG_VALID_PADDR

[mogasergiu]

I had some thoughts about this.

The value assigned to x86_pg_maxphysaddr is (1UL << max_addr_bit);, which is not the maximum address. E.g., the maximum possible address that can be indexed by 16-bit is 0xFFFF which is (1UL << max_addr_bit) - 1. X86_PG_VALID_PADDR makes up for this by using <. Your proposed fix would make sense, and make things more clear if we also fix x86_pg_maxphysaddr.

On the other hand, code that uses ukarch_paddr_range_isvalid seems to implicitly agree that the range for checking is [start, end) (e.g., ukarch_paddr_range_isvalid(start, start + len)) so the current PR also not wrong.

In the end, what do you think of the following fixes:

• Fix the address assignment to x86_pg_maxphysaddr
• My fix to ukarch_paddr_range_isvalid, and update its comment to make it more clear.
• Your fix to X86_PG_VALID_PADDR

I would rather have it aligned with ARM, check ARM64_PADDR_MAX and ARM64_PADDR_VALID

[razvanvirtan]

@kha-dinh Thanks for the PR!
I agree that the code should be aligned with arm64, so the first and third bullets in your list should be applied for sure.
As about the second bullet, I think it is also needed to ensure the correctness of the existing code (e.g. https://github.com/unikraft/unikraft/blob/staging/lib/ukfallocbuddy/fallocbuddy.c#L498). @mogasergiu WDYT?

[mogasergiu]

Lol so we are back to the way it was initially? 🤣 Anyway, it's a needed fix and I guess this is way too trivial (unlike your SEV series 😉) to ponder upon for too long.

Reviewed-by: Sergiu Moga sergiu@unikraft.io

[mogasergiu]

Hmm, wait does this mean that this change should be done for ARM as well? This looks inconsistent. Am I missing something?

[kha-dinh]

Hey, I think this solution is not right after all. It is the user of the function that uses it incorrectly by overestimating the end address, so it should be fixed there instead. What do you think about removing this commit and keeping the previous one?

[mogasergiu]

I think this solution is not right after all

Agreed.

It is the user of the function that uses it incorrectly by overestimating the end address

Where was this wrong use again?

What do you think about removing this commit and keeping the previous one?

I agree 👍. This would also align this with ARM, right? So this means we'll only keep the first commit here.

[kha-dinh]

Where was this wrong use again?

It was a case where UEFI contained an address range that would trigger an assertion failure. There are some changes to my environment, so I could not get this behavior again 🥲. The bug was triggered when ukplat_pt_add_mem was called on the memory range.

[    0.000000] Info: [libkvmplat] <bootinfo.c @  101>  0000feffc000-0000ff000000 000000004000 r-- 00000000feffc000 rsvd
[    0.000000] Info: [libkvmplat] <bootinfo.c @  101>  0000ffc00000-000100000000 000000400000 rw- 00000000ffc00000 rsvd
[    0.000000] Info: [libkvmplat] <bootinfo.c @  101>  00fd00000000-010000000000 000300000000 r-- 000000fd00000000 rsvd

Now, looking at it again, 010000000000 would be out of valid address space by 1, where the max address is supposed to be 0xFFFFF.... This is still true if we align the max physical address definition to ARM64.

[mogasergiu]

Where was this wrong use again?

It was a case where UEFI contained an address range that would trigger an assertion failure. There are some changes to my environment, so I could not get this behavior again 🥲. The bug was triggered when ukplat_pt_add_mem was called on the memory range.

[    0.000000] Info: [libkvmplat] <bootinfo.c @  101>  0000feffc000-0000ff000000 000000004000 r-- 00000000feffc000 rsvd
[    0.000000] Info: [libkvmplat] <bootinfo.c @  101>  0000ffc00000-000100000000 000000400000 rw- 00000000ffc00000 rsvd
[    0.000000] Info: [libkvmplat] <bootinfo.c @  101>  00fd00000000-010000000000 000300000000 r-- 000000fd00000000 rsvd

Now, looking at it again, 010000000000 would be out of valid address space by 1, where the max address is supposed to be 0xFFFFF.... This is still true if we align the max physical address definition to ARM64.

Ah, I remember now... this means this stuff can also happen for ARM with such a region. What if we did it like this for this PR

--- a/arch/arm/arm64/include/uk/asm/paging.h
+++ b/arch/arm/arm64/include/uk/asm/paging.h
@@ -146,8 +146,8 @@ struct ukarch_pagetable {
 /* Any PTE with bit[0] == 0 is invalid */
 #define PT_Lx_PTE_INVALID(lvl)         0x0UL
 
-#define ARM64_PADDR_MAX                        ((1ULL << ARM64_PADDR_BITS) - 1)
-#define ARM64_VADDR_MAX                        ((1ULL << ARM64_VADDR_BITS) - 1)
+#define ARM64_PADDR_MAX                        (1ULL << ARM64_PADDR_BITS)
+#define ARM64_VADDR_MAX                        (1ULL << ARM64_VADDR_BITS)
 
 #define ARM64_PADDR_VALID(paddr)       (paddr <= (__paddr_t)ARM64_PADDR_MAX)
 #define ARM64_VADDR_VALID(vaddr)                               \
--- a/include/uk/arch/paging.h
+++ b/include/uk/arch/paging.h
@@ -370,7 +370,8 @@ int PAGE_Lx_IS(__pte_t pte, unsigned int lvl);
  * 64 bits for the virtual address.
  *
  * @param start the start of the virtual address range
- * @param end the end of the virtual address range
+ * @param end the end of the virtual address range, where end is equal to
+ *       (start + length of range)
  *
  * @return a non-zero value if the addresses in the range are supported
  */
@@ -386,7 +387,8 @@ int ukarch_vaddr_range_isvalid(__vaddr_t start, __vaddr_t end);
  * installed in the system.
  *
  * @param start the start of the physical address range
- * @param end the end of the physical address range
+ * @param end the end of the physical address range, where end is equal to
+ *       (start + length of range)
  *
  * @return a non-zero value if the addresses in the range are supported
  */
 --- a/plat/common/include/x86/paging.h
+++ b/plat/common/include/x86/paging.h
@@ -244,7 +244,7 @@ pgarch_kunmap(struct uk_pagetable *pt __unused, __vaddr_t vaddr __unused,
 
 static __paddr_t x86_pg_maxphysaddr;
 
-#define X86_PG_VALID_PADDR(paddr)      ((paddr) < x86_pg_maxphysaddr)
+#define X86_PG_VALID_PADDR(paddr)      ((paddr) <= x86_pg_maxphysaddr)
 
 int ukarch_paddr_range_isvalid(__paddr_t start, __paddr_t end)
 {

I think this way the two arch's would be aligned and the addition to the function's description could clarify to its potential user what it expects. And have this all in a single commit like:
arch/*: Align paddr/vaddr range validation between x86 and ARM64

Hmm, the validation of the vaddr is also misaligned: x86 only checks for cannonical addresses and thus no need for range, while ARM checks for range. Commit b0c5224663c0a3b133703012892bff11d3e264a0 seems to be clear as to why: the upper canonnical range is considered as invalid so I guess it is fine.

    arch/arm: plat/common: Implement paging support for arm64
    
    This commit implements virtual memory support for arm64 as defined
    by VMSAv8-64. The implementation supports 48-bit VAs with a 4KiB granule,
    and uses the lower address space controlled by TTBR0_EL1, i.e.
    0x0000_0000_0000_0000 - 0x0000_ffff_ffff_ffff.
    
    The last 512GiB (0x0000_ff80_0000_0000) are reserved, and are direct-mapped
    to the first 512GiB (0x0000_0000_0000_0000) to allow fast translation of
    the pagetables.
    
    The upper range controlled by TTBR1_EL1 at 0xffff_0000_0000_0000 is
    configured as invalid.

Since you were part of the discussion of this commit @michpappas @razvanvirtan. What do you whink about the proposed changes.

I would go with the proposed diff above. What do you think @kha-dinh? Could there be any potential pitfalls? 🤔

[razvanvirtan]

I agree with the fact that taking the address and length as parameters in ukarch_paddr_range_isvalid() makes more sense for the future.

For the purpose of this PR, I think we should also keep the logic for the macros names. This means that on both ARM64 and x86, the max paddr macros should be defined as (1 << PADDRBITS) - 1, because they are supposed to represent the maximum usable physical address, not the physical memory size. Therefore, I suggest the following changes:

• we keep the first commit in this PR, so that we correctly define x86_pg_maxphysaddr
• we keep the second commit in this PR, correctly defining ukarch_paddr_range_isvalid() on x86
• we add a similar fix for ukarch_paddr_range_isvalid() on ARM64 (checking for end - 1 instead of end)
• we add the comments suggested above by @mogasergiu, so that we have the ukarch_paddr_range_isvalid() function clearly documented

WDYT?

[mogasergiu]

Yes, I also like that. Actually, more than what I have previously suggested. You are making a very good point @razvanvirtan. 🧠 🧐 🥇

[kha-dinh]

Sounds good to me 💯.

[michpappas]

To me it seems that the root of the problem is that the current interface is easy to misuse, so instead of working around the problem, perhaps we should apply @mogasergiu's suggestion to pass (addr, len`)? My guess would be that it should require minimal effort. But since folks already came into consensus, the currently proposed approach is also okay.

[mogasergiu]

include/uk/arch/paging.h:380:	ukarch_vaddr_range_isvalid(vaddr, vaddr)			\
include/uk/arch/paging.h:396:	ukarch_paddr_range_isvalid(paddr, paddr)
lib/ukfallocbuddy/fallocbuddy.c:498:	UK_ASSERT(ukarch_paddr_range_isvalid(start, start + len));
lib/ukfallocbuddy/fallocbuddy.c:1369:	UK_ASSERT(ukarch_paddr_range_isvalid(paddr, paddr + len));
lib/ukfallocbuddy/fallocbuddy.c:1443:	UK_ASSERT(ukarch_paddr_range_isvalid(paddr, paddr + len));
lib/ukvmem/vma_dma.c:41:	UK_ASSERT(ukarch_paddr_range_isvalid(args->paddr, args->paddr + len));
lib/ukvmem/vmem.c:65:	UK_ASSERT(ukarch_vaddr_isvalid(vas->vma_base));
lib/ukvmem/vmem.c:218:	UK_ASSERT(ukarch_vaddr_range_isvalid(vstart, vend));
lib/ukvmem/vmem.c:665:	UK_ASSERT(ukarch_vaddr_range_isvalid(va, va + len));
plat/common/include/arm/arm64/paging.h:349:	UK_ASSERT(ukarch_paddr_range_isvalid(start, start + len));
plat/common/include/x86/paging.h:326:	UK_ASSERT(ukarch_paddr_range_isvalid(start, start + len));
plat/common/paging.c:270:	UK_ASSERT(ukarch_paddr_range_isvalid(start, start + len));
plat/common/paging.c:310:	UK_ASSERT(ukarch_paddr_range_isvalid(start, start + len));
plat/common/paging.c:387:	UK_ASSERT(ukarch_vaddr_isvalid(vaddr));
plat/common/paging.c:536:	UK_ASSERT(ukarch_vaddr_range_isvalid(vaddr, vaddr + len));
plat/common/paging.c:541:		UK_ASSERT(ukarch_paddr_range_isvalid(paddr, paddr + len));
plat/common/paging.c:959:		UK_ASSERT(ukarch_vaddr_range_isvalid(vaddr, vaddr + len));
plat/common/paging.c:1240:		UK_ASSERT(ukarch_vaddr_range_isvalid(vaddr, vaddr + len));

These would be the places that would need necessary changes if the interface were to be changed. This might require extensive testing 🤔.

My guess would be that it should require minimal effort. But since folks already came into consensus, the currently proposed approach is also okay.

👍 If @kha-dinh agrees to also changing those bits and pieces of code shown above (hopefully I did not miss any) as well as the interface signature, I am fine with that. But I'd say let's go with what @razvanvirtan suggested for now, and put this change somewhere in the backlog if @kha-dinh decides to not do these changes.

Feel free to go with whatever you like @kha-dinh 🙏. Either way is fine IMO 👍.

[razvanvirtan]

@kha-dinh Can you please update the PR according to our conversation above? 🙏

[michpappas]

@kha-dinh thanks a lot for the updates. One minor detail on the commit messages, IMO it would be preferable to repeat the description rather than stating "same as last commit", as commits can be in principle cherry-picked here and there.

Once that is fixed, it's all good from my side.

Reviewed-by: Michalis Pappas michalis@unikraft.io

[mogasergiu]

Hi @kha-dinh. Wow, you went the extra mile and also updated the signature as well as introduced a per-arch address validation function to avoid that 1 as a second argument to the range validation function 🎉. Your cooperation is greatly appreciated 💪.

I have one minor comment regarding the commit placement. I would like the last three commits, the ones that update the length arguments, to be squashed into arch/*: Update and align paddr/vaddr validation on x86 and arm64. This way we can avoid runtime errors as well as build warnings when git bisecting. But it just so happens that there have been extensive discussions about this sort of stuff during plat re-arch related work. Therefore, I am going to CC @michpappas, the plat re-arch master himself, to make sure we have consensus on this somewhat trivial matter before asking you to make this change.

But also, @kha-dinh, what is your opinion regarding this matter: would you leave the commits as is to have higher granularity and ease review, or squash to avoid issues when git bisecting as well as having duplicate commits in the history?

Either way, I am really thankful for your responsiveness and cooperation. Great OSS spirit right here 💯.

[razvanvirtan]

Thanks a lot for the patience of applying all the comments so far @kha-dinh!

I agree with @michpappas regarding repeating the commit message instead of the "Same as the previous commit." line.

One additional thought that is kind of out of the purpose of this PR, but I would like to ask: isn't it a bit strange that for ARM64 we implement the validation functions for both physical and virtual addresses in the arch/asm/ header, while for x86_64 the physical address validation is implemented in the plat/common header and the virtual address validation is implemented in arch/asm/? (Not sure if this has already been part of a plat re-arch discussion.)

Reviewed-by: Razvan Virtan virtanrazvan@gmail.com

[kha-dinh]

I updated the commit messages as suggested by @michpappas and @razvanvirtan.

But also, @kha-dinh, what is your opinion regarding this matter: would you leave the commits as is to have higher granularity and ease review, or squash to avoid issues when git bisecting as well as having duplicate commits in the history?

I am fine with either way. I initially split the commits so that the owner of each module can review them. I have never tried bisecting, but it seems to be a valid concern. For now, I would just update the commit messages and leave the discussion for another day.

As a related question, do we have a rulebook for git, e.g., regarding how to handle PR dependencies, etc..

[mogasergiu]

I am fine with either way. I initially split the commits so that the owner of each module can review them.

Totally agree. Perhaps we could make a rule for making high granularity commits in the beginning just for review purposes, but then we could squash right before merge, so that git bisecting is not affected and the commit history is not polluted with duplicate commits.

As a related question, do we have a rulebook for git, e.g., regarding how to handle PR dependencies, etc..

Good point. Not yet unfortunately, or not that I know of :(. We will have some related discussions about this stuff during our maintainers' meeting 💪.

@michpappas @razvanvirtan What do you think? Now that we are about to merge, shall we squash the last three commits, as they seem to be duplicate? This could be the first PR that would follow this methodology: high commits granularity during review (duplicate looking commits), squash right before merge after consensus is reached.

For now, I would just update the commit messages and leave the discussion for another day.

I am fine with this either way 👍. I would like for @michpappas @razvanvirtan to answer the above question though. But since we are on a schedule and this discussion has been going on for too long, I'll give my approval anyway 😄.

Reviewed-by: Sergiu Moga sergiu@unikraft.io

[razvanvirtan]

@mogasergiu @kha-dinh Squashing sounds good to me. I am thinking about the scenario where one tries to find the commit that introduced a certain bug. Even though that commit may not be part of this PR, when you revert back to let's say 19b41d7, you will see additional problems because 04a1ad7 and 199adbf are not there anymore.

For me that kind of situation seems a bit confusing and it would be solved by a single commit.

[michpappas]

To summarize the current policy on this type of PRs:

If a commit changes an API, all updates in the various affected libraries go into the same commit so that we don't break the build. The namespace in the subject of the commit message should be set to the library that owns the API.

All other cases where a PR involves changes into multiple libraries that don't have compile-time dependencies, require these changes into separate commits per library.

[kha-dinh]

I squashed the commits since it is the consensus.

[razvand]

@michpappas , @razvanvirtan , @michpappas , please reply if you agree with the current content of this PR. After that, I will merge it.

[mogasergiu]

Reviewed-by: Sergiu Moga sergiu@unikraft.io

[michpappas]

Thanks @kha-dinh

Reviewed-by: Michalis Pappas michalis@unikraft.io

[razvand]

Approved-by: Razvan Deaconescu razvand@unikraft.io