Re: [PATCH v5 02/13] KVM: s390: refactor crypto initialization

[Cornelia Huck]

On Wed, 16 May 2018 14:17:52 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:

> On 16/05/2018 13:14, Tony Krowiak wrote:
> > On 05/16/2018 04:51 AM, Pierre Morel wrote:  
> >> On 07/05/2018 17:11, Tony Krowiak wrote:  

> >>> @@ -1896,19 +1917,17 @@ static u64 kvm_s390_get_initial_cpuid(void)
> >>>
> >>> Â static void kvm_s390_crypto_init(struct kvm *kvm)
> >>> Â {
> >>> -ÂÂÂ if (!test_kvm_facility(kvm, 76))
> >>> +ÂÂÂ /*
> >>> +ÂÂÂÂ * If neither the AP instructions nor the message security assist
> >>> +ÂÂÂÂ * extension 3 (MSAX3) are installed, there is no need to 
> >>> initialize a
> >>> +ÂÂÂÂ * crypto control block (CRYCB) for the guest.
> >>> +ÂÂÂÂ */
> >>> +ÂÂÂ if (!kvm_ap_instructions_available() && !test_kvm_facility(kvm, 
> >>> 76))
> >>> ÂÂÂÂÂÂÂÂÂ return;
> >>>
> >>> ÂÂÂÂÂ kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
> >>> -ÂÂÂ kvm_s390_set_crycb_format(kvm);  
> >>
> >>
> >> For my point of view the all patch can be reduced to putting this
> >> call (kvm_s390_set_crycb_format(kvm);) before testing for facility 76.
> >>
> >> (and setting the format correctly in kvm_s390_set_crycb_format(kvm))  
> >
> > I don't see what that buys us; it will just be reshuffling of the logic.
> > The idea here is that all of the code related to formatting the CRYCB for
> > use by the guest is contained in the kvm_s390_format_crycb(kvm) function.
> > We don't need a CRYCB, however, if the AP instructions are not installed
> > and the MSAX3 facility is not installed, so why even call
> > kvm_s390_format_crycb(kvm) in that case?  
> 
> It byes a lot of lines.
> I mean that you do exactly the same by only using 3 lines inserted 
> instead of 65 changes.
> No logic change.

Sounds like a winner from my POV :)