LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Li Fei1 <fei1.li@intel.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org, yu1.wang@intel.com,
	shuox.liu@gmail.com, fei1.li@intel.com
Subject: Re: [PATCH v2 2/3] virt: acrn: Introduce interfaces for virtual device creating/destroying
Date: Tue, 31 Aug 2021 14:32:15 +0800	[thread overview]
Message-ID: <20210831063215.GB8717@louislifei-OptiPlex-7050> (raw)
In-Reply-To: <YSimrfWz+p7Wf/nC@kroah.com>

On Fri, Aug 27, 2021 at 10:47:41AM +0200, Greg KH wrote:
> On Wed, Aug 25, 2021 at 05:01:41PM +0800, Fei Li wrote:
> > From: Shuo Liu <shuo.a.liu@intel.com>
> > 
> > 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@intel.com>
> > Signed-off-by: Fei Li <fei1.li@intel.com>
> > ---
> >  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));
Hi Greg

> 
> No validation of the structure fields?

Service VM knows nothing about how to create these vdevs, it only needs to
pass through this structure to ACRN Hypervisor to let ACRN Hypervisor check it.
> 
> 
> > +		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?

Service VM knows nothing about how to destroy these vdevs, it only needs to
pass through this structure to ACRN Hypervisor to let ACRN Hypervisor check it.

> 
> > +		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?

little-endian or big-endian ?


> 
> > +			__u32 legacy_id;
> 
> What is "legacy"?  What types of devices?

A PCI device which under a PCI bridge.
> 
> 
> > +		} 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?
For different kinds of vdevs, it represents differently.
For current usages, it may be:
a) a vdev's name of a virtual PCI device which used to communicate between VMs
b) an index of virtual Uart
c) a structure to represent a virtual Root Port.


> 
> > +};
> > +
> >  /**
> >   * 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?
We need the id field + other fields (optional, the slot or the args) to identify a vdev.

thanks.
> 
> thanks,
> 
> greg k-h

  reply	other threads:[~2021-08-31  6:31 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-25  9:01 [PATCH v2 0/3] Introduce some interfaces for ACRN hypervisor Fei Li
2021-08-25  9:01 ` [PATCH v2 1/3] virt: acrn: Introduce interfaces for MMIO device passthrough Fei Li
2021-08-25 20:43   ` Greg KH
2021-08-26  1:38     ` Li Fei1
2021-08-27  8:45       ` Greg KH
2021-08-31  6:10         ` Li Fei1
2021-08-25  9:01 ` [PATCH v2 2/3] virt: acrn: Introduce interfaces for virtual device creating/destroying Fei Li
2021-08-27  8:47   ` Greg KH
2021-08-31  6:32     ` Li Fei1 [this message]
2021-08-25  9:01 ` [PATCH v2 3/3] virt: acrn: Introduce interface to fetch platform info from the hypervisor Fei Li
2021-08-27  8:49   ` Greg KH
2021-08-31  6:43     ` Li Fei1

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210831063215.GB8717@louislifei-OptiPlex-7050 \
    --to=fei1.li@intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=shuox.liu@gmail.com \
    --cc=yu1.wang@intel.com \
    --subject='Re: [PATCH v2 2/3] virt: acrn: Introduce interfaces for virtual device creating/destroying' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).