On Thu, Dec 08, 2022, Oliver Upton wrote:
On Thu, Dec 08, 2022 at 12:18:07AM +0000, Sean Christopherson wrote:
[...]
Together, what about? The #ifdef is a bit gross, especially around "hi_start", but it's less duplicate code. And IMO, having things bundled in the same place makes it a lot easier for newbies (to arm64 or kernel coding in general) to understand what's going on and why arm64 is different.
I'd rather we not go this route. We really shouldn't make any attempt to de-dupe something that is inherently architecture specific.
For example:
- /*
* All architectures supports splitting the virtual address space into* a high and a low half. Populate both halves, except for arm64 which* currently uses only TTBR0_EL1 (arbitrary selftests "logic"), i.e.* only has a valid low half.*/- sparsebit_num_t nr_va_bits = (1ULL << (vm->va_bits - 1)) >> vm->page_shift;
This is still wrong for arm64. When we say the VA space is 48 bits, we really do mean that TTBR0 is able to address a full 48 bits. So this truncates the MSB for the addressing mode.
Ah, I missed the lack of a "-1" in the arm64 code.
With the code living in the arm64 side of the shop, I can also tailor the comment to directly match the architecture to provide breadcrumbs tying it back to the Arm ARM.
The main reason why I don't like splitting the code this way is that it makes it harder for non-arm64 folks to understand what makes arm64 different. Case in point, my overlooking of the "-1". I read the changelog and the comment and still missed that small-but-important detail, largely because I am completely unfamiliar with how TTBR{0,1}_EL1 works.
Actually, before we do anything, we should get confirmation from the s390 and RISC-V folks on whether they have a canonical hole like x86, i.e. maybe x86 is the oddball.
Anyways, assuming one architecture is the oddball (I'm betting it's x86), I have no objection to bleeding some of the details into the common code, including a large comment to document the gory details. If every architecture manges to be different, then yeah, a hook is probably warranted.
That said, I also don't mind shoving a bit of abstraction into arch code if that avoids some #ifdef ugliness or allows for better documentation, flexibility, etc. What I don't like is duplicating the logic of turning "VA bits" into the bitmap.
E.g. something like this would also be an option. Readers would obviously need to track down has_split_va_space, but that should be fairly easy and can come with a big arch-specific comment, and meanwhile the core logic of how selftests populate the va bitmaps is common.
Or if arm64 is the only arch without a split, invert the flag and have arm64 set the vm->has_combined_va_space or whatever.
static void vm_vaddr_populate_bitmap(struct kvm_vm *vm) { unsigned int eff_va_bits = vm->va_bits; sparsebit_num_t nr_bits;
/* blah blah blah */ if (vm->has_split_va_space) eff_va_bits--;
nr_bits = (1ULL << eff_va_bits) >> vm->page_shift;
sparsebit_set_num(vm->vpages_valid, 0, nr_va_bits);
if (vm->has_split_va_space) sparsebit_set_num(vm->vpages_valid, (~((1ULL << eff_va_bits) - 1)) >> vm->page_shift, nr_bits); }