Re: [PATCH v11 2/3] KVM: s390: guest support for topology function

[Pierre Morel]

On 7/4/22 13:17, Janosch Frank wrote:
> On 7/4/22 13:02, Pierre Morel wrote:
> > On 7/4/22 11:08, Janis Schoetterl-Glausch wrote:
> > > On 7/1/22 18:25, Pierre Morel wrote:
> > > > We report a topology change to the guest for any CPU hotplug.
> > > >
> > > > The reporting to the guest is done using the Multiprocessor
> > > > Topology-Change-Report (MTCR) bit of the utility entry in the guest's
> > > > SCA which will be cleared during the interpretation of PTF.
> > > >
> > > > On every vCPU creation we set the MCTR bit to let the guest know the
> > > > next time he uses the PTF with command 2 instruction that the> 
> > > > topology changed and that he should use the STSI(15.1.x) instruction
> > > s/he/it (twice)
> > > > to get the topology details.
> > > >
> > > > STSI(15.1.x) gives information on the CPU configuration topology.
> > > > Let's accept the interception of STSI with the function code 15 and
> > > > let the userland part of the hypervisor handle it when userland
> > > > support the CPU Topology facility.And the user STSI capability.
> > > Also: supportS.
> > > > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > > > Reviewed-by: Nico Boehr <nrb@xxxxxxxxxxxxx>
> > > > ---
> > > >    arch/s390/include/asm/kvm_host.h | 18 +++++++++++++---
> > > >    arch/s390/kvm/kvm-s390.c         | 36 
> > > > ++++++++++++++++++++++++++++++++
> > > >    arch/s390/kvm/priv.c             | 16 ++++++++++----
> > > >    arch/s390/kvm/vsie.c             |  8 +++++++
> > > >    4 files changed, 71 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/arch/s390/include/asm/kvm_host.h 
> > > > b/arch/s390/include/asm/kvm_host.h
> > > > index 766028d54a3e..ae6bd3d607de 100644
> > > > --- a/arch/s390/include/asm/kvm_host.h
> > > > +++ b/arch/s390/include/asm/kvm_host.h
> > > > @@ -93,19 +93,30 @@ union ipte_control {
> > > >        };
> > > >    };
> > > [...]
> > >
> > > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > > > index 8fcb56141689..ee59b03f2e45 100644
> > > > --- a/arch/s390/kvm/kvm-s390.c
> > > > +++ b/arch/s390/kvm/kvm-s390.c
> > > > @@ -1691,6 +1691,31 @@ static int kvm_s390_get_cpu_model(struct kvm 
> > > > *kvm, struct kvm_device_attr *attr)
> > > >        return ret;
> > > >    }
> > > > +/**
> > > > + * kvm_s390_update_topology_change_report - update CPU topology 
> > > > change report
> > > > + * @kvm: guest KVM description
> > > > + * @val: set or clear the MTCR bit
> > > > + *
> > > > + * Updates the Multiprocessor Topology-Change-Report bit to signal
> > > > + * the guest with a topology change.
> > > > + * This is only relevant if the topology facility is present.
> > > > + *
> > > > + * The SCA version, bsca or esca, doesn't matter as offset is the 
> > > > same.
> > > > + */
> > > > +static void kvm_s390_update_topology_change_report(struct kvm *kvm, 
> > > > bool val)
> > > > +{
> > > > +    struct bsca_block *sca = kvm->arch.sca;
> > > > +    union sca_utility new, old;
> > > > +
> > > > +    read_lock(&kvm->arch.sca_lock);
> > >
> > > You forgot to put the assignment of sca under the lock.
> >
> > Should I really?
> > What we want to protect here is the content of the sca.
> > The sca itself does not change during the life of the KVM AFAIK.
>
> The SCA origin as well as the SCA contents can change within the 
> lifetime of a KVM VM.
>
> When we switch from bsca to esca we'll use new pages.
> When we add/remove cpus we'll update the MCN and the CPU entry.

Oh! then Yes, right.
thanks


--
Pierre Morel
IBM Lab Boeblingen