Re: [PATCH v4 28/34] KVM: arm64: Use page-table to track page ownership

On Friday 12 Mar 2021 at 09:32:06 (+0000), Will Deacon wrote:

I'm not saying to use the VMID directly, just that allocating half of the
pte feels a bit OTT given that the state of things after this patch series
is that we're using exactly 1 bit.

Right, and that was the reason for the PROT_NONE approach in the
previous version, but we agreed it'd be worth generalizing to allow for
future use-cases :-)

quoted

quoted

quoted

@@ -517,28 +543,36 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level,
 	if (!kvm_block_mapping_supported(addr, end, phys, level))
 		return -E2BIG;
 
-	new = kvm_init_valid_leaf_pte(phys, data->attr, level);
-	if (kvm_pte_valid(old)) {
+	if (kvm_pte_valid(data->attr))

This feels like a bit of a hack to me: the 'attr' field in stage2_map_data
is intended to correspond directly to the lower/upper attributes of the
descriptor as per the architecture, so tagging the valid bit in there is
pretty grotty. However, I can see the significant advantage in being able
to re-use the stage2_map_walker functionality, so about instead of nobbling
attr, you set phys to something invalid instead, e.g.:

	#define KVM_PHYS_SET_OWNER	(-1ULL)

That'll confuse kvm_block_mapping_supported() and friends I think, at
least in their current form. If you _really_ don't like this, maybe we
could have an extra 'flags' field in stage2_map_data?

I was pondering this last night and I thought of two ways to do it:

1. Add a 'bool valid' and then stick the owner and the phys in a union.
   (yes, you'll need to update the block mapping checks to look at the
    valid flag)

Right, though that is also used for the hyp s1 which doesn't use any of
this ATM. That shouldn't be too bad to change, I'll have a look.

2. Go with my latter suggestion:

quoted

quoted

Is there ever a reason to use kvm_pgtable_stage2_set_owner() to set an
owner of 0, or should you just use the map/unmap APIs for that? If so,
then maybe the key is simply if owner_id is non-zero, then an invalid
entry is installed?

I couldn't find a good reason to restrict it, as that wouldn't change
the implementation much anyway. Also, if we added the right CMOs, we
could probably remove the unmap walker and re-express it in terms of
set_owner(0) ... But I suppose that is for later :-)

The idea being that if owner is 0, then we install a mapping for phys, but
if owner is !0 then we set the invalid mapping.

And I could even special-case set_owner(0) by calling unmap under the
hood so the caller doesn't need to care, but it's a bit yuck.

(1) is probably the less hacky option... what do you reckon?

Agreed, (1) is a bit nicer. I was also considering setting phys = BIT(63)
in the set_owner() path. That makes it obvious it is an invalid PA, and
it should retain the nice alignment properties I need. But I suppose an
explicit flag makes it easier to reason about, so I'll have a go at it.

Thanks,
Quentin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel