Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6daf3aa9-5b91-1948-0c43-b8407fe3bea2@linux.ibm.com>
Date:   Mon, 12 Sep 2022 10:03:07 +0200
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] KVM: s390: vsie: fix crycb virtual vs physical usage
Content-Language: en-US
To:     David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org
Cc:     linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        borntraeger@de.ibm.com, frankja@linux.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: <20220905084148.234821-1-pmorel@linux.ibm.com>
 <08c54ddd-b74e-9f6c-f5eb-13e994530ad6@redhat.com>
From:   Pierre Morel <pmorel@linux.ibm.com>
In-Reply-To: <08c54ddd-b74e-9f6c-f5eb-13e994530ad6@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


Hi David,

sorry for the delay, just came back from vacation.

On 9/5/22 18:32, David Hildenbrand wrote:
> On 05.09.22 10:41, Pierre Morel wrote:
>> Prepare VSIE for architectural changes where lowmem kernel real and
>> kernel virtual address are different.
> 
> Bear with me, it used to be
> 
>      crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;
>      apcb_o = (unsigned long) &crycb->apcb0;
> 
> and now it's
> 
>      apcb_o = crycb_o + offsetof(struct kvm_s390_crypto_cb, apcb0);
> 
> 
> So the real issue seems to be
> 
>      crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;
> 
> because crycb_o actually is a guest address and not a host address.

Yes, right this is the real issue, however...

> 
> 
> But now I'm confused, because I would have thought that the result 
> produced by both code would be identical (I completely agree that the 
> new variant is better).
> 
> How does this interact with "lowmem kernel real and kernel virtual 
> address are different." -- I would have thought that &crycb->apcb0 
> doesn't actually access any memory and only performs arithmetical 
> operations?

...you are right and the result is identical.


> 
>>
>> When we get the original crycb from the guest crycb we can use the
>> phys_to_virt transformation, which will use the host transformations,
>> but we must use an offset to calculate the guest real address apcb
>> and give it to read_guest_real().
> 
> Can you elaborate where phys_to_virt() comes into play?

No, it does not have to do with phys_to_virt(), I first started to work 
on the line:
- crycb = (struct kvm_s390_crypto_cb *) (unsigned long)crycb_o;

and did not notice that after the simplifications the result was identical.
So The comment is wrong and this patch is only making the code clearer.

Thanks for your clarifications,

Regards,
Pierre


> 
> If this is an actual fix (as indicated in the patch subject), should 
> this carry a
> 
>      Fixes: 56019f9aca22 ("KVM: s390: vsie: Allow CRYCB FORMAT-2")
> 

-- 
Pierre Morel
IBM Lab Boeblingen