Re: [RFC PATCH 11/21] KVM: SEV: Add TIO VMGEXIT and bind TDI

[Alexey Kardashevskiy <aik@amd.com>]

On 30/8/24 17:02, Xu Yilun wrote:
> On Fri, Aug 30, 2024 at 02:00:30PM +1000, Alexey Kardashevskiy wrote:
>>
>>
>> On 29/8/24 20:08, Xu Yilun wrote:
>>>> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
>>>> index 76b7f6085dcd..a4e9db212adc 100644
>>>> --- a/virt/kvm/vfio.c
>>>> +++ b/virt/kvm/vfio.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <linux/slab.h>
>>>>    #include <linux/uaccess.h>
>>>>    #include <linux/vfio.h>
>>>> +#include <linux/tsm.h>
>>>>    #include "vfio.h"
>>>>    #ifdef CONFIG_SPAPR_TCE_IOMMU
>>>> @@ -29,8 +30,14 @@ struct kvm_vfio_file {
>>>>    #endif
>>>>    };
>>>> +struct kvm_vfio_tdi {
>>>> +	struct list_head node;
>>>> +	struct vfio_device *vdev;
>>>> +};
>>>> +
>>>>    struct kvm_vfio {
>>>>    	struct list_head file_list;
>>>> +	struct list_head tdi_list;
>>>>    	struct mutex lock;
>>>>    	bool noncoherent;
>>>>    };
>>>> @@ -80,6 +87,22 @@ static bool kvm_vfio_file_is_valid(struct file *file)
>>>>    	return ret;
>>>>    }
>>>> +static struct vfio_device *kvm_vfio_file_device(struct file *file)
>>>> +{
>>>> +	struct vfio_device *(*fn)(struct file *file);
>>>> +	struct vfio_device *ret;
>>>> +
>>>> +	fn = symbol_get(vfio_file_device);
>>>> +	if (!fn)
>>>> +		return NULL;
>>>> +
>>>> +	ret = fn(file);
>>>> +
>>>> +	symbol_put(vfio_file_device);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>>    #ifdef CONFIG_SPAPR_TCE_IOMMU
>>>>    static struct iommu_group *kvm_vfio_file_iommu_group(struct file *file)
>>>>    {
>>>> @@ -297,6 +320,103 @@ static int kvm_vfio_set_file(struct kvm_device *dev, long attr,
>>>>    	return -ENXIO;
>>>>    }
>>>> +static int kvm_dev_tsm_bind(struct kvm_device *dev, void __user *arg)
>>>> +{
>>>> +	struct kvm_vfio *kv = dev->private;
>>>> +	struct kvm_vfio_tsm_bind tb;
>>>> +	struct kvm_vfio_tdi *ktdi;
>>>> +	struct vfio_device *vdev;
>>>> +	struct fd fdev;
>>>> +	int ret;
>>>> +
>>>> +	if (copy_from_user(&tb, arg, sizeof(tb)))
>>>> +		return -EFAULT;
>>>> +
>>>> +	ktdi = kzalloc(sizeof(*ktdi), GFP_KERNEL_ACCOUNT);
>>>> +	if (!ktdi)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	fdev = fdget(tb.devfd);
>>>> +	if (!fdev.file)
>>>> +		return -EBADF;
>>>> +
>>>> +	ret = -ENOENT;
>>>> +
>>>> +	mutex_lock(&kv->lock);
>>>> +
>>>> +	vdev = kvm_vfio_file_device(fdev.file);
>>>> +	if (vdev) {
>>>> +		ret = kvm_arch_tsm_bind(dev->kvm, vdev->dev, tb.guest_rid);
>>>> +		if (!ret) {
>>>> +			ktdi->vdev = vdev;
>>>> +			list_add_tail(&ktdi->node, &kv->tdi_list);
>>>> +		} else {
>>>> +			vfio_put_device(vdev);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	fdput(fdev);
>>>> +	mutex_unlock(&kv->lock);
>>>> +	if (ret)
>>>> +		kfree(ktdi);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int kvm_dev_tsm_unbind(struct kvm_device *dev, void __user *arg)
>>>> +{
>>>> +	struct kvm_vfio *kv = dev->private;
>>>> +	struct kvm_vfio_tsm_bind tb;
>>>> +	struct kvm_vfio_tdi *ktdi;
>>>> +	struct vfio_device *vdev;
>>>> +	struct fd fdev;
>>>> +	int ret;
>>>> +
>>>> +	if (copy_from_user(&tb, arg, sizeof(tb)))
>>>> +		return -EFAULT;
>>>> +
>>>> +	fdev = fdget(tb.devfd);
>>>> +	if (!fdev.file)
>>>> +		return -EBADF;
>>>> +
>>>> +	ret = -ENOENT;
>>>> +
>>>> +	mutex_lock(&kv->lock);
>>>> +
>>>> +	vdev = kvm_vfio_file_device(fdev.file);
>>>> +	if (vdev) {
>>>> +		list_for_each_entry(ktdi, &kv->tdi_list, node) {
>>>> +			if (ktdi->vdev != vdev)
>>>> +				continue;
>>>> +
>>>> +			kvm_arch_tsm_unbind(dev->kvm, vdev->dev);
>>>> +			list_del(&ktdi->node);
>>>> +			kfree(ktdi);
>>>> +			vfio_put_device(vdev);
>>>> +			ret = 0;
>>>> +			break;
>>>> +		}
>>>> +		vfio_put_device(vdev);
>>>> +	}
>>>> +
>>>> +	fdput(fdev);
>>>> +	mutex_unlock(&kv->lock);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int kvm_vfio_set_device(struct kvm_device *dev, long attr,
>>>> +			       void __user *arg)
>>>> +{
>>>> +	switch (attr) {
>>>> +	case KVM_DEV_VFIO_DEVICE_TDI_BIND:
>>>> +		return kvm_dev_tsm_bind(dev, arg);
>>>
>>> I think the TDI bind operation should be under the control of the device
>>> owner (i.e. VFIO driver), rather than in this bridge driver.
>>
>> This is a valid point, although this means teaching VFIO about the KVM
>> lifetime (and KVM already holds references to VFIO groups) and
> 
> Not sure if I understand, VFIO already knows KVM lifetime via
> vfio_device_get_kvm_safe(), is it?

Yeah you're right.

>> guest BDFns (which have no meaning for VFIO in the host kernel).
> 
> KVM is not aware of the guest BDF today.
> 
> I think we need to pass a firmware recognizable TDI identifier, which
> is actually a magic number and specific to vendors. For TDX, it is the
> FUNCTION_ID. So I didn't think too much to whom the identifier is
> meaningful.

It needs to be the same id for "bind" operation (bind a TDI to a VM, the 
op performed by QEMU) and GUEST_REQUEST (VMGEXIT from the VM so the id 
comes from the guest). The host kernel is not going to parse it but just 
pass to the firmware so I guess it can be just an u32.


>>> The TDI bind
>>> means TDI would be transitioned to CONFIG_LOCKED state, and a bunch of
>>> device configurations breaks the state (TDISP spec 11.4.5/8/9). So the
>>> VFIO driver should be fully aware of the TDI bind and manage unwanted
>>> breakage.
>>
>> VFIO has no control over TDI any way, cannot even know what state it is in
>> without talking to the firmware.
> 
> I think VFIO could talk to the firmware, that's part of the reason we are
> working on the TSM module independent to KVM.
> 
>> When TDI goes into ERROR, this needs to be
>> propagated to the VM. At the moment (afaik) it does not tell the
> 
> I assume when TDISP ERROR happens, an interrupt (e.g. AER) would be sent
> to OS and VFIO driver is the one who handles it in the first place. So
> maybe there has to be some TDI stuff in VFIO?

Sounds reasonable, my test device just does not do this so I have not 
poked at the error handling much :) Thanks,


> Thanks,
> Yilun
> 
>> userspace/guest about IOMMU errors and it probably should but the existing
>> mechanism should be able to do so. Thanks,
>>
>>
>>>
>>> Thanks,
>>> Yilun
>>
>> -- 
>> Alexey
>>

-- 
Alexey