Re: [Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Liu, Yi L]

> From: Peter Xu [mailto:pet...@redhat.com]
> Sent: Friday, March 9, 2018 3:59 PM
> Subject: Re: [PATCH v3 12/12] intel_iommu: bind device to PASID tagged
> AddressSpace
> 
> On Thu, Mar 08, 2018 at 09:39:18AM +, Liu, Yi L wrote:
> > > From: Peter Xu [mailto:pet...@redhat.com]
> > > Sent: Tuesday, March 6, 2018 7:44 PM
> > > Subject: Re: [PATCH v3 12/12] intel_iommu: bind device to PASID
> > > tagged AddressSpace
> > >
> > > On Thu, Mar 01, 2018 at 06:33:35PM +0800, Liu, Yi L wrote:
> > > > This patch shows the idea of how a device is binded to a PASID
> > > > tagged AddressSpace.
> > > >
> > > > when Intel vIOMMU emulator detected a pasid table entry
> > > > programming from guest. Intel vIOMMU emulator firstly finds a
> > > > VTDPASIDAddressSpace with the pasid field of pasid cache invalidate 
> > > > request.
> > > >
> > > > * If it is to bind a device to a guest process, needs add the device
> > > >   to the device list behind the VTDPASIDAddressSpace. And if the device
> > > >   is assigned device, need to register sva_notfier for future tlb
> > > >   flushing if any mapping changed to the process address space.
> > > >
> > > > * If it is to unbind a device from a guest process, then need to remove
> > > >   the device from the device list behind the VTDPASIDAddressSpace.
> > > >   And also needs to unregister the sva_notfier if the device is assigned
> > > >   device.
> > > >
> > > > This patch hasn't added the unbind logic. It depends on guest
> > > > pasid table entry parsing which requires further emulation. Here
> > > > just want to show the idea for the PASID tagged AddressSpace management
> framework.
> > > > Full unregister logic would be included in future virt-SVA patchset.
> > > >
> > > > Signed-off-by: Liu, Yi L 
> > > > ---
> > > >  hw/i386/intel_iommu.c  | 119
> > > +
> > > >  hw/i386/intel_iommu_internal.h |  10 
> > > >  2 files changed, 129 insertions(+)
> > > >
> > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > > > b8e8dbb..ed07035 100644
> > > > --- a/hw/i386/intel_iommu.c
> > > > +++ b/hw/i386/intel_iommu.c
> > > > @@ -1801,6 +1801,118 @@ static bool
> > > > vtd_process_iotlb_desc(IntelIOMMUState
> > > *s, VTDInvDesc *inv_desc)
> > > >  return true;
> > > >  }
> > > >
> > > > +static VTDPASIDAddressSpace *vtd_get_pasid_as(IntelIOMMUState *s,
> > > > +  uint32_t pasid) {
> > > > +VTDPASIDAddressSpace *vtd_pasid_as = NULL;
> > > > +IntelPASIDNode *node;
> > > > +char name[128];
> > > > +
> > > > +QLIST_FOREACH(node, &(s->pasid_as_list), next) {
> > > > +vtd_pasid_as = node->pasid_as;
> > > > +if (pasid == vtd_pasid_as->sva_ctx.pasid) {
> > > > +return vtd_pasid_as;
> > > > +}
> > > > +}
> > >
> > > This seems to be a per-iommu pasid table.  However from the spec it
> > > looks more like that should be per-domain (I'm seeing figure 3-8).
> > > For example, each domain should be able to have its own pasid table.
> > > Then IIUC a pasid context will need a (domain, pasid) tuple to
> > > identify, not only the pasid itself?
> >
> > Yes, this is a per-iommu table here. Actually, how we assemble the
> > table here depends on the PASID namespace. You may refer to the iommu
> > driver code. intel-svm.c, it's actually per-iommu.
> >
> > /* Do not use PASID 0 in caching mode (virtualised IOMMU) */
> > ret = idr_alloc(>pasid_idr, svm,
> > !!cap_caching_mode(iommu->cap),
> > pasid_max - 1, GFP_KERNEL);
> 
> Thanks for the pointer.
> 
> However from the spec, I see that PASID table pointer is per-context, IMHO 
> which
> means that the spec will allow the PASID table to be different from one 
> device to
> another.  Even if current Linux is sharing a single PASID table now, I don't 
> know
> whether that can be expanded in the future.  Also, what if we run a guest with
> another OS besides Linux?
> 
> After all we are emulating the device, so IIUC the only thing we should 
> follow is the
> spec.

Agree. just echo Kevin's reply here. Let' me re-consider a way to maintain all 
the
PASID tagged address space here.

> 
> >
> > >
> > > And, do we need to destroy the pasid context after it's freed by the
> > > guest?  Here it seems that we'll cache it forever.
> >
> > If we need to do it. A PASID can be bind to multiple devices. If there
> > is no device binding on it, then needs to destroy it. This may be done
> > by refcount. As I mentioned in the description, that requires further
> > vIOMMU emulation, so I didn't include it. But it should be covered in
> > final version. Good catch.
> >
> > >
> > > > +
> > > > +vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
> > > > +vtd_pasid_as->iommu_state = s;
> > > > +snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
> > > > +

Re: [Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Tian, Kevin]

> From: Peter Xu [mailto:pet...@redhat.com]
> Sent: Friday, March 9, 2018 3:59 PM
> 
> On Thu, Mar 08, 2018 at 09:39:18AM +, Liu, Yi L wrote:
> > > From: Peter Xu [mailto:pet...@redhat.com]
> > > Sent: Tuesday, March 6, 2018 7:44 PM
> > > Subject: Re: [PATCH v3 12/12] intel_iommu: bind device to PASID tagged
> > > AddressSpace
> > >
> > > On Thu, Mar 01, 2018 at 06:33:35PM +0800, Liu, Yi L wrote:
> > > > This patch shows the idea of how a device is binded to a PASID tagged
> > > > AddressSpace.
> > > >
> > > > when Intel vIOMMU emulator detected a pasid table entry
> programming
> > > > from guest. Intel vIOMMU emulator firstly finds a
> VTDPASIDAddressSpace
> > > > with the pasid field of pasid cache invalidate request.
> > > >
> > > > * If it is to bind a device to a guest process, needs add the device
> > > >   to the device list behind the VTDPASIDAddressSpace. And if the
> device
> > > >   is assigned device, need to register sva_notfier for future tlb
> > > >   flushing if any mapping changed to the process address space.
> > > >
> > > > * If it is to unbind a device from a guest process, then need to remove
> > > >   the device from the device list behind the VTDPASIDAddressSpace.
> > > >   And also needs to unregister the sva_notfier if the device is assigned
> > > >   device.
> > > >
> > > > This patch hasn't added the unbind logic. It depends on guest pasid
> > > > table entry parsing which requires further emulation. Here just want
> > > > to show the idea for the PASID tagged AddressSpace management
> framework.
> > > > Full unregister logic would be included in future virt-SVA patchset.
> > > >
> > > > Signed-off-by: Liu, Yi L 
> > > > ---
> > > >  hw/i386/intel_iommu.c  | 119
> > > +
> > > >  hw/i386/intel_iommu_internal.h |  10 
> > > >  2 files changed, 129 insertions(+)
> > > >
> > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > index b8e8dbb..ed07035 100644
> > > > --- a/hw/i386/intel_iommu.c
> > > > +++ b/hw/i386/intel_iommu.c
> > > > @@ -1801,6 +1801,118 @@ static bool
> vtd_process_iotlb_desc(IntelIOMMUState
> > > *s, VTDInvDesc *inv_desc)
> > > >  return true;
> > > >  }
> > > >
> > > > +static VTDPASIDAddressSpace *vtd_get_pasid_as(IntelIOMMUState
> *s,
> > > > +  uint32_t pasid)
> > > > +{
> > > > +VTDPASIDAddressSpace *vtd_pasid_as = NULL;
> > > > +IntelPASIDNode *node;
> > > > +char name[128];
> > > > +
> > > > +QLIST_FOREACH(node, &(s->pasid_as_list), next) {
> > > > +vtd_pasid_as = node->pasid_as;
> > > > +if (pasid == vtd_pasid_as->sva_ctx.pasid) {
> > > > +return vtd_pasid_as;
> > > > +}
> > > > +}
> > >
> > > This seems to be a per-iommu pasid table.  However from the spec it
> > > looks more like that should be per-domain (I'm seeing figure 3-8).
> > > For example, each domain should be able to have its own pasid table.
> > > Then IIUC a pasid context will need a (domain, pasid) tuple to
> > > identify, not only the pasid itself?
> >
> > Yes, this is a per-iommu table here. Actually, how we assemble the
> > table here depends on the PASID namespace. You may refer to the
> > iommu driver code. intel-svm.c, it's actually per-iommu.
> >
> > /* Do not use PASID 0 in caching mode (virtualised IOMMU)
> */
> > ret = idr_alloc(>pasid_idr, svm,
> > !!cap_caching_mode(iommu->cap),
> > pasid_max - 1, GFP_KERNEL);
> 
> Thanks for the pointer.
> 
> However from the spec, I see that PASID table pointer is per-context,
> IMHO which means that the spec will allow the PASID table to be
> different from one device to another.  Even if current Linux is
> sharing a single PASID table now, I don't know whether that can be
> expanded in the future.  Also, what if we run a guest with another OS
> besides Linux?
> 
> After all we are emulating the device, so IIUC the only thing we
> should follow is the spec.
> 
> >
> > >
> > > And, do we need to destroy the pasid context after it's freed by the
> > > guest?  Here it seems that we'll cache it forever.
> >
> > If we need to do it. A PASID can be bind to multiple devices. If there
> > is no device binding on it, then needs to destroy it. This may be done
> > by refcount. As I mentioned in the description, that requires further
> > vIOMMU emulation, so I didn't include it. But it should be covered
> > in final version. Good catch.
> >
> > >
> > > > +
> > > > +vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
> > > > +vtd_pasid_as->iommu_state = s;
> > > > +snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
> > > > +address_space_init(_pasid_as->as, NULL, "pasid");
> > >
> > > I saw that this is only inited and never used.  Could I ask when this
> > > will be used?
> >
> > AddressSpace is actually introduced for future support of emulated
> > 

Re: [Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Peter Xu]

On Thu, Mar 08, 2018 at 09:39:18AM +, Liu, Yi L wrote:
> > From: Peter Xu [mailto:pet...@redhat.com]
> > Sent: Tuesday, March 6, 2018 7:44 PM
> > Subject: Re: [PATCH v3 12/12] intel_iommu: bind device to PASID tagged
> > AddressSpace
> > 
> > On Thu, Mar 01, 2018 at 06:33:35PM +0800, Liu, Yi L wrote:
> > > This patch shows the idea of how a device is binded to a PASID tagged
> > > AddressSpace.
> > >
> > > when Intel vIOMMU emulator detected a pasid table entry programming
> > > from guest. Intel vIOMMU emulator firstly finds a VTDPASIDAddressSpace
> > > with the pasid field of pasid cache invalidate request.
> > >
> > > * If it is to bind a device to a guest process, needs add the device
> > >   to the device list behind the VTDPASIDAddressSpace. And if the device
> > >   is assigned device, need to register sva_notfier for future tlb
> > >   flushing if any mapping changed to the process address space.
> > >
> > > * If it is to unbind a device from a guest process, then need to remove
> > >   the device from the device list behind the VTDPASIDAddressSpace.
> > >   And also needs to unregister the sva_notfier if the device is assigned
> > >   device.
> > >
> > > This patch hasn't added the unbind logic. It depends on guest pasid
> > > table entry parsing which requires further emulation. Here just want
> > > to show the idea for the PASID tagged AddressSpace management framework.
> > > Full unregister logic would be included in future virt-SVA patchset.
> > >
> > > Signed-off-by: Liu, Yi L 
> > > ---
> > >  hw/i386/intel_iommu.c  | 119
> > +
> > >  hw/i386/intel_iommu_internal.h |  10 
> > >  2 files changed, 129 insertions(+)
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index b8e8dbb..ed07035 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -1801,6 +1801,118 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState
> > *s, VTDInvDesc *inv_desc)
> > >  return true;
> > >  }
> > >
> > > +static VTDPASIDAddressSpace *vtd_get_pasid_as(IntelIOMMUState *s,
> > > +  uint32_t pasid)
> > > +{
> > > +VTDPASIDAddressSpace *vtd_pasid_as = NULL;
> > > +IntelPASIDNode *node;
> > > +char name[128];
> > > +
> > > +QLIST_FOREACH(node, &(s->pasid_as_list), next) {
> > > +vtd_pasid_as = node->pasid_as;
> > > +if (pasid == vtd_pasid_as->sva_ctx.pasid) {
> > > +return vtd_pasid_as;
> > > +}
> > > +}
> > 
> > This seems to be a per-iommu pasid table.  However from the spec it
> > looks more like that should be per-domain (I'm seeing figure 3-8).
> > For example, each domain should be able to have its own pasid table.
> > Then IIUC a pasid context will need a (domain, pasid) tuple to
> > identify, not only the pasid itself?
> 
> Yes, this is a per-iommu table here. Actually, how we assemble the
> table here depends on the PASID namespace. You may refer to the
> iommu driver code. intel-svm.c, it's actually per-iommu.
> 
>   /* Do not use PASID 0 in caching mode (virtualised IOMMU) */
>   ret = idr_alloc(>pasid_idr, svm,
>   !!cap_caching_mode(iommu->cap),
>   pasid_max - 1, GFP_KERNEL);

Thanks for the pointer.

However from the spec, I see that PASID table pointer is per-context,
IMHO which means that the spec will allow the PASID table to be
different from one device to another.  Even if current Linux is
sharing a single PASID table now, I don't know whether that can be
expanded in the future.  Also, what if we run a guest with another OS
besides Linux?

After all we are emulating the device, so IIUC the only thing we
should follow is the spec.

> 
> > 
> > And, do we need to destroy the pasid context after it's freed by the
> > guest?  Here it seems that we'll cache it forever.
> 
> If we need to do it. A PASID can be bind to multiple devices. If there
> is no device binding on it, then needs to destroy it. This may be done
> by refcount. As I mentioned in the description, that requires further
> vIOMMU emulation, so I didn't include it. But it should be covered
> in final version. Good catch.
> 
> > 
> > > +
> > > +vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
> > > +vtd_pasid_as->iommu_state = s;
> > > +snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
> > > +address_space_init(_pasid_as->as, NULL, "pasid");
> > 
> > I saw that this is only inited and never used.  Could I ask when this
> > will be used?
> 
> AddressSpace is actually introduced for future support of emulated
> SVA capable devices and possible 1st level paging shadowing(similar
> to the 2nd level page table shadowing you upstreamed).

I am not sure whether that can be useful even if there will be such a
device.  The reason is that if you see current with-IOMMU guests, they
are actually "somehow" 

Re: [Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Liu, Yi L]

> From: Peter Xu [mailto:pet...@redhat.com]
> Sent: Tuesday, March 6, 2018 7:44 PM
> Subject: Re: [PATCH v3 12/12] intel_iommu: bind device to PASID tagged
> AddressSpace
> 
> On Thu, Mar 01, 2018 at 06:33:35PM +0800, Liu, Yi L wrote:
> > This patch shows the idea of how a device is binded to a PASID tagged
> > AddressSpace.
> >
> > when Intel vIOMMU emulator detected a pasid table entry programming
> > from guest. Intel vIOMMU emulator firstly finds a VTDPASIDAddressSpace
> > with the pasid field of pasid cache invalidate request.
> >
> > * If it is to bind a device to a guest process, needs add the device
> >   to the device list behind the VTDPASIDAddressSpace. And if the device
> >   is assigned device, need to register sva_notfier for future tlb
> >   flushing if any mapping changed to the process address space.
> >
> > * If it is to unbind a device from a guest process, then need to remove
> >   the device from the device list behind the VTDPASIDAddressSpace.
> >   And also needs to unregister the sva_notfier if the device is assigned
> >   device.
> >
> > This patch hasn't added the unbind logic. It depends on guest pasid
> > table entry parsing which requires further emulation. Here just want
> > to show the idea for the PASID tagged AddressSpace management framework.
> > Full unregister logic would be included in future virt-SVA patchset.
> >
> > Signed-off-by: Liu, Yi L 
> > ---
> >  hw/i386/intel_iommu.c  | 119
> +
> >  hw/i386/intel_iommu_internal.h |  10 
> >  2 files changed, 129 insertions(+)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index b8e8dbb..ed07035 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -1801,6 +1801,118 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState
> *s, VTDInvDesc *inv_desc)
> >  return true;
> >  }
> >
> > +static VTDPASIDAddressSpace *vtd_get_pasid_as(IntelIOMMUState *s,
> > +  uint32_t pasid)
> > +{
> > +VTDPASIDAddressSpace *vtd_pasid_as = NULL;
> > +IntelPASIDNode *node;
> > +char name[128];
> > +
> > +QLIST_FOREACH(node, &(s->pasid_as_list), next) {
> > +vtd_pasid_as = node->pasid_as;
> > +if (pasid == vtd_pasid_as->sva_ctx.pasid) {
> > +return vtd_pasid_as;
> > +}
> > +}
> 
> This seems to be a per-iommu pasid table.  However from the spec it
> looks more like that should be per-domain (I'm seeing figure 3-8).
> For example, each domain should be able to have its own pasid table.
> Then IIUC a pasid context will need a (domain, pasid) tuple to
> identify, not only the pasid itself?

Yes, this is a per-iommu table here. Actually, how we assemble the
table here depends on the PASID namespace. You may refer to the
iommu driver code. intel-svm.c, it's actually per-iommu.

/* Do not use PASID 0 in caching mode (virtualised IOMMU) */
ret = idr_alloc(>pasid_idr, svm,
!!cap_caching_mode(iommu->cap),
pasid_max - 1, GFP_KERNEL);

> 
> And, do we need to destroy the pasid context after it's freed by the
> guest?  Here it seems that we'll cache it forever.

If we need to do it. A PASID can be bind to multiple devices. If there
is no device binding on it, then needs to destroy it. This may be done
by refcount. As I mentioned in the description, that requires further
vIOMMU emulation, so I didn't include it. But it should be covered
in final version. Good catch.

> 
> > +
> > +vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
> > +vtd_pasid_as->iommu_state = s;
> > +snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
> > +address_space_init(_pasid_as->as, NULL, "pasid");
> 
> I saw that this is only inited and never used.  Could I ask when this
> will be used?

AddressSpace is actually introduced for future support of emulated
SVA capable devices and possible 1st level paging shadowing(similar
to the 2nd level page table shadowing you upstreamed).

> 
> > +QLIST_INIT(_pasid_as->device_list);
> > +
> > +node = g_malloc0(sizeof(*node));
> > +node->pasid_as = vtd_pasid_as;
> > +QLIST_INSERT_HEAD(>pasid_as_list, node, next);
> > +
> > +return vtd_pasid_as;
> > +}
> > +
> > +static void vtd_bind_device_to_pasid_as(VTDPASIDAddressSpace *vtd_pasid_as,
> > +PCIBus *bus, uint8_t devfn)
> > +{
> > +VTDDeviceNode *node = NULL;
> > +
> > +QLIST_FOREACH(node, &(vtd_pasid_as->device_list), next) {
> > +if (node->bus == bus && node->devfn == devfn) {
> > +return;
> > +}
> > +}
> > +
> > +node = g_malloc0(sizeof(*node));
> > +node->bus = bus;
> > +node->devfn = devfn;
> > +QLIST_INSERT_HEAD(&(vtd_pasid_as->device_list), node, next);
> 
> So here I have the same confusion - IIUC according to the spec two
> devices can 

Re: [Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Peter Xu]

On Thu, Mar 01, 2018 at 06:33:35PM +0800, Liu, Yi L wrote:
> This patch shows the idea of how a device is binded to a PASID tagged
> AddressSpace.
> 
> when Intel vIOMMU emulator detected a pasid table entry programming
> from guest. Intel vIOMMU emulator firstly finds a VTDPASIDAddressSpace
> with the pasid field of pasid cache invalidate request.
> 
> * If it is to bind a device to a guest process, needs add the device
>   to the device list behind the VTDPASIDAddressSpace. And if the device
>   is assigned device, need to register sva_notfier for future tlb
>   flushing if any mapping changed to the process address space.
> 
> * If it is to unbind a device from a guest process, then need to remove
>   the device from the device list behind the VTDPASIDAddressSpace.
>   And also needs to unregister the sva_notfier if the device is assigned
>   device.
> 
> This patch hasn't added the unbind logic. It depends on guest pasid
> table entry parsing which requires further emulation. Here just want
> to show the idea for the PASID tagged AddressSpace management framework.
> Full unregister logic would be included in future virt-SVA patchset.
> 
> Signed-off-by: Liu, Yi L 
> ---
>  hw/i386/intel_iommu.c  | 119 
> +
>  hw/i386/intel_iommu_internal.h |  10 
>  2 files changed, 129 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index b8e8dbb..ed07035 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1801,6 +1801,118 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState 
> *s, VTDInvDesc *inv_desc)
>  return true;
>  }
>  
> +static VTDPASIDAddressSpace *vtd_get_pasid_as(IntelIOMMUState *s,
> +  uint32_t pasid)
> +{
> +VTDPASIDAddressSpace *vtd_pasid_as = NULL;
> +IntelPASIDNode *node;
> +char name[128];
> +
> +QLIST_FOREACH(node, &(s->pasid_as_list), next) {
> +vtd_pasid_as = node->pasid_as;
> +if (pasid == vtd_pasid_as->sva_ctx.pasid) {
> +return vtd_pasid_as;
> +}
> +}

This seems to be a per-iommu pasid table.  However from the spec it
looks more like that should be per-domain (I'm seeing figure 3-8).
For example, each domain should be able to have its own pasid table.
Then IIUC a pasid context will need a (domain, pasid) tuple to
identify, not only the pasid itself?

And, do we need to destroy the pasid context after it's freed by the
guest?  Here it seems that we'll cache it forever.

> +
> +vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
> +vtd_pasid_as->iommu_state = s;
> +snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
> +address_space_init(_pasid_as->as, NULL, "pasid");

I saw that this is only inited and never used.  Could I ask when this
will be used?

> +QLIST_INIT(_pasid_as->device_list);
> +
> +node = g_malloc0(sizeof(*node));
> +node->pasid_as = vtd_pasid_as;
> +QLIST_INSERT_HEAD(>pasid_as_list, node, next);
> +
> +return vtd_pasid_as;
> +}
> +
> +static void vtd_bind_device_to_pasid_as(VTDPASIDAddressSpace *vtd_pasid_as,
> +PCIBus *bus, uint8_t devfn)
> +{
> +VTDDeviceNode *node = NULL;
> +
> +QLIST_FOREACH(node, &(vtd_pasid_as->device_list), next) {
> +if (node->bus == bus && node->devfn == devfn) {
> +return;
> +}
> +}
> +
> +node = g_malloc0(sizeof(*node));
> +node->bus = bus;
> +node->devfn = devfn;
> +QLIST_INSERT_HEAD(&(vtd_pasid_as->device_list), node, next);

So here I have the same confusion - IIUC according to the spec two
devices can have differnet pasid tables, however they can be sharing
the same PASID number (e.g., pasid=1) in the table.  Here since
vtd_pasid_as is only per-IOMMU, could it possible that we put multiple
devices under same PASID context while actually they are not sharing
the same process page table?  Problematic?

Please correct me if needed.

> +
> +pci_device_sva_register_notifier(bus, devfn, _pasid_as->sva_ctx);
> +
> +return;
> +}
> +
> +static bool vtd_process_pc_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
> +{
> +
> +IntelIOMMUAssignedDeviceNode *node = NULL;
> +int ret = 0;
> +
> +uint16_t domain_id;
> +uint32_t pasid;
> +VTDPASIDAddressSpace *vtd_pasid_as;
> +
> +if ((inv_desc->lo & VTD_INV_DESC_PASIDC_RSVD_LO) ||
> +(inv_desc->hi & VTD_INV_DESC_PASIDC_RSVD_HI)) {
> +return false;
> +}
> +
> +domain_id = VTD_INV_DESC_PASIDC_DID(inv_desc->lo);
> +
> +switch (inv_desc->lo & VTD_INV_DESC_PASIDC_G) {
> +case VTD_INV_DESC_PASIDC_ALL_ALL:
> +/* TODO: invalidate all pasid related cache */

I think it's fine as RFC, but we'd better have this in the final
version?

IIUC you'll need caching-mode too for virt-sva, and here you'll
possibly need to walk and scan every context entry that has the same
domain ID specified in 

Re: [Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Liu, Yi L]

On Fri, Mar 02, 2018 at 03:51:53PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 11:33, Liu, Yi L wrote:
> > +IntelPASIDNode *node;
> > +char name[128];
> > +
> > +QLIST_FOREACH(node, &(s->pasid_as_list), next) {
> > +vtd_pasid_as = node->pasid_as;
> > +if (pasid == vtd_pasid_as->sva_ctx.pasid) {
> > +return vtd_pasid_as;
> > +}
> > +}
> > +
> > +vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
> > +vtd_pasid_as->iommu_state = s;
> > +snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
> > +address_space_init(_pasid_as->as, NULL, "pasid");
> 
> The name is unused here.  The call to address_space_init should probably
> use it.

yes, it is. I missed it. Thanks for catching it.

> You also don't need the separate IntelPASIDNode, because the
> QLIST_ENTRY can be placed directly in VTDPASIDAddressSpace.

Would refine it in next version.

Ragards,
Yi Liu

Re: [Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Paolo Bonzini]

On 01/03/2018 11:33, Liu, Yi L wrote:
> +IntelPASIDNode *node;
> +char name[128];
> +
> +QLIST_FOREACH(node, &(s->pasid_as_list), next) {
> +vtd_pasid_as = node->pasid_as;
> +if (pasid == vtd_pasid_as->sva_ctx.pasid) {
> +return vtd_pasid_as;
> +}
> +}
> +
> +vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
> +vtd_pasid_as->iommu_state = s;
> +snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
> +address_space_init(_pasid_as->as, NULL, "pasid");

The name is unused here.  The call to address_space_init should probably
use it.

You also don't need the separate IntelPASIDNode, because the
QLIST_ENTRY can be placed directly in VTDPASIDAddressSpace.

Paolo

[Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Liu, Yi L]

This patch shows the idea of how a device is binded to a PASID tagged
AddressSpace.

when Intel vIOMMU emulator detected a pasid table entry programming
from guest. Intel vIOMMU emulator firstly finds a VTDPASIDAddressSpace
with the pasid field of pasid cache invalidate request.

* If it is to bind a device to a guest process, needs add the device
  to the device list behind the VTDPASIDAddressSpace. And if the device
  is assigned device, need to register sva_notfier for future tlb
  flushing if any mapping changed to the process address space.

* If it is to unbind a device from a guest process, then need to remove
  the device from the device list behind the VTDPASIDAddressSpace.
  And also needs to unregister the sva_notfier if the device is assigned
  device.

This patch hasn't added the unbind logic. It depends on guest pasid
table entry parsing which requires further emulation. Here just want
to show the idea for the PASID tagged AddressSpace management framework.
Full unregister logic would be included in future virt-SVA patchset.

Signed-off-by: Liu, Yi L 
---
 hw/i386/intel_iommu.c  | 119 +
 hw/i386/intel_iommu_internal.h |  10 
 2 files changed, 129 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b8e8dbb..ed07035 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1801,6 +1801,118 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, 
VTDInvDesc *inv_desc)
 return true;
 }
 
+static VTDPASIDAddressSpace *vtd_get_pasid_as(IntelIOMMUState *s,
+  uint32_t pasid)
+{
+VTDPASIDAddressSpace *vtd_pasid_as = NULL;
+IntelPASIDNode *node;
+char name[128];
+
+QLIST_FOREACH(node, &(s->pasid_as_list), next) {
+vtd_pasid_as = node->pasid_as;
+if (pasid == vtd_pasid_as->sva_ctx.pasid) {
+return vtd_pasid_as;
+}
+}
+
+vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
+vtd_pasid_as->iommu_state = s;
+snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
+address_space_init(_pasid_as->as, NULL, "pasid");
+QLIST_INIT(_pasid_as->device_list);
+
+node = g_malloc0(sizeof(*node));
+node->pasid_as = vtd_pasid_as;
+QLIST_INSERT_HEAD(>pasid_as_list, node, next);
+
+return vtd_pasid_as;
+}
+
+static void vtd_bind_device_to_pasid_as(VTDPASIDAddressSpace *vtd_pasid_as,
+PCIBus *bus, uint8_t devfn)
+{
+VTDDeviceNode *node = NULL;
+
+QLIST_FOREACH(node, &(vtd_pasid_as->device_list), next) {
+if (node->bus == bus && node->devfn == devfn) {
+return;
+}
+}
+
+node = g_malloc0(sizeof(*node));
+node->bus = bus;
+node->devfn = devfn;
+QLIST_INSERT_HEAD(&(vtd_pasid_as->device_list), node, next);
+
+pci_device_sva_register_notifier(bus, devfn, _pasid_as->sva_ctx);
+
+return;
+}
+
+static bool vtd_process_pc_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
+{
+
+IntelIOMMUAssignedDeviceNode *node = NULL;
+int ret = 0;
+
+uint16_t domain_id;
+uint32_t pasid;
+VTDPASIDAddressSpace *vtd_pasid_as;
+
+if ((inv_desc->lo & VTD_INV_DESC_PASIDC_RSVD_LO) ||
+(inv_desc->hi & VTD_INV_DESC_PASIDC_RSVD_HI)) {
+return false;
+}
+
+domain_id = VTD_INV_DESC_PASIDC_DID(inv_desc->lo);
+
+switch (inv_desc->lo & VTD_INV_DESC_PASIDC_G) {
+case VTD_INV_DESC_PASIDC_ALL_ALL:
+/* TODO: invalidate all pasid related cache */
+break;
+
+case VTD_INV_DESC_PASIDC_PASID_SI:
+pasid = VTD_INV_DESC_PASIDC_PASID(inv_desc->lo);
+vtd_pasid_as = vtd_get_pasid_as(s, pasid);
+QLIST_FOREACH(node, &(s->assigned_device_list), next) {
+VTDAddressSpace *vtd_as = node->vtd_as;
+VTDContextEntry ce;
+uint16_t did;
+uint8_t bus = pci_bus_num(vtd_as->bus);
+ret = vtd_dev_to_context_entry(s, bus,
+   vtd_as->devfn, );
+if (ret != 0) {
+continue;
+}
+
+did = VTD_CONTEXT_ENTRY_DID(ce.hi);
+/*
+ * If did field equals to the domain_id field of inv_descriptor,
+ * then the device is affect by this invalidate request, need to
+ * bind or unbind the device to the pasid tagged address space.
+ * a) If it is bind, need to add the device to the device list,
+ *add register tlb flush notifier for it
+ * b) If it is unbind, need to remove the device from the device
+ *list, and unregister the tlb flush notifier
+ * TODO: add unbind logic accordingly, depends on the parsing of
+ *   guest pasid table entry pasrsing, here has no parsing to
+ *   pasid table entry.
+ *
+ */
+if (did == domain_id) {
+

[Qemu-devel] [PATCH v3 12/12] intel_iommu: bind device to PASID tagged AddressSpace

[Liu, Yi L]

This patch shows the idea of how a device is binded to a PASID tagged
AddressSpace.

when Intel vIOMMU emulator detected a pasid table entry programming
from guest. Intel vIOMMU emulator firstly finds a VTDPASIDAddressSpace
with the pasid field of pasid cache invalidate request.

* If it is to bind a device to a guest process, needs add the device
  to the device list behind the VTDPASIDAddressSpace. And if the device
  is assigned device, need to register sva_notfier for future tlb
  flushing if any mapping changed to the process address space.

* If it is to unbind a device from a guest process, then need to remove
  the device from the device list behind the VTDPASIDAddressSpace.
  And also needs to unregister the sva_notfier if the device is assigned
  device.

This patch hasn't added the unbind logic. It depends on guest pasid
table entry parsing which requires further emulation. Here just want
to show the idea for the PASID tagged AddressSpace management framework.
Full unregister logic would be included in future virt-SVA patchset.

Signed-off-by: Liu, Yi L 
---
 hw/i386/intel_iommu.c  | 119 +
 hw/i386/intel_iommu_internal.h |  10 
 2 files changed, 129 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b8e8dbb..ed07035 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1801,6 +1801,118 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, 
VTDInvDesc *inv_desc)
 return true;
 }
 
+static VTDPASIDAddressSpace *vtd_get_pasid_as(IntelIOMMUState *s,
+  uint32_t pasid)
+{
+VTDPASIDAddressSpace *vtd_pasid_as = NULL;
+IntelPASIDNode *node;
+char name[128];
+
+QLIST_FOREACH(node, &(s->pasid_as_list), next) {
+vtd_pasid_as = node->pasid_as;
+if (pasid == vtd_pasid_as->sva_ctx.pasid) {
+return vtd_pasid_as;
+}
+}
+
+vtd_pasid_as = g_malloc0(sizeof(*vtd_pasid_as));
+vtd_pasid_as->iommu_state = s;
+snprintf(name, sizeof(name), "intel_iommu_pasid_%d", pasid);
+address_space_init(_pasid_as->as, NULL, "pasid");
+QLIST_INIT(_pasid_as->device_list);
+
+node = g_malloc0(sizeof(*node));
+node->pasid_as = vtd_pasid_as;
+QLIST_INSERT_HEAD(>pasid_as_list, node, next);
+
+return vtd_pasid_as;
+}
+
+static void vtd_bind_device_to_pasid_as(VTDPASIDAddressSpace *vtd_pasid_as,
+PCIBus *bus, uint8_t devfn)
+{
+VTDDeviceNode *node = NULL;
+
+QLIST_FOREACH(node, &(vtd_pasid_as->device_list), next) {
+if (node->bus == bus && node->devfn == devfn) {
+return;
+}
+}
+
+node = g_malloc0(sizeof(*node));
+node->bus = bus;
+node->devfn = devfn;
+QLIST_INSERT_HEAD(&(vtd_pasid_as->device_list), node, next);
+
+pci_device_sva_register_notifier(bus, devfn, _pasid_as->sva_ctx);
+
+return;
+}
+
+static bool vtd_process_pc_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
+{
+
+IntelIOMMUAssignedDeviceNode *node = NULL;
+int ret = 0;
+
+uint16_t domain_id;
+uint32_t pasid;
+VTDPASIDAddressSpace *vtd_pasid_as;
+
+if ((inv_desc->lo & VTD_INV_DESC_PASIDC_RSVD_LO) ||
+(inv_desc->hi & VTD_INV_DESC_PASIDC_RSVD_HI)) {
+return false;
+}
+
+domain_id = VTD_INV_DESC_PASIDC_DID(inv_desc->lo);
+
+switch (inv_desc->lo & VTD_INV_DESC_PASIDC_G) {
+case VTD_INV_DESC_PASIDC_ALL_ALL:
+/* TODO: invalidate all pasid related cache */
+break;
+
+case VTD_INV_DESC_PASIDC_PASID_SI:
+pasid = VTD_INV_DESC_PASIDC_PASID(inv_desc->lo);
+vtd_pasid_as = vtd_get_pasid_as(s, pasid);
+QLIST_FOREACH(node, &(s->assigned_device_list), next) {
+VTDAddressSpace *vtd_as = node->vtd_as;
+VTDContextEntry ce;
+uint16_t did;
+uint8_t bus = pci_bus_num(vtd_as->bus);
+ret = vtd_dev_to_context_entry(s, bus,
+   vtd_as->devfn, );
+if (ret != 0) {
+continue;
+}
+
+did = VTD_CONTEXT_ENTRY_DID(ce.hi);
+/*
+ * If did field equals to the domain_id field of inv_descriptor,
+ * then the device is affect by this invalidate request, need to
+ * bind or unbind the device to the pasid tagged address space.
+ * a) If it is bind, need to add the device to the device list,
+ *add register tlb flush notifier for it
+ * b) If it is unbind, need to remove the device from the device
+ *list, and unregister the tlb flush notifier
+ * TODO: add unbind logic accordingly, depends on the parsing of
+ *   guest pasid table entry pasrsing, here has no parsing to
+ *   pasid table entry.
+ *
+ */
+if (did == domain_id) {
+