Re: [PATCH RFC 3/5] KVM: s390: Shadow VSIE SCA in guest-1

[Janosch Frank <frankja@linux.ibm.com>]

On 3/19/25 3:41 PM, Christoph Schlameuss wrote:
> On Wed Mar 19, 2025 at 2:41 PM CET, Janosch Frank wrote:
>> On 3/18/25 7:59 PM, Christoph Schlameuss wrote:
>>> Introduce a new shadow_sca function into kvm_s390_handle_vsie.
>>> kvm_s390_handle_vsie is called within guest-1 when guest-2 initiates the
>>> VSIE.
>>>
>>> shadow_sca and unshadow_sca create and manage ssca_block structs in
>>> guest-1 memory. References to the created ssca_blocks are kept within an
>>> array and limited to the number of cpus. This ensures each VSIE in
>>> execution can have its own SCA. Having the amount of shadowed SCAs
>>> configurable above this is left to another patch.
>>>
>>> SCAOL/H in the VSIE control block are overwritten with references to the
>>> shadow SCA. The original SCA pointer is saved in the vsie_page and
>>> restored on VSIE exit. This limits the amount of change in the
>>> preexisting VSIE pin and shadow functions.
>>>
>>> The shadow SCA contains the addresses of the original guest-3 SCA as
>>> well as the original VSIE control blocks. With these addresses the
>>> machine can directly monitor the intervention bits within the original
>>> SCA entries.
>>>
>>> The ssca_blocks are also kept within a radix tree to reuse already
>>> existing ssca_blocks efficiently. While the radix tree and array with
>>> references to the ssca_blocks are held in kvm_s390_vsie.
>>> The use of the ssca_blocks is tracked using an ref_count on the block
>>> itself.
>>>
>>> No strict mapping between the guest-1 vcpu and guest-3 vcpu is enforced.
>>> Instead each VSIE entry updates the shadow SCA creating a valid mapping
>>> for all cpus currently in VSIE.
>>>
>>> Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com>
>>> ---
>>>    arch/s390/include/asm/kvm_host.h |  22 +++-
>>>    arch/s390/kvm/vsie.c             | 264 ++++++++++++++++++++++++++++++++++++++-
>>>    2 files changed, 281 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index 0aca5fa01f3d772c3b3dd62a22134c0d4cb9dc22..4ab196caa9e79e4c4d295d23fed65e1a142e6ab1 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -29,6 +29,7 @@
>>>    #define KVM_S390_BSCA_CPU_SLOTS 64
>>>    #define KVM_S390_ESCA_CPU_SLOTS 248
>>>    #define KVM_MAX_VCPUS 255
>>> +#define KVM_S390_MAX_VCPUS 256
>>
>> #define KVM_S390_SSCA_CPU_SLOTS 256
>>
>> Yes, I'm aware, that ESCA and MAX_VCPUS are pretty confusing.
>> I'm searching for solutions but they might take a while.
>>
>>>    
>>>    #define KVM_INTERNAL_MEM_SLOTS 1
>>>    
>>> @@ -137,13 +138,23 @@ struct esca_block {
>>>    
>>>    /*
>>>     * The shadow sca / ssca needs to cover both bsca and esca depending on what the
>>> - * guest uses so we use KVM_S390_ESCA_CPU_SLOTS.
>>> + * guest uses so we allocate space for 256 entries that are defined in the
>>> + * architecture.
>>>     * The header part of the struct must not cross page boundaries.
>>>     */
>>>    struct ssca_block {
>>>    	__u64	osca;
>>>    	__u64	reserved08[7];
>>> -	struct ssca_entry cpu[KVM_S390_ESCA_CPU_SLOTS];
>>> +	struct ssca_entry cpu[KVM_S390_MAX_VCPUS];
>>
>> This should have been resolved in the previous patch, no?
>>
> 
> Oops, yes, exactly.
> 
>>> +};
>>> +
>>> +/*
>>> + * Store the vsie ssca block and accompanied management data.
>>> + */
>>> +struct ssca_vsie {
>>> +	struct ssca_block ssca;			/* 0x0000 */
>>> +	__u8	reserved[0x2200 - 0x2040];	/* 0x2040 */
>>> +	atomic_t ref_count;			/* 0x2200 */
>>>    };
>>>    
>>
>> [...]
>>
>>>    void kvm_s390_vsie_gmap_notifier(struct gmap *gmap, unsigned long start,
>>>    				 unsigned long end)
>>>    {
>>> @@ -699,6 +932,9 @@ static void unpin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>    
>>>    	hpa = (u64) scb_s->scaoh << 32 | scb_s->scaol;
>>>    	if (hpa) {
>>> +		/* with vsie_sigpif scaoh/l was pointing to g1 ssca_block but
>>> +		 * should have been reset in unshadow_sca()
>>> +		 */
>>
>> There shouldn't be text in the first or last line of multi-line comments.
>>
> 
> Will fix. Thx. (checkpatch seems to be fine with this, so I assume just in not
> desired?)

Might either be a personal preference as well or something that we don't 
really do in s390 KVM code.