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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
