On 22.01.2018 13:50, David Hildenbrand wrote:
>
>>>
>>>> + if (!pmdp || pmd_none(*pmdp)) {
>>>> + spin_unlock(&gmap->guest_table_lock);
>>>> + return NULL;
>>>> + }
>>>> + /*
>>>> + * For plain 4k guests that do not run under the vsie it
>>>> + * suffices to take the pte lock later on. Thus we can unlock
>>>> + * the guest_table_lock here.
>>>> + */
>>>
>>> As discussed, the gmap_is_shadow() check is not needed. The comment
>>> should be something like
>>
>> IFF we'll never use this function to walk shadow tables, then you are
>> right. We can make it a policy and throw in a BUG_ON.
>
> Right. We never protect anything on a shadow gmap. We only mirror the
> access tights requested by the guest (which are then valid in the host).
For now I'll introduce a comment, a BUG_ON and get rid of the check.
>
>>
>> [...]
>>>> +static int gmap_protect_pte(struct gmap *gmap, unsigned long gaddr,
>>>> + pmd_t *pmdp, int prot, unsigned long bits)
>>>> +{
>>>> + int rc;
>>>> + pte_t *ptep;
>>>> + spinlock_t *ptl = NULL;
>>>> +
>>>> + /* We have no upper segment, let's go back and fix this up. */
>>>> + if (pmd_val(*pmdp) & _SEGMENT_ENTRY_INVALID)
>>>> + return -EAGAIN;
>>>
>>> This is essentially pmd_none(*pmdp), which you already verified in
>>> gmap_pmd_op_walk().
>>
>> Well, not really pmd_none is entry == ENTRY_EMPTY (only I bit set) not
>> entry & I.
>> Is there a path where we have an I bit on a pmd entry which has a valid pto?
>>
>
> Thing idte only sets the invalid bit. But can this check than go into
> gmap_pmd_op_walk? (replacing pmd_none() ?)
I'll have to think about that, but quite possibly yes.
The problem comes from Martin's wish to properly handle prot_none entries.
Attachment:
signature.asc
Description: OpenPGP digital signature
