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

[Claudio Imbrenda]

On Mon, 06 Nov 2023 12:38:55 +0100
Nina Schoetterl-Glausch <nsg@xxxxxxxxxxxxx> 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)  
> >   
>