The function pointer to the interception handler for the PQAP instruction
can get changed during the interception process. Let's add a
semaphore to struct kvm_s390_crypto to control read/write access to the
function pointer contained therein.
The semaphore must be locked for write access by the vfio_ap device driver
when notified that the KVM pointer has been set or cleared. It must be
locked for read access by the interception framework when the PQAP
instruction is intercepted.
Signed-off-by: Tony Krowiak <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
---
arch/s390/include/asm/kvm_host.h | 8 +++-----
arch/s390/kvm/kvm-s390.c | 1 +
arch/s390/kvm/priv.c | 10 ++++++----
drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------
drivers/s390/crypto/vfio_ap_private.h | 2 +-
5 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 9b4473f76e56..f18849d259e6 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
unsigned short ibc;
};
-struct kvm_s390_module_hook {
- int (*hook)(struct kvm_vcpu *vcpu);
- struct module *owner;
-};
+typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
struct kvm_s390_crypto {
struct kvm_s390_crypto_cb *crycb;
- struct kvm_s390_module_hook *pqap_hook;
+ struct rw_semaphore pqap_hook_rwsem;
+ crypto_hook *pqap_hook;
__u32 crycbd;
__u8 aes_kw;
__u8 dea_kw;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index b655a7d82bf0..a08f242a9f27 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
{
kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
kvm_s390_set_crycb_format(kvm);
+ init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
if (!test_kvm_facility(kvm, 76))
return;
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 9928f785c677..6bed9406c1f3 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
static int handle_pqap(struct kvm_vcpu *vcpu)
{
struct ap_queue_status status = {};
+ crypto_hook pqap_hook;
unsigned long reg0;
int ret;
uint8_t fc;
@@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
* Verify that the hook callback is registered, lock the owner
* and call the hook.
*/
+ down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
if (vcpu->kvm->arch.crypto.pqap_hook) {
- if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
- return -EOPNOTSUPP;
- ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
- module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
+ pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
+ ret = pqap_hook(vcpu);
if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000)
kvm_s390_set_psw_cc(vcpu, 3);
+ up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
return ret;
}
+ up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
/*
* A vfio_driver must register a hook.
* No hook means no driver to enable the SIE CRYCB and no queues.
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 122c85c22469..742277bc8d1c 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -352,8 +352,7 @@ static int vfio_ap_mdev_create(struct mdev_device *mdev)
vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
init_waitqueue_head(&matrix_mdev->wait_for_kvm);
mdev_set_drvdata(mdev, matrix_mdev);
- matrix_mdev->pqap_hook.hook = handle_pqap;
- matrix_mdev->pqap_hook.owner = THIS_MODULE;
+ matrix_mdev->pqap_hook = handle_pqap;
mutex_lock(&matrix_dev->lock);
list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
mutex_unlock(&matrix_dev->lock);
@@ -1115,15 +1114,20 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
}
kvm_get_kvm(kvm);
+ matrix_mdev->kvm = kvm;
matrix_mdev->kvm_busy = true;
mutex_unlock(&matrix_dev->lock);
+
+ down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
+ kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
+ up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
+
kvm_arch_crypto_set_masks(kvm,
matrix_mdev->matrix.apm,
matrix_mdev->matrix.aqm,
matrix_mdev->matrix.adm);
+
mutex_lock(&matrix_dev->lock);
- kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
- matrix_mdev->kvm = kvm;
matrix_mdev->kvm_busy = false;
wake_up_all(&matrix_mdev->wait_for_kvm);
}
@@ -1189,10 +1193,17 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
if (matrix_mdev->kvm) {
matrix_mdev->kvm_busy = true;
mutex_unlock(&matrix_dev->lock);
- kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+
+ if (matrix_mdev->kvm->arch.crypto.crycbd) {
+ down_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
+ matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+ up_write(&matrix_mdev->kvm->arch.crypto.pqap_hook_rwsem);
+
+ kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+ }
+
mutex_lock(&matrix_dev->lock);
vfio_ap_mdev_reset_queues(matrix_mdev->mdev);
- matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
kvm_put_kvm(matrix_mdev->kvm);
matrix_mdev->kvm = NULL;
matrix_mdev->kvm_busy = false;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index f82a6396acae..e12218e5a629 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -86,7 +86,7 @@ struct ap_matrix_mdev {
bool kvm_busy;
wait_queue_head_t wait_for_kvm;
struct kvm *kvm;
- struct kvm_s390_module_hook pqap_hook;
+ crypto_hook pqap_hook;
struct mdev_device *mdev;
};
--
2.30.2
On 19.07.21 21:35, Tony Krowiak wrote:
> The function pointer to the interception handler for the PQAP instruction
> can get changed during the interception process. Let's add a
> semaphore to struct kvm_s390_crypto to control read/write access to the
> function pointer contained therein.
>
> The semaphore must be locked for write access by the vfio_ap device driver
> when notified that the KVM pointer has been set or cleared. It must be
> locked for read access by the interception framework when the PQAP
> instruction is intercepted.
>
> Signed-off-by: Tony Krowiak <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> ---
> arch/s390/include/asm/kvm_host.h | 8 +++-----
> arch/s390/kvm/kvm-s390.c | 1 +
> arch/s390/kvm/priv.c | 10 ++++++----
> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------
> drivers/s390/crypto/vfio_ap_private.h | 2 +-
> 5 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 9b4473f76e56..f18849d259e6 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
> unsigned short ibc;
> };
>
> -struct kvm_s390_module_hook {
> - int (*hook)(struct kvm_vcpu *vcpu);
> - struct module *owner;
> -};
> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
>
> struct kvm_s390_crypto {
> struct kvm_s390_crypto_cb *crycb;
> - struct kvm_s390_module_hook *pqap_hook;
> + struct rw_semaphore pqap_hook_rwsem;
> + crypto_hook *pqap_hook;
> __u32 crycbd;
> __u8 aes_kw;
> __u8 dea_kw;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index b655a7d82bf0..a08f242a9f27 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
> {
> kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
> kvm_s390_set_crycb_format(kvm);
> + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
>
> if (!test_kvm_facility(kvm, 76))
> return;
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 9928f785c677..6bed9406c1f3 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
> static int handle_pqap(struct kvm_vcpu *vcpu)
> {
> struct ap_queue_status status = {};
> + crypto_hook pqap_hook;
> unsigned long reg0;
> int ret;
> uint8_t fc;
> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
> * Verify that the hook callback is registered, lock the owner
> * and call the hook.
> */
> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
> if (vcpu->kvm->arch.crypto.pqap_hook) {
> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> - return -EOPNOTSUPP;
> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
Dont we have to check for NULL here? If not can you add a comment why?
Otherwise this looks good.
> + ret = pqap_hook(vcpu);
[...]
On Wed, 18 Aug 2021 19:03:33 +0200
Christian Borntraeger <[email protected]> wrote:
> On 19.07.21 21:35, Tony Krowiak wrote:
> > The function pointer to the interception handler for the PQAP instruction
> > can get changed during the interception process. Let's add a
> > semaphore to struct kvm_s390_crypto to control read/write access to the
> > function pointer contained therein.
> >
> > The semaphore must be locked for write access by the vfio_ap device driver
> > when notified that the KVM pointer has been set or cleared. It must be
> > locked for read access by the interception framework when the PQAP
> > instruction is intercepted.
> >
> > Signed-off-by: Tony Krowiak <[email protected]>
> > Reviewed-by: Jason Gunthorpe <[email protected]>
> > ---
> > arch/s390/include/asm/kvm_host.h | 8 +++-----
> > arch/s390/kvm/kvm-s390.c | 1 +
> > arch/s390/kvm/priv.c | 10 ++++++----
> > drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------
> > drivers/s390/crypto/vfio_ap_private.h | 2 +-
> > 5 files changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> > index 9b4473f76e56..f18849d259e6 100644
> > --- a/arch/s390/include/asm/kvm_host.h
> > +++ b/arch/s390/include/asm/kvm_host.h
> > @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
> > unsigned short ibc;
> > };
> >
> > -struct kvm_s390_module_hook {
> > - int (*hook)(struct kvm_vcpu *vcpu);
> > - struct module *owner;
> > -};
> > +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
> >
> > struct kvm_s390_crypto {
> > struct kvm_s390_crypto_cb *crycb;
> > - struct kvm_s390_module_hook *pqap_hook;
> > + struct rw_semaphore pqap_hook_rwsem;
> > + crypto_hook *pqap_hook;
> > __u32 crycbd;
> > __u8 aes_kw;
> > __u8 dea_kw;
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index b655a7d82bf0..a08f242a9f27 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
> > {
> > kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
> > kvm_s390_set_crycb_format(kvm);
> > + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
> >
> > if (!test_kvm_facility(kvm, 76))
> > return;
> > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> > index 9928f785c677..6bed9406c1f3 100644
> > --- a/arch/s390/kvm/priv.c
> > +++ b/arch/s390/kvm/priv.c
> > @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
> > static int handle_pqap(struct kvm_vcpu *vcpu)
> > {
> > struct ap_queue_status status = {};
> > + crypto_hook pqap_hook;
> > unsigned long reg0;
> > int ret;
> > uint8_t fc;
> > @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
> > * Verify that the hook callback is registered, lock the owner
> > * and call the hook.
> > */
> > + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
> > if (vcpu->kvm->arch.crypto.pqap_hook) { <--- HERE
> > - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> > - return -EOPNOTSUPP;
> > - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> > - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> > + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>
> Dont we have to check for NULL here? If not can you add a comment why?
I believe we did the necessary check on the line I just marked with
"<--- HERE".
I find that "*" operator confusing in this context as it doesn't do
any good for us. I believe this situation is described in 6.5.3.2.4 of
the c11 standard. For convenience I will cite from the corresponding
draft:
"The unary * operator denotes indirection. If the operand points to a
function, the result is a function designator; if it points to an
object, the result is an lvalue designating the object. If the operand
has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
invalid value has been assigned to the pointer, the behavior of the
unary * operator is undefined."
Frankly I also fail to see the benefit of introducing the local variable
named "pqap_hook", but back then I decided to not complain about style.
Regards,
Halil
>
>
> > + ret = pqap_hook(vcpu);
> [...]
On 19.08.21 01:25, Halil Pasic wrote:
> On Wed, 18 Aug 2021 19:03:33 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> On 19.07.21 21:35, Tony Krowiak wrote:
>>> The function pointer to the interception handler for the PQAP instruction
>>> can get changed during the interception process. Let's add a
>>> semaphore to struct kvm_s390_crypto to control read/write access to the
>>> function pointer contained therein.
>>>
>>> The semaphore must be locked for write access by the vfio_ap device driver
>>> when notified that the KVM pointer has been set or cleared. It must be
>>> locked for read access by the interception framework when the PQAP
>>> instruction is intercepted.
>>>
>>> Signed-off-by: Tony Krowiak <[email protected]>
>>> Reviewed-by: Jason Gunthorpe <[email protected]>
>>> ---
>>> arch/s390/include/asm/kvm_host.h | 8 +++-----
>>> arch/s390/kvm/kvm-s390.c | 1 +
>>> arch/s390/kvm/priv.c | 10 ++++++----
>>> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------
>>> drivers/s390/crypto/vfio_ap_private.h | 2 +-
>>> 5 files changed, 28 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index 9b4473f76e56..f18849d259e6 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
>>> unsigned short ibc;
>>> };
>>>
>>> -struct kvm_s390_module_hook {
>>> - int (*hook)(struct kvm_vcpu *vcpu);
>>> - struct module *owner;
>>> -};
>>> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
>>>
>>> struct kvm_s390_crypto {
>>> struct kvm_s390_crypto_cb *crycb;
>>> - struct kvm_s390_module_hook *pqap_hook;
>>> + struct rw_semaphore pqap_hook_rwsem;
>>> + crypto_hook *pqap_hook;
>>> __u32 crycbd;
>>> __u8 aes_kw;
>>> __u8 dea_kw;
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index b655a7d82bf0..a08f242a9f27 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
>>> {
>>> kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>>> kvm_s390_set_crycb_format(kvm);
>>> + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
>>>
>>> if (!test_kvm_facility(kvm, 76))
>>> return;
>>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>>> index 9928f785c677..6bed9406c1f3 100644
>>> --- a/arch/s390/kvm/priv.c
>>> +++ b/arch/s390/kvm/priv.c
>>> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>>> static int handle_pqap(struct kvm_vcpu *vcpu)
>>> {
>>> struct ap_queue_status status = {};
>>> + crypto_hook pqap_hook;
>>> unsigned long reg0;
>>> int ret;
>>> uint8_t fc;
>>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>>> * Verify that the hook callback is registered, lock the owner
>>> * and call the hook.
>>> */
>>> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>>> if (vcpu->kvm->arch.crypto.pqap_hook) { <--- HERE
>>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
>>> - return -EOPNOTSUPP;
>>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
>>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
>>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>>
>> Dont we have to check for NULL here? If not can you add a comment why?
>
> I believe we did the necessary check on the line I just marked with
> "<--- HERE".
Right, I missed that.
Then
Reviewed-by: Christian Borntraeger <[email protected]>
>
> I find that "*" operator confusing in this context as it doesn't do
> any good for us. I believe this situation is described in 6.5.3.2.4 of
> the c11 standard. For convenience I will cite from the corresponding
> draft:
> "The unary * operator denotes indirection. If the operand points to a
> function, the result is a function designator; if it points to an
> object, the result is an lvalue designating the object. If the operand
> has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
> invalid value has been assigned to the pointer, the behavior of the
> unary * operator is undefined."
>
> Frankly I also fail to see the benefit of introducing the local variable
> named "pqap_hook", but back then I decided to not complain about style.
Right we can probably do better, but I would rather have the functional
fix in now.
>
> Regards,
> Halil
>
>>
>>
>>> + ret = pqap_hook(vcpu);
>> [...]
>
On 8/18/21 1:03 PM, Christian Borntraeger wrote:
> On 19.07.21 21:35, Tony Krowiak wrote:
>> The function pointer to the interception handler for the PQAP
>> instruction
>> can get changed during the interception process. Let's add a
>> semaphore to struct kvm_s390_crypto to control read/write access to the
>> function pointer contained therein.
>>
>> The semaphore must be locked for write access by the vfio_ap device
>> driver
>> when notified that the KVM pointer has been set or cleared. It must be
>> locked for read access by the interception framework when the PQAP
>> instruction is intercepted.
>>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> Reviewed-by: Jason Gunthorpe <[email protected]>
>> ---
>> arch/s390/include/asm/kvm_host.h | 8 +++-----
>> arch/s390/kvm/kvm-s390.c | 1 +
>> arch/s390/kvm/priv.c | 10 ++++++----
>> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------
>> drivers/s390/crypto/vfio_ap_private.h | 2 +-
>> 5 files changed, 28 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h
>> b/arch/s390/include/asm/kvm_host.h
>> index 9b4473f76e56..f18849d259e6 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
>> unsigned short ibc;
>> };
>> -struct kvm_s390_module_hook {
>> - int (*hook)(struct kvm_vcpu *vcpu);
>> - struct module *owner;
>> -};
>> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
>> struct kvm_s390_crypto {
>> struct kvm_s390_crypto_cb *crycb;
>> - struct kvm_s390_module_hook *pqap_hook;
>> + struct rw_semaphore pqap_hook_rwsem;
>> + crypto_hook *pqap_hook;
>> __u32 crycbd;
>> __u8 aes_kw;
>> __u8 dea_kw;
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index b655a7d82bf0..a08f242a9f27 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
>> {
>> kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>> kvm_s390_set_crycb_format(kvm);
>> + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
>> if (!test_kvm_facility(kvm, 76))
>> return;
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 9928f785c677..6bed9406c1f3 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>> static int handle_pqap(struct kvm_vcpu *vcpu)
>> {
>> struct ap_queue_status status = {};
>> + crypto_hook pqap_hook;
>> unsigned long reg0;
>> int ret;
>> uint8_t fc;
>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>> * Verify that the hook callback is registered, lock the owner
>> * and call the hook.
>> */
>> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>> if (vcpu->kvm->arch.crypto.pqap_hook) {
>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
>> - return -EOPNOTSUPP;
>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>
> Dont we have to check for NULL here? If not can you add a comment why?
Take a look above the removed lines: if (vcpu->kvm->arch.crypto.pqap_hook)
>
> Otherwise this looks good.
Also, in the cover letter I said this patch was already queued and was
included here because it pre-reqs the second patch. Is this patch not
already in Alex's tree?
>
>
>> + ret = pqap_hook(vcpu);
> [...]
On 8/18/21 7:25 PM, Halil Pasic wrote:
> On Wed, 18 Aug 2021 19:03:33 +0200
> Christian Borntraeger <[email protected]> wrote:
>
>> On 19.07.21 21:35, Tony Krowiak wrote:
>>> The function pointer to the interception handler for the PQAP instruction
>>> can get changed during the interception process. Let's add a
>>> semaphore to struct kvm_s390_crypto to control read/write access to the
>>> function pointer contained therein.
>>>
>>> The semaphore must be locked for write access by the vfio_ap device driver
>>> when notified that the KVM pointer has been set or cleared. It must be
>>> locked for read access by the interception framework when the PQAP
>>> instruction is intercepted.
>>>
>>> Signed-off-by: Tony Krowiak <[email protected]>
>>> Reviewed-by: Jason Gunthorpe <[email protected]>
>>> ---
>>> arch/s390/include/asm/kvm_host.h | 8 +++-----
>>> arch/s390/kvm/kvm-s390.c | 1 +
>>> arch/s390/kvm/priv.c | 10 ++++++----
>>> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------
>>> drivers/s390/crypto/vfio_ap_private.h | 2 +-
>>> 5 files changed, 28 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>>> index 9b4473f76e56..f18849d259e6 100644
>>> --- a/arch/s390/include/asm/kvm_host.h
>>> +++ b/arch/s390/include/asm/kvm_host.h
>>> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
>>> unsigned short ibc;
>>> };
>>>
>>> -struct kvm_s390_module_hook {
>>> - int (*hook)(struct kvm_vcpu *vcpu);
>>> - struct module *owner;
>>> -};
>>> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
>>>
>>> struct kvm_s390_crypto {
>>> struct kvm_s390_crypto_cb *crycb;
>>> - struct kvm_s390_module_hook *pqap_hook;
>>> + struct rw_semaphore pqap_hook_rwsem;
>>> + crypto_hook *pqap_hook;
>>> __u32 crycbd;
>>> __u8 aes_kw;
>>> __u8 dea_kw;
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index b655a7d82bf0..a08f242a9f27 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
>>> {
>>> kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
>>> kvm_s390_set_crycb_format(kvm);
>>> + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
>>>
>>> if (!test_kvm_facility(kvm, 76))
>>> return;
>>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>>> index 9928f785c677..6bed9406c1f3 100644
>>> --- a/arch/s390/kvm/priv.c
>>> +++ b/arch/s390/kvm/priv.c
>>> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>>> static int handle_pqap(struct kvm_vcpu *vcpu)
>>> {
>>> struct ap_queue_status status = {};
>>> + crypto_hook pqap_hook;
>>> unsigned long reg0;
>>> int ret;
>>> uint8_t fc;
>>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>>> * Verify that the hook callback is registered, lock the owner
>>> * and call the hook.
>>> */
>>> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>>> if (vcpu->kvm->arch.crypto.pqap_hook) { <--- HERE
>>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
>>> - return -EOPNOTSUPP;
>>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
>>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
>>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>> Dont we have to check for NULL here? If not can you add a comment why?
> I believe we did the necessary check on the line I just marked with
> "<--- HERE".
>
> I find that "*" operator confusing in this context as it doesn't do
> any good for us. I believe this situation is described in 6.5.3.2.4 of
> the c11 standard. For convenience I will cite from the corresponding
> draft:
> "The unary * operator denotes indirection. If the operand points to a
> function, the result is a function designator; if it points to an
> object, the result is an lvalue designating the object. If the operand
> has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
> invalid value has been assigned to the pointer, the behavior of the
> unary * operator is undefined."
>
> Frankly I also fail to see the benefit of introducing the local variable
> named "pqap_hook", but back then I decided to not complain about style.
The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function
pointer. The actual function pointer is stored in matrix_mdev->pqap_hook,
the reason being that the handle_pqap function in vfio_ap_ops.c
retrieves the matrix_mdev via a container_of macro. The dereferencing
of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was
to get the function pointer. There may have been a more stylish
way of doing this, but the functionality is there.
>
> Regards,
> Halil
>
>>
>>> + ret = pqap_hook(vcpu);
>> [...]
On Thu, 19 Aug 2021 09:20:28 -0400
Tony Krowiak <[email protected]> wrote:
> On 8/18/21 1:03 PM, Christian Borntraeger wrote:
> > On 19.07.21 21:35, Tony Krowiak wrote:
> >> The function pointer to the interception handler for the PQAP
> >> instruction
> >> can get changed during the interception process. Let's add a
> >> semaphore to struct kvm_s390_crypto to control read/write access to the
> >> function pointer contained therein.
> >>
> >> The semaphore must be locked for write access by the vfio_ap device
> >> driver
> >> when notified that the KVM pointer has been set or cleared. It must be
> >> locked for read access by the interception framework when the PQAP
> >> instruction is intercepted.
> >>
> >> Signed-off-by: Tony Krowiak <[email protected]>
> >> Reviewed-by: Jason Gunthorpe <[email protected]>
> >> ---
> >> arch/s390/include/asm/kvm_host.h | 8 +++-----
> >> arch/s390/kvm/kvm-s390.c | 1 +
> >> arch/s390/kvm/priv.c | 10 ++++++----
> >> drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++++++++++------
> >> drivers/s390/crypto/vfio_ap_private.h | 2 +-
> >> 5 files changed, 28 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/arch/s390/include/asm/kvm_host.h
> >> b/arch/s390/include/asm/kvm_host.h
> >> index 9b4473f76e56..f18849d259e6 100644
> >> --- a/arch/s390/include/asm/kvm_host.h
> >> +++ b/arch/s390/include/asm/kvm_host.h
> >> @@ -798,14 +798,12 @@ struct kvm_s390_cpu_model {
> >> unsigned short ibc;
> >> };
> >> -struct kvm_s390_module_hook {
> >> - int (*hook)(struct kvm_vcpu *vcpu);
> >> - struct module *owner;
> >> -};
> >> +typedef int (*crypto_hook)(struct kvm_vcpu *vcpu);
> >> struct kvm_s390_crypto {
> >> struct kvm_s390_crypto_cb *crycb;
> >> - struct kvm_s390_module_hook *pqap_hook;
> >> + struct rw_semaphore pqap_hook_rwsem;
> >> + crypto_hook *pqap_hook;
> >> __u32 crycbd;
> >> __u8 aes_kw;
> >> __u8 dea_kw;
> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> >> index b655a7d82bf0..a08f242a9f27 100644
> >> --- a/arch/s390/kvm/kvm-s390.c
> >> +++ b/arch/s390/kvm/kvm-s390.c
> >> @@ -2630,6 +2630,7 @@ static void kvm_s390_crypto_init(struct kvm *kvm)
> >> {
> >> kvm->arch.crypto.crycb = &kvm->arch.sie_page2->crycb;
> >> kvm_s390_set_crycb_format(kvm);
> >> + init_rwsem(&kvm->arch.crypto.pqap_hook_rwsem);
> >> if (!test_kvm_facility(kvm, 76))
> >> return;
> >> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> >> index 9928f785c677..6bed9406c1f3 100644
> >> --- a/arch/s390/kvm/priv.c
> >> +++ b/arch/s390/kvm/priv.c
> >> @@ -610,6 +610,7 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
> >> static int handle_pqap(struct kvm_vcpu *vcpu)
> >> {
> >> struct ap_queue_status status = {};
> >> + crypto_hook pqap_hook;
> >> unsigned long reg0;
> >> int ret;
> >> uint8_t fc;
> >> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
> >> * Verify that the hook callback is registered, lock the owner
> >> * and call the hook.
> >> */
> >> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
> >> if (vcpu->kvm->arch.crypto.pqap_hook) {
> >> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> >> - return -EOPNOTSUPP;
> >> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> >> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> >> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
> >
> > Dont we have to check for NULL here? If not can you add a comment why?
>
> Take a look above the removed lines: if (vcpu->kvm->arch.crypto.pqap_hook)
>
> >
> > Otherwise this looks good.
>
> Also, in the cover letter I said this patch was already queued and was
> included here because it pre-reqs the second patch. Is this patch not
> already in Alex's tree?
Nope. The only requests for merges through my tree that I'm aware of
were [1] and what I understand was the evolution of that here now [2].
Maybe you're thinking of [3], which I do see in mainline where this was
2/2 in that series but afaict only patch 1/2 was committed. I guess
that explains why there was no respin based on comments for this patch.
Thanks,
Alex
[1]https://lore.kernel.org/linux-s390/[email protected]/
[2]https://lore.kernel.org/linux-s390/[email protected]/
[3]https://lore.kernel.org/linux-s390/[email protected]/
On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
> Nope. The only requests for merges through my tree that I'm aware of
> were [1] and what I understand was the evolution of that here now [2].
> Maybe you're thinking of [3], which I do see in mainline where this was
> 2/2 in that series but afaict only patch 1/2 was committed. I guess
> that explains why there was no respin based on comments for this patch.
> Thanks,
Tony,
If you take Alex's tree from here:
https://github.com/awilliam/linux-vfio/commits/next
And rebase + repost exactly the patches you need applied it would be
helpful.
Jason
On Thu, 19 Aug 2021 09:36:34 -0400
Tony Krowiak <[email protected]> wrote:
> >>> static int handle_pqap(struct kvm_vcpu *vcpu)
> >>> {
> >>> struct ap_queue_status status = {};
> >>> + crypto_hook pqap_hook;
> >>> unsigned long reg0;
> >>> int ret;
> >>> uint8_t fc;
> >>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
> >>> * Verify that the hook callback is registered, lock the owner
> >>> * and call the hook.
> >>> */
> >>> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
> >>> if (vcpu->kvm->arch.crypto.pqap_hook) { <--- HERE
> >>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
> >>> - return -EOPNOTSUPP;
> >>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
> >>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
> >>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
> >> Dont we have to check for NULL here? If not can you add a comment why?
> > I believe we did the necessary check on the line I just marked with
> > "<--- HERE".
> >
> > I find that "*" operator confusing in this context as it doesn't do
> > any good for us. I believe this situation is described in 6.5.3.2.4 of
> > the c11 standard. For convenience I will cite from the corresponding
> > draft:
> > "The unary * operator denotes indirection. If the operand points to a
> > function, the result is a function designator; if it points to an
> > object, the result is an lvalue designating the object. If the operand
> > has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
> > invalid value has been assigned to the pointer, the behavior of the
> > unary * operator is undefined."
> >
> > Frankly I also fail to see the benefit of introducing the local variable
> > named "pqap_hook", but back then I decided to not complain about style.
>
> The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function
> pointer. The actual function pointer is stored in matrix_mdev->pqap_hook,
> the reason being that the handle_pqap function in vfio_ap_ops.c
> retrieves the matrix_mdev via a container_of macro. The dereferencing
> of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was
> to get the function pointer. There may have been a more stylish
> way of doing this, but the functionality is there.
You are right, and I was wrong. But then we do have to distinct pointer
deferences, and we check for NULL only once.
I still do believe we do not have a potential null pointer dereference
here, but the reason for that is that vfio-ap (the party that manages
these pointers) guarantees that whenever
vcpu->kvm->arch.crypto.pqap_hook != NULL is true,
*vcpu->kvm->arch.crypto.pqap_hook != NULL is also true (and also that
the function pointer is a valid one). Which is the case, because we
set matrix_mdev->pqap_hook in vfio_ap_mdev_create() and don't touch
it any more.
In my opinion it is worth a comment.
>
> >
> > Regards,
> > Halil
> >
> >>
> >>> + ret = pqap_hook(vcpu);
BTW the second dereference takes place here.
If we wanted, we could make sure we don't dereference a null pointer
here but I think that would be an overkill.
Regards,
Halil
> >> [...]
On 8/19/21 1:58 PM, Jason Gunthorpe wrote:
> On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
>
>> Nope. The only requests for merges through my tree that I'm aware of
>> were [1] and what I understand was the evolution of that here now [2].
>> Maybe you're thinking of [3], which I do see in mainline where this was
>> 2/2 in that series but afaict only patch 1/2 was committed. I guess
>> that explains why there was no respin based on comments for this patch.
>> Thanks,
> Tony,
>
> If you take Alex's tree from here:
>
> https://github.com/awilliam/linux-vfio/commits/next
>
> And rebase + repost exactly the patches you need applied it would be
> helpful.
Will do.
>
> Jason
On 8/19/21 1:58 PM, Jason Gunthorpe wrote:
> On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
>
>> Nope. The only requests for merges through my tree that I'm aware of
>> were [1] and what I understand was the evolution of that here now [2].
>> Maybe you're thinking of [3], which I do see in mainline where this was
>> 2/2 in that series but afaict only patch 1/2 was committed. I guess
>> that explains why there was no respin based on comments for this patch.
>> Thanks,
> Tony,
>
> If you take Alex's tree from here:
>
> https://github.com/awilliam/linux-vfio/commits/next
I navigated to this URL and clicked the green 'Code'
button. I was given the option to download the zip file or
use git to checkout the code at the URL displayed
'https://github.com/awilliam/linux-vfio.git'. I cloned the
repo at that URL and the code was definitely not in any
way similar to my code base. In particular, the
arch/s390/include/asm/kvm_host.h file did not have any
of the crypto structures.
I then downloaded the zip file and expanded it. The code
looked legitimate, but this was not a git repository, so I
had no way to cherry-pick my patches nor format patches
to post to this mailing list.
Next, I tried cloning from
'https://github.com/awilliam/linux-vfio-next.git',
but I was prompted for uid/pw.
So, the question is, how to I get the linux-vfio-next repo upon which I
can rebase my patches? I apologize for my ignorance.
>
> And rebase + repost exactly the patches you need applied it would be
> helpful.
>
> Jason
On Fri, Aug 20, 2021 at 06:05:08PM -0400, Tony Krowiak wrote:
> So, the question is, how to I get the linux-vfio-next repo upon which I
> can rebase my patches? I apologize for my ignorance.
Get yourself a kernel git tree somehow, eg by cloning one you already
have
Then something like
$ git fetch https://github.com/awilliam/linux-vfio.git next
$ git reset --hard FETCH_HEAD
Will sort it out, though there are many other varients such as adding
a remote/etc.
When you cloned it from github git checked out the wrong branch for
you - 'git reset --hard origin/next' would fix it too.
Jason
On Fri, 20 Aug 2021 18:05:08 -0400
Tony Krowiak <[email protected]> wrote:
> On 8/19/21 1:58 PM, Jason Gunthorpe wrote:
> > On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
> >
> >> Nope. The only requests for merges through my tree that I'm aware of
> >> were [1] and what I understand was the evolution of that here now [2].
> >> Maybe you're thinking of [3], which I do see in mainline where this was
> >> 2/2 in that series but afaict only patch 1/2 was committed. I guess
> >> that explains why there was no respin based on comments for this patch.
> >> Thanks,
> > Tony,
> >
> > If you take Alex's tree from here:
> >
> > https://github.com/awilliam/linux-vfio/commits/next
>
> I navigated to this URL and clicked the green 'Code'
> button. I was given the option to download the zip file or
> use git to checkout the code at the URL displayed
> 'https://github.com/awilliam/linux-vfio.git'. I cloned the
> repo at that URL and the code was definitely not in any
> way similar to my code base. In particular, the
> arch/s390/include/asm/kvm_host.h file did not have any
> of the crypto structures.
>
> I then downloaded the zip file and expanded it. The code
> looked legitimate, but this was not a git repository, so I
> had no way to cherry-pick my patches nor format patches
> to post to this mailing list.
>
> Next, I tried cloning from
> 'https://github.com/awilliam/linux-vfio-next.git',
> but I was prompted for uid/pw.
>
> So, the question is, how to I get the linux-vfio-next repo upon which I
> can rebase my patches? I apologize for my ignorance.
You can use git fetch to download the objects, ex:
$ git fetch git://github.com/awilliam/linux-vfio.git next
$ git checkout FETCH_HEAD
Or you could add a remote, ex:
$ git remote add vfio git://github.com/awilliam/linux-vfio.git
$ git remote update vfio
$ git checkout vfio/next
The former might be easier and add a lot less crufty objects to your
local tree if this is a one-off activity. Thanks,
Alex
On 8/19/21 5:42 PM, Halil Pasic wrote:
> On Thu, 19 Aug 2021 09:36:34 -0400
> Tony Krowiak <[email protected]> wrote:
>
>>>>> static int handle_pqap(struct kvm_vcpu *vcpu)
>>>>> {
>>>>> struct ap_queue_status status = {};
>>>>> + crypto_hook pqap_hook;
>>>>> unsigned long reg0;
>>>>> int ret;
>>>>> uint8_t fc;
>>>>> @@ -657,15 +658,16 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>>>>> * Verify that the hook callback is registered, lock the owner
>>>>> * and call the hook.
>>>>> */
>>>>> + down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
>>>>> if (vcpu->kvm->arch.crypto.pqap_hook) { <--- HERE
>>>>> - if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
>>>>> - return -EOPNOTSUPP;
>>>>> - ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
>>>>> - module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
>>>>> + pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook;
>>>> Dont we have to check for NULL here? If not can you add a comment why?
>>> I believe we did the necessary check on the line I just marked with
>>> "<--- HERE".
>>>
>>> I find that "*" operator confusing in this context as it doesn't do
>>> any good for us. I believe this situation is described in 6.5.3.2.4 of
>>> the c11 standard. For convenience I will cite from the corresponding
>>> draft:
>>> "The unary * operator denotes indirection. If the operand points to a
>>> function, the result is a function designator; if it points to an
>>> object, the result is an lvalue designating the object. If the operand
>>> has type ‘‘pointer to type’’, the result has type ‘‘type’’. If an
>>> invalid value has been assigned to the pointer, the behavior of the
>>> unary * operator is undefined."
>>>
>>> Frankly I also fail to see the benefit of introducing the local variable
>>> named "pqap_hook", but back then I decided to not complain about style.
>> The vcpu->kvm->arch.crypto.pqap_hook is a pointer to a function
>> pointer. The actual function pointer is stored in matrix_mdev->pqap_hook,
>> the reason being that the handle_pqap function in vfio_ap_ops.c
>> retrieves the matrix_mdev via a container_of macro. The dereferencing
>> of the vcpu->kvm->arch.crypto.pqap_hook into a local variable was
>> to get the function pointer. There may have been a more stylish
>> way of doing this, but the functionality is there.
> You are right, and I was wrong. But then we do have to distinct pointer
> deferences, and we check for NULL only once.
>
> I still do believe we do not have a potential null pointer dereference
> here, but the reason for that is that vfio-ap (the party that manages
> these pointers) guarantees that whenever
> vcpu->kvm->arch.crypto.pqap_hook != NULL is true,
> *vcpu->kvm->arch.crypto.pqap_hook != NULL is also true (and also that
> the function pointer is a valid one). Which is the case, because we
> set matrix_mdev->pqap_hook in vfio_ap_mdev_create() and don't touch
> it any more.
>
> In my opinion it is worth a comment.
Even I had to look at it again to respond to you, so a comment
is probably called for.
>
>
>>> Regards,
>>> Halil
>>>
>>>>
>>>>> + ret = pqap_hook(vcpu);
> BTW the second dereference takes place here.
>
> If we wanted, we could make sure we don't dereference a null pointer
> here but I think that would be an overkill.
I agree, it is overkill.
>
> Regards,
> Halil
>>>> [...]
Thanks, I think I've got it now.
On 8/20/21 6:30 PM, Jason Gunthorpe wrote:
> git reset --hard origin/next
On 8/20/21 6:41 PM, Alex Williamson wrote:
> On Fri, 20 Aug 2021 18:05:08 -0400
> Tony Krowiak <[email protected]> wrote:
>
>> On 8/19/21 1:58 PM, Jason Gunthorpe wrote:
>>> On Thu, Aug 19, 2021 at 11:54:33AM -0600, Alex Williamson wrote:
>>>
>>>> Nope. The only requests for merges through my tree that I'm aware of
>>>> were [1] and what I understand was the evolution of that here now [2].
>>>> Maybe you're thinking of [3], which I do see in mainline where this was
>>>> 2/2 in that series but afaict only patch 1/2 was committed. I guess
>>>> that explains why there was no respin based on comments for this patch.
>>>> Thanks,
>>> Tony,
>>>
>>> If you take Alex's tree from here:
>>>
>>> https://github.com/awilliam/linux-vfio/commits/next
>> I navigated to this URL and clicked the green 'Code'
>> button. I was given the option to download the zip file or
>> use git to checkout the code at the URL displayed
>> 'https://github.com/awilliam/linux-vfio.git'. I cloned the
>> repo at that URL and the code was definitely not in any
>> way similar to my code base. In particular, the
>> arch/s390/include/asm/kvm_host.h file did not have any
>> of the crypto structures.
>>
>> I then downloaded the zip file and expanded it. The code
>> looked legitimate, but this was not a git repository, so I
>> had no way to cherry-pick my patches nor format patches
>> to post to this mailing list.
>>
>> Next, I tried cloning from
>> 'https://github.com/awilliam/linux-vfio-next.git',
>> but I was prompted for uid/pw.
>>
>> So, the question is, how to I get the linux-vfio-next repo upon which I
>> can rebase my patches? I apologize for my ignorance.
> You can use git fetch to download the objects, ex:
>
> $ git fetch git://github.com/awilliam/linux-vfio.git next
> $ git checkout FETCH_HEAD
>
> Or you could add a remote, ex:
>
> $ git remote add vfio git://github.com/awilliam/linux-vfio.git
> $ git remote update vfio
> $ git checkout vfio/next
>
> The former might be easier and add a lot less crufty objects to your
> local tree if this is a one-off activity. Thanks,
>
> Alex
Thanks Alex, I was able to get the repo, cherry-pick my patches and
build and test the kernel. Barring any anomalies, I will be posting the
patches today.
>