Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Mon, 6 Nov 2023 13:18:03 +0100
From:   Claudio Imbrenda <imbrenda@linux.ibm.com>
To:     Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        linux-s390@vger.kernel.org,
        Cornelia Huck <cornelia.huck@de.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Michael Mueller <mimu@linux.vnet.ibm.com>,
        David Hildenbrand <dahi@linux.vnet.ibm.com>
Subject: Re: [PATCH 4/4] KVM: s390: Minor refactor of base/ext facility
 lists
Message-ID: <20231106131803.15985f2e@p-imbrenda>
In-Reply-To: <44148ab315f28a6d77627675cbde26977418c5df.camel@linux.ibm.com>
References: <20231103173008.630217-1-nsg@linux.ibm.com>
        <20231103173008.630217-5-nsg@linux.ibm.com>
        <20231103193254.7deef2e5@p-imbrenda>
        <44148ab315f28a6d77627675cbde26977418c5df.camel@linux.ibm.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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