| Message ID | 1536781396-13601-12-git-send-email-akrowiak@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | guest dedicated crypto adapters | expand |
On 09/12/2018 09:43 PM, Tony Krowiak wrote: > +/** > + * vfio_ap_mdev_open_once > + * > + * @matrix_mdev: a mediated matrix device > + * > + * Return 0 if no other mediated matrix device has been opened for the > + * KVM guest assigned to @matrix_mdev; otherwise, returns an error. > + */ > +static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev, > + struct kvm *kvm) > +{ > + struct ap_matrix_mdev *m; > + > + mutex_lock(&matrix_dev->lock); > + > + list_for_each_entry(m, &matrix_dev->mdev_list, node) { > + if ((m != matrix_mdev) && (m->kvm == kvm)) { > + mutex_unlock(&matrix_dev->lock); > + return -EPERM; > + } > + } > + > + mutex_unlock(&matrix_dev->lock); > + > + return 0; > +} > + > +static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + int ret; > + struct ap_matrix_mdev *matrix_mdev; > + > + if (action != VFIO_GROUP_NOTIFY_SET_KVM) > + return NOTIFY_OK; > + > + matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier); > + > + if (!data) { > + matrix_mdev->kvm = NULL; > + return NOTIFY_OK; > + } > + > + ret = vfio_ap_mdev_open_once(matrix_mdev, data); This could be racy. Two threads doing vfio_ap_mdev_group_notifier() can first get 0 here in a sense that there is no such kvm in the list, and then both set the very same kvm three lines below. Which would result in what we are trying to prevent. Also vfio_ap_mdev_open_once() does not seem like an appropriate name any more. If we were to do the matrix_mdev->kvm = kvm in there we could call it something like vfio_ap_mdev_set_kvm(). > + if (ret) > + return NOTIFY_DONE; > + > + matrix_mdev->kvm = data; > + > + ret = kvm_ap_validate_crypto_setup(matrix_mdev->kvm); > + if (ret) > + return ret; > + > + vfio_ap_mdev_copy_masks(matrix_mdev); > + > + return NOTIFY_OK; > +}
On 09/18/2018 01:00 PM, Halil Pasic wrote: > > On 09/12/2018 09:43 PM, Tony Krowiak wrote: >> +/** >> + * vfio_ap_mdev_open_once >> + * >> + * @matrix_mdev: a mediated matrix device >> + * >> + * Return 0 if no other mediated matrix device has been opened for the >> + * KVM guest assigned to @matrix_mdev; otherwise, returns an error. >> + */ >> +static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev, >> + struct kvm *kvm) >> +{ >> + struct ap_matrix_mdev *m; >> + >> + mutex_lock(&matrix_dev->lock); >> + >> + list_for_each_entry(m, &matrix_dev->mdev_list, node) { >> + if ((m != matrix_mdev) && (m->kvm == kvm)) { >> + mutex_unlock(&matrix_dev->lock); >> + return -EPERM; >> + } >> + } >> + >> + mutex_unlock(&matrix_dev->lock); >> + >> + return 0; >> +} >> + >> +static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, >> + unsigned long action, void *data) >> +{ >> + int ret; >> + struct ap_matrix_mdev *matrix_mdev; >> + >> + if (action != VFIO_GROUP_NOTIFY_SET_KVM) >> + return NOTIFY_OK; >> + >> + matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier); >> + >> + if (!data) { >> + matrix_mdev->kvm = NULL; >> + return NOTIFY_OK; >> + } >> + >> + ret = vfio_ap_mdev_open_once(matrix_mdev, data); > This could be racy. Two threads doing vfio_ap_mdev_group_notifier() > can first get 0 here in a sense that there is no such kvm in the list, > and then both set the very same kvm three lines below. Which would > result in what we are trying to prevent. > > Also vfio_ap_mdev_open_once() does not seem like an appropriate name > any more. If we were to do the matrix_mdev->kvm = kvm in there we could > call it something like vfio_ap_mdev_set_kvm(). I'm moving the matrix-mdev->kvm = kvm inside the mutex lock in vfio_ap_mdev_open_once() ... also renaming it to vfio_ap_mdev_set_kvm(). > >> + if (ret) >> + return NOTIFY_DONE; >> + >> + matrix_mdev->kvm = data; >> + >> + ret = kvm_ap_validate_crypto_setup(matrix_mdev->kvm); >> + if (ret) >> + return ret; >> + >> + vfio_ap_mdev_copy_masks(matrix_mdev); >> + >> + return NOTIFY_OK; >> +}
On 09/12/2018 03:43 PM, Tony Krowiak wrote: > From: Tony Krowiak <akrowiak@linux.ibm.com> > > Implements the open callback on the mediated matrix device. > The function registers a group notifier to receive notification > of the VFIO_GROUP_NOTIFY_SET_KVM event. When notified, > the vfio_ap device driver will get access to the guest's > kvm structure. The open callback must ensure that only one > mediated device shall be opened per guest. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > Acked-by: Halil Pasic <pasic@linux.ibm.com> > Tested-by: Michael Mueller <mimu@linux.ibm.com> > Tested-by: Farhan Ali <alifm@linux.ibm.com> > Tested-by: Pierre Morel <pmorel@linux.ibm.com> > Acked-by: Pierre Morel <pmorel@linux.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > drivers/s390/crypto/vfio_ap_ops.c | 168 +++++++++++++++++++++++++++++++++ > drivers/s390/crypto/vfio_ap_private.h | 5 + > 3 files changed, 174 insertions(+), 0 deletions(-) > (...) > @@ -699,12 +730,149 @@ static ssize_t matrix_show(struct device *dev, struct device_attribute *attr, > NULL > }; > > +/** > + * Verify that the AP instructions are available on the guest. This is indicated > + * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. > + */ > +static int kvm_ap_validate_crypto_setup(struct kvm *kvm) > +{ > + if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) > + return 0; > + > + return -EOPNOTSUPP; > +} > + (...) > + > +/** > + * vfio_ap_mdev_open_once > + * > + * @matrix_mdev: a mediated matrix device > + * > + * Return 0 if no other mediated matrix device has been opened for the > + * KVM guest assigned to @matrix_mdev; otherwise, returns an error. > + */ > +static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev, > + struct kvm *kvm) > +{ > + struct ap_matrix_mdev *m; > + > + mutex_lock(&matrix_dev->lock); > + > + list_for_each_entry(m, &matrix_dev->mdev_list, node) { > + if ((m != matrix_mdev) && (m->kvm == kvm)) { > + mutex_unlock(&matrix_dev->lock); > + return -EPERM; > + } > + } > + > + mutex_unlock(&matrix_dev->lock); > + > + return 0; > +} > + > +static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + int ret; > + struct ap_matrix_mdev *matrix_mdev; > + > + if (action != VFIO_GROUP_NOTIFY_SET_KVM) > + return NOTIFY_OK; > + > + matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier); > + > + if (!data) { > + matrix_mdev->kvm = NULL; > + return NOTIFY_OK; > + } > + > + ret = vfio_ap_mdev_open_once(matrix_mdev, data); > + if (ret) > + return NOTIFY_DONE; > + > + matrix_mdev->kvm = data; > + > + ret = kvm_ap_validate_crypto_setup(matrix_mdev->kvm); > + if (ret) > + return ret; > + > + vfio_ap_mdev_copy_masks(matrix_mdev); > + > + 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; > + > + > + if (!try_module_get(THIS_MODULE)) > + return -ENODEV; > + > + 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) { > + module_put(THIS_MODULE); > + return ret; > + } > + > + return 0; > +} > + (...) The fixup! patch below modifies this patch (11/26) to illustrate how David's recommendation will be implemented for v11 of the series. It is one of three fixup! patches (the other two are in responses to 03/26 and 25/26) included to generate discussion in v10 rather than waiting until v11 for comments. -----------------------------------8<----------------------------------- From: Tony Krowiak <akrowiak@linux.ibm.com> Date: Thu, 20 Sep 2018 12:01:53 -0400 Subject: [FIXUP v10] fixup!: s390: vfio-ap: implement mediated device open callback * Fix race condition in KVM notifier * Remove test for KVM_S390_VM_CPU_FEAT_AP --- drivers/s390/crypto/vfio_ap_ops.c | 26 +++++++++++++++----------- 1 files changed, 15 insertions(+), 11 deletions(-) diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 8bc0cdd..573a5cc 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -731,12 +731,12 @@ static ssize_t matrix_show(struct device *dev, struct device_attribute *attr, }; /** - * Verify that the AP instructions are available on the guest. This is indicated - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. + * Verify that the AP instructions are being interpreted by firmware for the + * guest. This is indicated by the kvm->arch.crypto.apie flag. */ static int kvm_ap_validate_crypto_setup(struct kvm *kvm) { - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) + if (kvm->arch.crypto.apie) return 0; return -EOPNOTSUPP; @@ -772,15 +772,19 @@ static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev) } /** - * vfio_ap_mdev_open_once + * vfio_ap_mdev_set_kvm * * @matrix_mdev: a mediated matrix device + * @kvm: reference to KVM instance * - * Return 0 if no other mediated matrix device has been opened for the - * KVM guest assigned to @matrix_mdev; otherwise, returns an error. + * Verifies no other mediated matrix device has a reference to @kvm and sets a + * reference to it in @matrix_mdev->kvm. + * + * Return 0 if no other mediated matrix device has a reference to @kvm; + * otherwise, returns -EPERM. */ -static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev, - struct kvm *kvm) +static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, + struct kvm *kvm) { struct ap_matrix_mdev *m; @@ -793,6 +797,7 @@ static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev, } } + matrix_mdev->kvm = kvm; mutex_unlock(&matrix_dev->lock); return 0; @@ -814,16 +819,15 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, return NOTIFY_OK; } - ret = vfio_ap_mdev_open_once(matrix_mdev, data); + ret = vfio_ap_mdev_set_kvm(matrix_mdev, data); if (ret) return NOTIFY_DONE; - matrix_mdev->kvm = data; - ret = kvm_ap_validate_crypto_setup(matrix_mdev->kvm); if (ret) return ret; + vfio_ap_mdev_copy_masks(matrix_mdev); return NOTIFY_OK;
> /** > - * Verify that the AP instructions are available on the guest. This is > indicated > - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. > + * Verify that the AP instructions are being interpreted by firmware > for the > + * guest. This is indicated by the kvm->arch.crypto.apie flag. > */ > static int kvm_ap_validate_crypto_setup(struct kvm *kvm) > { > - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) > + if (kvm->arch.crypto.apie) > return 0; I wonder if this check makes sense, because apie can be toggled during runtime. I guess it would be sufficient to check if the ap control block is available and apie is supported by the HW.
On 09/24/2018 04:40 AM, David Hildenbrand wrote: > >> /** >> - * Verify that the AP instructions are available on the guest. This is >> indicated >> - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. >> + * Verify that the AP instructions are being interpreted by firmware >> for the >> + * guest. This is indicated by the kvm->arch.crypto.apie flag. >> */ >> static int kvm_ap_validate_crypto_setup(struct kvm *kvm) >> { >> - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) >> + if (kvm->arch.crypto.apie) >> return 0; > > I wonder if this check makes sense, because apie can be toggled during > runtime. I guess it would be sufficient to check if the ap control block > is available and apie is supported by the HW. I am not clear about what you are getting at here, but I'll attempt to respond. There is no need to check if the AP control block (CRYCB) is available as the address is set in the CRYCBD three instructions above, even if AP instructions are not available. Regarding whether apie is supported by the hardware, the value of vcpu->kvm->arch.crypto.apie can not be set unless it is supported by the HW. In the patch (24/26) that provides the VM attributes to toggle this value, it can only be turned on if the AP instructions are available. I might also note that the kvm_ap_validate_crypto_setup() function is called whenever one of the VM crypto attributes is changed, so it makes sense that decisions made in this function are based on a change to a VM crypto attribute. In my first pass at changing this function, I checked ap_instructions_available() here, but after considering all of the above, it made sense to me to check the apie flag. > >
On 24/09/2018 18:07, Tony Krowiak wrote: > On 09/24/2018 04:40 AM, David Hildenbrand wrote: >> >>> /** >>> - * Verify that the AP instructions are available on the guest. This is >>> indicated >>> - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. >>> + * Verify that the AP instructions are being interpreted by firmware >>> for the >>> + * guest. This is indicated by the kvm->arch.crypto.apie flag. >>> */ >>> static int kvm_ap_validate_crypto_setup(struct kvm *kvm) >>> { >>> - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) >>> + if (kvm->arch.crypto.apie) >>> return 0; >> >> I wonder if this check makes sense, because apie can be toggled during >> runtime. I guess it would be sufficient to check if the ap control block >> is available and apie is supported by the HW. > > I am not clear about what you are getting at here, but I'll attempt > to respond. There is no need to check if the AP control block (CRYCB) > is available as the address is set in the CRYCBD three instructions > above, even if AP instructions are not available. Regarding whether apie > is supported by the hardware, the value of vcpu->kvm->arch.crypto.apie > can not be set unless it is supported by the HW. In the patch (24/26) > that provides the VM attributes to toggle this value, it can only be > turned on if the AP instructions are available. I might also note that > the kvm_ap_validate_crypto_setup() function is called whenever one of > the VM crypto attributes is changed, so it makes sense that decisions > made in this function are based on a change to a VM crypto attribute. In > my first pass at changing this function, I checked > ap_instructions_available() here, but after considering all of the > above, it made sense to me to check the apie flag. > I prefer ap_instructions_available(). As I said, kvm->arch.crypto.apie is a moving target.
On 09/24/2018 02:40 PM, David Hildenbrand wrote: > On 24/09/2018 18:07, Tony Krowiak wrote: >> On 09/24/2018 04:40 AM, David Hildenbrand wrote: >>> >>>> /** >>>> - * Verify that the AP instructions are available on the guest. This is >>>> indicated >>>> - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. >>>> + * Verify that the AP instructions are being interpreted by firmware >>>> for the >>>> + * guest. This is indicated by the kvm->arch.crypto.apie flag. >>>> */ >>>> static int kvm_ap_validate_crypto_setup(struct kvm *kvm) >>>> { >>>> - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) >>>> + if (kvm->arch.crypto.apie) >>>> return 0; >>> >>> I wonder if this check makes sense, because apie can be toggled during >>> runtime. I guess it would be sufficient to check if the ap control block >>> is available and apie is supported by the HW. >> >> I am not clear about what you are getting at here, but I'll attempt >> to respond. There is no need to check if the AP control block (CRYCB) >> is available as the address is set in the CRYCBD three instructions >> above, even if AP instructions are not available. Regarding whether apie >> is supported by the hardware, the value of vcpu->kvm->arch.crypto.apie >> can not be set unless it is supported by the HW. In the patch (24/26) >> that provides the VM attributes to toggle this value, it can only be >> turned on if the AP instructions are available. I might also note that >> the kvm_ap_validate_crypto_setup() function is called whenever one of >> the VM crypto attributes is changed, so it makes sense that decisions >> made in this function are based on a change to a VM crypto attribute. In >> my first pass at changing this function, I checked >> ap_instructions_available() here, but after considering all of the >> above, it made sense to me to check the apie flag. >> > > I prefer ap_instructions_available(). As I said, kvm->arch.crypto.apie > is a moving target. Okay then. >
On 09/24/2018 02:40 PM, David Hildenbrand wrote: > On 24/09/2018 18:07, Tony Krowiak wrote: >> On 09/24/2018 04:40 AM, David Hildenbrand wrote: >>> >>>> /** >>>> - * Verify that the AP instructions are available on the guest. This is >>>> indicated >>>> - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. >>>> + * Verify that the AP instructions are being interpreted by firmware >>>> for the >>>> + * guest. This is indicated by the kvm->arch.crypto.apie flag. >>>> */ >>>> static int kvm_ap_validate_crypto_setup(struct kvm *kvm) >>>> { >>>> - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) >>>> + if (kvm->arch.crypto.apie) >>>> return 0; >>> >>> I wonder if this check makes sense, because apie can be toggled during >>> runtime. I guess it would be sufficient to check if the ap control block >>> is available and apie is supported by the HW. >> >> I am not clear about what you are getting at here, but I'll attempt >> to respond. There is no need to check if the AP control block (CRYCB) >> is available as the address is set in the CRYCBD three instructions >> above, even if AP instructions are not available. Regarding whether apie >> is supported by the hardware, the value of vcpu->kvm->arch.crypto.apie >> can not be set unless it is supported by the HW. In the patch (24/26) >> that provides the VM attributes to toggle this value, it can only be >> turned on if the AP instructions are available. I might also note that >> the kvm_ap_validate_crypto_setup() function is called whenever one of >> the VM crypto attributes is changed, so it makes sense that decisions >> made in this function are based on a change to a VM crypto attribute. In >> my first pass at changing this function, I checked >> ap_instructions_available() here, but after considering all of the >> above, it made sense to me to check the apie flag. >> > > I prefer ap_instructions_available(). As I said, kvm->arch.crypto.apie > is a moving target. Looking at this again, I think I responded before my brain shifted from digesting comments about patch 24/26 (enable/disable APIE) to the context for your comment here; namely, the device open callback. My comment above makes no sense in this context. From the perspective of the vfio_ap device driver, there is one requirement that must be met in order to provide pass-through functionality: The AP instructions must be must be interpreted by the HW (i.e., ECA.28 == 1). Checking whether AP instructions are available does not tell us whether they are being interpreted by HW. Checking whether the AP control block (i.e., CRYCB) is available, even when combined with the instruction availability check, does not provide any more insight into the value of ECA.28 becuase the CRYCB will be provided if the MSAX3 facility is installed (STFLE.76) for the guest regardless of whether AP instructions are available or not. There is no doubt that if the AP instructions are not available, then the mdev open callback should fail, but it doesn't tell the whole story. I realize that our CPU model protects against configuring a vfio-ap device for the guest if ap=off, but this function knows nothing about userspace. I can make a similar argument that kvm->arch.crypto.apie will be switched on only if ap=on but again, that is userspace configuration. Having said all of the above, maybe it doesn't really matter whether AP instructions are being interpreted or not. If ECA.28 == 0, then the AP masks may very well be ignored since all AP instructions will be intercepted; so, maybe checking AP instruction availability is all that is needed. I will verify this and if I'm correct, I'll make the change you suggested. >
On 24/09/2018 21:46, Tony Krowiak wrote: > On 09/24/2018 02:40 PM, David Hildenbrand wrote: >> On 24/09/2018 18:07, Tony Krowiak wrote: >>> On 09/24/2018 04:40 AM, David Hildenbrand wrote: >>>> >>>>> /** >>>>> - * Verify that the AP instructions are available on the guest. This is >>>>> indicated >>>>> - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. >>>>> + * Verify that the AP instructions are being interpreted by firmware >>>>> for the >>>>> + * guest. This is indicated by the kvm->arch.crypto.apie flag. >>>>> */ >>>>> static int kvm_ap_validate_crypto_setup(struct kvm *kvm) >>>>> { >>>>> - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) >>>>> + if (kvm->arch.crypto.apie) >>>>> return 0; >>>> >>>> I wonder if this check makes sense, because apie can be toggled during >>>> runtime. I guess it would be sufficient to check if the ap control block >>>> is available and apie is supported by the HW. >>> >>> I am not clear about what you are getting at here, but I'll attempt >>> to respond. There is no need to check if the AP control block (CRYCB) >>> is available as the address is set in the CRYCBD three instructions >>> above, even if AP instructions are not available. Regarding whether apie >>> is supported by the hardware, the value of vcpu->kvm->arch.crypto.apie >>> can not be set unless it is supported by the HW. In the patch (24/26) >>> that provides the VM attributes to toggle this value, it can only be >>> turned on if the AP instructions are available. I might also note that >>> the kvm_ap_validate_crypto_setup() function is called whenever one of >>> the VM crypto attributes is changed, so it makes sense that decisions >>> made in this function are based on a change to a VM crypto attribute. In >>> my first pass at changing this function, I checked >>> ap_instructions_available() here, but after considering all of the >>> above, it made sense to me to check the apie flag. >>> >> >> I prefer ap_instructions_available(). As I said, kvm->arch.crypto.apie >> is a moving target. > > Looking at this again, I think I responded before my brain shifted from > digesting comments about patch 24/26 (enable/disable APIE) to the > context for your comment here; namely, the device open callback. My > comment above makes no sense in this context. From the perspective of > the vfio_ap device driver, there is one requirement that must be met in > order to provide pass-through functionality: The AP instructions must be > must be interpreted by the HW (i.e., ECA.28 == 1). Checking whether AP > instructions are available does not tell us whether they are being > interpreted by HW. Checking whether the AP control block (i.e., CRYCB) > is available, even when combined with the instruction availability > check, does not provide any more insight into the value of ECA.28 > becuase the CRYCB will be provided if the MSAX3 facility is installed > (STFLE.76) for the guest regardless of whether AP instructions are > available or not. There is no doubt that if the AP instructions are > not available, then the mdev open callback should fail, but it doesn't > tell the whole story. > > I realize that our CPU model protects against configuring a vfio-ap > device for the guest if ap=off, but this function knows nothing about > userspace. I can make a similar argument that kvm->arch.crypto.apie > will be switched on only if ap=on but again, that is userspace > configuration. > > Having said all of the above, maybe it doesn't really matter whether > AP instructions are being interpreted or not. If ECA.28 == 0, then > the AP masks may very well be ignored since all AP instructions will > be intercepted; so, maybe checking AP instruction availability is all > that is needed. I will verify this and if I'm correct, I'll make the > change you suggested. Yes, that was exactly what I had in mind - we just have to make sure that the ap control block exists, so we can set the right mask bits. If QEMU asks for an intercept, it shall get an intercept. But please proceed with whatever you think is best!
On 09/24/2018 03:55 PM, David Hildenbrand wrote: > On 24/09/2018 21:46, Tony Krowiak wrote: >> On 09/24/2018 02:40 PM, David Hildenbrand wrote: >>> On 24/09/2018 18:07, Tony Krowiak wrote: >>>> On 09/24/2018 04:40 AM, David Hildenbrand wrote: >>>>> >>>>>> /** >>>>>> - * Verify that the AP instructions are available on the guest. This is >>>>>> indicated >>>>>> - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. >>>>>> + * Verify that the AP instructions are being interpreted by firmware >>>>>> for the >>>>>> + * guest. This is indicated by the kvm->arch.crypto.apie flag. >>>>>> */ >>>>>> static int kvm_ap_validate_crypto_setup(struct kvm *kvm) >>>>>> { >>>>>> - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) >>>>>> + if (kvm->arch.crypto.apie) >>>>>> return 0; >>>>> >>>>> I wonder if this check makes sense, because apie can be toggled during >>>>> runtime. I guess it would be sufficient to check if the ap control block >>>>> is available and apie is supported by the HW. >>>> >>>> I am not clear about what you are getting at here, but I'll attempt >>>> to respond. There is no need to check if the AP control block (CRYCB) >>>> is available as the address is set in the CRYCBD three instructions >>>> above, even if AP instructions are not available. Regarding whether apie >>>> is supported by the hardware, the value of vcpu->kvm->arch.crypto.apie >>>> can not be set unless it is supported by the HW. In the patch (24/26) >>>> that provides the VM attributes to toggle this value, it can only be >>>> turned on if the AP instructions are available. I might also note that >>>> the kvm_ap_validate_crypto_setup() function is called whenever one of >>>> the VM crypto attributes is changed, so it makes sense that decisions >>>> made in this function are based on a change to a VM crypto attribute. In >>>> my first pass at changing this function, I checked >>>> ap_instructions_available() here, but after considering all of the >>>> above, it made sense to me to check the apie flag. >>>> >>> >>> I prefer ap_instructions_available(). As I said, kvm->arch.crypto.apie >>> is a moving target. >> >> Looking at this again, I think I responded before my brain shifted from >> digesting comments about patch 24/26 (enable/disable APIE) to the >> context for your comment here; namely, the device open callback. My >> comment above makes no sense in this context. From the perspective of >> the vfio_ap device driver, there is one requirement that must be met in >> order to provide pass-through functionality: The AP instructions must be >> must be interpreted by the HW (i.e., ECA.28 == 1). Checking whether AP >> instructions are available does not tell us whether they are being >> interpreted by HW. Checking whether the AP control block (i.e., CRYCB) >> is available, even when combined with the instruction availability >> check, does not provide any more insight into the value of ECA.28 >> becuase the CRYCB will be provided if the MSAX3 facility is installed >> (STFLE.76) for the guest regardless of whether AP instructions are >> available or not. There is no doubt that if the AP instructions are >> not available, then the mdev open callback should fail, but it doesn't >> tell the whole story. >> >> I realize that our CPU model protects against configuring a vfio-ap >> device for the guest if ap=off, but this function knows nothing about >> userspace. I can make a similar argument that kvm->arch.crypto.apie >> will be switched on only if ap=on but again, that is userspace >> configuration. >> >> Having said all of the above, maybe it doesn't really matter whether >> AP instructions are being interpreted or not. If ECA.28 == 0, then >> the AP masks may very well be ignored since all AP instructions will >> be intercepted; so, maybe checking AP instruction availability is all >> that is needed. I will verify this and if I'm correct, I'll make the >> change you suggested. > > Yes, that was exactly what I had in mind - we just have to make sure > that the ap control block exists, so we can set the right mask bits. If > QEMU asks for an intercept, it shall get an intercept. > > But please proceed with whatever you think is best! After discussing this with Halil, here's what I decided: * There will be no check for kvm->arch.crypto.apie here * A check for ap_instructions_available() will not be executed here, but inserted into the vfio_ap module init function. The module init function will fail (ENODEV) if the AP instructions are not installed. In my (our) opinion that makes more sense given the purpose of the vfio_ap driver is to pass through the AP instructions to the guest. * A check will be added here to verify the CRYCB is available (i.e., matrix_mdev->kvm->arch.crypto.crycbd != 0). > >
On 25/09/2018 21:54, Tony Krowiak wrote: > On 09/24/2018 03:55 PM, David Hildenbrand wrote: >> On 24/09/2018 21:46, Tony Krowiak wrote: >>> On 09/24/2018 02:40 PM, David Hildenbrand wrote: >>>> On 24/09/2018 18:07, Tony Krowiak wrote: >>>>> On 09/24/2018 04:40 AM, David Hildenbrand wrote: >>>>>> >>>>>>> /** >>>>>>> - * Verify that the AP instructions are available on the guest. This is >>>>>>> indicated >>>>>>> - * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. >>>>>>> + * Verify that the AP instructions are being interpreted by firmware >>>>>>> for the >>>>>>> + * guest. This is indicated by the kvm->arch.crypto.apie flag. >>>>>>> */ >>>>>>> static int kvm_ap_validate_crypto_setup(struct kvm *kvm) >>>>>>> { >>>>>>> - if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) >>>>>>> + if (kvm->arch.crypto.apie) >>>>>>> return 0; >>>>>> >>>>>> I wonder if this check makes sense, because apie can be toggled during >>>>>> runtime. I guess it would be sufficient to check if the ap control block >>>>>> is available and apie is supported by the HW. >>>>> >>>>> I am not clear about what you are getting at here, but I'll attempt >>>>> to respond. There is no need to check if the AP control block (CRYCB) >>>>> is available as the address is set in the CRYCBD three instructions >>>>> above, even if AP instructions are not available. Regarding whether apie >>>>> is supported by the hardware, the value of vcpu->kvm->arch.crypto.apie >>>>> can not be set unless it is supported by the HW. In the patch (24/26) >>>>> that provides the VM attributes to toggle this value, it can only be >>>>> turned on if the AP instructions are available. I might also note that >>>>> the kvm_ap_validate_crypto_setup() function is called whenever one of >>>>> the VM crypto attributes is changed, so it makes sense that decisions >>>>> made in this function are based on a change to a VM crypto attribute. In >>>>> my first pass at changing this function, I checked >>>>> ap_instructions_available() here, but after considering all of the >>>>> above, it made sense to me to check the apie flag. >>>>> >>>> >>>> I prefer ap_instructions_available(). As I said, kvm->arch.crypto.apie >>>> is a moving target. >>> >>> Looking at this again, I think I responded before my brain shifted from >>> digesting comments about patch 24/26 (enable/disable APIE) to the >>> context for your comment here; namely, the device open callback. My >>> comment above makes no sense in this context. From the perspective of >>> the vfio_ap device driver, there is one requirement that must be met in >>> order to provide pass-through functionality: The AP instructions must be >>> must be interpreted by the HW (i.e., ECA.28 == 1). Checking whether AP >>> instructions are available does not tell us whether they are being >>> interpreted by HW. Checking whether the AP control block (i.e., CRYCB) >>> is available, even when combined with the instruction availability >>> check, does not provide any more insight into the value of ECA.28 >>> becuase the CRYCB will be provided if the MSAX3 facility is installed >>> (STFLE.76) for the guest regardless of whether AP instructions are >>> available or not. There is no doubt that if the AP instructions are >>> not available, then the mdev open callback should fail, but it doesn't >>> tell the whole story. >>> >>> I realize that our CPU model protects against configuring a vfio-ap >>> device for the guest if ap=off, but this function knows nothing about >>> userspace. I can make a similar argument that kvm->arch.crypto.apie >>> will be switched on only if ap=on but again, that is userspace >>> configuration. >>> >>> Having said all of the above, maybe it doesn't really matter whether >>> AP instructions are being interpreted or not. If ECA.28 == 0, then >>> the AP masks may very well be ignored since all AP instructions will >>> be intercepted; so, maybe checking AP instruction availability is all >>> that is needed. I will verify this and if I'm correct, I'll make the >>> change you suggested. >> >> Yes, that was exactly what I had in mind - we just have to make sure >> that the ap control block exists, so we can set the right mask bits. If >> QEMU asks for an intercept, it shall get an intercept. >> >> But please proceed with whatever you think is best! > > After discussing this with Halil, here's what I decided: > * There will be no check for kvm->arch.crypto.apie here > * A check for ap_instructions_available() will not be executed > here, but inserted into the vfio_ap module init function. > The module init function will fail (ENODEV) if the AP > instructions are not installed. In my (our) opinion that > makes more sense given the purpose of the vfio_ap driver is > to pass through the AP instructions to the guest. > * A check will be added here to verify the CRYCB is available (i.e., > matrix_mdev->kvm->arch.crypto.crycbd != 0). > Sounds good to me!
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 1e758fe..b32bd1b 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -258,6 +258,7 @@ struct kvm_s390_sie_block { __u64 tecmc; /* 0x00e8 */ __u8 reservedf0[12]; /* 0x00f0 */ #define CRYCB_FORMAT_MASK 0x00000003 +#define CRYCB_FORMAT0 0x00000000 #define CRYCB_FORMAT1 0x00000001 #define CRYCB_FORMAT2 0x00000003 __u32 crycbd; /* 0x00fc */ diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 2a2dcf7..8bc0cdd 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -13,6 +13,10 @@ #include <linux/device.h> #include <linux/list.h> #include <linux/ctype.h> +#include <linux/bitops.h> +#include <linux/kvm_host.h> +#include <linux/module.h> +#include <asm/kvm.h> #include <asm/zcrypt.h> #include "vfio_ap_private.h" @@ -54,6 +58,9 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev) { struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + if (matrix_mdev->kvm) + return -EBUSY; + mutex_lock(&matrix_dev->lock); list_del(&matrix_mdev->node); mutex_unlock(&matrix_dev->lock); @@ -309,6 +316,10 @@ static ssize_t assign_adapter_store(struct device *dev, struct mdev_device *mdev = mdev_from_dev(dev); struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + /* If the guest is running, disallow assignment of adapter */ + if (matrix_mdev->kvm) + return -EBUSY; + ret = kstrtoul(buf, 0, &apid); if (ret) return ret; @@ -370,6 +381,10 @@ static ssize_t unassign_adapter_store(struct device *dev, struct mdev_device *mdev = mdev_from_dev(dev); struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + /* If the guest is running, disallow un-assignment of adapter */ + if (matrix_mdev->kvm) + return -EBUSY; + ret = kstrtoul(buf, 0, &apid); if (ret) return ret; @@ -447,6 +462,10 @@ static ssize_t assign_domain_store(struct device *dev, struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); unsigned long max_apqi = matrix_mdev->matrix.aqm_max; + /* If the guest is running, disallow assignment of domain */ + if (matrix_mdev->kvm) + return -EBUSY; + ret = kstrtoul(buf, 0, &apqi); if (ret) return ret; @@ -504,6 +523,10 @@ static ssize_t unassign_domain_store(struct device *dev, struct mdev_device *mdev = mdev_from_dev(dev); struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + /* If the guest is running, disallow un-assignment of domain */ + if (matrix_mdev->kvm) + return -EBUSY; + ret = kstrtoul(buf, 0, &apqi); if (ret) return ret; @@ -544,6 +567,10 @@ static ssize_t assign_control_domain_store(struct device *dev, struct mdev_device *mdev = mdev_from_dev(dev); struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + /* If the guest is running, disallow assignment of control domain */ + if (matrix_mdev->kvm) + return -EBUSY; + ret = kstrtoul(buf, 0, &id); if (ret) return ret; @@ -590,6 +617,10 @@ static ssize_t unassign_control_domain_store(struct device *dev, struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); unsigned long max_domid = matrix_mdev->matrix.adm_max; + /* If the guest is running, disallow un-assignment of control domain */ + if (matrix_mdev->kvm) + return -EBUSY; + ret = kstrtoul(buf, 0, &domid); if (ret) return ret; @@ -699,12 +730,149 @@ static ssize_t matrix_show(struct device *dev, struct device_attribute *attr, NULL }; +/** + * Verify that the AP instructions are available on the guest. This is indicated + * via the KVM_S390_VM_CPU_FEAT_AP CPU model feature. + */ +static int kvm_ap_validate_crypto_setup(struct kvm *kvm) +{ + if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat)) + return 0; + + return -EOPNOTSUPP; +} + +static void vfio_ap_mdev_copy_masks(struct ap_matrix_mdev *matrix_mdev) +{ + int nbytes; + unsigned long *apm, *aqm, *adm; + struct kvm_s390_crypto_cb *crycb = matrix_mdev->kvm->arch.crypto.crycb; + + switch (matrix_mdev->kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) { + case CRYCB_FORMAT2: + apm = (unsigned long *)crycb->apcb1.apm; + aqm = (unsigned long *)crycb->apcb1.aqm; + adm = (unsigned long *)crycb->apcb1.adm; + break; + case CRYCB_FORMAT1: + case CRYCB_FORMAT0: + default: + apm = (unsigned long *)crycb->apcb0.apm; + aqm = (unsigned long *)crycb->apcb0.aqm; + adm = (unsigned long *)crycb->apcb0.adm; + break; + } + + nbytes = DIV_ROUND_UP(matrix_mdev->matrix.apm_max + 1, BITS_PER_BYTE); + memcpy(apm, matrix_mdev->matrix.apm, nbytes); + nbytes = DIV_ROUND_UP(matrix_mdev->matrix.aqm_max + 1, BITS_PER_BYTE); + memcpy(aqm, matrix_mdev->matrix.aqm, nbytes); + nbytes = DIV_ROUND_UP(matrix_mdev->matrix.adm_max + 1, BITS_PER_BYTE); + memcpy(adm, matrix_mdev->matrix.adm, nbytes); +} + +/** + * vfio_ap_mdev_open_once + * + * @matrix_mdev: a mediated matrix device + * + * Return 0 if no other mediated matrix device has been opened for the + * KVM guest assigned to @matrix_mdev; otherwise, returns an error. + */ +static int vfio_ap_mdev_open_once(struct ap_matrix_mdev *matrix_mdev, + struct kvm *kvm) +{ + struct ap_matrix_mdev *m; + + mutex_lock(&matrix_dev->lock); + + list_for_each_entry(m, &matrix_dev->mdev_list, node) { + if ((m != matrix_mdev) && (m->kvm == kvm)) { + mutex_unlock(&matrix_dev->lock); + return -EPERM; + } + } + + mutex_unlock(&matrix_dev->lock); + + return 0; +} + +static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, + unsigned long action, void *data) +{ + int ret; + struct ap_matrix_mdev *matrix_mdev; + + if (action != VFIO_GROUP_NOTIFY_SET_KVM) + return NOTIFY_OK; + + matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier); + + if (!data) { + matrix_mdev->kvm = NULL; + return NOTIFY_OK; + } + + ret = vfio_ap_mdev_open_once(matrix_mdev, data); + if (ret) + return NOTIFY_DONE; + + matrix_mdev->kvm = data; + + ret = kvm_ap_validate_crypto_setup(matrix_mdev->kvm); + if (ret) + return ret; + + vfio_ap_mdev_copy_masks(matrix_mdev); + + 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; + + + if (!try_module_get(THIS_MODULE)) + return -ENODEV; + + 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) { + module_put(THIS_MODULE); + return ret; + } + + return 0; +} + +static void vfio_ap_mdev_release(struct mdev_device *mdev) +{ + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + + if (matrix_mdev->kvm) + kvm_arch_crypto_clear_masks(matrix_mdev->kvm); + + vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY, + &matrix_mdev->group_notifier); + matrix_mdev->kvm = NULL; + module_put(THIS_MODULE); +} + static const struct mdev_parent_ops vfio_ap_matrix_ops = { .owner = THIS_MODULE, .supported_type_groups = vfio_ap_mdev_type_groups, .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(void) diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h index a2eab78..ebad5db 100644 --- a/drivers/s390/crypto/vfio_ap_private.h +++ b/drivers/s390/crypto/vfio_ap_private.h @@ -71,10 +71,15 @@ struct ap_matrix { * @list: allows the ap_matrix_mdev struct to be added to a list * @matrix: the adapters, usage domains and control domains assigned to the * mediated matrix device. + * @group_notifier: notifier block used for specifying callback function for + * handling the VFIO_GROUP_NOTIFY_SET_KVM event + * @kvm: the struct holding guest's state */ struct ap_matrix_mdev { struct list_head node; struct ap_matrix matrix; + struct notifier_block group_notifier; + struct kvm *kvm; }; extern int vfio_ap_mdev_register(void);