Message-ID: <c6896c31-b9db-e241-5f47-fc96fd53a2cb@linux.ibm.com>
Date:   Tue, 7 Dec 2021 10:27:08 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v3 0/3] KVM: s390: Some gaccess cleanup
Content-Language: en-US
To:     Janis Schoetterl-Glausch <scgl@linux.ibm.com>,
        Christian Borntraeger <borntraeger@de.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        kvm@vger.kernel.org, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20211126164549.7046-1-scgl@linux.ibm.com>
From:   Janosch Frank <frankja@linux.ibm.com>
In-Reply-To: <20211126164549.7046-1-scgl@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/26/21 17:45, Janis Schoetterl-Glausch wrote:
> Cleanup s390 guest access code a bit, getting rid of some code
> duplication and improving readability.
> 
> v2 -> v3
> 	minor changes only
> 		typo fixes
> 		whitespace
> 		line reordering
> 		picked up Reviewed-by's
> 
> v1 -> v2
> 	separate patch for renamed variable
> 		fragment_len instead of seg
> 	expand comment of guest_range_to_gpas
> 	fix nits

Thanks, picked

> 
> Janis Schoetterl-Glausch (3):
>    KVM: s390: gaccess: Refactor gpa and length calculation
>    KVM: s390: gaccess: Refactor access address range check
>    KVM: s390: gaccess: Cleanup access to guest pages
> 
>   arch/s390/kvm/gaccess.c | 158 +++++++++++++++++++++++-----------------
>   1 file changed, 92 insertions(+), 66 deletions(-)
> 
> Range-diff against v2:
> 1:  60d050210198 ! 1:  e5d7d2d7a4da KVM: s390: gaccess: Refactor gpa and length calculation
>      @@ Metadata
>        ## Commit message ##
>           KVM: s390: gaccess: Refactor gpa and length calculation
>       
>      -    Improve readability be renaming the length variable and
>      +    Improve readability by renaming the length variable and
>           not calculating the offset manually.
>       
>           Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>      +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>       
>        ## arch/s390/kvm/gaccess.c ##
>       @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
>      @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long g
>        	psw_t *psw = &vcpu->arch.sie_block->gpsw;
>       -	unsigned long _len, nr_pages, gpa, idx;
>       +	unsigned long nr_pages, gpa, idx;
>      -+	unsigned int fragment_len;
>        	unsigned long pages_array[2];
>      ++	unsigned int fragment_len;
>        	unsigned long *pages;
>        	int need_ipte_lock;
>      + 	union asce asce;
>       @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar, void *data,
>        		ipte_lock(vcpu);
>        	rc = guest_page_range(vcpu, ga, ar, pages, nr_pages, asce, mode);
> 2:  7080846c8c07 ! 2:  91cadb42cbbc KVM: s390: gaccess: Refactor access address range check
>      @@ Commit message
>           range.
>       
>           Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>      +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>       
>        ## arch/s390/kvm/gaccess.c ##
>       @@ arch/s390/kvm/gaccess.c: static int low_address_protection_enabled(struct kvm_vcpu *vcpu,
>      @@ arch/s390/kvm/gaccess.c: static int low_address_protection_enabled(struct kvm_vc
>       + * a correct exception into the guest.
>       + * The resulting gpas are stored into @gpas, unless it is NULL.
>       + *
>      -+ * Note: All gpas except the first one start at the beginning of a page.
>      ++ * Note: All fragments except the first one start at the beginning of a page.
>       + *       When deriving the boundaries of a fragment from a gpa, all but the last
>       + *       fragment end at the end of the page.
>       + *
>      @@ arch/s390/kvm/gaccess.c: int access_guest(struct kvm_vcpu *vcpu, unsigned long g
>        {
>        	psw_t *psw = &vcpu->arch.sie_block->gpsw;
>       -	unsigned long nr_pages, gpa, idx;
>      +-	unsigned long pages_array[2];
>       +	unsigned long nr_pages, idx;
>      ++	unsigned long gpa_array[2];
>        	unsigned int fragment_len;
>      --	unsigned long pages_array[2];
>       -	unsigned long *pages;
>      -+	unsigned long gpa_array[2];
>       +	unsigned long *gpas;
>        	int need_ipte_lock;
>        	union asce asce;
> 3:  c991cbdbfbd5 ! 3:  f5000a22efcd KVM: s390: gaccess: Cleanup access to guest frames
>      @@ Metadata
>       Author: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>       
>        ## Commit message ##
>      -    KVM: s390: gaccess: Cleanup access to guest frames
>      +    KVM: s390: gaccess: Cleanup access to guest pages
>       
>           Introduce a helper function for guest frame access.
>       
>           Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>      +    Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>       
>        ## arch/s390/kvm/gaccess.c ##
>       @@ arch/s390/kvm/gaccess.c: static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
>      @@ arch/s390/kvm/gaccess.c: static int guest_range_to_gpas(struct kvm_vcpu *vcpu, u
>        }
>        
>       +static int access_guest_page(struct kvm *kvm, enum gacc_mode mode, gpa_t gpa,
>      -+			      void *data, unsigned int len)
>      ++			     void *data, unsigned int len)
>       +{
>       +	const unsigned int offset = offset_in_page(gpa);
>       +	const gfn_t gfn = gpa_to_gfn(gpa);
> 
> base-commit: d25f27432f80a800a3592db128254c8140bd71bf
>