[RFC PATCH v2 0/2] kvm-vfio: implement the vfio skeleton for VT-d Posted-Interrupts

[RFC PATCH v2 0/2] kvm-vfio: implement the vfio skeleton for VT-d Posted-Interrupts

[Feng Wu]

VT-d Posted-Interrupts is an enhancement to CPU side Posted-Interrupt.
With VT-d Posted-Interrupts enabled, external interrupts from
direct-assigned devices can be delivered to guests without VMM
intervention when guest is running in non-root mode.

You can find the VT-d Posted-Interrtups Spec. in the following URL:
http://www.intel.com/content/www/us/en/intelligent-systems/intel-technology/vt-directed-io-spec.html

This patchset adds the kvm-vfio interface for VT-d Posted-Interrrupts.

In the second patch of this patchset, I leave function
kvm_update_pi_irte() empty, since the purpose of this patch set is
to implement the VFIO related stuff for VT-d PI. kvm_update_pi_irte()
will do the real things, such as, updating IRTE. In fact, I think this
function will be implemented in another file instead of vfio.c. At the
current stage I just list it here to make the build successful. After
some other dependencies (such as, irq core changes in Linux kernel) is
resolved, I will send out the rest part of the VT-d PI patchset.

This patchset is based on the following Eric's VFIO patchset:
[PATCH v3 0/8] KVM-VFIO IRQ forward control

v1->v2
- Re-use KVM_DEV_VFIO_DEVICE group for VT-d PI.
- Define a new attribute in KVM_DEV_VFIO_DEVICE group.
- Teach KVM about sturct pci_dev, and get host irq from it. 

Feng Wu (2):
  KVM: kvm-vfio: User API for VT-d Posted-Interrupts
  KVM: kvm-vfio: implement the VFIO skeleton for VT-d Posted-Interrupts

 Documentation/virtual/kvm/devices/vfio.txt |    9 ++
 include/uapi/linux/kvm.h                   |   10 +++
 virt/kvm/vfio.c                            |  115 ++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+), 0 deletions(-)

[RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Feng Wu]

This patch adds and documents a new attribute
KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE group.
This new attribute is used for VT-d Posted-Interrupts.

When guest OS changes the interrupt configuration for an
assigned device, such as, MSI/MSIx data/address fields,
QEMU will use this IRQ attribute to tell KVM to update the
related IRTE according the VT-d Posted-Interrrupts Specification,
such as, the guest vector should be updated in the related IRTE.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
 include/uapi/linux/kvm.h                   |   10 ++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/Documentation/virtual/kvm/devices/vfio.txt b/Documentation/virtual/kvm/devices/vfio.txt
index f7aff29..39dee86 100644
--- a/Documentation/virtual/kvm/devices/vfio.txt
+++ b/Documentation/virtual/kvm/devices/vfio.txt
@@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been called to trigger the IRQ
 or associate an eventfd to it. Unforwarding can only be called while the
 signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this condition is
 not satisfied, the command returns an -EBUSY.
+
+  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups mechanism to post
+                                   the IRQ to guests.
+For this attribute, kvm_device_attr.addr points to a kvm_posted_intr struct.
+
+When guest OS changes the interrupt configuration for an assigned device,
+such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
+to tell KVM to update the related IRTE according the VT-d Posted-Interrrupts
+Specification, such as, the guest vector should be updated in the related IRTE.
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index a269a42..e5f86ad 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -949,6 +949,7 @@ struct kvm_device_attr {
 #define  KVM_DEV_VFIO_DEVICE			2
 #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
 #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
+#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
 
 enum kvm_device_type {
 	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
@@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
 	__u32 gsi; /* gsi, ie. virtual IRQ number */
 };
 
+struct kvm_posted_intr {
+	__u32	argsz;
+	__u32	fd;		/* file descriptor of the VFIO device */
+	__u32	index;		/* VFIO device IRQ index */
+	__u32	start;
+	__u32	count;
+	int	virq[0];	/* gsi, ie. virtual IRQ number */
+};
+
 /*
  * ioctls for VM fds
  */
-- 
1.7.1

Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Eric Auger]

On 11/25/2014 01:23 PM, Feng Wu wrote:
> This patch adds and documents a new attribute
> KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE group.
> This new attribute is used for VT-d Posted-Interrupts.
> 
> When guest OS changes the interrupt configuration for an
> assigned device, such as, MSI/MSIx data/address fields,
> QEMU will use this IRQ attribute to tell KVM to update the
> related IRTE according the VT-d Posted-Interrrupts Specification,
> such as, the guest vector should be updated in the related IRTE.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
>  include/uapi/linux/kvm.h                   |   10 ++++++++++
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/vfio.txt b/Documentation/virtual/kvm/devices/vfio.txt
> index f7aff29..39dee86 100644
> --- a/Documentation/virtual/kvm/devices/vfio.txt
> +++ b/Documentation/virtual/kvm/devices/vfio.txt
> @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been called to trigger the IRQ
>  or associate an eventfd to it. Unforwarding can only be called while the
>  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this condition is
>  not satisfied, the command returns an -EBUSY.
> +
> +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups mechanism to post
> +                                   the IRQ to guests.
> +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr struct.
> +
> +When guest OS changes the interrupt configuration for an assigned device,
> +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
> +to tell KVM to update the related IRTE according the VT-d Posted-Interrrupts
> +Specification, such as, the guest vector should be updated in the related IRTE.
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index a269a42..e5f86ad 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -949,6 +949,7 @@ struct kvm_device_attr {
>  #define  KVM_DEV_VFIO_DEVICE			2
>  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
>  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
> +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
>  
>  enum kvm_device_type {
>  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
>  	__u32 gsi; /* gsi, ie. virtual IRQ number */
>  };
>  
> +struct kvm_posted_intr {
> +	__u32	argsz;
> +	__u32	fd;		/* file descriptor of the VFIO device */
> +	__u32	index;		/* VFIO device IRQ index */
> +	__u32	start;
> +	__u32	count;
> +	int	virq[0];	/* gsi, ie. virtual IRQ number */
> +};
Hi Feng,

This struct could be used by arm code too. If Alex agrees I could use
that one instead. We just need to find a common sensible name

Best Regards
Eric
> +
>  /*
>   * ioctls for VM fds
>   */
> 

Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Alex Williamson]

On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
> On 11/25/2014 01:23 PM, Feng Wu wrote:
> > This patch adds and documents a new attribute
> > KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE group.
> > This new attribute is used for VT-d Posted-Interrupts.
> > 
> > When guest OS changes the interrupt configuration for an
> > assigned device, such as, MSI/MSIx data/address fields,
> > QEMU will use this IRQ attribute to tell KVM to update the
> > related IRTE according the VT-d Posted-Interrrupts Specification,
> > such as, the guest vector should be updated in the related IRTE.
> > 
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> >  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
> >  include/uapi/linux/kvm.h                   |   10 ++++++++++
> >  2 files changed, 19 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/devices/vfio.txt b/Documentation/virtual/kvm/devices/vfio.txt
> > index f7aff29..39dee86 100644
> > --- a/Documentation/virtual/kvm/devices/vfio.txt
> > +++ b/Documentation/virtual/kvm/devices/vfio.txt
> > @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been called to trigger the IRQ
> >  or associate an eventfd to it. Unforwarding can only be called while the
> >  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this condition is
> >  not satisfied, the command returns an -EBUSY.
> > +
> > +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups mechanism to post
> > +                                   the IRQ to guests.
> > +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr struct.
> > +
> > +When guest OS changes the interrupt configuration for an assigned device,
> > +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
> > +to tell KVM to update the related IRTE according the VT-d Posted-Interrrupts
> > +Specification, such as, the guest vector should be updated in the related IRTE.
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index a269a42..e5f86ad 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -949,6 +949,7 @@ struct kvm_device_attr {
> >  #define  KVM_DEV_VFIO_DEVICE			2
> >  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
> >  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
> > +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
> >  
> >  enum kvm_device_type {
> >  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> > @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
> >  	__u32 gsi; /* gsi, ie. virtual IRQ number */
> >  };
> >  
> > +struct kvm_posted_intr {
> > +	__u32	argsz;
> > +	__u32	fd;		/* file descriptor of the VFIO device */
> > +	__u32	index;		/* VFIO device IRQ index */
> > +	__u32	start;
> > +	__u32	count;
> > +	int	virq[0];	/* gsi, ie. virtual IRQ number */
> > +};
> Hi Feng,
> 
> This struct could be used by arm code too. If Alex agrees I could use
> that one instead. We just need to find a common sensible name

Yep, the interface might as well support batch setup.  The vfio code
uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we could
let the data in the structure define which operation to do.  Ideally the
code in virt/kvm/vfio.c would be almost entirely shared and just make
different arch_foo() callouts.  The PCI smarts in 2/2 here should
probably be moved out to that same arch_ code.  Thanks,

Alex

RE: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Wu, Feng]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Wednesday, November 26, 2014 12:10 AM
> To: Eric Auger
> Cc: Wu, Feng; pbonzini@redhat.com; gleb@kernel.org; kvm@vger.kernel.org
> Subject: Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d
> Posted-Interrupts
> 
> On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
> > On 11/25/2014 01:23 PM, Feng Wu wrote:
> > > This patch adds and documents a new attribute
> > > KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE group.
> > > This new attribute is used for VT-d Posted-Interrupts.
> > >
> > > When guest OS changes the interrupt configuration for an
> > > assigned device, such as, MSI/MSIx data/address fields,
> > > QEMU will use this IRQ attribute to tell KVM to update the
> > > related IRTE according the VT-d Posted-Interrrupts Specification,
> > > such as, the guest vector should be updated in the related IRTE.
> > >
> > > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > > ---
> > >  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
> > >  include/uapi/linux/kvm.h                   |   10 ++++++++++
> > >  2 files changed, 19 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/Documentation/virtual/kvm/devices/vfio.txt
> b/Documentation/virtual/kvm/devices/vfio.txt
> > > index f7aff29..39dee86 100644
> > > --- a/Documentation/virtual/kvm/devices/vfio.txt
> > > +++ b/Documentation/virtual/kvm/devices/vfio.txt
> > > @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been
> called to trigger the IRQ
> > >  or associate an eventfd to it. Unforwarding can only be called while the
> > >  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this condition
> is
> > >  not satisfied, the command returns an -EBUSY.
> > > +
> > > +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups
> mechanism to post
> > > +                                   the IRQ to guests.
> > > +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr
> struct.
> > > +
> > > +When guest OS changes the interrupt configuration for an assigned device,
> > > +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
> > > +to tell KVM to update the related IRTE according the VT-d
> Posted-Interrrupts
> > > +Specification, such as, the guest vector should be updated in the related
> IRTE.
> > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > index a269a42..e5f86ad 100644
> > > --- a/include/uapi/linux/kvm.h
> > > +++ b/include/uapi/linux/kvm.h
> > > @@ -949,6 +949,7 @@ struct kvm_device_attr {
> > >  #define  KVM_DEV_VFIO_DEVICE			2
> > >  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
> > >  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
> > > +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
> > >
> > >  enum kvm_device_type {
> > >  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> > > @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
> > >  	__u32 gsi; /* gsi, ie. virtual IRQ number */
> > >  };
> > >
> > > +struct kvm_posted_intr {
> > > +	__u32	argsz;
> > > +	__u32	fd;		/* file descriptor of the VFIO device */
> > > +	__u32	index;		/* VFIO device IRQ index */
> > > +	__u32	start;
> > > +	__u32	count;
> > > +	int	virq[0];	/* gsi, ie. virtual IRQ number */
> > > +};
> > Hi Feng,
> >
> > This struct could be used by arm code too. If Alex agrees I could use
> > that one instead. We just need to find a common sensible name
> 
> Yep, the interface might as well support batch setup.  The vfio code
> uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we could
> let the data in the structure define which operation to do.  Ideally the
> code in virt/kvm/vfio.c would be almost entirely shared and just make
> different arch_foo() callouts.  The PCI smarts in 2/2 here should
> probably be moved out to that same arch_ code.  Thanks,
> 
> Alex

That would be great if we share the same data structure!

Thanks,
Feng

Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Eric Auger]

On 11/25/2014 05:10 PM, Alex Williamson wrote:
> On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
>> On 11/25/2014 01:23 PM, Feng Wu wrote:
>>> This patch adds and documents a new attribute
>>> KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE group.
>>> This new attribute is used for VT-d Posted-Interrupts.
>>>
>>> When guest OS changes the interrupt configuration for an
>>> assigned device, such as, MSI/MSIx data/address fields,
>>> QEMU will use this IRQ attribute to tell KVM to update the
>>> related IRTE according the VT-d Posted-Interrrupts Specification,
>>> such as, the guest vector should be updated in the related IRTE.
>>>
>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>> ---
>>>  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
>>>  include/uapi/linux/kvm.h                   |   10 ++++++++++
>>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/virtual/kvm/devices/vfio.txt b/Documentation/virtual/kvm/devices/vfio.txt
>>> index f7aff29..39dee86 100644
>>> --- a/Documentation/virtual/kvm/devices/vfio.txt
>>> +++ b/Documentation/virtual/kvm/devices/vfio.txt
>>> @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been called to trigger the IRQ
>>>  or associate an eventfd to it. Unforwarding can only be called while the
>>>  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this condition is
>>>  not satisfied, the command returns an -EBUSY.
>>> +
>>> +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups mechanism to post
>>> +                                   the IRQ to guests.
>>> +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr struct.
>>> +
>>> +When guest OS changes the interrupt configuration for an assigned device,
>>> +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
>>> +to tell KVM to update the related IRTE according the VT-d Posted-Interrrupts
>>> +Specification, such as, the guest vector should be updated in the related IRTE.
>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index a269a42..e5f86ad 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
>>>  #define  KVM_DEV_VFIO_DEVICE			2
>>>  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
>>>  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
>>> +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
>>>  
>>>  enum kvm_device_type {
>>>  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
>>> @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
>>>  	__u32 gsi; /* gsi, ie. virtual IRQ number */
>>>  };
>>>  
Hi Feng, Alex,
I am currently reworking my code to use something closer to this struct.
Would you agree with following changes?
>>> +struct kvm_posted_intr {
kvm_posted_irq
>>> +	__u32	argsz;
>>> +	__u32	fd;		/* file descriptor of the VFIO device */
>>> +	__u32	index;		/* VFIO device IRQ index */
>>> +	__u32	start;
>>> +	__u32	count;
>>> +	int	virq[0];	/* gsi, ie. virtual IRQ number */
__u32 gsi[];

>>> +};
>> Hi Feng,
>>
>> This struct could be used by arm code too. If Alex agrees I could use
>> that one instead. We just need to find a common sensible name
> 
> Yep, the interface might as well support batch setup.  The vfio code
> uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we could
> let the data in the structure define which operation to do. 

In case we remove the unforward and use fd=1 to tear down, the virq=gsi
must uniquely identify the struct. For ARM I think this is true, we
cannot have several physical IRQ forwarded to the same GSI. I don't know
about posted irqs or other archs.

Best Regards

Eric
 Ideally the
> code in virt/kvm/vfio.c would be almost entirely shared and just make
> different arch_foo() callouts.  The PCI smarts in 2/2 here should
> probably be moved out to that same arch_ code.  Thanks,
> 
> Alex
> 

RE: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Wu, Feng]

> -----Original Message-----
> From: Eric Auger [mailto:eric.auger@linaro.org]
> Sent: Monday, December 01, 2014 6:10 PM
> To: Alex Williamson
> Cc: Wu, Feng; pbonzini@redhat.com; gleb@kernel.org; kvm@vger.kernel.org
> Subject: Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d
> Posted-Interrupts
> 
> On 11/25/2014 05:10 PM, Alex Williamson wrote:
> > On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
> >> On 11/25/2014 01:23 PM, Feng Wu wrote:
> >>> This patch adds and documents a new attribute
> >>> KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE
> group.
> >>> This new attribute is used for VT-d Posted-Interrupts.
> >>>
> >>> When guest OS changes the interrupt configuration for an
> >>> assigned device, such as, MSI/MSIx data/address fields,
> >>> QEMU will use this IRQ attribute to tell KVM to update the
> >>> related IRTE according the VT-d Posted-Interrrupts Specification,
> >>> such as, the guest vector should be updated in the related IRTE.
> >>>
> >>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> >>> ---
> >>>  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
> >>>  include/uapi/linux/kvm.h                   |   10 ++++++++++
> >>>  2 files changed, 19 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/Documentation/virtual/kvm/devices/vfio.txt
> b/Documentation/virtual/kvm/devices/vfio.txt
> >>> index f7aff29..39dee86 100644
> >>> --- a/Documentation/virtual/kvm/devices/vfio.txt
> >>> +++ b/Documentation/virtual/kvm/devices/vfio.txt
> >>> @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been
> called to trigger the IRQ
> >>>  or associate an eventfd to it. Unforwarding can only be called while the
> >>>  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this
> condition is
> >>>  not satisfied, the command returns an -EBUSY.
> >>> +
> >>> +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups
> mechanism to post
> >>> +                                   the IRQ to guests.
> >>> +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr
> struct.
> >>> +
> >>> +When guest OS changes the interrupt configuration for an assigned
> device,
> >>> +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
> >>> +to tell KVM to update the related IRTE according the VT-d
> Posted-Interrrupts
> >>> +Specification, such as, the guest vector should be updated in the related
> IRTE.
> >>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >>> index a269a42..e5f86ad 100644
> >>> --- a/include/uapi/linux/kvm.h
> >>> +++ b/include/uapi/linux/kvm.h
> >>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
> >>>  #define  KVM_DEV_VFIO_DEVICE			2
> >>>  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
> >>>  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
> >>> +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
> >>>
> >>>  enum kvm_device_type {
> >>>  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> >>> @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
> >>>  	__u32 gsi; /* gsi, ie. virtual IRQ number */
> >>>  };
> >>>
> Hi Feng, Alex,
> I am currently reworking my code to use something closer to this struct.
> Would you agree with following changes?
> >>> +struct kvm_posted_intr {
> kvm_posted_irq

Hi Alex,

Do you mean changing the structure name to "kvm_posted_irq"? I am okay
If you think this name is also suitable for ARM forwarded irq. Or we can find
a more common name, such as "struct kvm_accel_irq", what is your opinion, Alex?

> >>> +	__u32	argsz;
> >>> +	__u32	fd;		/* file descriptor of the VFIO device */
> >>> +	__u32	index;		/* VFIO device IRQ index */
> >>> +	__u32	start;
> >>> +	__u32	count;
> >>> +	int	virq[0];	/* gsi, ie. virtual IRQ number */
> __u32 gsi[];

I think this change is okay to me. If Alex also agree, I will follow this in the
next post. 

Thanks,
Feng


> >>> +};
> >> Hi Feng,
> >>
> >> This struct could be used by arm code too. If Alex agrees I could use
> >> that one instead. We just need to find a common sensible name
> >
> > Yep, the interface might as well support batch setup.  The vfio code
> > uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we could
> > let the data in the structure define which operation to do.
> 
> In case we remove the unforward and use fd=1 to tear down, the virq=gsi
> must uniquely identify the struct. For ARM I think this is true, we
> cannot have several physical IRQ forwarded to the same GSI. I don't know
> about posted irqs or other archs.
> 
> Best Regards
> 
> Eric
>  Ideally the
> > code in virt/kvm/vfio.c would be almost entirely shared and just make
> > different arch_foo() callouts.  The PCI smarts in 2/2 here should
> > probably be moved out to that same arch_ code.  Thanks,
> >
> > Alex
> >

Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Alex Williamson]

On Tue, 2014-12-02 at 02:08 +0000, Wu, Feng wrote:
> 
> > -----Original Message-----
> > From: Eric Auger [mailto:eric.auger@linaro.org]
> > Sent: Monday, December 01, 2014 6:10 PM
> > To: Alex Williamson
> > Cc: Wu, Feng; pbonzini@redhat.com; gleb@kernel.org; kvm@vger.kernel.org
> > Subject: Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d
> > Posted-Interrupts
> > 
> > On 11/25/2014 05:10 PM, Alex Williamson wrote:
> > > On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
> > >> On 11/25/2014 01:23 PM, Feng Wu wrote:
> > >>> This patch adds and documents a new attribute
> > >>> KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE
> > group.
> > >>> This new attribute is used for VT-d Posted-Interrupts.
> > >>>
> > >>> When guest OS changes the interrupt configuration for an
> > >>> assigned device, such as, MSI/MSIx data/address fields,
> > >>> QEMU will use this IRQ attribute to tell KVM to update the
> > >>> related IRTE according the VT-d Posted-Interrrupts Specification,
> > >>> such as, the guest vector should be updated in the related IRTE.
> > >>>
> > >>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> > >>> ---
> > >>>  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
> > >>>  include/uapi/linux/kvm.h                   |   10 ++++++++++
> > >>>  2 files changed, 19 insertions(+), 0 deletions(-)
> > >>>
> > >>> diff --git a/Documentation/virtual/kvm/devices/vfio.txt
> > b/Documentation/virtual/kvm/devices/vfio.txt
> > >>> index f7aff29..39dee86 100644
> > >>> --- a/Documentation/virtual/kvm/devices/vfio.txt
> > >>> +++ b/Documentation/virtual/kvm/devices/vfio.txt
> > >>> @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been
> > called to trigger the IRQ
> > >>>  or associate an eventfd to it. Unforwarding can only be called while the
> > >>>  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this
> > condition is
> > >>>  not satisfied, the command returns an -EBUSY.
> > >>> +
> > >>> +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups
> > mechanism to post
> > >>> +                                   the IRQ to guests.
> > >>> +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr
> > struct.
> > >>> +
> > >>> +When guest OS changes the interrupt configuration for an assigned
> > device,
> > >>> +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
> > >>> +to tell KVM to update the related IRTE according the VT-d
> > Posted-Interrrupts
> > >>> +Specification, such as, the guest vector should be updated in the related
> > IRTE.
> > >>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > >>> index a269a42..e5f86ad 100644
> > >>> --- a/include/uapi/linux/kvm.h
> > >>> +++ b/include/uapi/linux/kvm.h
> > >>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
> > >>>  #define  KVM_DEV_VFIO_DEVICE			2
> > >>>  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
> > >>>  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
> > >>> +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
> > >>>
> > >>>  enum kvm_device_type {
> > >>>  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> > >>> @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
> > >>>  	__u32 gsi; /* gsi, ie. virtual IRQ number */
> > >>>  };
> > >>>
> > Hi Feng, Alex,
> > I am currently reworking my code to use something closer to this struct.
> > Would you agree with following changes?
> > >>> +struct kvm_posted_intr {
> > kvm_posted_irq
> 
> Hi Alex,
> 
> Do you mean changing the structure name to "kvm_posted_irq"? I am okay
> If you think this name is also suitable for ARM forwarded irq. Or we can find
> a more common name, such as "struct kvm_accel_irq", what is your opinion, Alex?

I'd think something like struct kvm_vfio_dev_irq describes it fairly
well.

> > >>> +	__u32	argsz;
> > >>> +	__u32	fd;		/* file descriptor of the VFIO device */
> > >>> +	__u32	index;		/* VFIO device IRQ index */
> > >>> +	__u32	start;
> > >>> +	__u32	count;
> > >>> +	int	virq[0];	/* gsi, ie. virtual IRQ number */
> > __u32 gsi[];
> 
> I think this change is okay to me. If Alex also agree, I will follow this in the
> next post. 
> 
> > >>> +};
> > >> Hi Feng,
> > >>
> > >> This struct could be used by arm code too. If Alex agrees I could use
> > >> that one instead. We just need to find a common sensible name
> > >
> > > Yep, the interface might as well support batch setup.  The vfio code
> > > uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we could
> > > let the data in the structure define which operation to do.
> > 
> > In case we remove the unforward and use fd=1 to tear down, the virq=gsi
> > must uniquely identify the struct. For ARM I think this is true, we
> > cannot have several physical IRQ forwarded to the same GSI. I don't know
> > about posted irqs or other archs.

It makes more sense to me that fd is the real vfio_device fd that we
uniquely match to existing forwarded/posted IRQs by
{vfio_device,index,start[count]}.  If gsi was then signed, passing -1
could be used to teardown that forward/posting.  Passing fd=1, ie.
stdout, is pretty non-intuitive to me.  Thanks,

Alex

RE: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Wu, Feng]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Tuesday, December 02, 2014 12:48 PM
> To: Wu, Feng
> Cc: Eric Auger; pbonzini@redhat.com; gleb@kernel.org; kvm@vger.kernel.org
> Subject: Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d
> Posted-Interrupts
> 
> On Tue, 2014-12-02 at 02:08 +0000, Wu, Feng wrote:
> >
> > > -----Original Message-----
> > > From: Eric Auger [mailto:eric.auger@linaro.org]
> > > Sent: Monday, December 01, 2014 6:10 PM
> > > To: Alex Williamson
> > > Cc: Wu, Feng; pbonzini@redhat.com; gleb@kernel.org;
> kvm@vger.kernel.org
> > > Subject: Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d
> > > Posted-Interrupts
> > >
> > > On 11/25/2014 05:10 PM, Alex Williamson wrote:
> > > > On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
> > > >> On 11/25/2014 01:23 PM, Feng Wu wrote:
> > > >>> This patch adds and documents a new attribute
> > > >>> KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE
> > > group.
> > > >>> This new attribute is used for VT-d Posted-Interrupts.
> > > >>>
> > > >>> When guest OS changes the interrupt configuration for an
> > > >>> assigned device, such as, MSI/MSIx data/address fields,
> > > >>> QEMU will use this IRQ attribute to tell KVM to update the
> > > >>> related IRTE according the VT-d Posted-Interrrupts Specification,
> > > >>> such as, the guest vector should be updated in the related IRTE.
> > > >>>
> > > >>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> > > >>> ---
> > > >>>  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
> > > >>>  include/uapi/linux/kvm.h                   |   10 ++++++++++
> > > >>>  2 files changed, 19 insertions(+), 0 deletions(-)
> > > >>>
> > > >>> diff --git a/Documentation/virtual/kvm/devices/vfio.txt
> > > b/Documentation/virtual/kvm/devices/vfio.txt
> > > >>> index f7aff29..39dee86 100644
> > > >>> --- a/Documentation/virtual/kvm/devices/vfio.txt
> > > >>> +++ b/Documentation/virtual/kvm/devices/vfio.txt
> > > >>> @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has
> been
> > > called to trigger the IRQ
> > > >>>  or associate an eventfd to it. Unforwarding can only be called while
> the
> > > >>>  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this
> > > condition is
> > > >>>  not satisfied, the command returns an -EBUSY.
> > > >>> +
> > > >>> +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups
> > > mechanism to post
> > > >>> +                                   the IRQ to guests.
> > > >>> +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr
> > > struct.
> > > >>> +
> > > >>> +When guest OS changes the interrupt configuration for an assigned
> > > device,
> > > >>> +such as, MSI/MSIx data/address fields, QEMU will use this IRQ
> attribute
> > > >>> +to tell KVM to update the related IRTE according the VT-d
> > > Posted-Interrrupts
> > > >>> +Specification, such as, the guest vector should be updated in the
> related
> > > IRTE.
> > > >>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > >>> index a269a42..e5f86ad 100644
> > > >>> --- a/include/uapi/linux/kvm.h
> > > >>> +++ b/include/uapi/linux/kvm.h
> > > >>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
> > > >>>  #define  KVM_DEV_VFIO_DEVICE			2
> > > >>>  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
> > > >>>  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ
> 	2
> > > >>> +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
> > > >>>
> > > >>>  enum kvm_device_type {
> > > >>>  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> > > >>> @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
> > > >>>  	__u32 gsi; /* gsi, ie. virtual IRQ number */
> > > >>>  };
> > > >>>
> > > Hi Feng, Alex,
> > > I am currently reworking my code to use something closer to this struct.
> > > Would you agree with following changes?
> > > >>> +struct kvm_posted_intr {
> > > kvm_posted_irq
> >
> > Hi Alex,
> >
> > Do you mean changing the structure name to "kvm_posted_irq"? I am okay
> > If you think this name is also suitable for ARM forwarded irq. Or we can find
> > a more common name, such as "struct kvm_accel_irq", what is your opinion,
> Alex?
> 
> I'd think something like struct kvm_vfio_dev_irq describes it fairly
> well.

No problem! I will follow this in the next post.

> 
> > > >>> +	__u32	argsz;
> > > >>> +	__u32	fd;		/* file descriptor of the VFIO device */
> > > >>> +	__u32	index;		/* VFIO device IRQ index */
> > > >>> +	__u32	start;
> > > >>> +	__u32	count;
> > > >>> +	int	virq[0];	/* gsi, ie. virtual IRQ number */
> > > __u32 gsi[];
> >
> > I think this change is okay to me. If Alex also agree, I will follow this in the
> > next post.
> >
> > > >>> +};
> > > >> Hi Feng,
> > > >>
> > > >> This struct could be used by arm code too. If Alex agrees I could use
> > > >> that one instead. We just need to find a common sensible name
> > > >
> > > > Yep, the interface might as well support batch setup.  The vfio code
> > > > uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we
> could
> > > > let the data in the structure define which operation to do.
> > >
> > > In case we remove the unforward and use fd=1 to tear down, the virq=gsi
> > > must uniquely identify the struct. For ARM I think this is true, we
> > > cannot have several physical IRQ forwarded to the same GSI. I don't know
> > > about posted irqs or other archs.
> 
> It makes more sense to me that fd is the real vfio_device fd that we
> uniquely match to existing forwarded/posted IRQs by
> {vfio_device,index,start[count]}.  If gsi was then signed, passing -1
> could be used to teardown that forward/posting.  Passing fd=1, ie.
> stdout, is pretty non-intuitive to me.  Thanks,

So, do you mean we still need use "int gsi[]" in "struct kvm_vfio_dev_irq"?

Thanks,
Feng

> 
> Alex
> 

Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Eric Auger]

On 12/02/2014 05:48 AM, Alex Williamson wrote:
> On Tue, 2014-12-02 at 02:08 +0000, Wu, Feng wrote:
>>
>>> -----Original Message-----
>>> From: Eric Auger [mailto:eric.auger@linaro.org]
>>> Sent: Monday, December 01, 2014 6:10 PM
>>> To: Alex Williamson
>>> Cc: Wu, Feng; pbonzini@redhat.com; gleb@kernel.org; kvm@vger.kernel.org
>>> Subject: Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d
>>> Posted-Interrupts
>>>
>>> On 11/25/2014 05:10 PM, Alex Williamson wrote:
>>>> On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
>>>>> On 11/25/2014 01:23 PM, Feng Wu wrote:
>>>>>> This patch adds and documents a new attribute
>>>>>> KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE
>>> group.
>>>>>> This new attribute is used for VT-d Posted-Interrupts.
>>>>>>
>>>>>> When guest OS changes the interrupt configuration for an
>>>>>> assigned device, such as, MSI/MSIx data/address fields,
>>>>>> QEMU will use this IRQ attribute to tell KVM to update the
>>>>>> related IRTE according the VT-d Posted-Interrrupts Specification,
>>>>>> such as, the guest vector should be updated in the related IRTE.
>>>>>>
>>>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>>>>> ---
>>>>>>  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
>>>>>>  include/uapi/linux/kvm.h                   |   10 ++++++++++
>>>>>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/virtual/kvm/devices/vfio.txt
>>> b/Documentation/virtual/kvm/devices/vfio.txt
>>>>>> index f7aff29..39dee86 100644
>>>>>> --- a/Documentation/virtual/kvm/devices/vfio.txt
>>>>>> +++ b/Documentation/virtual/kvm/devices/vfio.txt
>>>>>> @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been
>>> called to trigger the IRQ
>>>>>>  or associate an eventfd to it. Unforwarding can only be called while the
>>>>>>  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this
>>> condition is
>>>>>>  not satisfied, the command returns an -EBUSY.
>>>>>> +
>>>>>> +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups
>>> mechanism to post
>>>>>> +                                   the IRQ to guests.
>>>>>> +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr
>>> struct.
>>>>>> +
>>>>>> +When guest OS changes the interrupt configuration for an assigned
>>> device,
>>>>>> +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
>>>>>> +to tell KVM to update the related IRTE according the VT-d
>>> Posted-Interrrupts
>>>>>> +Specification, such as, the guest vector should be updated in the related
>>> IRTE.
>>>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>>>>> index a269a42..e5f86ad 100644
>>>>>> --- a/include/uapi/linux/kvm.h
>>>>>> +++ b/include/uapi/linux/kvm.h
>>>>>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
>>>>>>  #define  KVM_DEV_VFIO_DEVICE			2
>>>>>>  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
>>>>>>  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
>>>>>> +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
>>>>>>
>>>>>>  enum kvm_device_type {
>>>>>>  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
>>>>>> @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
>>>>>>  	__u32 gsi; /* gsi, ie. virtual IRQ number */
>>>>>>  };
>>>>>>
>>> Hi Feng, Alex,
>>> I am currently reworking my code to use something closer to this struct.
>>> Would you agree with following changes?
>>>>>> +struct kvm_posted_intr {
>>> kvm_posted_irq
>>
>> Hi Alex,
>>
>> Do you mean changing the structure name to "kvm_posted_irq"? I am okay
>> If you think this name is also suitable for ARM forwarded irq. Or we can find
>> a more common name, such as "struct kvm_accel_irq", what is your opinion, Alex?
> 
> I'd think something like struct kvm_vfio_dev_irq describes it fairly
> well.
ok for that name
> 
>>>>>> +	__u32	argsz;
>>>>>> +	__u32	fd;		/* file descriptor of the VFIO device */
>>>>>> +	__u32	index;		/* VFIO device IRQ index */
>>>>>> +	__u32	start;
>>>>>> +	__u32	count;
>>>>>> +	int	virq[0];	/* gsi, ie. virtual IRQ number */
>>> __u32 gsi[];
>>
>> I think this change is okay to me. If Alex also agree, I will follow this in the
>> next post. 
>>
>>>>>> +};
>>>>> Hi Feng,
>>>>>
>>>>> This struct could be used by arm code too. If Alex agrees I could use
>>>>> that one instead. We just need to find a common sensible name
>>>>
>>>> Yep, the interface might as well support batch setup.  The vfio code
>>>> uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we could
>>>> let the data in the structure define which operation to do.
>>>
>>> In case we remove the unforward and use fd=1 to tear down, the virq=gsi
>>> must uniquely identify the struct. For ARM I think this is true, we
>>> cannot have several physical IRQ forwarded to the same GSI. I don't know
>>> about posted irqs or other archs.
> 
> It makes more sense to me that fd is the real vfio_device fd that we
> uniquely match to existing forwarded/posted IRQs by
> {vfio_device,index,start[count]}.  If gsi was then signed, passing -1
> could be used to teardown that forward/posting.  Passing fd=1, ie.
> stdout, is pretty non-intuitive to me.  Thanks,
sorry meant fd = -1 (typo) as in the original VFIO API. Personally I
think I would prefer keeping the UNFORWARD but I will follow your guidance.

Eric
> 
> Alex
> 
> 

Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Alex Williamson]

On Tue, 2014-12-02 at 08:52 +0100, Eric Auger wrote:
> On 12/02/2014 05:48 AM, Alex Williamson wrote:
> > On Tue, 2014-12-02 at 02:08 +0000, Wu, Feng wrote:
> >>
> >>> -----Original Message-----
> >>> From: Eric Auger [mailto:eric.auger@linaro.org]
> >>> Sent: Monday, December 01, 2014 6:10 PM
> >>> To: Alex Williamson
> >>> Cc: Wu, Feng; pbonzini@redhat.com; gleb@kernel.org; kvm@vger.kernel.org
> >>> Subject: Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d
> >>> Posted-Interrupts
> >>>
> >>> On 11/25/2014 05:10 PM, Alex Williamson wrote:
> >>>> On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
> >>>>> On 11/25/2014 01:23 PM, Feng Wu wrote:
> >>>>>> This patch adds and documents a new attribute
> >>>>>> KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE
> >>> group.
> >>>>>> This new attribute is used for VT-d Posted-Interrupts.
> >>>>>>
> >>>>>> When guest OS changes the interrupt configuration for an
> >>>>>> assigned device, such as, MSI/MSIx data/address fields,
> >>>>>> QEMU will use this IRQ attribute to tell KVM to update the
> >>>>>> related IRTE according the VT-d Posted-Interrrupts Specification,
> >>>>>> such as, the guest vector should be updated in the related IRTE.
> >>>>>>
> >>>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> >>>>>> ---
> >>>>>>  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
> >>>>>>  include/uapi/linux/kvm.h                   |   10 ++++++++++
> >>>>>>  2 files changed, 19 insertions(+), 0 deletions(-)
> >>>>>>
> >>>>>> diff --git a/Documentation/virtual/kvm/devices/vfio.txt
> >>> b/Documentation/virtual/kvm/devices/vfio.txt
> >>>>>> index f7aff29..39dee86 100644
> >>>>>> --- a/Documentation/virtual/kvm/devices/vfio.txt
> >>>>>> +++ b/Documentation/virtual/kvm/devices/vfio.txt
> >>>>>> @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been
> >>> called to trigger the IRQ
> >>>>>>  or associate an eventfd to it. Unforwarding can only be called while the
> >>>>>>  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this
> >>> condition is
> >>>>>>  not satisfied, the command returns an -EBUSY.
> >>>>>> +
> >>>>>> +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups
> >>> mechanism to post
> >>>>>> +                                   the IRQ to guests.
> >>>>>> +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr
> >>> struct.
> >>>>>> +
> >>>>>> +When guest OS changes the interrupt configuration for an assigned
> >>> device,
> >>>>>> +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
> >>>>>> +to tell KVM to update the related IRTE according the VT-d
> >>> Posted-Interrrupts
> >>>>>> +Specification, such as, the guest vector should be updated in the related
> >>> IRTE.
> >>>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >>>>>> index a269a42..e5f86ad 100644
> >>>>>> --- a/include/uapi/linux/kvm.h
> >>>>>> +++ b/include/uapi/linux/kvm.h
> >>>>>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
> >>>>>>  #define  KVM_DEV_VFIO_DEVICE			2
> >>>>>>  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
> >>>>>>  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
> >>>>>> +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
> >>>>>>
> >>>>>>  enum kvm_device_type {
> >>>>>>  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
> >>>>>> @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
> >>>>>>  	__u32 gsi; /* gsi, ie. virtual IRQ number */
> >>>>>>  };
> >>>>>>
> >>> Hi Feng, Alex,
> >>> I am currently reworking my code to use something closer to this struct.
> >>> Would you agree with following changes?
> >>>>>> +struct kvm_posted_intr {
> >>> kvm_posted_irq
> >>
> >> Hi Alex,
> >>
> >> Do you mean changing the structure name to "kvm_posted_irq"? I am okay
> >> If you think this name is also suitable for ARM forwarded irq. Or we can find
> >> a more common name, such as "struct kvm_accel_irq", what is your opinion, Alex?
> > 
> > I'd think something like struct kvm_vfio_dev_irq describes it fairly
> > well.
> ok for that name
> > 
> >>>>>> +	__u32	argsz;
> >>>>>> +	__u32	fd;		/* file descriptor of the VFIO device */
> >>>>>> +	__u32	index;		/* VFIO device IRQ index */
> >>>>>> +	__u32	start;
> >>>>>> +	__u32	count;
> >>>>>> +	int	virq[0];	/* gsi, ie. virtual IRQ number */
> >>> __u32 gsi[];
> >>
> >> I think this change is okay to me. If Alex also agree, I will follow this in the
> >> next post. 
> >>
> >>>>>> +};
> >>>>> Hi Feng,
> >>>>>
> >>>>> This struct could be used by arm code too. If Alex agrees I could use
> >>>>> that one instead. We just need to find a common sensible name
> >>>>
> >>>> Yep, the interface might as well support batch setup.  The vfio code
> >>>> uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we could
> >>>> let the data in the structure define which operation to do.
> >>>
> >>> In case we remove the unforward and use fd=1 to tear down, the virq=gsi
> >>> must uniquely identify the struct. For ARM I think this is true, we
> >>> cannot have several physical IRQ forwarded to the same GSI. I don't know
> >>> about posted irqs or other archs.
> > 
> > It makes more sense to me that fd is the real vfio_device fd that we
> > uniquely match to existing forwarded/posted IRQs by
> > {vfio_device,index,start[count]}.  If gsi was then signed, passing -1
> > could be used to teardown that forward/posting.  Passing fd=1, ie.
> > stdout, is pretty non-intuitive to me.  Thanks,
> sorry meant fd = -1 (typo) as in the original VFIO API. Personally I
> think I would prefer keeping the UNFORWARD but I will follow your guidance.

If fd=-1 we can't uniquely identify the device and irq when there are
multiple devices.  I'm not necessarily opposed to an UNFORWARD, just
show how it works better or more cleanly in the code.  Thanks,

Alex

Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d Posted-Interrupts

[Eric Auger]

On 12/02/2014 05:02 PM, Alex Williamson wrote:
> On Tue, 2014-12-02 at 08:52 +0100, Eric Auger wrote:
>> On 12/02/2014 05:48 AM, Alex Williamson wrote:
>>> On Tue, 2014-12-02 at 02:08 +0000, Wu, Feng wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Eric Auger [mailto:eric.auger@linaro.org]
>>>>> Sent: Monday, December 01, 2014 6:10 PM
>>>>> To: Alex Williamson
>>>>> Cc: Wu, Feng; pbonzini@redhat.com; gleb@kernel.org; kvm@vger.kernel.org
>>>>> Subject: Re: [RFC PATCH v2 1/2] KVM: kvm-vfio: User API for VT-d
>>>>> Posted-Interrupts
>>>>>
>>>>> On 11/25/2014 05:10 PM, Alex Williamson wrote:
>>>>>> On Tue, 2014-11-25 at 16:01 +0100, Eric Auger wrote:
>>>>>>> On 11/25/2014 01:23 PM, Feng Wu wrote:
>>>>>>>> This patch adds and documents a new attribute
>>>>>>>> KVM_DEV_VFIO_DEVICE_POSTING_IRQ in KVM_DEV_VFIO_DEVICE
>>>>> group.
>>>>>>>> This new attribute is used for VT-d Posted-Interrupts.
>>>>>>>>
>>>>>>>> When guest OS changes the interrupt configuration for an
>>>>>>>> assigned device, such as, MSI/MSIx data/address fields,
>>>>>>>> QEMU will use this IRQ attribute to tell KVM to update the
>>>>>>>> related IRTE according the VT-d Posted-Interrrupts Specification,
>>>>>>>> such as, the guest vector should be updated in the related IRTE.
>>>>>>>>
>>>>>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>>>>>>> ---
>>>>>>>>  Documentation/virtual/kvm/devices/vfio.txt |    9 +++++++++
>>>>>>>>  include/uapi/linux/kvm.h                   |   10 ++++++++++
>>>>>>>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/virtual/kvm/devices/vfio.txt
>>>>> b/Documentation/virtual/kvm/devices/vfio.txt
>>>>>>>> index f7aff29..39dee86 100644
>>>>>>>> --- a/Documentation/virtual/kvm/devices/vfio.txt
>>>>>>>> +++ b/Documentation/virtual/kvm/devices/vfio.txt
>>>>>>>> @@ -42,3 +42,12 @@ activated before VFIO_DEVICE_SET_IRQS has been
>>>>> called to trigger the IRQ
>>>>>>>>  or associate an eventfd to it. Unforwarding can only be called while the
>>>>>>>>  signaling has been disabled with VFIO_DEVICE_SET_IRQS. If this
>>>>> condition is
>>>>>>>>  not satisfied, the command returns an -EBUSY.
>>>>>>>> +
>>>>>>>> +  KVM_DEV_VFIO_DEVICE_POSTING_IRQ: Use posted interrtups
>>>>> mechanism to post
>>>>>>>> +                                   the IRQ to guests.
>>>>>>>> +For this attribute, kvm_device_attr.addr points to a kvm_posted_intr
>>>>> struct.
>>>>>>>> +
>>>>>>>> +When guest OS changes the interrupt configuration for an assigned
>>>>> device,
>>>>>>>> +such as, MSI/MSIx data/address fields, QEMU will use this IRQ attribute
>>>>>>>> +to tell KVM to update the related IRTE according the VT-d
>>>>> Posted-Interrrupts
>>>>>>>> +Specification, such as, the guest vector should be updated in the related
>>>>> IRTE.
>>>>>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>>>>>>> index a269a42..e5f86ad 100644
>>>>>>>> --- a/include/uapi/linux/kvm.h
>>>>>>>> +++ b/include/uapi/linux/kvm.h
>>>>>>>> @@ -949,6 +949,7 @@ struct kvm_device_attr {
>>>>>>>>  #define  KVM_DEV_VFIO_DEVICE			2
>>>>>>>>  #define   KVM_DEV_VFIO_DEVICE_FORWARD_IRQ			1
>>>>>>>>  #define   KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ			2
>>>>>>>> +#define   KVM_DEV_VFIO_DEVICE_POSTING_IRQ			3
>>>>>>>>
>>>>>>>>  enum kvm_device_type {
>>>>>>>>  	KVM_DEV_TYPE_FSL_MPIC_20	= 1,
>>>>>>>> @@ -973,6 +974,15 @@ struct kvm_arch_forwarded_irq {
>>>>>>>>  	__u32 gsi; /* gsi, ie. virtual IRQ number */
>>>>>>>>  };
>>>>>>>>
>>>>> Hi Feng, Alex,
>>>>> I am currently reworking my code to use something closer to this struct.
>>>>> Would you agree with following changes?
>>>>>>>> +struct kvm_posted_intr {
>>>>> kvm_posted_irq
>>>>
>>>> Hi Alex,
>>>>
>>>> Do you mean changing the structure name to "kvm_posted_irq"? I am okay
>>>> If you think this name is also suitable for ARM forwarded irq. Or we can find
>>>> a more common name, such as "struct kvm_accel_irq", what is your opinion, Alex?
>>>
>>> I'd think something like struct kvm_vfio_dev_irq describes it fairly
>>> well.
>> ok for that name
>>>
>>>>>>>> +	__u32	argsz;
>>>>>>>> +	__u32	fd;		/* file descriptor of the VFIO device */
>>>>>>>> +	__u32	index;		/* VFIO device IRQ index */
>>>>>>>> +	__u32	start;
>>>>>>>> +	__u32	count;
>>>>>>>> +	int	virq[0];	/* gsi, ie. virtual IRQ number */
>>>>> __u32 gsi[];
>>>>
>>>> I think this change is okay to me. If Alex also agree, I will follow this in the
>>>> next post. 
>>>>
>>>>>>>> +};
>>>>>>> Hi Feng,
>>>>>>>
>>>>>>> This struct could be used by arm code too. If Alex agrees I could use
>>>>>>> that one instead. We just need to find a common sensible name
>>>>>>
>>>>>> Yep, the interface might as well support batch setup.  The vfio code
>>>>>> uses -1 for teardown if we want to avoid FORWARD vs UNFORWARD we could
>>>>>> let the data in the structure define which operation to do.
>>>>>
>>>>> In case we remove the unforward and use fd=1 to tear down, the virq=gsi
>>>>> must uniquely identify the struct. For ARM I think this is true, we
>>>>> cannot have several physical IRQ forwarded to the same GSI. I don't know
>>>>> about posted irqs or other archs.
>>>
>>> It makes more sense to me that fd is the real vfio_device fd that we
>>> uniquely match to existing forwarded/posted IRQs by
>>> {vfio_device,index,start[count]}.  If gsi was then signed, passing -1
>>> could be used to teardown that forward/posting.  Passing fd=1, ie.
>>> stdout, is pretty non-intuitive to me.  Thanks,
>> sorry meant fd = -1 (typo) as in the original VFIO API. Personally I
>> think I would prefer keeping the UNFORWARD but I will follow your guidance.
> 
> If fd=-1 we can't uniquely identify the device and irq when there are
> multiple devices.  I'm not necessarily opposed to an UNFORWARD, just
> show how it works better or more cleanly in the code.  Thanks,
OK thanks.

Eric
> 
> Alex
> 

[RFC PATCH v2 2/2] KVM: kvm-vfio: implement the VFIO skeleton for VT-d Posted-Interrupts

[Feng Wu]

This patch adds the kvm-vfio interface for VT-d Posted-Interrrupts.
When guests updates MSI/MSI-x information for an assigned-device,
QEMU will use KVM_DEV_VFIO_DEVICE_POSTING_IRQ attribute to setup
IRTE for VT-d PI. This patch implement this IRQ attribute.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 virt/kvm/vfio.c |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 6bc7001..435adf4 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -446,6 +446,115 @@ out:
 	return ret;
 }
 
+static int kvm_update_pi_irte(struct kvm *kvm, int host_irq, int guest_irq)
+{
+	/*
+	 * TODO: need to add the real code to update the related IRTE,
+	 * Basically, This fucntion will do the following things:
+	 * - Get struct kvm_kernel_irq_routing_entry from guest irq
+	 * - Get the destination vCPU of the interrupts
+	 * - Update the IRTE according the VT-d PI Spec.
+	 *   1) guest vector
+	 *   2) Posted-Interrupts descritpor addresss
+	 */
+
+	return 0;
+}
+
+static int kvm_vfio_pci_get_irq_count(struct pci_dev *pdev, int irq_type)
+{
+	if (irq_type == VFIO_PCI_INTX_IRQ_INDEX) {
+		u8 pin;
+		pci_read_config_byte(pdev, PCI_INTERRUPT_PIN, &pin);
+		if (pin)
+			return 1;
+	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
+		u8 pos;
+		u16 flags;
+
+		pos = pdev->msi_cap;
+		if (pos) {
+			pci_read_config_word(pdev,
+					     pos + PCI_MSI_FLAGS, &flags);
+			return 1 << ((flags & PCI_MSI_FLAGS_QMASK) >> 1);
+		}
+	} else if (irq_type == VFIO_PCI_MSIX_IRQ_INDEX) {
+		u8 pos;
+		u16 flags;
+
+		pos = pdev->msix_cap;
+		if (pos) {
+			pci_read_config_word(pdev,
+					     pos + PCI_MSIX_FLAGS, &flags);
+
+			return (flags & PCI_MSIX_FLAGS_QSIZE) + 1;
+		}
+	} else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
+		if (pci_is_pcie(pdev))
+			return 1;
+
+	return 0;
+}
+
+static int kvm_vfio_set_pi(struct kvm_device *kdev, int32_t __user *argp)
+{
+	struct kvm_posted_intr pi_info;
+	int *virq;
+	unsigned long minsz;
+	struct vfio_device *vdev;
+	struct msi_desc *entry;
+	struct device *dev;
+	struct pci_dev *pdev;
+	int i, max, ret;
+
+	minsz = offsetofend(struct kvm_posted_intr, count);
+
+	if (copy_from_user(&pi_info, (void __user *)argp, minsz))
+		return -EFAULT;
+
+	if (pi_info.argsz < minsz || pi_info.index >= VFIO_PCI_NUM_IRQS)
+		return -EINVAL;
+
+	vdev = kvm_vfio_get_vfio_device(pi_info.fd);
+	if (IS_ERR(vdev))
+		return PTR_ERR(vdev);
+
+	dev = kvm_vfio_external_base_device(vdev);
+	if (!dev)
+		return -EFAULT;
+
+	pdev = to_pci_dev(dev);
+
+	max = kvm_vfio_pci_get_irq_count(pdev, pi_info.index);
+
+	if (pi_info.argsz - minsz < pi_info.count * sizeof(int) ||
+	    pi_info.start >= max || pi_info.start + pi_info.count > max)
+		return -EINVAL;
+
+	virq = memdup_user((void __user *)((unsigned long)argp + minsz),
+			   pi_info.count * sizeof(int));
+	if (IS_ERR(virq))
+		return PTR_ERR(virq);
+
+	for (i=0; i<pi_info.count; i++) {
+		list_for_each_entry(entry, &pdev->msi_list, list) {
+			if (entry->msi_attrib.entry_nr != pi_info.start+i)
+				continue;
+
+			ret = kvm_update_pi_irte(kdev->kvm,
+						 entry->irq, virq[i]);
+			if (ret) {
+				kfree(virq);
+				return -EFAULT;
+			}
+		}
+	}
+
+	kfree(virq);
+
+	return 0;
+}
+
 static int kvm_vfio_set_device(struct kvm_device *kdev, long attr, u64 arg)
 {
 	int32_t __user *argp = (int32_t __user *)(unsigned long)arg;
@@ -456,6 +565,9 @@ static int kvm_vfio_set_device(struct kvm_device *kdev, long attr, u64 arg)
 	case KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ:
 		ret = kvm_vfio_control_irq_forward(kdev, attr, argp);
 		break;
+	case KVM_DEV_VFIO_DEVICE_POSTING_IRQ:
+		ret = kvm_vfio_set_pi(kdev, argp);
+		break;
 	default:
 		ret = -ENXIO;
 	}
@@ -511,6 +623,9 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
 		case KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ:
 			return 0;
 #endif
+		case KVM_DEV_VFIO_DEVICE_POSTING_IRQ:
+			return 0;
+
 		}
 		break;
 	}
-- 
1.7.1

Re: [RFC PATCH v2 2/2] KVM: kvm-vfio: implement the VFIO skeleton for VT-d Posted-Interrupts

[Alex Williamson]

On Tue, 2014-11-25 at 20:23 +0800, Feng Wu wrote:
> This patch adds the kvm-vfio interface for VT-d Posted-Interrrupts.
> When guests updates MSI/MSI-x information for an assigned-device,
> QEMU will use KVM_DEV_VFIO_DEVICE_POSTING_IRQ attribute to setup
> IRTE for VT-d PI. This patch implement this IRQ attribute.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  virt/kvm/vfio.c |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 115 insertions(+), 0 deletions(-)
> 
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index 6bc7001..435adf4 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -446,6 +446,115 @@ out:
>  	return ret;
>  }
>  
> +static int kvm_update_pi_irte(struct kvm *kvm, int host_irq, int guest_irq)
> +{
> +	/*
> +	 * TODO: need to add the real code to update the related IRTE,
> +	 * Basically, This fucntion will do the following things:
> +	 * - Get struct kvm_kernel_irq_routing_entry from guest irq
> +	 * - Get the destination vCPU of the interrupts
> +	 * - Update the IRTE according the VT-d PI Spec.
> +	 *   1) guest vector
> +	 *   2) Posted-Interrupts descritpor addresss
> +	 */
> +
> +	return 0;

Why not make this an arch_ call like Eric did?

> +}
> +
> +static int kvm_vfio_pci_get_irq_count(struct pci_dev *pdev, int irq_type)
> +{
> +	if (irq_type == VFIO_PCI_INTX_IRQ_INDEX) {
> +		u8 pin;
> +		pci_read_config_byte(pdev, PCI_INTERRUPT_PIN, &pin);
> +		if (pin)
> +			return 1;
> +	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
> +		u8 pos;
> +		u16 flags;
> +
> +		pos = pdev->msi_cap;
> +		if (pos) {
> +			pci_read_config_word(pdev,
> +					     pos + PCI_MSI_FLAGS, &flags);
> +			return 1 << ((flags & PCI_MSI_FLAGS_QMASK) >> 1);
> +		}

Looks like a copy of vfio code, but since that was written helpers have
been added to the kernel:

return pci_msi_vec_count(pdev);

> +	} else if (irq_type == VFIO_PCI_MSIX_IRQ_INDEX) {
> +		u8 pos;
> +		u16 flags;
> +
> +		pos = pdev->msix_cap;
> +		if (pos) {
> +			pci_read_config_word(pdev,
> +					     pos + PCI_MSIX_FLAGS, &flags);
> +
> +			return (flags & PCI_MSIX_FLAGS_QSIZE) + 1;
> +		}

return pci_msix_vec_count(pdev);

> +	} else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
> +		if (pci_is_pcie(pdev))
> +			return 1;

This is a virtual interrupt, this interface should never be used for it.

> +
> +	return 0;
> +}
> +
> +static int kvm_vfio_set_pi(struct kvm_device *kdev, int32_t __user *argp)
> +{
> +	struct kvm_posted_intr pi_info;
> +	int *virq;
> +	unsigned long minsz;
> +	struct vfio_device *vdev;
> +	struct msi_desc *entry;
> +	struct device *dev;
> +	struct pci_dev *pdev;
> +	int i, max, ret;
> +
> +	minsz = offsetofend(struct kvm_posted_intr, count);
> +
> +	if (copy_from_user(&pi_info, (void __user *)argp, minsz))
> +		return -EFAULT;
> +
> +	if (pi_info.argsz < minsz || pi_info.index >= VFIO_PCI_NUM_IRQS)
> +		return -EINVAL;
> +
> +	vdev = kvm_vfio_get_vfio_device(pi_info.fd);
> +	if (IS_ERR(vdev))
> +		return PTR_ERR(vdev);
> +
> +	dev = kvm_vfio_external_base_device(vdev);
> +	if (!dev)
> +		return -EFAULT;

Missing put

> +
> +	pdev = to_pci_dev(dev);

We can't assume the user called with a PCI device, test it.

if (!dev_is_pci(dev)) {
	put
	return errno
}

pdev = to_pci_dev(dev);
...

> +
> +	max = kvm_vfio_pci_get_irq_count(pdev, pi_info.index);
> +
> +	if (pi_info.argsz - minsz < pi_info.count * sizeof(int) ||
> +	    pi_info.start >= max || pi_info.start + pi_info.count > max)
> +		return -EINVAL;

Missing put

> +
> +	virq = memdup_user((void __user *)((unsigned long)argp + minsz),
> +			   pi_info.count * sizeof(int));
> +	if (IS_ERR(virq))
> +		return PTR_ERR(virq);

put...

> +
> +	for (i=0; i<pi_info.count; i++) {
> +		list_for_each_entry(entry, &pdev->msi_list, list) {

This is unique to MSI/X but INTx and others can still make it here.
Also, this won't compile unless CONFIG_PCI_MSI.  Does anything protect
this list?

> +			if (entry->msi_attrib.entry_nr != pi_info.start+i)
> +				continue;
> +
> +			ret = kvm_update_pi_irte(kdev->kvm,
> +						 entry->irq, virq[i]);
> +			if (ret) {
> +				kfree(virq);
> +				return -EFAULT;

put...

> +			}
> +		}
> +	}
> +
> +	kfree(virq);

put...

> +
> +	return 0;
> +}
> +
>  static int kvm_vfio_set_device(struct kvm_device *kdev, long attr, u64 arg)
>  {
>  	int32_t __user *argp = (int32_t __user *)(unsigned long)arg;
> @@ -456,6 +565,9 @@ static int kvm_vfio_set_device(struct kvm_device *kdev, long attr, u64 arg)
>  	case KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ:
>  		ret = kvm_vfio_control_irq_forward(kdev, attr, argp);
>  		break;
> +	case KVM_DEV_VFIO_DEVICE_POSTING_IRQ:
> +		ret = kvm_vfio_set_pi(kdev, argp);
> +		break;

ARCH #ifdefs?

>  	default:
>  		ret = -ENXIO;
>  	}
> @@ -511,6 +623,9 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
>  		case KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ:
>  			return 0;
>  #endif
> +		case KVM_DEV_VFIO_DEVICE_POSTING_IRQ:
> +			return 0;
> +

Same here

>  		}
>  		break;
>  	}

I only see setup where, what if we want to stop posted interrupts?  What
if we switch to a different mode, for example the guest reboots or
unloads the driver or switches to a different driver.  Thanks,

Alex

RE: [RFC PATCH v2 2/2] KVM: kvm-vfio: implement the VFIO skeleton for VT-d Posted-Interrupts

[Wu, Feng]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Wednesday, November 26, 2014 12:04 AM
> To: Wu, Feng
> Cc: pbonzini@redhat.com; gleb@kernel.org; kvm@vger.kernel.org;
> eric.auger@linaro.org
> Subject: Re: [RFC PATCH v2 2/2] KVM: kvm-vfio: implement the VFIO skeleton
> for VT-d Posted-Interrupts
> 
> On Tue, 2014-11-25 at 20:23 +0800, Feng Wu wrote:
> > This patch adds the kvm-vfio interface for VT-d Posted-Interrrupts.
> > When guests updates MSI/MSI-x information for an assigned-device,
> > QEMU will use KVM_DEV_VFIO_DEVICE_POSTING_IRQ attribute to setup
> > IRTE for VT-d PI. This patch implement this IRQ attribute.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> >  virt/kvm/vfio.c |  115
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 115 insertions(+), 0 deletions(-)
> >
> > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> > index 6bc7001..435adf4 100644
> > --- a/virt/kvm/vfio.c
> > +++ b/virt/kvm/vfio.c
> > @@ -446,6 +446,115 @@ out:
> >  	return ret;
> >  }
> >
> > +static int kvm_update_pi_irte(struct kvm *kvm, int host_irq, int guest_irq)
> > +{
> > +	/*
> > +	 * TODO: need to add the real code to update the related IRTE,
> > +	 * Basically, This fucntion will do the following things:
> > +	 * - Get struct kvm_kernel_irq_routing_entry from guest irq
> > +	 * - Get the destination vCPU of the interrupts
> > +	 * - Update the IRTE according the VT-d PI Spec.
> > +	 *   1) guest vector
> > +	 *   2) Posted-Interrupts descritpor addresss
> > +	 */
> > +
> > +	return 0;
> 
> Why not make this an arch_ call like Eric did?

Yes, that is a good idea. In fact, as I mentioned in the [0/1] patch, this function should
be in another file. I will move it to the arch specific file. But there are some other
dependency to implement this function. I will implement it later.

> 
> > +}
> > +
> > +static int kvm_vfio_pci_get_irq_count(struct pci_dev *pdev, int irq_type)
> > +{
> > +	if (irq_type == VFIO_PCI_INTX_IRQ_INDEX) {
> > +		u8 pin;
> > +		pci_read_config_byte(pdev, PCI_INTERRUPT_PIN, &pin);
> > +		if (pin)
> > +			return 1;
> > +	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
> > +		u8 pos;
> > +		u16 flags;
> > +
> > +		pos = pdev->msi_cap;
> > +		if (pos) {
> > +			pci_read_config_word(pdev,
> > +					     pos + PCI_MSI_FLAGS, &flags);
> > +			return 1 << ((flags & PCI_MSI_FLAGS_QMASK) >> 1);
> > +		}
> 
> Looks like a copy of vfio code, but since that was written helpers have
> been added to the kernel:

Yes, this function is copied from vfio pci code, I will clean up it according to
your comments below.

> 
> return pci_msi_vec_count(pdev);
> 
> > +	} else if (irq_type == VFIO_PCI_MSIX_IRQ_INDEX) {
> > +		u8 pos;
> > +		u16 flags;
> > +
> > +		pos = pdev->msix_cap;
> > +		if (pos) {
> > +			pci_read_config_word(pdev,
> > +					     pos + PCI_MSIX_FLAGS, &flags);
> > +
> > +			return (flags & PCI_MSIX_FLAGS_QSIZE) + 1;
> > +		}
> 
> return pci_msix_vec_count(pdev);
> 
> > +	} else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
> > +		if (pci_is_pcie(pdev))
> > +			return 1;
> 
> This is a virtual interrupt, this interface should never be used for it.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int kvm_vfio_set_pi(struct kvm_device *kdev, int32_t __user *argp)
> > +{
> > +	struct kvm_posted_intr pi_info;
> > +	int *virq;
> > +	unsigned long minsz;
> > +	struct vfio_device *vdev;
> > +	struct msi_desc *entry;
> > +	struct device *dev;
> > +	struct pci_dev *pdev;
> > +	int i, max, ret;
> > +
> > +	minsz = offsetofend(struct kvm_posted_intr, count);
> > +
> > +	if (copy_from_user(&pi_info, (void __user *)argp, minsz))
> > +		return -EFAULT;
> > +
> > +	if (pi_info.argsz < minsz || pi_info.index >= VFIO_PCI_NUM_IRQS)
> > +		return -EINVAL;
> > +
> > +	vdev = kvm_vfio_get_vfio_device(pi_info.fd);
> > +	if (IS_ERR(vdev))
> > +		return PTR_ERR(vdev);
> > +
> > +	dev = kvm_vfio_external_base_device(vdev);
> > +	if (!dev)
> > +		return -EFAULT;
> 
> Missing put

Will do. Thanks!

> 
> > +
> > +	pdev = to_pci_dev(dev);
> 
> We can't assume the user called with a PCI device, test it.
> 
> if (!dev_is_pci(dev)) {
> 	put
> 	return errno
> }
> 
> pdev = to_pci_dev(dev);
> ...
> 
> > +
> > +	max = kvm_vfio_pci_get_irq_count(pdev, pi_info.index);
> > +
> > +	if (pi_info.argsz - minsz < pi_info.count * sizeof(int) ||
> > +	    pi_info.start >= max || pi_info.start + pi_info.count > max)
> > +		return -EINVAL;
> 
> Missing put
> 
> > +
> > +	virq = memdup_user((void __user *)((unsigned long)argp + minsz),
> > +			   pi_info.count * sizeof(int));
> > +	if (IS_ERR(virq))
> > +		return PTR_ERR(virq);
> 
> put...
> 
> > +
> > +	for (i=0; i<pi_info.count; i++) {
> > +		list_for_each_entry(entry, &pdev->msi_list, list) {
> 
> This is unique to MSI/X but INTx and others can still make it here.
> Also, this won't compile unless CONFIG_PCI_MSI.  Does anything protect
> this list?

We only support posting MSI/MSIx interrupt, so INTx and others cannot get here.

pdev->msi_list is created in pci_enable_msix() and destroyed in pci_disable_msix(),
there are no other places where this list is changed. So kernel doesn't use lock to
protect the accesses to this list, like many other places where this list is accessed
in the kernel code, I think it is safe to iterate the list here.

> 
> > +			if (entry->msi_attrib.entry_nr != pi_info.start+i)
> > +				continue;
> > +
> > +			ret = kvm_update_pi_irte(kdev->kvm,
> > +						 entry->irq, virq[i]);
> > +			if (ret) {
> > +				kfree(virq);
> > +				return -EFAULT;
> 
> put...
> 
> > +			}
> > +		}
> > +	}
> > +
> > +	kfree(virq);
> 
> put...
> 
> > +
> > +	return 0;
> > +}
> > +
> >  static int kvm_vfio_set_device(struct kvm_device *kdev, long attr, u64 arg)
> >  {
> >  	int32_t __user *argp = (int32_t __user *)(unsigned long)arg;
> > @@ -456,6 +565,9 @@ static int kvm_vfio_set_device(struct kvm_device
> *kdev, long attr, u64 arg)
> >  	case KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ:
> >  		ret = kvm_vfio_control_irq_forward(kdev, attr, argp);
> >  		break;
> > +	case KVM_DEV_VFIO_DEVICE_POSTING_IRQ:
> > +		ret = kvm_vfio_set_pi(kdev, argp);
> > +		break;
> 
> ARCH #ifdefs?
> 
> >  	default:
> >  		ret = -ENXIO;
> >  	}
> > @@ -511,6 +623,9 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
> >  		case KVM_DEV_VFIO_DEVICE_UNFORWARD_IRQ:
> >  			return 0;
> >  #endif
> > +		case KVM_DEV_VFIO_DEVICE_POSTING_IRQ:
> > +			return 0;
> > +
> 
> Same here
> 
> >  		}
> >  		break;
> >  	}
> 
> I only see setup where, what if we want to stop posted interrupts?  What
> if we switch to a different mode, for example the guest reboots or
> unloads the driver or switches to a different driver.  Thanks,

In fact, I don't think we need to stop the posted-interrupts. For setting
posted interrupts, we update the related IRTE according to the new
format. If the guest reboots, or unload the drivers, or some other
operations, the msi/msix will be disabled first, in this path, the irq
will be disabled the related IRTE is not used anymore. 

Thanks,
Feng

> 
> Alex