[PATCH v9 5/6] KVM: arm64: Allow cacheable stage 2 mapping using VMA flags

[Ankit Agrawal]

Thank you Jason and David to review this and other patches in the series!
Comments inline.

>>> +             * COW VM_PFNMAP is possible when doing a MAP_PRIVATE
>>> +             * /dev/mem mapping on systems that allow such mapping.
>>> +             * Reject such case.
>>>               */
>>> -            s2_force_noncacheable = true;
>>> +            if (is_cow_mapping(vm_flags))
>>> +                    return -EINVAL;
>>
>> I still would like an explanation why we need to block this.
>>
>> COW PFNMAP is like MIXEDMAP, you end up with a VMA where there is a
>> mixture of MMIO and normal pages. Arguably you are supposed to use
>> vm_normal_page() not pfn_is_map_memory(), but that seems difficult for
>> KVM.
>>
>> Given we exclude the cachable case with the pfn_is_map_memory() we
>> know this is the non-struct page memory already, so why do we need to
>> block the COW?
>>
>> I think the basic rule we are going for is that within the VMA the
>> non-normal/special PTE have to follow the vma->vm_pgprot while the
>> normal pages have to be cachable.
>>
>> So if we find a normal page (ie pfn_is_map_memory()) then we know it
>> is cachable and s2_force_noncacheable = false. Otherwise we use the
>> vm_pgprot to decide if the special PTE is cachable.
>>
>> David can you think of any reason to have this is_cow_mapping() test?
>
> I think with that reasoning, it should be fine to drop it.
>
> I think, the COW test made sense when we were talking about limiting it
> to VM_PFNMAP only and simplifying by dropping other checks. Then, it
> would have identified that something is certainly not "normal" memory.
>

Ack, I'll drop the check in the next version.

> Then this logic that immediately follows:
>
>        if (is_vma_cacheable && s2_force_noncacheable)
>                return -EINVAL;
>
> Doesn't make alot of sense either, the only cases that set
> s2_force_noncacheable=true are the else block of 'if (is_vma_cacheable)'
> so this is dead code too.

I had left it there so that that condition doesn't get violated in the
future. But perhaps it doesn't make much sense to keep it. I'll remove in
the next version.

> Seems like this still needs some cleanup to remove these impossible
> conditions. The logic make sense to me otherwise though.

Sure, I'll fix that in the next version.