On 08.11.2017 11:40, David Hildenbrand wrote:
> On 06.11.2017 23:29, Janosch Frank wrote:
>> This patch reworks the gmap_protect_range logic and extracts the pte
>> handling into an own function. Also we do now walk to the pmd and make
>> it accessible in the function for later use. This way we can add huge
>> page handling logic more easily.
>
> I just realized (and hope it is correct), that any gmap_shadow() checks
> in e.g. gmap_pte_op_walk() are superfluous. This code is never reached.
>
> This would imply that also in this patch, you can drop all
> gmap_is_shadow(gmap) checks and instead add BUG_ON(gmap_is_shadow()) to
> all functions.
>
> Will double check and prepare a cleanup for existing code.
Hrm, looks like it.
Be aware, that I'm very protective about changes in the GMAP, especially
on the VSIE part. I've had minimal changes on these patches setting me
back for weeks, so if you want to change things, be absolutely sure,
that it will work (also with further changes) or add them after this set
is merged.
Thanks for having a look!
>> +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;
>> +
>> + if (gmap_is_shadow(gmap)) {
>> + ptep = pte_offset_map(pmdp, gaddr);
>> + } else {
>> + ptep = pte_alloc_map_lock(gmap->mm, pmdp, gaddr, &ptl);
>> + if (!ptep)
>> + return -ENOMEM;
>> + }
>
> if (gmap_is_shadow(gmap))
> ptep = pte_offset_map(pmdp, gaddr);
> else
> ptep = pte_alloc_map_lock(gmap->mm, pmdp, gaddr, &ptl);
>
> if (!ptep)
> return -ENOMEM;
>
> Makes it a little bit nicer to read.
Sure
>
>> +
>> + /* Protect and unlock. */
>> + rc = ptep_force_prot(gmap->mm, gaddr, ptep, prot, bits);
>> + if (ptl)
>> + gmap_pte_op_end(ptl);
>
> Would it make sense to move the ptl test into gmap_pte_op_end() ?
That's a great idea, I have a lot of these in the following patches.
>
>> + return rc;
>> +}
>> +
>> @@ -913,10 +1002,7 @@ static int gmap_protect_range(struct gmap *gmap, unsigned long gaddr,
>> rc = gmap_pte_op_fixup(gmap, gaddr, vmaddr, prot);
>> if (rc)
>> return rc;
>> - continue;
>> }
>> - gaddr += PAGE_SIZE;
>> - len -= PAGE_SIZE;
>
> Not sure if I like this change. I think I prefer it the way it is now
> (keeps the loop body shorter).
Well, this loop and a lot of others will get longer through the other
patches in the set.
They need some refactoring, but just to be safe, start getting used to it :)
Attachment:
signature.asc
Description: OpenPGP digital signature
