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

Alexey Kardashevskiy
2024-09-02T01:24:40Z

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