Message-ID: <d32a4080-3c74-27f0-2ef7-78755da98022@linux.ibm.com>
Date:   Thu, 16 Feb 2023 14:41:22 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.2
Subject: Re: [PATCH v3 1/1] KVM: s390: vsie: clarifications on setting the
 APCB
To:     Pierre Morel <pmorel@linux.ibm.com>, david@redhat.com
Cc:     kvm@vger.kernel.org, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, borntraeger@de.ibm.com,
        cohuck@redhat.com, thuth@redhat.com, imbrenda@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com, svens@linux.ibm.com
References: <20230214122841.13066-1-pmorel@linux.ibm.com>
 <20230214122841.13066-2-pmorel@linux.ibm.com>
Content-Language: en-US
From:   Janosch Frank <frankja@linux.ibm.com>
In-Reply-To: <20230214122841.13066-2-pmorel@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/14/23 13:28, Pierre Morel wrote:
> The APCB is part of the CRYCB.
> The calculation of the APCB origin can be done by adding
> the APCB offset to the CRYCB origin.
> 
> Current code makes confusing transformations, converting
> the CRYCB origin to a pointer to calculate the APCB origin.
> 
> Let's make things simpler and keep the CRYCB origin to make
> these calculations.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

LGTM:
Acked-by: Janosch Frank <frankja@linux.ibm.com>

> ---
>   arch/s390/kvm/vsie.c | 50 +++++++++++++++++++++++++-------------------
>   1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index b6a0219e470a..8d6b765abf29 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -138,11 +138,15 @@ static int prepare_cpuflags(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>   }
>   /* Copy to APCB FORMAT1 from APCB FORMAT0 */
>   static int setup_apcb10(struct kvm_vcpu *vcpu, struct kvm_s390_apcb1 *apcb_s,
> -			unsigned long apcb_o, struct kvm_s390_apcb1 *apcb_h)
> +			unsigned long crycb_gpa, struct kvm_s390_apcb1 *apcb_h)
>   {
>   	struct kvm_s390_apcb0 tmp;
> +	unsigned long apcb_gpa;
>   
> -	if (read_guest_real(vcpu, apcb_o, &tmp, sizeof(struct kvm_s390_apcb0)))
> +	apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb0);
> +
> +	if (read_guest_real(vcpu, apcb_gpa, &tmp,
> +			    sizeof(struct kvm_s390_apcb0)))
>   		return -EFAULT;
>   
>   	apcb_s->apm[0] = apcb_h->apm[0] & tmp.apm[0];
> @@ -157,15 +161,19 @@ static int setup_apcb10(struct kvm_vcpu *vcpu, struct kvm_s390_apcb1 *apcb_s,
>    * setup_apcb00 - Copy to APCB FORMAT0 from APCB FORMAT0
>    * @vcpu: pointer to the virtual CPU
>    * @apcb_s: pointer to start of apcb in the shadow crycb
> - * @apcb_o: pointer to start of original apcb in the guest2
> + * @crycb_gpa: guest physical address to start of original guest crycb
>    * @apcb_h: pointer to start of apcb in the guest1
>    *
>    * Returns 0 and -EFAULT on error reading guest apcb
>    */
>   static int setup_apcb00(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
> -			unsigned long apcb_o, unsigned long *apcb_h)
> +			unsigned long crycb_gpa, unsigned long *apcb_h)
>   {
> -	if (read_guest_real(vcpu, apcb_o, apcb_s,
> +	unsigned long apcb_gpa;
> +
> +	apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb0);
> +
> +	if (read_guest_real(vcpu, apcb_gpa, apcb_s,
>   			    sizeof(struct kvm_s390_apcb0)))
>   		return -EFAULT;
>   
> @@ -178,16 +186,20 @@ static int setup_apcb00(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
>    * setup_apcb11 - Copy the FORMAT1 APCB from the guest to the shadow CRYCB
>    * @vcpu: pointer to the virtual CPU
>    * @apcb_s: pointer to start of apcb in the shadow crycb
> - * @apcb_o: pointer to start of original guest apcb
> + * @crycb_gpa: guest physical address to start of original guest crycb
>    * @apcb_h: pointer to start of apcb in the host
>    *
>    * Returns 0 and -EFAULT on error reading guest apcb
>    */
>   static int setup_apcb11(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
> -			unsigned long apcb_o,
> +			unsigned long crycb_gpa,
>   			unsigned long *apcb_h)
>   {
> -	if (read_guest_real(vcpu, apcb_o, apcb_s,
> +	unsigned long apcb_gpa;
> +
> +	apcb_gpa = crycb_gpa + offsetof(struct kvm_s390_crypto_cb, apcb1);
> +
> +	if (read_guest_real(vcpu, apcb_gpa, apcb_s,
>   			    sizeof(struct kvm_s390_apcb1)))
>   		return -EFAULT;
>   
> @@ -200,7 +212,7 @@ static int setup_apcb11(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
>    * setup_apcb - Create a shadow copy of the apcb.
>    * @vcpu: pointer to the virtual CPU
>    * @crycb_s: pointer to shadow crycb
> - * @crycb_o: pointer to original guest crycb
> + * @crycb_gpa: guest physical address of original guest crycb
>    * @crycb_h: pointer to the host crycb
>    * @fmt_o: format of the original guest crycb.
>    * @fmt_h: format of the host crycb.
> @@ -211,50 +223,46 @@ static int setup_apcb11(struct kvm_vcpu *vcpu, unsigned long *apcb_s,
>    * Return 0 or an error number if the guest and host crycb are incompatible.
>    */
>   static int setup_apcb(struct kvm_vcpu *vcpu, struct kvm_s390_crypto_cb *crycb_s,
> -	       const u32 crycb_o,
> +	       const u32 crycb_gpa,
>   	       struct kvm_s390_crypto_cb *crycb_h,
>   	       int fmt_o, int fmt_h)
>   {
> -	struct kvm_s390_crypto_cb *crycb;
> -
> -	crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;
> -
>   	switch (fmt_o) {
>   	case CRYCB_FORMAT2:
> -		if ((crycb_o & PAGE_MASK) != ((crycb_o + 256) & PAGE_MASK))
> +		if ((crycb_gpa & PAGE_MASK) != ((crycb_gpa + 256) & PAGE_MASK))
>   			return -EACCES;
>   		if (fmt_h != CRYCB_FORMAT2)
>   			return -EINVAL;
>   		return setup_apcb11(vcpu, (unsigned long *)&crycb_s->apcb1,
> -				    (unsigned long) &crycb->apcb1,
> +				    crycb_gpa,
>   				    (unsigned long *)&crycb_h->apcb1);
>   	case CRYCB_FORMAT1:
>   		switch (fmt_h) {
>   		case CRYCB_FORMAT2:
>   			return setup_apcb10(vcpu, &crycb_s->apcb1,
> -					    (unsigned long) &crycb->apcb0,
> +					    crycb_gpa,
>   					    &crycb_h->apcb1);
>   		case CRYCB_FORMAT1:
>   			return setup_apcb00(vcpu,
>   					    (unsigned long *) &crycb_s->apcb0,
> -					    (unsigned long) &crycb->apcb0,
> +					    crycb_gpa,
>   					    (unsigned long *) &crycb_h->apcb0);
>   		}
>   		break;
>   	case CRYCB_FORMAT0:
> -		if ((crycb_o & PAGE_MASK) != ((crycb_o + 32) & PAGE_MASK))
> +		if ((crycb_gpa & PAGE_MASK) != ((crycb_gpa + 32) & PAGE_MASK))
>   			return -EACCES;
>   
>   		switch (fmt_h) {
>   		case CRYCB_FORMAT2:
>   			return setup_apcb10(vcpu, &crycb_s->apcb1,
> -					    (unsigned long) &crycb->apcb0,
> +					    crycb_gpa,
>   					    &crycb_h->apcb1);
>   		case CRYCB_FORMAT1:
>   		case CRYCB_FORMAT0:
>   			return setup_apcb00(vcpu,
>   					    (unsigned long *) &crycb_s->apcb0,
> -					    (unsigned long) &crycb->apcb0,
> +					    crycb_gpa,
>   					    (unsigned long *) &crycb_h->apcb0);
>   		}
>   	}