linux-kernel - Re: [PATCH 4/4] KVM: s390: Minor refactor of base/ext facility lists

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Nov 2023 13:18:03 +0100
From:   Claudio Imbrenda <imbrenda@...ux.ibm.com>
To:     Nina Schoetterl-Glausch <nsg@...ux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-s390@...r.kernel.org,
        Cornelia Huck <cornelia.huck@...ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Michael Mueller <mimu@...ux.vnet.ibm.com>,
        David Hildenbrand <dahi@...ux.vnet.ibm.com>
Subject: Re: [PATCH 4/4] KVM: s390: Minor refactor of base/ext facility
 lists

On Mon, 06 Nov 2023 12:38:55 +0100
Nina Schoetterl-Glausch <nsg@...ux.ibm.com> wrote:

[...]

> > this was sized to [SIZE_INTERNAL], now it doesn't have a fixed size. is
> > this intentional?  
> 
> Yes, it's as big as it needs to be, that way it cannot be too small, so one
> less thing to consider.

fair enough
 
> [...]
> > >  /* available cpu features supported by kvm */
> > >  static DECLARE_BITMAP(kvm_s390_available_cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
> > > @@ -3341,13 +3333,16 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > >  	kvm->arch.sie_page2->kvm = kvm;
> > >  	kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
> > >  
> > > -	for (i = 0; i < kvm_s390_fac_size(); i++) {
> > > +	for (i = 0; i < ARRAY_SIZE(kvm_s390_fac_base); i++) {
> > >  		kvm->arch.model.fac_mask[i] = stfle_fac_list[i] &
> > > -					      (kvm_s390_fac_base[i] |
> > > -					       kvm_s390_fac_ext[i]);
> > > +					      kvm_s390_fac_base[i];
> > >  		kvm->arch.model.fac_list[i] = stfle_fac_list[i] &
> > >  					      kvm_s390_fac_base[i];
> > >  	}
> > > +	for (i = 0; i < ARRAY_SIZE(kvm_s390_fac_ext); i++) {
> > > +		kvm->arch.model.fac_mask[i] |= stfle_fac_list[i] &
> > > +					       kvm_s390_fac_ext[i];
> > > +	}  
> > 
> > I like it better when it's all in one place, instead of having two loops  
> 
> Hmm, it's the result of the arrays being different lengths now.

ah, I had missed that, the names are very similar.

> 
> [...]
> 
> > > -	for (i = 0; i < 16; i++)
> > > -		kvm_s390_fac_base[i] |=
> > > -			stfle_fac_list[i] & nonhyp_mask(i);
> > > +	for (i = 0; i < HMFAI_DWORDS; i++)
> > > +		kvm_s390_fac_base[i] |= nonhyp_mask(i);  
> > 
> > where did the stfle_fac_list[i] go?  
> 
> I deleted it. That's what I meant by "Get rid of implicit double
> anding of stfle_fac_list".
> Besides it being redundant I didn't like it conceptually.
> kvm_s390_fac_base specifies the facilities we support, regardless
> if they're installed in the configuration. The hypervisor managed
> ones are unconditionally declared via FACILITIES_KVM and we can add
> the non hypervisor managed ones unconditionally, too.

makes sense

> 
> > >  	r = __kvm_s390_init();
> > >  	if (r)  
> >   
>