Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1502472 > unrolled thread

[PATCH v9 00/12] Add Mediated device support

Back to article view | Back to linux.kernel

Contents

  [PATCH v9 00/12] Add Mediated device support Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
    [PATCH v9 10/12] vfio: Add function to get device_api string from vfio_device_info.flags Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
      Re: [PATCH v9 10/12] vfio: Add function to get device_api string  from vfio_device_info.flags Alex Williamson <alex.williamson@redhat.com> - 2016-10-20 21:40 +0200
        Re: [PATCH v9 10/12] vfio: Add function to get device_api string from  vfio_device_info.flags Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-20 22:40 +0200
          Re: [PATCH v9 10/12] vfio: Add function to get device_api string  from vfio_device_info.flags Alex Williamson <alex.williamson@redhat.com> - 2016-10-20 23:10 +0200
            Re: [PATCH v9 10/12] vfio: Add function to get device_api string from  vfio_device_info.flags Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-20 23:20 +0200
              Re: [PATCH v9 10/12] vfio: Add function to get device_api string  from vfio_device_info.flags Alex Williamson <alex.williamson@redhat.com> - 2016-10-20 23:30 +0200
                Re: [PATCH v9 10/12] vfio: Add function to get device_api string from  vfio_device_info.flags Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-21 05:10 +0200
                  Re: [PATCH v9 10/12] vfio: Add function to get device_api string  from vfio_device_info.flags Alex Williamson <alex.williamson@redhat.com> - 2016-10-21 05:30 +0200
    [PATCH v9 02/12] vfio: VFIO based driver for Mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
      RE: [PATCH v9 02/12] vfio: VFIO based driver for Mediated devices "Tian, Kevin" <kevin.tian@intel.com> - 2016-10-26 09:00 +0200
        Re: [PATCH v9 02/12] vfio: VFIO based driver for Mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-26 17:10 +0200
    [PATCH v9 08/12] vfio_pci: Updated to use vfio_set_irqs_validate_and_prepare() Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
    [PATCH v9 11/12] docs: Add Documentation for Mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
      Re: [PATCH v9 11/12] docs: Add Documentation for Mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-10-25 18:20 +0200
    [PATCH v9 09/12] vfio_platform: Updated to use vfio_set_irqs_validate_and_prepare() Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
    [PATCH v9 04/12] vfio iommu: Add support for mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
      Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-10-19 23:10 +0200
        Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-20 22:20 +0200
          Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-10-24 04:40 +0200
            RE: [PATCH v9 04/12] vfio iommu: Add support for mediated devices "Tian, Kevin" <kevin.tian@intel.com> - 2016-10-26 09:30 +0200
              Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-26 17:10 +0200
        RE: [PATCH v9 04/12] vfio iommu: Add support for mediated devices "Tian, Kevin" <kevin.tian@intel.com> - 2016-10-26 10:00 +0200
          Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-10-26 17:30 +0200
        RE: [PATCH v9 04/12] vfio iommu: Add support for mediated devices "Tian, Kevin" <kevin.tian@intel.com> - 2016-10-26 10:00 +0200
          Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-10-26 17:20 +0200
      Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Jike Song <jike.song@intel.com> - 2016-10-21 10:00 +0200
        Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-10-21 16:40 +0200
          Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-24 12:40 +0200
      Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-10-27 09:30 +0200
        Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-27 16:00 +0200
          Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-10-27 16:40 +0200
            Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-27 18:00 +0200
          Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-10-28 04:40 +0200
            Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-01 15:10 +0100
              Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-02 02:30 +0100
                Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-02 04:40 +0100
                  Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-02 05:20 +0100
                    Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated  devices Jike Song <jike.song@intel.com> - 2016-11-02 13:30 +0100
                      Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-02 13:50 +0100
                        Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated  devices Jike Song <jike.song@intel.com> - 2016-11-02 14:10 +0100
                          Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-02 14:30 +0100
                            Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated  devices Jike Song <jike.song@intel.com> - 2016-11-02 14:40 +0100
                            Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for  mediated devices Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-03 05:40 +0100
    [PATCH v9 06/12] vfio_pci: Update vfio_pci to use vfio_info_add_capability() Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
      Re: [PATCH v9 06/12] vfio_pci: Update vfio_pci to use  vfio_info_add_capability() Alex Williamson <alex.williamson@redhat.com> - 2016-10-20 21:30 +0200
        Re: [PATCH v9 06/12] vfio_pci: Update vfio_pci to use  vfio_info_add_capability() Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-24 23:30 +0200
          Re: [PATCH v9 06/12] vfio_pci: Update vfio_pci to use  vfio_info_add_capability() Alex Williamson <alex.williamson@redhat.com> - 2016-10-24 23:40 +0200
    [PATCH v9 03/12] vfio: Rearrange functions to get vfio_group from dev Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
      Re: [PATCH v9 03/12] vfio: Rearrange functions to get vfio_group  from dev Alex Williamson <alex.williamson@redhat.com> - 2016-10-19 19:30 +0200
    [PATCH v9 05/12] vfio: Introduce common function to add capabilities Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
      Re: [PATCH v9 05/12] vfio: Introduce common function to add  capabilities Alex Williamson <alex.williamson@redhat.com> - 2016-10-20 21:30 +0200
        Re: [PATCH v9 05/12] vfio: Introduce common function to add  capabilities Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-24 23:30 +0200
          Re: [PATCH v9 05/12] vfio: Introduce common function to add  capabilities Alex Williamson <alex.williamson@redhat.com> - 2016-10-24 23:40 +0200
    [PATCH v9 01/12] vfio: Mediated device Core driver Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-17 23:30 +0200
      Re: [PATCH v9 01/12] vfio: Mediated device Core driver Alex Williamson <alex.williamson@redhat.com> - 2016-10-19 01:20 +0200
        Re: [PATCH v9 01/12] vfio: Mediated device Core driver Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-19 21:20 +0200
          Re: [PATCH v9 01/12] vfio: Mediated device Core driver Alex Williamson <alex.williamson@redhat.com> - 2016-10-20 00:30 +0200
      Re: [PATCH v9 01/12] vfio: Mediated device Core driver Jike Song <jike.song@intel.com> - 2016-10-20 09:30 +0200
        Re: [PATCH v9 01/12] vfio: Mediated device Core driver Alex Williamson <alex.williamson@redhat.com> - 2016-10-20 19:20 +0200
          Re: [PATCH v9 01/12] vfio: Mediated device Core driver Jike Song <jike.song@intel.com> - 2016-10-21 04:50 +0200
          Re: [PATCH v9 01/12] vfio: Mediated device Core driver Jike Song <jike.song@intel.com> - 2016-10-27 08:10 +0200
      RE: [PATCH v9 01/12] vfio: Mediated device Core driver "Tian, Kevin" <kevin.tian@intel.com> - 2016-10-26 09:00 +0200
        Re: [PATCH v9 01/12] vfio: Mediated device Core driver Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-26 17:00 +0200
    Re: [PATCH v9 00/12] Add Mediated device support Alex Williamson <alex.williamson@redhat.com> - 2016-10-17 23:50 +0200
    Re: [PATCH v9 12/12] docs: Sample driver to demonstrate how to use  Mediated device framework. Alex Williamson <alex.williamson@redhat.com> - 2016-10-18 19:20 +0200
      Re: [PATCH v9 12/12] docs: Sample driver to demonstrate how to use  Mediated device framework. Kirti Wankhede <kwankhede@nvidia.com> - 2016-10-19 21:20 +0200
    Re: [PATCH v9 00/12] Add Mediated device support Jike Song <jike.song@intel.com> - 2016-10-24 09:20 +0200

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

#1508869 — RE: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Monday, October 24, 2016 10:32 AM
> 
> > >> -static long vfio_unpin_pages(unsigned long pfn, long npage,
> > >> -			     int prot, bool do_accounting)
> > >> +static long __vfio_unpin_pages_remote(struct vfio_iommu *iommu,
> > >> +				      unsigned long pfn, long npage, int prot,
> > >> +				      bool do_accounting)
> > >
> > > Have you noticed that it's kind of confusing that
> > > __vfio_{un}pin_pages_remote() uses current, which does a
> > > get_user_pages_fast() while "local" uses a provided task_struct and
> > > uses get_user_pages_*remote*()?  And also what was effectively local
> > > (ie. we're pinning for our own use here) is now "remote" and pinning
> > > for a remote, vendor driver consumer, is now "local".  It's not very
> > > intuitive.
> > >

I questioned this confusing naming in v8 too...

> >
> > 'local' in local_domain was suggested to describe the domain for local
> > page tracking. Earlier suggestions to have 'mdev' or 'noimmu' in this
> > name were discarded. May be we should revisit what the name should be.
> > Any suggestion?
> >
> > For local_domain, to pin pages, flow is:
> >
> > for local_domain
> >     |- vfio_pin_pages()
> >         |- vfio_iommu_type1_pin_pages()
> >             |- __vfio_pin_page_local()
> >                 |-  vaddr_get_pfn(task->mm)
> >                     |- get_user_pages_remote()
> >
> > __vfio_pin_page_local() --> get_user_pages_remote()
> 
> 
> In vfio.c we have the concept of an external user, perhaps that could
> be continued here.  An mdev driver would be an external, or remote
> pinning.
> 

I prefer to use remote here. It's aligned with underlying mm operations

Thanks
Kevin

[toc] | [prev] | [next] | [standalone]

#1509516 — Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 10/26/2016 12:49 PM, Tian, Kevin wrote:
>> From: Alex Williamson [mailto:alex.williamson@redhat.com]
>> Sent: Monday, October 24, 2016 10:32 AM
>>
>>>>> -static long vfio_unpin_pages(unsigned long pfn, long npage,
>>>>> -			     int prot, bool do_accounting)
>>>>> +static long __vfio_unpin_pages_remote(struct vfio_iommu *iommu,
>>>>> +				      unsigned long pfn, long npage, int prot,
>>>>> +				      bool do_accounting)
>>>>
>>>> Have you noticed that it's kind of confusing that
>>>> __vfio_{un}pin_pages_remote() uses current, which does a
>>>> get_user_pages_fast() while "local" uses a provided task_struct and
>>>> uses get_user_pages_*remote*()?  And also what was effectively local
>>>> (ie. we're pinning for our own use here) is now "remote" and pinning
>>>> for a remote, vendor driver consumer, is now "local".  It's not very
>>>> intuitive.
>>>>
> 
> I questioned this confusing naming in v8 too...
> 

I do tried to address your concerns on v8.

>>>
>>> 'local' in local_domain was suggested to describe the domain for local
>>> page tracking. Earlier suggestions to have 'mdev' or 'noimmu' in this
>>> name were discarded. May be we should revisit what the name should be.
>>> Any suggestion?
>>>
>>> For local_domain, to pin pages, flow is:
>>>
>>> for local_domain
>>>     |- vfio_pin_pages()
>>>         |- vfio_iommu_type1_pin_pages()
>>>             |- __vfio_pin_page_local()
>>>                 |-  vaddr_get_pfn(task->mm)
>>>                     |- get_user_pages_remote()
>>>
>>> __vfio_pin_page_local() --> get_user_pages_remote()
>>
>>
>> In vfio.c we have the concept of an external user, perhaps that could
>> be continued here.  An mdev driver would be an external, or remote
>> pinning.
>>
> 
> I prefer to use remote here. It's aligned with underlying mm operations
> 

Using 'remote' in this case is also confusing since it is already used
in this file. I liked Alex's suggestion to use external and I'll have
those changed in next version of patch set.

Kirti

[toc] | [prev] | [next] | [standalone]

#1508891 — RE: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

> From: Tian, Kevin
> Sent: Wednesday, October 26, 2016 3:54 PM
> 
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Thursday, October 20, 2016 5:03 AM
> > > @@ -83,6 +92,21 @@ struct vfio_group {
> > >  };
> > >
> > >  /*
> > > + * Guest RAM pinning working set or DMA target
> > > + */
> > > +struct vfio_pfn {
> > > +	struct rb_node		node;
> > > +	unsigned long		vaddr;		/* virtual addr */
> > > +	dma_addr_t		iova;		/* IOVA */
> > > +	unsigned long		pfn;		/* Host pfn */
> > > +	int			prot;
> > > +	atomic_t		ref_count;
> > > +};
> >
> > Somehow we're going to need to fit an invalidation callback here too.
> > How would we handle a case where there are multiple mdev devices, from
> > different vendor drivers, that all have the same pfn pinned?  I'm
> > already concerned about the per pfn overhead we're introducing here so
> > clearly we cannot store an invalidation callback per pinned page, per
> > vendor driver.  Perhaps invalidations should be done using a notifier
> > chain per vfio_iommu, the vendor drivers are required to register on
> > that chain (fail pinning with empty notifier list) user unmapping
> > will be broadcast to the notifier chain, the vendor driver will be
> > responsible for deciding if each unmap is relevant to them (potentially
> > it's for a pinning from another driver).
> >
> > I expect we also need to enforce that vendors perform a synchronous
> > unmap such that after returning from the notifier list call, the
> > vfio_pfn should no longer exist.  If it does we might need to BUG_ON.
> > Also be careful to pay attention to the locking of the notifier vs
> > unpin callbacks to avoid deadlocks.
> >
> 
> What about just requesting vendor driver to provide a callback in parent
> device ops?
> 
> Curious in which scenario the user application (say Qemu here) may
> unmap memory pages which are still pinned by vendor driver... Is it
> purely about a corner case which we want to handle elegantly?
> 
> If yes, possibly a simpler way is to force destroying mdev instead of
> asking vendor driver to take care of each invalidation request under
> such situation. Since anyway the mdev device won't be in an usable
> state anymore... (sorry if I missed the key problem here.)
> 

or calling reset callback of parent device driver, if we don't want to
break libvirt's expectation by blindly removing mdev device...

[toc] | [prev] | [next] | [standalone]

#1509528 — Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On Wed, 26 Oct 2016 07:54:56 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Tian, Kevin
> > Sent: Wednesday, October 26, 2016 3:54 PM
> >   
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Thursday, October 20, 2016 5:03 AM  
> > > > @@ -83,6 +92,21 @@ struct vfio_group {
> > > >  };
> > > >
> > > >  /*
> > > > + * Guest RAM pinning working set or DMA target
> > > > + */
> > > > +struct vfio_pfn {
> > > > +	struct rb_node		node;
> > > > +	unsigned long		vaddr;		/* virtual addr */
> > > > +	dma_addr_t		iova;		/* IOVA */
> > > > +	unsigned long		pfn;		/* Host pfn */
> > > > +	int			prot;
> > > > +	atomic_t		ref_count;
> > > > +};  
> > >
> > > Somehow we're going to need to fit an invalidation callback here too.
> > > How would we handle a case where there are multiple mdev devices, from
> > > different vendor drivers, that all have the same pfn pinned?  I'm
> > > already concerned about the per pfn overhead we're introducing here so
> > > clearly we cannot store an invalidation callback per pinned page, per
> > > vendor driver.  Perhaps invalidations should be done using a notifier
> > > chain per vfio_iommu, the vendor drivers are required to register on
> > > that chain (fail pinning with empty notifier list) user unmapping
> > > will be broadcast to the notifier chain, the vendor driver will be
> > > responsible for deciding if each unmap is relevant to them (potentially
> > > it's for a pinning from another driver).
> > >
> > > I expect we also need to enforce that vendors perform a synchronous
> > > unmap such that after returning from the notifier list call, the
> > > vfio_pfn should no longer exist.  If it does we might need to BUG_ON.
> > > Also be careful to pay attention to the locking of the notifier vs
> > > unpin callbacks to avoid deadlocks.
> > >  
> > 
> > What about just requesting vendor driver to provide a callback in parent
> > device ops?
> > 
> > Curious in which scenario the user application (say Qemu here) may
> > unmap memory pages which are still pinned by vendor driver... Is it
> > purely about a corner case which we want to handle elegantly?
> > 
> > If yes, possibly a simpler way is to force destroying mdev instead of
> > asking vendor driver to take care of each invalidation request under
> > such situation. Since anyway the mdev device won't be in an usable
> > state anymore... (sorry if I missed the key problem here.)
> >   
> 
> or calling reset callback of parent device driver, if we don't want to
> break libvirt's expectation by blindly removing mdev device...

I think we're going off into the weeds here.  mdev devices need to
honor the existing API, therefore an unmap should result in preventing
the device from further access to the unmapped pages, nothing more,
nothing less.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1508893 — RE: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Thursday, October 20, 2016 5:03 AM
> > @@ -83,6 +92,21 @@ struct vfio_group {
> >  };
> >
> >  /*
> > + * Guest RAM pinning working set or DMA target
> > + */
> > +struct vfio_pfn {
> > +	struct rb_node		node;
> > +	unsigned long		vaddr;		/* virtual addr */
> > +	dma_addr_t		iova;		/* IOVA */
> > +	unsigned long		pfn;		/* Host pfn */
> > +	int			prot;
> > +	atomic_t		ref_count;
> > +};
> 
> Somehow we're going to need to fit an invalidation callback here too.
> How would we handle a case where there are multiple mdev devices, from
> different vendor drivers, that all have the same pfn pinned?  I'm
> already concerned about the per pfn overhead we're introducing here so
> clearly we cannot store an invalidation callback per pinned page, per
> vendor driver.  Perhaps invalidations should be done using a notifier
> chain per vfio_iommu, the vendor drivers are required to register on
> that chain (fail pinning with empty notifier list) user unmapping
> will be broadcast to the notifier chain, the vendor driver will be
> responsible for deciding if each unmap is relevant to them (potentially
> it's for a pinning from another driver).
> 
> I expect we also need to enforce that vendors perform a synchronous
> unmap such that after returning from the notifier list call, the
> vfio_pfn should no longer exist.  If it does we might need to BUG_ON.
> Also be careful to pay attention to the locking of the notifier vs
> unpin callbacks to avoid deadlocks.
> 

What about just requesting vendor driver to provide a callback in parent 
device ops? 

Curious in which scenario the user application (say Qemu here) may 
unmap memory pages which are still pinned by vendor driver... Is it 
purely about a corner case which we want to handle elegantly? 

If yes, possibly a simpler way is to force destroying mdev instead of 
asking vendor driver to take care of each invalidation request under
such situation. Since anyway the mdev device won't be in an usable
state anymore... (sorry if I missed the key problem here.)

Thanks
Kevin

[toc] | [prev] | [next] | [standalone]

#1509526 — Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On Wed, 26 Oct 2016 07:53:43 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Thursday, October 20, 2016 5:03 AM  
> > > @@ -83,6 +92,21 @@ struct vfio_group {
> > >  };
> > >
> > >  /*
> > > + * Guest RAM pinning working set or DMA target
> > > + */
> > > +struct vfio_pfn {
> > > +	struct rb_node		node;
> > > +	unsigned long		vaddr;		/* virtual addr */
> > > +	dma_addr_t		iova;		/* IOVA */
> > > +	unsigned long		pfn;		/* Host pfn */
> > > +	int			prot;
> > > +	atomic_t		ref_count;
> > > +};  
> > 
> > Somehow we're going to need to fit an invalidation callback here too.
> > How would we handle a case where there are multiple mdev devices, from
> > different vendor drivers, that all have the same pfn pinned?  I'm
> > already concerned about the per pfn overhead we're introducing here so
> > clearly we cannot store an invalidation callback per pinned page, per
> > vendor driver.  Perhaps invalidations should be done using a notifier
> > chain per vfio_iommu, the vendor drivers are required to register on
> > that chain (fail pinning with empty notifier list) user unmapping
> > will be broadcast to the notifier chain, the vendor driver will be
> > responsible for deciding if each unmap is relevant to them (potentially
> > it's for a pinning from another driver).
> > 
> > I expect we also need to enforce that vendors perform a synchronous
> > unmap such that after returning from the notifier list call, the
> > vfio_pfn should no longer exist.  If it does we might need to BUG_ON.
> > Also be careful to pay attention to the locking of the notifier vs
> > unpin callbacks to avoid deadlocks.
> >   
> 
> What about just requesting vendor driver to provide a callback in parent 
> device ops?

How does the iommu driver get to the mdev vendor driver callback?  We
can also have pages pinned by multiple vendor drivers, I don't think
we want the additional overhead of a per page list of invalidation
callbacks.
 
> Curious in which scenario the user application (say Qemu here) may 
> unmap memory pages which are still pinned by vendor driver... Is it 
> purely about a corner case which we want to handle elegantly? 

The vfio type1 iommu API provides a MAP and UNMAP interface.  The unmap
call is expected to work regardless of how it might inhibit the device
from working.  This is currently true of iommu protected devices today,
a user can unmap pages which might be DMA targets for the device and
the iommu prevents further access to those pages, possibly at the
expense of device operation.  We cannot support an interface where a
user can unmap a set of pages and map in new pages to replace them when
the vendor driver might be caching stale mappings.

In normal VM operation perhaps this is a corner case, but the API is
not defined only for the normal and expected behavior of a VM.
 
> If yes, possibly a simpler way is to force destroying mdev instead of 
> asking vendor driver to take care of each invalidation request under
> such situation. Since anyway the mdev device won't be in an usable
> state anymore... (sorry if I missed the key problem here.)

That's a pretty harsh response for an operation which is completely
valid from an API perspective.  What if the VM does an unmap of all
memory around reset?  We cannot guarantee that the guest driver will
have a chance to do cleanup, the guest may have crashed or a
system_reset invoked.  Would you have the mdev destroyed in this case?
How could QEMU, which has no device specific driver to know that vendor
pinnings are present, recover from this?  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1505541 — Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 10/18/2016 05:22 AM, Kirti Wankhede wrote:
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 2ba19424e4a1..5d67058a611d 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
[snip]
>  static int vfio_iommu_type1_attach_group(void *iommu_data,
>  					 struct iommu_group *iommu_group)
>  {
>  	struct vfio_iommu *iommu = iommu_data;
> -	struct vfio_group *group, *g;
> +	struct vfio_group *group;
>  	struct vfio_domain *domain, *d;
>  	struct bus_type *bus = NULL;
>  	int ret;
> @@ -746,10 +1136,14 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	mutex_lock(&iommu->lock);
>  
>  	list_for_each_entry(d, &iommu->domain_list, next) {
> -		list_for_each_entry(g, &d->group_list, next) {
> -			if (g->iommu_group != iommu_group)
> -				continue;
> +		if (find_iommu_group(d, iommu_group)) {
> +			mutex_unlock(&iommu->lock);
> +			return -EINVAL;
> +		}
> +	}
>  
> +	if (iommu->local_domain) {
> +		if (find_iommu_group(iommu->local_domain, iommu_group)) {
>  			mutex_unlock(&iommu->lock);
>  			return -EINVAL;
>  		}
> @@ -769,6 +1163,30 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	if (ret)
>  		goto out_free;
>  
> +	if (IS_ENABLED(CONFIG_VFIO_MDEV) && !iommu_present(bus) &&
> +	    (bus == &mdev_bus_type)) {

Hi Kirti,

By refering mdev_bus_type directly you are making vfio_iommu_type1.ko depends
on mdev.ko, but in Kconfig doesn't guarantee the dependency. For example,
if CONFIG_VFIO_IOMMU_TYPE1=y and CONFIG_VFIO_MDEV=m, the building will fail.


--
Thanks,
Jike

[toc] | [prev] | [next] | [standalone]

#1506004 — Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On Fri, 21 Oct 2016 15:49:07 +0800
Jike Song <jike.song@intel.com> wrote:

> On 10/18/2016 05:22 AM, Kirti Wankhede wrote:
> > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > index 2ba19424e4a1..5d67058a611d 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c  
> [snip]
> >  static int vfio_iommu_type1_attach_group(void *iommu_data,
> >  					 struct iommu_group *iommu_group)
> >  {
> >  	struct vfio_iommu *iommu = iommu_data;
> > -	struct vfio_group *group, *g;
> > +	struct vfio_group *group;
> >  	struct vfio_domain *domain, *d;
> >  	struct bus_type *bus = NULL;
> >  	int ret;
> > @@ -746,10 +1136,14 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> >  	mutex_lock(&iommu->lock);
> >  
> >  	list_for_each_entry(d, &iommu->domain_list, next) {
> > -		list_for_each_entry(g, &d->group_list, next) {
> > -			if (g->iommu_group != iommu_group)
> > -				continue;
> > +		if (find_iommu_group(d, iommu_group)) {
> > +			mutex_unlock(&iommu->lock);
> > +			return -EINVAL;
> > +		}
> > +	}
> >  
> > +	if (iommu->local_domain) {
> > +		if (find_iommu_group(iommu->local_domain, iommu_group)) {
> >  			mutex_unlock(&iommu->lock);
> >  			return -EINVAL;
> >  		}
> > @@ -769,6 +1163,30 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> >  	if (ret)
> >  		goto out_free;
> >  
> > +	if (IS_ENABLED(CONFIG_VFIO_MDEV) && !iommu_present(bus) &&
> > +	    (bus == &mdev_bus_type)) {  
> 
> Hi Kirti,
> 
> By refering mdev_bus_type directly you are making vfio_iommu_type1.ko depends
> on mdev.ko, but in Kconfig doesn't guarantee the dependency. For example,
> if CONFIG_VFIO_IOMMU_TYPE1=y and CONFIG_VFIO_MDEV=m, the building will fail.

Good point, Jike.  I don't think we want to make existing vfio modules
dependent on mdev modules.  I wonder if we can lookup the mdev_bus_type
symbol w/o triggering the module load.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1507082 — Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 10/21/2016 8:06 PM, Alex Williamson wrote:
> On Fri, 21 Oct 2016 15:49:07 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> On 10/18/2016 05:22 AM, Kirti Wankhede wrote:
>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>> index 2ba19424e4a1..5d67058a611d 100644
>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>> +++ b/drivers/vfio/vfio_iommu_type1.c  
>> [snip]
>>>  static int vfio_iommu_type1_attach_group(void *iommu_data,
>>>  					 struct iommu_group *iommu_group)
>>>  {
>>>  	struct vfio_iommu *iommu = iommu_data;
>>> -	struct vfio_group *group, *g;
>>> +	struct vfio_group *group;
>>>  	struct vfio_domain *domain, *d;
>>>  	struct bus_type *bus = NULL;
>>>  	int ret;
>>> @@ -746,10 +1136,14 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>>  	mutex_lock(&iommu->lock);
>>>  
>>>  	list_for_each_entry(d, &iommu->domain_list, next) {
>>> -		list_for_each_entry(g, &d->group_list, next) {
>>> -			if (g->iommu_group != iommu_group)
>>> -				continue;
>>> +		if (find_iommu_group(d, iommu_group)) {
>>> +			mutex_unlock(&iommu->lock);
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>>  
>>> +	if (iommu->local_domain) {
>>> +		if (find_iommu_group(iommu->local_domain, iommu_group)) {
>>>  			mutex_unlock(&iommu->lock);
>>>  			return -EINVAL;
>>>  		}
>>> @@ -769,6 +1163,30 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>>  	if (ret)
>>>  		goto out_free;
>>>  
>>> +	if (IS_ENABLED(CONFIG_VFIO_MDEV) && !iommu_present(bus) &&
>>> +	    (bus == &mdev_bus_type)) {  
>>
>> Hi Kirti,
>>
>> By refering mdev_bus_type directly you are making vfio_iommu_type1.ko depends
>> on mdev.ko, but in Kconfig doesn't guarantee the dependency. For example,
>> if CONFIG_VFIO_IOMMU_TYPE1=y and CONFIG_VFIO_MDEV=m, the building will fail.
> 
> Good point, Jike.  I don't think we want to make existing vfio modules
> dependent on mdev modules.  I wonder if we can lookup the mdev_bus_type
> symbol w/o triggering the module load.  Thanks,
> 

Ok. Modifying the check as below works in above case:

        mdev_bus = symbol_get(mdev_bus_type);

        if (mdev_bus && (bus == mdev_bus) && !iommu_present(bus) ) {
                symbol_put(mdev_bus_type);
                ...
        }

Kirti

[toc] | [prev] | [next] | [standalone]

#1510114 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 18/10/16 08:22, Kirti Wankhede wrote:
> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
> Mediated device only uses IOMMU APIs, the underlying hardware can be
> managed by an IOMMU domain.
> 
> Aim of this change is:
> - To use most of the code of TYPE1 IOMMU driver for mediated devices
> - To support direct assigned device and mediated device in single module
> 
> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
> IOMMU module that supports pining and unpinning pages for mdev devices
> should provide these functions.
> Added APIs for pining and unpining pages to VFIO module. These calls back
> into backend iommu module to actually pin and unpin pages.
> 
> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
> backend module. More details:
> - When iommu_group of mediated devices is attached, task structure is
>   cached which is used later to pin pages and page accounting.


For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
using @current or task as the process might be gone while VFIO container is
still alive and @mm might be needed to do proper cleanup; this might not be
an issue with this patchset now but still you seem to only use @mm from
task_struct.



> - It keeps track of pinned pages for mediated domain. This data is used to
>   verify unpinning request and to unpin remaining pages while detaching, if
>   there are any.
> - Used existing mechanism for page accounting. If iommu capable domain
>   exist in the container then all pages are already pinned and accounted.
>   Accouting for mdev device is only done if there is no iommu capable
>   domain in the container.
> - Page accouting is updated on hot plug and unplug mdev device and pass
>   through device.
> 
> Tested by assigning below combinations of devices to a single VM:
> - GPU pass through only
> - vGPU device only
> - One GPU pass through and one vGPU device
> - Linux VM hot plug and unplug vGPU device while GPU pass through device
>   exist
> - Linux VM hot plug and unplug GPU pass through device while vGPU device
>   exist
> 
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Neo Jia <cjia@nvidia.com>
> Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a


-- 
Alexey

[toc] | [prev] | [next] | [standalone]

#1510195 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
> On 18/10/16 08:22, Kirti Wankhede wrote:
>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>> managed by an IOMMU domain.
>>
>> Aim of this change is:
>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>> - To support direct assigned device and mediated device in single module
>>
>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>> IOMMU module that supports pining and unpinning pages for mdev devices
>> should provide these functions.
>> Added APIs for pining and unpining pages to VFIO module. These calls back
>> into backend iommu module to actually pin and unpin pages.
>>
>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>> backend module. More details:
>> - When iommu_group of mediated devices is attached, task structure is
>>   cached which is used later to pin pages and page accounting.
> 
> 
> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
> using @current or task as the process might be gone while VFIO container is
> still alive and @mm might be needed to do proper cleanup; this might not be
> an issue with this patchset now but still you seem to only use @mm from
> task_struct.
> 

Consider the example of QEMU process which creates VFIO container, QEMU
in its teardown path would release the container. How could container be
alive when process is gone?

Kirti

> 
> 
>> - It keeps track of pinned pages for mediated domain. This data is used to
>>   verify unpinning request and to unpin remaining pages while detaching, if
>>   there are any.
>> - Used existing mechanism for page accounting. If iommu capable domain
>>   exist in the container then all pages are already pinned and accounted.
>>   Accouting for mdev device is only done if there is no iommu capable
>>   domain in the container.
>> - Page accouting is updated on hot plug and unplug mdev device and pass
>>   through device.
>>
>> Tested by assigning below combinations of devices to a single VM:
>> - GPU pass through only
>> - vGPU device only
>> - One GPU pass through and one vGPU device
>> - Linux VM hot plug and unplug vGPU device while GPU pass through device
>>   exist
>> - Linux VM hot plug and unplug GPU pass through device while vGPU device
>>   exist
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>> Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a
> 
> 

[toc] | [prev] | [next] | [standalone]

#1510307 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On Thu, 27 Oct 2016 18:01:51 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
> > On 18/10/16 08:22, Kirti Wankhede wrote:  
> >> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
> >> Mediated device only uses IOMMU APIs, the underlying hardware can be
> >> managed by an IOMMU domain.
> >>
> >> Aim of this change is:
> >> - To use most of the code of TYPE1 IOMMU driver for mediated devices
> >> - To support direct assigned device and mediated device in single module
> >>
> >> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
> >> IOMMU module that supports pining and unpinning pages for mdev devices
> >> should provide these functions.
> >> Added APIs for pining and unpining pages to VFIO module. These calls back
> >> into backend iommu module to actually pin and unpin pages.
> >>
> >> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
> >> backend module. More details:
> >> - When iommu_group of mediated devices is attached, task structure is
> >>   cached which is used later to pin pages and page accounting.  
> > 
> > 
> > For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
> > atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
> > using @current or task as the process might be gone while VFIO container is
> > still alive and @mm might be needed to do proper cleanup; this might not be
> > an issue with this patchset now but still you seem to only use @mm from
> > task_struct.
> >   
> 
> Consider the example of QEMU process which creates VFIO container, QEMU
> in its teardown path would release the container. How could container be
> alive when process is gone?

If QEMU is sent a SIGKILL, does the process still exist?  We must be
able to perform cleanup regardless of the state, or existence, of the
task that created it.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1510426 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 10/27/2016 8:00 PM, Alex Williamson wrote:
> On Thu, 27 Oct 2016 18:01:51 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
>>> On 18/10/16 08:22, Kirti Wankhede wrote:  
>>>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>>>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>>>> managed by an IOMMU domain.
>>>>
>>>> Aim of this change is:
>>>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>>>> - To support direct assigned device and mediated device in single module
>>>>
>>>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>>>> IOMMU module that supports pining and unpinning pages for mdev devices
>>>> should provide these functions.
>>>> Added APIs for pining and unpining pages to VFIO module. These calls back
>>>> into backend iommu module to actually pin and unpin pages.
>>>>
>>>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>>>> backend module. More details:
>>>> - When iommu_group of mediated devices is attached, task structure is
>>>>   cached which is used later to pin pages and page accounting.  
>>>
>>>
>>> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
>>> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
>>> using @current or task as the process might be gone while VFIO container is
>>> still alive and @mm might be needed to do proper cleanup; this might not be
>>> an issue with this patchset now but still you seem to only use @mm from
>>> task_struct.
>>>   
>>
>> Consider the example of QEMU process which creates VFIO container, QEMU
>> in its teardown path would release the container. How could container be
>> alive when process is gone?
> 
> If QEMU is sent a SIGKILL, does the process still exist?  We must be
> able to perform cleanup regardless of the state, or existence, of the
> task that created it.  Thanks,
> 

The kernel closes all open file descriptors when any process is
terminated, so .release() from struct vfio_iommu_driver_ops gets called
on SIGKILL or SIGTERM and release() function do all cleanup.

Kirti

[toc] | [prev] | [next] | [standalone]

#1510820 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 27/10/16 23:31, Kirti Wankhede wrote:
> 
> 
> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
>> On 18/10/16 08:22, Kirti Wankhede wrote:
>>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>>> managed by an IOMMU domain.
>>>
>>> Aim of this change is:
>>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>>> - To support direct assigned device and mediated device in single module
>>>
>>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>>> IOMMU module that supports pining and unpinning pages for mdev devices
>>> should provide these functions.
>>> Added APIs for pining and unpining pages to VFIO module. These calls back
>>> into backend iommu module to actually pin and unpin pages.
>>>
>>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>>> backend module. More details:
>>> - When iommu_group of mediated devices is attached, task structure is
>>>   cached which is used later to pin pages and page accounting.
>>
>>
>> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
>> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
>> using @current or task as the process might be gone while VFIO container is
>> still alive and @mm might be needed to do proper cleanup; this might not be
>> an issue with this patchset now but still you seem to only use @mm from
>> task_struct.
>>
> 
> Consider the example of QEMU process which creates VFIO container, QEMU
> in its teardown path would release the container. How could container be
> alive when process is gone?

do_exit() in kernel/exit.c calls exit_mm() (which sets NULL to tsk->mm)
first, and then releases open files by calling  exit_files(). So
container's release() does not have current->mm.



> 
> Kirti
> 
>>
>>
>>> - It keeps track of pinned pages for mediated domain. This data is used to
>>>   verify unpinning request and to unpin remaining pages while detaching, if
>>>   there are any.
>>> - Used existing mechanism for page accounting. If iommu capable domain
>>>   exist in the container then all pages are already pinned and accounted.
>>>   Accouting for mdev device is only done if there is no iommu capable
>>>   domain in the container.
>>> - Page accouting is updated on hot plug and unplug mdev device and pass
>>>   through device.
>>>
>>> Tested by assigning below combinations of devices to a single VM:
>>> - GPU pass through only
>>> - vGPU device only
>>> - One GPU pass through and one vGPU device
>>> - Linux VM hot plug and unplug vGPU device while GPU pass through device
>>>   exist
>>> - Linux VM hot plug and unplug GPU pass through device while vGPU device
>>>   exist
>>>
>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>>> Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a
>>
>>


-- 
Alexey

[toc] | [prev] | [next] | [standalone]

#1513280 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 10/28/2016 7:48 AM, Alexey Kardashevskiy wrote:
> On 27/10/16 23:31, Kirti Wankhede wrote:
>>
>>
>> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
>>> On 18/10/16 08:22, Kirti Wankhede wrote:
>>>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>>>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>>>> managed by an IOMMU domain.
>>>>
>>>> Aim of this change is:
>>>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>>>> - To support direct assigned device and mediated device in single module
>>>>
>>>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>>>> IOMMU module that supports pining and unpinning pages for mdev devices
>>>> should provide these functions.
>>>> Added APIs for pining and unpining pages to VFIO module. These calls back
>>>> into backend iommu module to actually pin and unpin pages.
>>>>
>>>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>>>> backend module. More details:
>>>> - When iommu_group of mediated devices is attached, task structure is
>>>>   cached which is used later to pin pages and page accounting.
>>>
>>>
>>> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
>>> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
>>> using @current or task as the process might be gone while VFIO container is
>>> still alive and @mm might be needed to do proper cleanup; this might not be
>>> an issue with this patchset now but still you seem to only use @mm from
>>> task_struct.
>>>
>>
>> Consider the example of QEMU process which creates VFIO container, QEMU
>> in its teardown path would release the container. How could container be
>> alive when process is gone?
> 
> do_exit() in kernel/exit.c calls exit_mm() (which sets NULL to tsk->mm)
> first, and then releases open files by calling  exit_files(). So
> container's release() does not have current->mm.
> 

Incrementing usage count (get_task_struct()) while saving task structure
and decementing it (put_task_struct()) from release() should  work here.
Updating the patch.

Thanks,
Kirti

[toc] | [prev] | [next] | [standalone]

#1513608 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 02/11/16 01:01, Kirti Wankhede wrote:
> 
> 
> On 10/28/2016 7:48 AM, Alexey Kardashevskiy wrote:
>> On 27/10/16 23:31, Kirti Wankhede wrote:
>>>
>>>
>>> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
>>>> On 18/10/16 08:22, Kirti Wankhede wrote:
>>>>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>>>>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>>>>> managed by an IOMMU domain.
>>>>>
>>>>> Aim of this change is:
>>>>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>>>>> - To support direct assigned device and mediated device in single module
>>>>>
>>>>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>>>>> IOMMU module that supports pining and unpinning pages for mdev devices
>>>>> should provide these functions.
>>>>> Added APIs for pining and unpining pages to VFIO module. These calls back
>>>>> into backend iommu module to actually pin and unpin pages.
>>>>>
>>>>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>>>>> backend module. More details:
>>>>> - When iommu_group of mediated devices is attached, task structure is
>>>>>   cached which is used later to pin pages and page accounting.
>>>>
>>>>
>>>> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
>>>> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
>>>> using @current or task as the process might be gone while VFIO container is
>>>> still alive and @mm might be needed to do proper cleanup; this might not be
>>>> an issue with this patchset now but still you seem to only use @mm from
>>>> task_struct.
>>>>
>>>
>>> Consider the example of QEMU process which creates VFIO container, QEMU
>>> in its teardown path would release the container. How could container be
>>> alive when process is gone?
>>
>> do_exit() in kernel/exit.c calls exit_mm() (which sets NULL to tsk->mm)
>> first, and then releases open files by calling  exit_files(). So
>> container's release() does not have current->mm.
>>
> 
> Incrementing usage count (get_task_struct()) while saving task structure
> and decementing it (put_task_struct()) from release() should  work here.
> Updating the patch.

I cannot see how the task->usage counter prevents do_exit() from performing
the exit, can you?



-- 
Alexey

[toc] | [prev] | [next] | [standalone]

#1513636 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 11/2/2016 6:54 AM, Alexey Kardashevskiy wrote:
> On 02/11/16 01:01, Kirti Wankhede wrote:
>>
>>
>> On 10/28/2016 7:48 AM, Alexey Kardashevskiy wrote:
>>> On 27/10/16 23:31, Kirti Wankhede wrote:
>>>>
>>>>
>>>> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
>>>>> On 18/10/16 08:22, Kirti Wankhede wrote:
>>>>>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>>>>>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>>>>>> managed by an IOMMU domain.
>>>>>>
>>>>>> Aim of this change is:
>>>>>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>>>>>> - To support direct assigned device and mediated device in single module
>>>>>>
>>>>>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>>>>>> IOMMU module that supports pining and unpinning pages for mdev devices
>>>>>> should provide these functions.
>>>>>> Added APIs for pining and unpining pages to VFIO module. These calls back
>>>>>> into backend iommu module to actually pin and unpin pages.
>>>>>>
>>>>>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>>>>>> backend module. More details:
>>>>>> - When iommu_group of mediated devices is attached, task structure is
>>>>>>   cached which is used later to pin pages and page accounting.
>>>>>
>>>>>
>>>>> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
>>>>> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
>>>>> using @current or task as the process might be gone while VFIO container is
>>>>> still alive and @mm might be needed to do proper cleanup; this might not be
>>>>> an issue with this patchset now but still you seem to only use @mm from
>>>>> task_struct.
>>>>>
>>>>
>>>> Consider the example of QEMU process which creates VFIO container, QEMU
>>>> in its teardown path would release the container. How could container be
>>>> alive when process is gone?
>>>
>>> do_exit() in kernel/exit.c calls exit_mm() (which sets NULL to tsk->mm)
>>> first, and then releases open files by calling  exit_files(). So
>>> container's release() does not have current->mm.
>>>
>>
>> Incrementing usage count (get_task_struct()) while saving task structure
>> and decementing it (put_task_struct()) from release() should  work here.
>> Updating the patch.
> 
> I cannot see how the task->usage counter prevents do_exit() from performing
> the exit, can you?
> 

It will not prevent exit from do_exit(), but that will make sure that we
don't have stale pointer of task structure. Then we can check whether
the task is alive and get mm pointer in teardown path as below:

{
        struct task_struct *task = domain->external_addr_space->task;
        struct mm_struct *mm = NULL;

        put_pfn(pfn, prot);

        if (pid_alive(task))
                mm = get_task_mm(task);

        if (mm) {
                if (do_accounting)
                        vfio_lock_acct(task, -1);

                mmput(mm);
        }
}

Thanks,
Kirti

[toc] | [prev] | [next] | [standalone]

#1513640 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 02/11/16 14:29, Kirti Wankhede wrote:
> 
> 
> On 11/2/2016 6:54 AM, Alexey Kardashevskiy wrote:
>> On 02/11/16 01:01, Kirti Wankhede wrote:
>>>
>>>
>>> On 10/28/2016 7:48 AM, Alexey Kardashevskiy wrote:
>>>> On 27/10/16 23:31, Kirti Wankhede wrote:
>>>>>
>>>>>
>>>>> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
>>>>>> On 18/10/16 08:22, Kirti Wankhede wrote:
>>>>>>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>>>>>>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>>>>>>> managed by an IOMMU domain.
>>>>>>>
>>>>>>> Aim of this change is:
>>>>>>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>>>>>>> - To support direct assigned device and mediated device in single module
>>>>>>>
>>>>>>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>>>>>>> IOMMU module that supports pining and unpinning pages for mdev devices
>>>>>>> should provide these functions.
>>>>>>> Added APIs for pining and unpining pages to VFIO module. These calls back
>>>>>>> into backend iommu module to actually pin and unpin pages.
>>>>>>>
>>>>>>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>>>>>>> backend module. More details:
>>>>>>> - When iommu_group of mediated devices is attached, task structure is
>>>>>>>   cached which is used later to pin pages and page accounting.
>>>>>>
>>>>>>
>>>>>> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
>>>>>> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
>>>>>> using @current or task as the process might be gone while VFIO container is
>>>>>> still alive and @mm might be needed to do proper cleanup; this might not be
>>>>>> an issue with this patchset now but still you seem to only use @mm from
>>>>>> task_struct.
>>>>>>
>>>>>
>>>>> Consider the example of QEMU process which creates VFIO container, QEMU
>>>>> in its teardown path would release the container. How could container be
>>>>> alive when process is gone?
>>>>
>>>> do_exit() in kernel/exit.c calls exit_mm() (which sets NULL to tsk->mm)
>>>> first, and then releases open files by calling  exit_files(). So
>>>> container's release() does not have current->mm.
>>>>
>>>
>>> Incrementing usage count (get_task_struct()) while saving task structure
>>> and decementing it (put_task_struct()) from release() should  work here.
>>> Updating the patch.
>>
>> I cannot see how the task->usage counter prevents do_exit() from performing
>> the exit, can you?
>>
> 
> It will not prevent exit from do_exit(), but that will make sure that we
> don't have stale pointer of task structure. Then we can check whether
> the task is alive and get mm pointer in teardown path as below:


Or you could just reference and use @mm as KVM and others do. Or there is
anything else you need from @current than just @mm?


> 
> {
>         struct task_struct *task = domain->external_addr_space->task;
>         struct mm_struct *mm = NULL;
> 
>         put_pfn(pfn, prot);
> 
>         if (pid_alive(task))
>                 mm = get_task_mm(task);
> 
>         if (mm) {
>                 if (do_accounting)
>                         vfio_lock_acct(task, -1);
> 
>                 mmput(mm);
>         }
> }



-- 
Alexey

[toc] | [prev] | [next] | [standalone]

#1513821 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 11/02/2016 12:09 PM, Alexey Kardashevskiy wrote:
> On 02/11/16 14:29, Kirti Wankhede wrote:
>>
>>
>> On 11/2/2016 6:54 AM, Alexey Kardashevskiy wrote:
>>> On 02/11/16 01:01, Kirti Wankhede wrote:
>>>>
>>>>
>>>> On 10/28/2016 7:48 AM, Alexey Kardashevskiy wrote:
>>>>> On 27/10/16 23:31, Kirti Wankhede wrote:
>>>>>>
>>>>>>
>>>>>> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
>>>>>>> On 18/10/16 08:22, Kirti Wankhede wrote:
>>>>>>>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>>>>>>>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>>>>>>>> managed by an IOMMU domain.
>>>>>>>>
>>>>>>>> Aim of this change is:
>>>>>>>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>>>>>>>> - To support direct assigned device and mediated device in single module
>>>>>>>>
>>>>>>>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>>>>>>>> IOMMU module that supports pining and unpinning pages for mdev devices
>>>>>>>> should provide these functions.
>>>>>>>> Added APIs for pining and unpining pages to VFIO module. These calls back
>>>>>>>> into backend iommu module to actually pin and unpin pages.
>>>>>>>>
>>>>>>>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>>>>>>>> backend module. More details:
>>>>>>>> - When iommu_group of mediated devices is attached, task structure is
>>>>>>>>   cached which is used later to pin pages and page accounting.
>>>>>>>
>>>>>>>
>>>>>>> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
>>>>>>> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
>>>>>>> using @current or task as the process might be gone while VFIO container is
>>>>>>> still alive and @mm might be needed to do proper cleanup; this might not be
>>>>>>> an issue with this patchset now but still you seem to only use @mm from
>>>>>>> task_struct.
>>>>>>>
>>>>>>
>>>>>> Consider the example of QEMU process which creates VFIO container, QEMU
>>>>>> in its teardown path would release the container. How could container be
>>>>>> alive when process is gone?
>>>>>
>>>>> do_exit() in kernel/exit.c calls exit_mm() (which sets NULL to tsk->mm)
>>>>> first, and then releases open files by calling  exit_files(). So
>>>>> container's release() does not have current->mm.
>>>>>
>>>>
>>>> Incrementing usage count (get_task_struct()) while saving task structure
>>>> and decementing it (put_task_struct()) from release() should  work here.
>>>> Updating the patch.
>>>
>>> I cannot see how the task->usage counter prevents do_exit() from performing
>>> the exit, can you?
>>>
>>
>> It will not prevent exit from do_exit(), but that will make sure that we
>> don't have stale pointer of task structure. Then we can check whether
>> the task is alive and get mm pointer in teardown path as below:
> 
> 
> Or you could just reference and use @mm as KVM and others do. Or there is
> anything else you need from @current than just @mm?
> 

I agree. If @mm is the only thing needed, there is really no reason to
refer to the @task :-)

--
Thanks,
Jike

[toc] | [prev] | [next] | [standalone]

#1513828 — Re: [Qemu-devel] [PATCH v9 04/12] vfio iommu: Add support for mediated devices

On 11/2/2016 5:51 PM, Jike Song wrote:
> On 11/02/2016 12:09 PM, Alexey Kardashevskiy wrote:
>> On 02/11/16 14:29, Kirti Wankhede wrote:
>>>
>>>
>>> On 11/2/2016 6:54 AM, Alexey Kardashevskiy wrote:
>>>> On 02/11/16 01:01, Kirti Wankhede wrote:
>>>>>
>>>>>
>>>>> On 10/28/2016 7:48 AM, Alexey Kardashevskiy wrote:
>>>>>> On 27/10/16 23:31, Kirti Wankhede wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10/27/2016 12:50 PM, Alexey Kardashevskiy wrote:
>>>>>>>> On 18/10/16 08:22, Kirti Wankhede wrote:
>>>>>>>>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>>>>>>>>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>>>>>>>>> managed by an IOMMU domain.
>>>>>>>>>
>>>>>>>>> Aim of this change is:
>>>>>>>>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>>>>>>>>> - To support direct assigned device and mediated device in single module
>>>>>>>>>
>>>>>>>>> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
>>>>>>>>> IOMMU module that supports pining and unpinning pages for mdev devices
>>>>>>>>> should provide these functions.
>>>>>>>>> Added APIs for pining and unpining pages to VFIO module. These calls back
>>>>>>>>> into backend iommu module to actually pin and unpin pages.
>>>>>>>>>
>>>>>>>>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>>>>>>>>> backend module. More details:
>>>>>>>>> - When iommu_group of mediated devices is attached, task structure is
>>>>>>>>>   cached which is used later to pin pages and page accounting.
>>>>>>>>
>>>>>>>>
>>>>>>>> For SPAPR TCE IOMMU driver, I ended up caching mm_struct with
>>>>>>>> atomic_inc(&container->mm->mm_count) (patches are on the way) instead of
>>>>>>>> using @current or task as the process might be gone while VFIO container is
>>>>>>>> still alive and @mm might be needed to do proper cleanup; this might not be
>>>>>>>> an issue with this patchset now but still you seem to only use @mm from
>>>>>>>> task_struct.
>>>>>>>>
>>>>>>>
>>>>>>> Consider the example of QEMU process which creates VFIO container, QEMU
>>>>>>> in its teardown path would release the container. How could container be
>>>>>>> alive when process is gone?
>>>>>>
>>>>>> do_exit() in kernel/exit.c calls exit_mm() (which sets NULL to tsk->mm)
>>>>>> first, and then releases open files by calling  exit_files(). So
>>>>>> container's release() does not have current->mm.
>>>>>>
>>>>>
>>>>> Incrementing usage count (get_task_struct()) while saving task structure
>>>>> and decementing it (put_task_struct()) from release() should  work here.
>>>>> Updating the patch.
>>>>
>>>> I cannot see how the task->usage counter prevents do_exit() from performing
>>>> the exit, can you?
>>>>
>>>
>>> It will not prevent exit from do_exit(), but that will make sure that we
>>> don't have stale pointer of task structure. Then we can check whether
>>> the task is alive and get mm pointer in teardown path as below:
>>
>>
>> Or you could just reference and use @mm as KVM and others do. Or there is
>> anything else you need from @current than just @mm?
>>
> 
> I agree. If @mm is the only thing needed, there is really no reason to
> refer to the @task :-)
> 

In vfio_lock_acct(), that is for page accounting, if mm->mmap_sem is
already held then page accounting is deferred, where task structure is
used to get mm and work is deferred only if mm exist:
	mm = get_task_mm(task);

That is where this module need task structure.

Thanks,
Kirti

[toc] | [prev] | [next] | [standalone]

Page 2 of 4 — ← Prev page 1 [2] 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web