Re: [PATCH v3 1/1] KVM: s390: vsie: clarifications on setting the APCB

[Pierre Morel <pmorel@xxxxxxxxxxxxx>]

On 2/16/23 14:41, Janosch Frank wrote:
> 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@xxxxxxxxxxxxx>
> > Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
>
> LGTM:
> Acked-by: Janosch Frank <frankja@xxxxxxxxxxxxx>

Thanks,
Pierre

> > ---
> >   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);
> >           }
> >       }

--
Pierre Morel
IBM Lab Boeblingen