Re: [PATCH v4 08/15] KVM: s390: interfaces to (de)configure guest's AP matrix

[Pierre Morel]

On 25/04/2018 18:21, Tony Krowiak wrote:
> On 04/23/2018 09:46 AM, Pierre Morel wrote:
> > On 15/04/2018 23:22, Tony Krowiak wrote:
> > > Provides interfaces to assign AP adapters, usage domains
> > > and control domains to a KVM guest.
...
> > > +/**
> > > + * kvm_ap_matrix_create
> > > + *
> > > + * Create an AP matrix to hold a configuration of AP adapters, 
> > > domains and
> > > + * control domains.
> > > + *
> > > + * @ap_matrix: holds the matrix that is created
> > > + *
> > > + * Returns 0 if the matrix is successfully created. Returns an 
> > > error if an APQN
> > > + * derived from the cross product of the AP adapter IDs and AP 
> > > queue indexes
> > > + * comprising the AP matrix is configured for another guest.
> > > + */
> > > +int kvm_ap_matrix_create(struct kvm_ap_matrix **ap_matrix);
> >
> > why not simply return the pointer?
>
> The function returns a value indicating the reason a matrix could not 
> be created.
> Returning a NULL pointer provides no clue as to why the call failed.

That is why the ERR_PTR exist :)


...
> > > + * Returns 0 if the APQNs are valid, otherwise; returns -EBUSY.
> > > + */
> > > +static int kvm_ap_validate_queue_sharing(struct kvm *kvm,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct kvm_ap_matrix *matrix)
> > > +{
> > > +ÂÂÂ struct kvm *vm;
> > > +ÂÂÂ unsigned long *apm, *aqm;
> > > +ÂÂÂ unsigned long apid, apqi;
> > > +
> > > +
> > > +ÂÂÂ /* No other VM may share an AP Queue with the input VM */
> >
> > I wonder if these functions and structures should really belong to KVM.
> > The only have sense with the VFIO driver.
> > My opinion is that they belong there, in the VFIO driver code.
>
> I disagree for two reasons:
>
> 1. The vfio_ap driver should not have to know how to configure the KVM
> ÂÂ guest's matrix nor anything else about KVM for that matter.
>
> 2. The interfaces and structures defined in kvm-ap.h and implemented
> in kvm-ap.c don't have anything to do with VFIO and can stand alone
> ÂÂ to be used by any client code to configure a guest's matrix.

Doing this you will have to change KVM if the AP VFIO matrix protocol to 
access the queues change.
i.e. suppose some day the queues may be shared between guests.
...
> > > +static int kvm_ap_matrix_apm_create(struct kvm_ap_matrix *ap_matrix,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct ap_config_info *config)
> > > +{
> > > +ÂÂÂ int apm_max = (config && config->apxa) ? config->Na + 1 : 16;
> >
> > At this moment you already know the format of the crycb.
>
> How?

you calculated this in kvm_ap_build_crycbd() which is called from 
kvm_s390_crypto_init()
itself called from kvm_arch_init_vm().
It is when starting the VM.

kvm_ap_matrix_apm_create() is called much later when realizing the device


...



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