Re: [PATCH v2 2/3] virt: acrn: Introduce interfaces for virtual device creating/destroying
From: Greg KH
Date: Fri Aug 27 2021 - 04:47:52 EST
On Wed, Aug 25, 2021 at 05:01:41PM +0800, Fei Li wrote:
> From: Shuo Liu <shuo.a.liu@xxxxxxxxx>
>
> The ACRN hypervisor can emulate a virtual device within hypervisor for a
> Guest VM. The emulated virtual device can work without the ACRN
> userspace after creation. The hypervisor do the emulation of that device.
>
> To support the virtual device creating/destroying, HSM provides the
> following ioctls:
> - ACRN_IOCTL_CREATE_VDEV
> Pass data struct acrn_vdev from userspace to the hypervisor, and inform
> the hypervisor to create a virtual device for a User VM.
> - ACRN_IOCTL_DESTROY_VDEV
> Pass data struct acrn_vdev from userspace to the hypervisor, and inform
> the hypervisor to destroy a virtual device of a User VM.
>
> Signed-off-by: Shuo Liu <shuo.a.liu@xxxxxxxxx>
> Signed-off-by: Fei Li <fei1.li@xxxxxxxxx>
> ---
> drivers/virt/acrn/hsm.c | 24 ++++++++++++++++++++
> drivers/virt/acrn/hypercall.h | 26 ++++++++++++++++++++++
> include/uapi/linux/acrn.h | 42 +++++++++++++++++++++++++++++++++++
> 3 files changed, 92 insertions(+)
>
> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
> index f567ca59d7c2..5419794fccf1 100644
> --- a/drivers/virt/acrn/hsm.c
> +++ b/drivers/virt/acrn/hsm.c
> @@ -118,6 +118,7 @@ static long acrn_dev_ioctl(struct file *filp, unsigned int cmd,
> struct acrn_msi_entry *msi;
> struct acrn_pcidev *pcidev;
> struct acrn_irqfd irqfd;
> + struct acrn_vdev *vdev;
> struct page *page;
> u64 cstate_cmd;
> int i, ret = 0;
> @@ -266,6 +267,29 @@ static long acrn_dev_ioctl(struct file *filp, unsigned int cmd,
> "Failed to deassign pci device!\n");
> kfree(pcidev);
> break;
> + case ACRN_IOCTL_CREATE_VDEV:
> + vdev = memdup_user((void __user *)ioctl_param,
> + sizeof(struct acrn_vdev));
> + if (IS_ERR(vdev))
> + return PTR_ERR(vdev);
> +
> + ret = hcall_create_vdev(vm->vmid, virt_to_phys(vdev));
No validation of the structure fields?
> + if (ret < 0)
> + dev_dbg(acrn_dev.this_device,
> + "Failed to create virtual device!\n");
> + kfree(vdev);
> + break;
> + case ACRN_IOCTL_DESTROY_VDEV:
> + vdev = memdup_user((void __user *)ioctl_param,
> + sizeof(struct acrn_vdev));
> + if (IS_ERR(vdev))
> + return PTR_ERR(vdev);
> + ret = hcall_destroy_vdev(vm->vmid, virt_to_phys(vdev));
Again, no validation?
> + if (ret < 0)
> + dev_dbg(acrn_dev.this_device,
> + "Failed to destroy virtual device!\n");
> + kfree(vdev);
> + break;
> case ACRN_IOCTL_SET_PTDEV_INTR:
> irq_info = memdup_user((void __user *)ioctl_param,
> sizeof(struct acrn_ptdev_irq));
> diff --git a/drivers/virt/acrn/hypercall.h b/drivers/virt/acrn/hypercall.h
> index f0c78e52cebb..71d300821a18 100644
> --- a/drivers/virt/acrn/hypercall.h
> +++ b/drivers/virt/acrn/hypercall.h
> @@ -43,6 +43,8 @@
> #define HC_DEASSIGN_PCIDEV _HC_ID(HC_ID, HC_ID_PCI_BASE + 0x06)
> #define HC_ASSIGN_MMIODEV _HC_ID(HC_ID, HC_ID_PCI_BASE + 0x07)
> #define HC_DEASSIGN_MMIODEV _HC_ID(HC_ID, HC_ID_PCI_BASE + 0x08)
> +#define HC_CREATE_VDEV _HC_ID(HC_ID, HC_ID_PCI_BASE + 0x09)
> +#define HC_DESTROY_VDEV _HC_ID(HC_ID, HC_ID_PCI_BASE + 0x0A)
>
> #define HC_ID_PM_BASE 0x80UL
> #define HC_PM_GET_CPU_STATE _HC_ID(HC_ID, HC_ID_PM_BASE + 0x00)
> @@ -196,6 +198,30 @@ static inline long hcall_set_memory_regions(u64 regions_pa)
> return acrn_hypercall1(HC_VM_SET_MEMORY_REGIONS, regions_pa);
> }
>
> +/**
> + * hcall_create_vdev() - Create a virtual device for a User VM
> + * @vmid: User VM ID
> + * @addr: Service VM GPA of the &struct acrn_vdev
> + *
> + * Return: 0 on success, <0 on failure
> + */
> +static inline long hcall_create_vdev(u64 vmid, u64 addr)
> +{
> + return acrn_hypercall2(HC_CREATE_VDEV, vmid, addr);
> +}
> +
> +/**
> + * hcall_destroy_vdev() - Destroy a virtual device of a User VM
> + * @vmid: User VM ID
> + * @addr: Service VM GPA of the &struct acrn_vdev
> + *
> + * Return: 0 on success, <0 on failure
> + */
> +static inline long hcall_destroy_vdev(u64 vmid, u64 addr)
> +{
> + return acrn_hypercall2(HC_DESTROY_VDEV, vmid, addr);
> +}
> +
> /**
> * hcall_assign_mmiodev() - Assign a MMIO device to a User VM
> * @vmid: User VM ID
> diff --git a/include/uapi/linux/acrn.h b/include/uapi/linux/acrn.h
> index 470036d6b1ac..1408d1063339 100644
> --- a/include/uapi/linux/acrn.h
> +++ b/include/uapi/linux/acrn.h
> @@ -441,6 +441,44 @@ struct acrn_mmiodev {
> } res[ACRN_MMIODEV_RES_NUM];
> };
>
> +/**
> + * struct acrn_vdev - Info for creating or destroying a virtual device
> + * @id: Union of identifier of the virtual device
> + * @id.value: Raw data of the identifier
> + * @id.fields.vendor: Vendor id of the virtual PCI device
> + * @id.fields.device: Device id of the virtual PCI device
> + * @id.fields.legacy_id: ID of the virtual device if not a PCI device
> + * @slot: Virtual Bus/Device/Function of the virtual
> + * device
> + * @io_base: IO resource base address of the virtual device
> + * @io_size: IO resource size of the virtual device
> + * @args: Arguments for the virtual device creation
> + *
> + * The created virtual device can be a PCI device or a legacy device (e.g.
> + * a virtual UART controller) and it is emulated by the hypervisor. This
> + * structure will be passed to hypervisor directly.
> + */
> +struct acrn_vdev {
> + /*
> + * the identifier of the device, the low 32 bits represent the vendor
> + * id and device id of PCI device and the high 32 bits represent the
> + * device number of the legacy device
> + */
> + union {
> + __u64 value;
> + struct {
> + __u16 vendor;
> + __u16 device;
Endian of these values?
> + __u32 legacy_id;
What is "legacy"? What types of devices?
> + } fields;
> + } id;
> +
> + __u64 slot;
> + __u32 io_addr[ACRN_PCI_NUM_BARS];
> + __u32 io_size[ACRN_PCI_NUM_BARS];
> + __u8 args[128];
What are these args for exactly?
> +};
> +
> /**
> * struct acrn_msi_entry - Info for injecting a MSI interrupt to a VM
> * @msi_addr: MSI addr[19:12] with dest vCPU ID
> @@ -596,6 +634,10 @@ struct acrn_irqfd {
> _IOW(ACRN_IOCTL_TYPE, 0x57, struct acrn_mmiodev)
> #define ACRN_IOCTL_DEASSIGN_MMIODEV \
> _IOW(ACRN_IOCTL_TYPE, 0x58, struct acrn_mmiodev)
> +#define ACRN_IOCTL_CREATE_VDEV \
> + _IOW(ACRN_IOCTL_TYPE, 0x59, struct acrn_vdev)
> +#define ACRN_IOCTL_DESTROY_VDEV \
> + _IOW(ACRN_IOCTL_TYPE, 0x5A, struct acrn_vdev)
Why do you need the full structure to destroy the device?
thanks,
greg k-h