On 22.01.2018 12:33, David Hildenbrand wrote:
> On 13.12.2017 13:53, 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.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
>> Reviewed-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>> Reviewed-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
>> ---
>> arch/s390/mm/gmap.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 92 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index 05d459b..8de8bf9 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -874,7 +874,88 @@ static int gmap_pte_op_fixup(struct gmap *gmap, unsigned long gaddr,
>> */
>> static void gmap_pte_op_end(spinlock_t *ptl)
>> {
>> - spin_unlock(ptl);
>> + if (ptl)
>> + spin_unlock(ptl);
>> +}
>> +
>> +/**
>> + * gmap_pmd_op_walk - walk the gmap tables, get the guest table lock
>> + * and return the pmd pointer
>> + * @gmap: pointer to guest mapping meta data structure
>> + * @gaddr: virtual address in the guest address space
>> + *
>> + * Returns a pointer to the pmd for a guest address, or NULL
>> + */
>> +static inline pmd_t *gmap_pmd_op_walk(struct gmap *gmap, unsigned long gaddr)
>> +{
>> + pmd_t *pmdp;
>> +
>> + spin_lock(&gmap->guest_table_lock);
>> + pmdp = (pmd_t *) gmap_table_walk(gmap, gaddr, 1);
>> +
>> + /*
>> + * Empty pmds can become large after we give up the
>> + * guest_table_lock, so we have to check for pmd_none
>> + * here.
>> + */
>
> Don't understand that comment. We give up the lock after we're done with
> the pmd either way. So I think this comment can go.
TBH I'm currently not able to recall the reason for this comment.
>
>> + 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.
[...]
>> +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?
>
> I suggest requiring for this function that the entry is valid (which is
> always the case) and getting rid of the -EAGAIN return code. Makes this
> function simpler.
>
>> +
>> + ptep = pte_alloc_map_lock(gmap->mm, pmdp, gaddr, &ptl);
>> + if (!ptep)
>> + return -ENOMEM;
>> +
>> + /* Protect and unlock. */
>> + rc = ptep_force_prot(gmap->mm, gaddr, ptep, prot, bits);
>> + gmap_pte_op_end(ptl);
>> + return rc;
>> }
>>
>> /*
>> @@ -896,16 +977,20 @@ static int gmap_protect_range(struct gmap *gmap, unsigned long gaddr,
>> unsigned long len, int prot, unsigned long bits)
>> {
>> unsigned long vmaddr;
>> - spinlock_t *ptl;
>> - pte_t *ptep;
>> + pmd_t *pmdp;
>> int rc;
>>
>> while (len) {
>> rc = -EAGAIN;
>> - ptep = gmap_pte_op_walk(gmap, gaddr, &ptl);
>> - if (ptep) {
>> - rc = ptep_force_prot(gmap->mm, gaddr, ptep, prot, bits);
>> - gmap_pte_op_end(ptl);
>> + pmdp = gmap_pmd_op_walk(gmap, gaddr);
>> + if (pmdp) {
>> + rc = gmap_protect_pte(gmap, gaddr, pmdp, prot,
>> + bits);
>> + if (!rc) {
>> + len -= PAGE_SIZE;
>> + gaddr += PAGE_SIZE;
>> + }
>> + gmap_pmd_op_end(gmap, pmdp);
>
> This change looks good to me.
Great :)
>
>> }
>> if (rc) {
>> vmaddr = __gmap_translate(gmap, gaddr);
>> @@ -914,10 +999,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;
>> }
>> return 0;
>> }
>>
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
