On 12/06/18 14:41, Will Deacon wrote:
>> +#define dev_err(vdev, fmt, ...) \
>> + pr_err("%s: " fmt, (vdev)->params->name, ##__VA_ARGS__)
>> +#define dev_warn(vdev, fmt, ...) \
>> + pr_warning("%s: " fmt, (vdev)->params->name, ##__VA_ARGS__)
>> +#define dev_info(vdev, fmt, ...) \
>> + pr_info("%s: " fmt, (vdev)->params->name, ##__VA_ARGS__)
>> +#define dev_dbg(vdev, fmt, ...) \
>> + pr_debug("%s: " fmt, (vdev)->params->name, ##__VA_ARGS__)
>> +#define dev_die(vdev, fmt, ...) \
>> + die("%s: " fmt, (vdev)->params->name, ##__VA_ARGS__)
>
> We probably want a vfio_ prefix on these macros, since they sound like
> they also apply to stuff like virtio_device.
Sure
>> diff --git a/vfio/core.c b/vfio/core.c
>> new file mode 100644
>> index 000000000000..25fae8aa783b
>> --- /dev/null
>> +++ b/vfio/core.c
>> @@ -0,0 +1,492 @@
>> +#include "kvm/kvm.h"
>> +#include "kvm/vfio.h"
>> +
>> +#include <linux/list.h>
>> +
>> +#define VFIO_DEV_DIR "/dev/vfio"
>> +#define VFIO_DEV_NODE VFIO_DEV_DIR "/vfio"
>> +#define IOMMU_GROUP_DIR "/sys/kernel/iommu_groups"
>> +
>> +static int vfio_container;
>> +static LIST_HEAD(vfio_groups);
>> +static struct vfio_device *vfio_devices;
>> +
>> +static int vfio_device_pci_parser(const struct option *opt, char *arg,
>> + struct vfio_device_params *dev)
>> +{
>> + unsigned int domain, bus, devnr, fn;
>> +
>> + int nr = sscanf(arg, "%4x:%2x:%2x.%1x", &domain, &bus, &devnr, &fn);
>> + if (nr < 4) {
>> + domain = 0;
>> + nr = sscanf(arg, "%2x:%2x.%1x", &bus, &devnr, &fn);
>> + if (nr < 3) {
>> + pr_err("Invalid device identifier %s", arg);
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + dev->type = VFIO_DEVICE_PCI;
>> + dev->bus = "pci";
>> + dev->name = malloc(13);
>
> /* Unlucky for us */
>
> Urgh. I hate string processing in C.
Me too, I usually draw the line at atoi. We could just ask users to pass
0000:B:D.F to remove some ugliness here, but I like using the short notation
>
>> + if (!dev->name)
>> + return -ENOMEM;
>> +
>> + snprintf(dev->name, 13, "%04x:%02x:%02x.%x", domain, bus, devnr, fn);
>> +
>> + return 0;
>> +}
>> +
>> +int vfio_device_parser(const struct option *opt, const char *arg, int unset)
>> +{
>> + int ret = -EINVAL;
>> + static int idx = 0;
>> + struct kvm *kvm = opt->ptr;
>> + struct vfio_device_params *dev, *devs;
>> + char *cur, *buf = strdup(arg);
>> +
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + if (idx >= MAX_VFIO_DEVICES) {
>> + pr_warning("Too many VFIO devices");
>> + goto out_free_buf;
>> + }
>> +
>> + devs = realloc(kvm->cfg.vfio_devices, sizeof(*dev) * (idx + 1));
>> + if (!devs) {
>> + ret = -ENOMEM;
>> + goto out_free_buf;
>> + }
>> +
>> + kvm->cfg.vfio_devices = devs;
>> + dev = &devs[idx];
>> +
>> + cur = strtok(buf, ",");
>> +
>> + if (!strcmp(opt->long_name, "vfio-pci"))
>> + ret = vfio_device_pci_parser(opt, cur, dev);
>
> Do you need to avoid passing cur == NULL to sscanf here?
Right, I should check this
>> +static int vfio_device_init(struct kvm *kvm, struct vfio_device *vdev)
>> +{
>> + int ret;
>> + char dev_path[PATH_MAX];
>> + struct vfio_group *group;
>> +
>> + ret = snprintf(dev_path, PATH_MAX, "/sys/bus/%s/devices/%s",
>> + vdev->params->bus, vdev->params->name);
>> + if (ret < 0 || ret == PATH_MAX)
>> + return -EINVAL;
>> +
>> + vdev->sysfs_path = strndup(dev_path, PATH_MAX);
>> + if (!vdev->sysfs_path)
>> + return -errno;
>> +
>> + group = vfio_group_get_for_dev(kvm, vdev);
>
> Hmm, so if I have a number of devices that share a group and I specify the
> BDF for one of those devices to lkvm, then will it pass through the entire
> group without warning? Is that desired behaviour?
No, devices that weren't explicitly specified on the command line are
not passed to the guest, all this does is obtain the device's group to
setup the IOMMU container (the SET_CONTAINER ioctl is on the group fd).
Doing operations on the group is required by VFIO for good reasons: if
any device in the group is bound to another driver, passing this device
to a guest isn't safe. So the user is expected to know a little about
IOMMU groups when preparing the device, at least that it should rebind
all devices in a group. Otherwise kvmtool exits with the "group N isn't
viable" error. In that situation QEMU adds a hint ("Please ensure all
devices within the iommu_group are bound to their vfio bus driver"),
perhaps adding something similar in kvmtool would be helpful.
Thanks,
Jean
