Re: [qemu-s390x] [Qemu-devel] [PATCH v3 5/7] s390x/vfio: ap: Introduce VFIO AP device

[Tony Krowiak]

On 03/16/2018 09:22 AM, Halil Pasic wrote:
> On 03/16/2018 11:42 AM, Pierre Morel wrote:
> > On 16/03/2018 00:24, Tony Krowiak wrote:
> > > Introduces a VFIO based AP device. The device is defined via
> > > the QEMU command line by specifying:
> > >
> > >       -device vfio-ap,sysfsdev=<path-to-mediated-matrix-device>
> > >
> > > The mediated matrix device is created by the VFIO AP device
> > > driver by writing a UUID to a sysfs attribute file (see
> > > docs/vfio-ap.txt). The mediated matrix device will be named
> > > after the UUID. Symbolic links to the $uuid are created in
> > > many places, so the path to the mediated matrix device $uuid
> > > can be specified in any of the following ways:
> > >
> > > /sys/devices/vfio_ap/matrix/$uuid
> > > /sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough/devices/$uuid
> > > /sys/bus/mdev/devices/$uuid
> > > /sys/bus/mdev/drivers/vfio_mdev/$uuid
> > >
> > > When the vfio-ap device is realized, it acquires and opens the
> > > VFIO iommu group to which the mediated matrix device is
> > > bound. This causes a VFIO group notification event to be
> > > signaled. The vfio_ap device driver's group notification
> > > handler will get called at which time the device driver
> > > will configure the the AP devices to which the guest will
> > > be granted access.
> > >
> > > Signed-off-by: Tony Krowiak <address@hidden>
> > > ---
> [..]
> > > +static void vfio_ap_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    VFIODevice *vbasedev;
> > > +    VFIOGroup *vfio_group;
> > > +    APDevice *apdev = DO_UPCAST(APDevice, parent_obj, dev);
> > > +    VFIOAPDevice *vapdev = DO_UPCAST(VFIOAPDevice, apdev, apdev);
> > > +    char *mdevid;
> > > +    Error *local_err = NULL;
> > > +    int ret;
> > > +
> > > +    if (!s390_has_feat(S390_FEAT_AP)) {
> > > +        error_setg(&local_err, "AP support not enabled");
> > > +        goto out_err;
> > > +    }
> > > +
> > > +    ret = kvm_s390_set_interpret_ap(1);
> > If we have several devices, this is called once per device.
> I don't think having several of these in a single vm makes
> any sense. Or does it? IMHO we should make sure there is at
> most one device taking care of the crypto pass-through.
My first thought was that only one vfio-ap device was necessary
to provide the AP matrix to a guest. Allowing additional vfio-ap
devices would definitely be superfluous, so I think it makes sense
to allow at most one vfio-ap device. In that case, setting AP
interpretive execution here makes sense.
> > However it is a general switch for the VM.
> > It looks strange to have this inside the realize callback
> I kind of semi agree.
>
> The previous solution (having this transparent for QEMU) had
> benefits. Why did we move away from that again?
This was done to address the multitude of objections and opinions 
related to
how/when/where ECA_APIE is set. Pierre summed it up best with his 
comment "we
need to separate the CPU feature defining 'if AP instructions are 
available'
from the QEMU property defining 'How we provide the instructions'. I 
agree and
this is how I chose to implement that.
> Regards,
> Halil