On Mon May 26, 2025 at 10:22 AM CEST, Janosch Frank wrote:
> On 5/22/25 11:31 AM, Christoph Schlameuss wrote:
>> Instead of allocating a BSCA and upgrading it for PV or when adding the
>> 65th cpu we can always use the ESCA.
>>
>> The only downside of the change is that we will always allocate 4 pages
>> for a 248 cpu ESCA instead of a single page for the BSCA per VM.
>> In return we can delete a bunch of checks and special handling depending
>> on the SCA type as well as the whole BSCA to ESCA conversion.
>>
>> As a fallback we can still run without SCA entries when the SIGP
>> interpretation facility or ESCA are not available.
>>
>> Signed-off-by: Christoph Schlameuss <schlameuss@xxxxxxxxxxxxx>
>> ---
>> arch/s390/include/asm/kvm_host.h | 1 -
>> arch/s390/kvm/interrupt.c | 71 +++++------------
>> arch/s390/kvm/kvm-s390.c | 161 ++++++---------------------------------
>> arch/s390/kvm/kvm-s390.h | 4 +-
>> 4 files changed, 45 insertions(+), 192 deletions(-)
>
> [...]
>
>> @@ -80,33 +70,17 @@ static int sca_inject_ext_call(struct kvm_vcpu *vcpu, int src_id)
>>
>> BUG_ON(!kvm_s390_use_sca_entries());
>> read_lock(&vcpu->kvm->arch.sca_lock);
>> - if (vcpu->kvm->arch.use_esca) {
>> - struct esca_block *sca = vcpu->kvm->arch.sca;
>> - union esca_sigp_ctrl *sigp_ctrl =
>> - &(sca->cpu[vcpu->vcpu_id].sigp_ctrl);
>> - union esca_sigp_ctrl new_val = {0}, old_val;
>> -
>> - old_val = READ_ONCE(*sigp_ctrl);
>> - new_val.scn = src_id;
>> - new_val.c = 1;
>> - old_val.c = 0;
>> -
>> - expect = old_val.value;
>> - rc = cmpxchg(&sigp_ctrl->value, old_val.value, new_val.value);
>> - } else {
>> - struct bsca_block *sca = vcpu->kvm->arch.sca;
>> - union bsca_sigp_ctrl *sigp_ctrl =
>> - &(sca->cpu[vcpu->vcpu_id].sigp_ctrl);
>> - union bsca_sigp_ctrl new_val = {0}, old_val;
>> + struct esca_block *sca = vcpu->kvm->arch.sca;
>> + union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
>> + union esca_sigp_ctrl new_val = {0}, old_val;
>
>
> Since we don't have a need for inline declarations anymore, could you
> move those to the beginning of the function?
That would mean moving the sca access here out of the read lock. So I would
rather include that in a patch removing the sca_lock completely.
>
> @Christian @Claudio:
> Another interesting question is locking.
> The SCA RW lock protected against the bsca->esca switch which never
> happens after this patch.
>
> Can't we rip out that lock and maybe get a bit of performance and even
> less code? (In another patch set to limit the destructive potential)
