[PATCH v4 06/21] KVM: arm64: Add support for stage-2 map()/unmap() in generic page-table

[Alexandru Elisei]

Hi Will,

On 9/10/20 1:34 PM, Will Deacon wrote:
> On Thu, Sep 10, 2020 at 12:20:42PM +0100, Alexandru Elisei wrote:
>> On 9/7/20 4:23 PM, Will Deacon wrote:
>>> +static int stage2_map_walk_table_post(u64 addr, u64 end, u32 level,
>>> +				      kvm_pte_t *ptep,
>>> +				      struct stage2_map_data *data)
>>> +{
>>> +	int ret = 0;
>>> +
>>> +	if (!data->anchor)
>>> +		return 0;
>>> +
>>> +	free_page((unsigned long)kvm_pte_follow(*ptep));
>>> +	put_page(virt_to_page(ptep));
>>> +
>>> +	if (data->anchor == ptep) {
>>> +		data->anchor = NULL;
>>> +		ret = stage2_map_walk_leaf(addr, end, level, ptep, data);
>>> +	}
>> I had another look at this function. If we're back to the anchor entry, then that
>> means that we know from the pre-order visitor that 1. the mapping is supported at
>> this level and 2. that the pte was invalidated. This means that
>> kvm_set_valid_leaf_pte() will succeed in changing the entry. How about instead of
>> calling stage2_map_walk_leaf() -> stage2_map_walker_try_leaf() ->
>> kvm_set_valid_leaf_pte() we call kvm_set_valid_leaf_pte() directly, followed by
>> get_page(virt_to_page(ptep)? It would make the code a lot easier to follow
>> (stage2_map_walk_leaf() is pretty complicated, imo, but that can't really be
>> avoided), and also slightly faster.
> I'm not sure I agree. I would consider kvm_set_valid_leaf_pte() to be lower
> level, and not the right function to be calling here. Rather,
> stage2_map_walk_leaf() is what would have been called if we hadn't spotted
> the potential for a block mapping anyway, so I much prefer that symmetry. It
> also means that if stage2_map_walk_leaf() grows some extra logic in future
> that we need to take into account here, we won't miss anything.

Sure, that makes sense.

Thanks,
Alex