Messages in this thread Patch in this message | | | Subject | Re: [PATCH v4 13/15] KVM: s390: configure the guest's AP devices | From | Tony Krowiak <> | Date | Tue, 17 Apr 2018 12:36:33 -0400 |
| |
On 04/17/2018 12:18 PM, Pierre Morel wrote: > On 17/04/2018 18:08, Tony Krowiak wrote: >> On 04/16/2018 09:05 AM, Pierre Morel wrote: >>> On 15/04/2018 23:22, Tony Krowiak wrote: >>>> Registers a group notifier during the open of the mediated >>>> matrix device to get information on KVM presence through the >>>> VFIO_GROUP_NOTIFY_SET_KVM event. When notified, the pointer >>>> to the kvm structure is saved inside the mediated matrix >>>> device. Once the VFIO AP device driver has access to KVM, >>>> access to the APs can be configured for the guest. >>>> >>>> Access to APs is configured when the file descriptor for the >>>> mediated matrix device is opened by userspace. The items to be >>>> configured are: >>>> >>>> 1. The ECA.28 bit in the SIE state description determines whether >>>> AP instructions are interpreted by the hardware or intercepted. >>>> The VFIO AP device driver relies interpretive execution of >>>> AP instructions so the ECA.28 bit will be set >>>> >>>> 2. Guest access to AP adapters, usage domains and control domains >>>> is controlled by three bit masks referenced from the >>>> Crypto Control Block (CRYCB) referenced from the guest's SIE state >>>> description: >>>> >>>> * The AP Mask (APM) controls access to the AP adapters. Each bit >>>> in the APM represents an adapter number - from most significant >>>> to least significant bit - from 0 to 255. The bits in the APM >>>> are set according to the adapter numbers assigned to the >>>> mediated >>>> matrix device via its 'assign_adapter' sysfs attribute file. >>>> >>>> * The AP Queue (AQM) controls access to the AP queues. Each bit >>>> in the AQM represents an AP queue index - from most significant >>>> to least significant bit - from 0 to 255. A queue index >>>> references >>>> a specific domain and is synonymous with the domian number. The >>>> bits in the AQM are set according to the domain numbers assigned >>>> to the mediated matrix device via its 'assign_domain' sysfs >>>> attribute file. >>>> >>>> * The AP Domain Mask (ADM) controls access to the AP control >>>> domains. >>>> Each bit in the ADM represents a control domain - from most >>>> significant to least significant bit - from 0-255. The >>>> bits in the ADM are set according to the domain numbers assigned >>>> to the mediated matrix device via its 'assign_control_domain' >>>> sysfs attribute file. >>>> >>>> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>> --- >>>> drivers/s390/crypto/vfio_ap_ops.c | 50 >>>> +++++++++++++++++++++++++++++++++ >>>> drivers/s390/crypto/vfio_ap_private.h | 2 + >>>> 2 files changed, 52 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >>>> b/drivers/s390/crypto/vfio_ap_ops.c >>>> index bc2b05e..e3ff5ab 100644 >>>> --- a/drivers/s390/crypto/vfio_ap_ops.c >>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c >>>> @@ -53,6 +53,54 @@ static int vfio_ap_mdev_remove(struct >>>> mdev_device *mdev) >>>> return 0; >>>> } >>>> >>>> +static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, >>>> + unsigned long action, void *data) >>>> +{ >>>> + struct ap_matrix_mdev *matrix_mdev; >>>> + >>>> + if (action == VFIO_GROUP_NOTIFY_SET_KVM) { >>>> + matrix_mdev = container_of(nb, struct ap_matrix_mdev, >>>> + group_notifier); >>>> + matrix_mdev->kvm = data; >>>> + } >>>> + >>>> + return NOTIFY_OK; >>>> +} >>>> + >>>> +static int vfio_ap_mdev_open(struct mdev_device *mdev) >>>> +{ >>>> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); >>>> + unsigned long events; >>>> + int ret; >>>> + >>>> + matrix_mdev->group_notifier.notifier_call = >>>> vfio_ap_mdev_group_notifier; >>>> + events = VFIO_GROUP_NOTIFY_SET_KVM; >>>> + >>>> + ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, >>>> + &events, &matrix_mdev->group_notifier); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = kvm_ap_interpret_instructions(matrix_mdev->kvm, true); >>> >>> Do you need this call ? >>> apie is always enabled in KVM if AP instructions are available. >> >> I suppose we don't, in which case we don't need the >> kvm_ap_interpret_instructions() >> function either ... at least not until we implement interception. >> >>> >>> >>> Setting or not the interpretation is done for the VM in a all. >>> It is not the right place to do it here since open is device dependent. >> >> As I stated above, at this time we probably do not need this, however; >> that will not always be the case. The setting is and always will be >> for the >> VM in all - unless the architecture changes - because it is >> controlled by a >> single bit (ECA.28). If you recall, I originally set interpretation >> in the >> vfio_ap device driver when notified of the VFIO_GROUP_NOTIFY_SET_KVM >> event. >> I believe ultimately that it is the device driver that should set the >> value >> for apie. >> >> >> >>> >>> >>> Or we only have one device in the VM at a time. >>> In this case, shouldn't we make it official by returning -EEXIST for >>> the second call? >> >> We do allow only one vfio-ap device at a time. QEMU will allow only >> one vfio-ap device >> to be configured for a guest. Should we also put a check in here? > > QEMU is not the only possible user of this interface.
True .... I will put a check in here to make sure only one device is created.
> > >> >>> >>> >>> >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = kvm_ap_configure_matrix(matrix_mdev->kvm, >>>> + matrix_mdev->matrix); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static void vfio_ap_mdev_release(struct mdev_device *mdev) >>>> +{ >>>> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); >>>> + >>>> + kvm_ap_deconfigure_matrix(matrix_mdev->kvm); >>>> + kvm_ap_interpret_instructions(matrix_mdev->kvm, false); >>> >>> This call clears the apie in KVM. >>> This is only OK if we have a single device present until the end of >>> the VM, >>> otherwise AP instructions in the guest will fail after the release >>> until the end of the VM >>> or until a new device is plugged. >> >> See Message ID: >> <1523819244-29954-5-git-send-email-akrowiak@linux.vnet.ibm.com> on the >> qemu mailing list. There will be only one vfio-ap device allowed for >> the MVP model. > > dito. > Anyone can write a userland application using this interface.
See comments above, not to mention I will probably remove this call.
> > <2b053349-071e-17ed-6ebd-a37bcfd2f330@linux.vnet.ibm.com> >> >>> >>> >>> >>>> + vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, >>>> + &matrix_mdev->group_notifier); >>>> +} >>>> + >>>> static ssize_t name_show(struct kobject *kobj, struct device >>>> *dev, char *buf) >>>> { >>>> return sprintf(buf, "%s\n", VFIO_AP_MDEV_NAME_HWVIRT); >>>> @@ -754,6 +802,8 @@ static ssize_t matrix_show(struct device *dev, >>>> struct device_attribute *attr, >>>> .mdev_attr_groups = vfio_ap_mdev_attr_groups, >>>> .create = vfio_ap_mdev_create, >>>> .remove = vfio_ap_mdev_remove, >>>> + .open = vfio_ap_mdev_open, >>>> + .release = vfio_ap_mdev_release, >>>> }; >>>> >>>> int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) >>>> diff --git a/drivers/s390/crypto/vfio_ap_private.h >>>> b/drivers/s390/crypto/vfio_ap_private.h >>>> index f248faf..48e2806 100644 >>>> --- a/drivers/s390/crypto/vfio_ap_private.h >>>> +++ b/drivers/s390/crypto/vfio_ap_private.h >>>> @@ -31,6 +31,8 @@ struct ap_matrix { >>>> >>>> struct ap_matrix_mdev { >>>> struct kvm_ap_matrix *matrix; >>>> + struct notifier_block group_notifier; >>>> + struct kvm *kvm; >>>> }; >>>> >>>> static inline struct ap_matrix *to_ap_matrix(struct device *dev) >>> >>> >> >
| |