Re: [qemu-s390x] [PATCH v3] s390x/cpumodel: Set up CPU model for AQIC interception

[Pierre Morel]

On 15/02/2019 14:52, David Hildenbrand wrote:
> On 14.02.19 13:14, Pierre Morel wrote:
> > A new CPU model facilities is introduced to support AP devices
> > interruption interception for a KVM guest.
>
> This sentence is confusing. At first I thought you would introduce a new
> _fake_ facility.
>
> "Let's add support for the AP-Queue interruption facility to the CPU model".

OK, Thx will do so.

> > "APQI" for "AP-Queue Interruption" facility
> >
> > The S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, CPU facility indicates
> > whether the PQAP instruction with the AQIC command is available
> > to the guest.
> > This feature will be enabled only if the AP instructions are
> > available on the linux host and AQIC facility is installed on
> > the host.
> >
> > This feature must be turned on from userspace to intercept AP
> > instructions on the KVM guest. The QEMU command line to turn
> > this feature on looks something like this:
> >
> >      qemu-system-s390x ... -cpu xxx,apqi=on ...
> >
> > Signed-off-by: Pierre Morel <address@hidden>
> > Reviewed-by: Tony Krowiak <address@hidden>
> > Reviewed-by: Christian Borntraeger <address@hidden>
> > Reviewed-by: Halil Pasic <address@hidden>
> > ---
> >   target/s390x/cpu_features.c     | 1 +
> >   target/s390x/cpu_features_def.h | 1 +
> >   target/s390x/cpu_models.c       | 1 +
> >   target/s390x/gen-features.c     | 1 +
> >   4 files changed, 4 insertions(+)
> >
> > diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> > index 60cfeba..99bd382 100644
> > --- a/target/s390x/cpu_features.c
> > +++ b/target/s390x/cpu_features.c
> > @@ -84,6 +84,7 @@ static const S390FeatDef s390_features[] = {
> >       FEAT_INIT("sema", S390_FEAT_TYPE_STFL, 59, "Semaphore-assist facility"),
> >       FEAT_INIT("tsi", S390_FEAT_TYPE_STFL, 60, "Time-slice Instrumentation 
> > facility"),
> >       FEAT_INIT("ri", S390_FEAT_TYPE_STFL, 64, "CPU runtime-instrumentation 
> > facility"),
> > +    FEAT_INIT("apqi", S390_FEAT_TYPE_STFL, 65, "AP-Queue interruption 
> > facility"),
> >       FEAT_INIT("zpci", S390_FEAT_TYPE_STFL, 69, "z/PCI facility"),
> >       FEAT_INIT("aen", S390_FEAT_TYPE_STFL, 71, 
> > "General-purpose-adapter-event-notification facility"),
> >       FEAT_INIT("ais", S390_FEAT_TYPE_STFL, 72, 
> > "General-purpose-adapter-interruption-suppression facility"),
> > diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
> > index 5fc7e7b..3f22780 100644
> > --- a/target/s390x/cpu_features_def.h
> > +++ b/target/s390x/cpu_features_def.h
> > @@ -72,6 +72,7 @@ typedef enum {
> >       S390_FEAT_SEMAPHORE_ASSIST,
> >       S390_FEAT_TIME_SLICE_INSTRUMENTATION,
> >       S390_FEAT_RUNTIME_INSTRUMENTATION,
> > +    S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL,
> >       S390_FEAT_ZPCI,
> >       S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
> >       S390_FEAT_ADAPTER_INT_SUPPRESSION,
> > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> > index 7c253ff..6b5e94b 100644
> > --- a/target/s390x/cpu_models.c
> > +++ b/target/s390x/cpu_models.c
> > @@ -788,6 +788,7 @@ static void check_consistency(const S390CPUModel *model)
> >           { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
> >           { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
> >           { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
> > +        { S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, S390_FEAT_AP },
> >       };
> >       int i;
> >   
> > diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> > index 70015ea..b051221 100644
> > --- a/target/s390x/gen-features.c
> > +++ b/target/s390x/gen-features.c
> > @@ -448,6 +448,7 @@ static uint16_t full_GEN12_GA1[] = {
> >       S390_FEAT_EDAT_2,
> >       S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
> >       S390_FEAT_AP_QUERY_CONFIG_INFO,
> > +    S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL,
> >       S390_FEAT_AP_FACILITIES_TEST,
> >       S390_FEAT_AP,
> >   };
>
> Looks good to me. In QEMU, we enable/indicate facilities only when all
> required parts were implemented on the QEMU side.
>
> Is there anything we have to do regarding
> - migration support unlocked with this facility
> - functional changes we have to implement?
>
> Or is this really a "kernel does it all, migration not relevant" ?

Sorry for the delay.
Yes kernel do everything, we only allow to enable the facility here.

Do I add your R-B ?

Thanks
Pierre

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany