Date:   Tue, 19 Mar 2019 15:47:05 +0100
From:   Pierre Morel <pmorel@...ux.ibm.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     borntraeger@...ibm.com, alex.williamson@...hat.com,
        cohuck@...hat.com, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, kvm@...r.kernel.org,
        frankja@...ux.ibm.com, akrowiak@...ux.ibm.com, david@...hat.com,
        schwidefsky@...ibm.com, heiko.carstens@...ibm.com,
        freude@...ux.ibm.com, mimu@...ux.ibm.com
Subject: Re: [PATCH v5 4/7] s390: ap: setup relation betwen KVM and mediated
 device

On 19/03/2019 15:23, Pierre Morel wrote:
> On 19/03/2019 12:54, Halil Pasic wrote:
>> On Tue, 19 Mar 2019 10:38:42 +0100
>> Pierre Morel <pmorel@...ux.ibm.com> wrote:
>>
>>> On 15/03/2019 19:15, Halil Pasic wrote:
>>>> On Wed, 13 Mar 2019 17:05:01 +0100
>>>> Pierre Morel <pmorel@...ux.ibm.com> wrote:
>>>>
>>>>> When the mediated device is open we setup the relation with KVM 
>>>>> unset it
>>>>> when the mediated device is released.
>>>>>
>>>>> We ensure KVM is present on opening of the mediated device.
>>>>>
>>>>> We ensure that KVM survives the mediated device, and establish a 
>>>>> direct
>>>>
>>>> survives?
>>>
>>> what alternative do you prefer?
>>>
>>
>> Increase kvm's refcount to ensure the guest is alive when the
>> ap_matrix_mdev is active. An ap mp_matrix becomes active with
>> a successful open() and ceases to be active with a release().
> 
> Right, it is mdev usage not mdev.
> 
>>
>> Your sentence was materially wrong as the mdev is allowed to outlive
>> the KVM. BTW survive tends to have an 'in spite of' note to it, which
>> outlive does not. vfio-ap is, I hope, not a calamity that threatens
>> the life of KVM ;). https://en.oxforddictionaries.com/definition/survive
> 
> Thanks, your description is much better.
> 
>>
>>>>
>>>>> link from KVM to the mediated device to simplify the relationship.
>>>>>
>>>>> Signed-off-by: Pierre Morel <pmorel@...ux.ibm.com>
>>>>> ---
>>>
>>> ...snip...
>>>
>>>>>    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_set_kvm(matrix_mdev, data);
>>>>> -    if (ret)
>>>>> -        return NOTIFY_DONE;
>>>>> -
>>>>> -    /* If there is no CRYCB pointer, then we can't copy the masks */
>>>>> -    if (!matrix_mdev->kvm->arch.crypto.crycbd)
>>>>> -        return NOTIFY_DONE;
>>>>> -
>>>>> -    kvm_arch_crypto_set_masks(matrix_mdev->kvm, 
>>>>> matrix_mdev->matrix.apm,
>>>>> -                  matrix_mdev->matrix.aqm,
>>>>> -                  matrix_mdev->matrix.adm);
>>>>> +    matrix_mdev->kvm = data;
>>>>>        return NOTIFY_OK;
>>>>>    }
>>>>> @@ -888,6 +873,12 @@ static int vfio_ap_mdev_open(struct 
>>>>> mdev_device *mdev)
>>>>>        if (ret)
>>>>>            goto err_group;
>>>>> +    /* We do not support opening the mediated device without KVM */
>>>>> +    if (!matrix_mdev->kvm) {
>>>>> +        ret = -ENODEV;
>>>>> +        goto err_group;
>>>>> +    }
>>>>> +
>>>>>        matrix_mdev->iommu_notifier.notifier_call = 
>>>>> vfio_ap_mdev_iommu_notifier;
>>>>>        events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>>>>> @@ -896,8 +887,15 @@ static int vfio_ap_mdev_open(struct 
>>>>> mdev_device *mdev)
>>>>>        if (ret)
>>>>>            goto err_iommu;
>>>>> +    ret = vfio_ap_mdev_set_kvm(matrix_mdev);
>>>>
>>>> At this point the matrix_mdev->kvm ain't guaranteed to be valid 
>>>> IMHO. Or
>>>> am I wrong? If I'm right kvm_get_kvm(matrix_mdev->kvm) could be too 
>>>> late.
>>>
>>> What about the if (!matrix_mdev->kvm) 10 lines above ?
>>>
>>
>> That check is not sufficient.
>>
>> You should do the kvm_get_kvm() in vfio_ap_mdev_group_notifier(). VFIO
>> must ensure that the kvm pointer you get is valid, in a sense that it
>> points to a valid struct kvm and the kvm object is alive, while you are
>> in the callback. But not beyond.
>>
>> If another thread were to decrement the refcount of the kvm object you
>> would end up with matrix_mdev->kvm pointing to an object that has already
>> died.
>>
>> Does my analysis make sense to you?
> 
> Yes thanks the explication is good, it would have been worth to get it 
> the first time.
> 
>>
>>>>
>>>>> +    if (ret)
>>>>> +        goto err_kvm;
>>>>> +
>>>>>        return 0;
>>>>> +err_kvm:
>>>>> +    vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>>>>> +                 &matrix_mdev->iommu_notifier);
>>>>>    err_iommu:
>>>>>        vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>>>>                     &matrix_mdev->group_notifier);
>>>>> @@ -906,19 +904,33 @@ static int vfio_ap_mdev_open(struct 
>>>>> mdev_device *mdev)
>>>>>        return ret;
>>>>>    }
>>>>> -static void vfio_ap_mdev_release(struct mdev_device *mdev)
>>>>> +static int vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
>>>>>    {
>>>>> -    struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>>>> +    struct kvm *kvm = matrix_mdev->kvm;
>>>>>        if (matrix_mdev->kvm)
>>>>>            kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>>>>
>>>> This still conditional?
>>>
>>> Yes, nothing to clear if there is no KVM.
>>>
>>
>> Since we have ensured the open only works if there is a KVM at that
>> point in time, and we have taken a reference to KVM, I would expect
>> KVM can not go away before we give up our reference.
> 
> Right.

Right but based on the assumption we do a kvm_get_kvm() during open.

But now we will do it inside the notifier, so the logic is to do a 
kvm_put_kvm in the notifier too.
This is important because userland will ask us to release the KVM/VFIO 
link through this notifier.
So I will have to rework this part where KVM==NULL in the notifier too.

Regards,
Pierre


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

Powered by blists - more mailing lists