diff mbox

FIXME question

Message ID 945CA011AD5F084CBEA3E851C0AB28894B8F6A68@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quan Xu June 27, 2016, 9:23 a.m. UTC 
Hi,

When I read IOMMU code,
In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte()..
There are a FIXME --
''
* FIXME: For performance reasons we should store the 'iommu' pointer in
* 'struct msi_desc' in some other place, so we don't need to waste
* time searching it here.
"

IMO, we are better to store the 'iommu' pointer in pci_dev, then
could I fix it as:

1. save a void *iommu  in pci_dev structure:



-Quan

Comments

Wu, Feng June 27, 2016, 9:30 a.m. UTC | #1 
> -----Original Message-----
> From: Xu, Quan
> Sent: Monday, June 27, 2016 5:24 PM
> To: xen-devel@lists.xen.org
> Cc: Jan Beulich <JBeulich@suse.com>; Tian, Kevin <kevin.tian@intel.com>; Wu,
> Feng <feng.wu@intel.com>
> Subject: FIXME question
> 
> Hi,
> 
> When I read IOMMU code,
> In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte()..
> There are a FIXME --
> ''
> * FIXME: For performance reasons we should store the 'iommu' pointer in
> * 'struct msi_desc' in some other place, so we don't need to waste
> * time searching it here.
> "
> 
> IMO, we are better to store the 'iommu' pointer in pci_dev, then
> could I fix it as:

Yes, I think saving the 'iommu' pointer in pci_dev is the right direction.

Thanks,
Feng

> 
> 1. save a void *iommu  in pci_dev structure:
> 
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -83,6 +83,8 @@ struct pci_dev {
>  #define PT_FAULT_THRESHOLD 10
>      } fault;
>      u64 vf_rlen[6];
> +
> +   void *iommu; /* No common IOMMU struct so use void pointer */
>  };
> 
> 
> 
> 2. Save iommu pointer in 'struct pci_dev' when to add device:
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1994,6 +1994,7 @@ static int intel_iommu_enable_device(struct pci_dev
> *pdev)
>      if ( ret <= 0 )
>          return ret;
> 
> +    pdev->iommu = drhd->iommu;
>      ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu);
> 
> 3. use iommu pointer from pci_dev instead of calling
> acpi_find_matched_drhd_unit each time (also fix the related code).
> 
> 
> -Quan
>
Jan Beulich June 27, 2016, 9:32 a.m. UTC | #2 
>>> On 27.06.16 at 11:23, <quan.xu@intel.com> wrote:
> Hi,
> 
> When I read IOMMU code,
> In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte()..
> There are a FIXME --
> ''
> * FIXME: For performance reasons we should store the 'iommu' pointer in
> * 'struct msi_desc' in some other place, so we don't need to waste
> * time searching it here.
> "
> 
> IMO, we are better to store the 'iommu' pointer in pci_dev, then
> could I fix it as:

Sounds reasonable.

Jan

> 1. save a void *iommu  in pci_dev structure:
> 
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -83,6 +83,8 @@ struct pci_dev {
>  #define PT_FAULT_THRESHOLD 10
>      } fault;
>      u64 vf_rlen[6];
> +
> +   void *iommu; /* No common IOMMU struct so use void pointer */
>  };
> 
> 
> 
> 2. Save iommu pointer in 'struct pci_dev' when to add device:
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1994,6 +1994,7 @@ static int intel_iommu_enable_device(struct pci_dev 
> *pdev)
>      if ( ret <= 0 )
>          return ret;
> 
> +    pdev->iommu = drhd->iommu;
>      ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu);
> 
> 3. use iommu pointer from pci_dev instead of calling 
> acpi_find_matched_drhd_unit each time (also fix the related code).
> 
> 
> -Quan
>
Andrew Cooper June 27, 2016, 9:50 a.m. UTC | #3 
On 27/06/16 10:32, Jan Beulich wrote:
>>>> On 27.06.16 at 11:23, <quan.xu@intel.com> wrote:
>> Hi,
>>
>> When I read IOMMU code,
>> In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte()..
>> There are a FIXME --
>> ''
>> * FIXME: For performance reasons we should store the 'iommu' pointer in
>> * 'struct msi_desc' in some other place, so we don't need to waste
>> * time searching it here.
>> "
>>
>> IMO, we are better to store the 'iommu' pointer in pci_dev, then
>> could I fix it as:
> Sounds reasonable.

I agree.

>
> Jan
>
>> 1. save a void *iommu  in pci_dev structure:
>>
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -83,6 +83,8 @@ struct pci_dev {
>>  #define PT_FAULT_THRESHOLD 10
>>      } fault;
>>      u64 vf_rlen[6];
>> +
>> +   void *iommu; /* No common IOMMU struct so use void pointer */

Longer term, it would be nice to have concrete IOMMU type with
implementation specific fields hidden in a union, but void * will work
for now.

~Andrew
Wu, Feng June 27, 2016, 9:53 a.m. UTC | #4 
> -----Original Message-----

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]

> Sent: Monday, June 27, 2016 5:51 PM

> To: Jan Beulich <JBeulich@suse.com>; Xu, Quan <quan.xu@intel.com>

> Cc: Tian, Kevin <kevin.tian@intel.com>; Wu, Feng <feng.wu@intel.com>; xen-

> devel@lists.xen.org

> Subject: Re: [Xen-devel] FIXME question

> 

> On 27/06/16 10:32, Jan Beulich wrote:

> >>>> On 27.06.16 at 11:23, <quan.xu@intel.com> wrote:

> >> Hi,

> >>

> >> When I read IOMMU code,

> >> In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte()..

> >> There are a FIXME --

> >> ''

> >> * FIXME: For performance reasons we should store the 'iommu' pointer in

> >> * 'struct msi_desc' in some other place, so we don't need to waste

> >> * time searching it here.

> >> "

> >>

> >> IMO, we are better to store the 'iommu' pointer in pci_dev, then

> >> could I fix it as:

> > Sounds reasonable.

> 

> I agree.

> 

> >

> > Jan

> >

> >> 1. save a void *iommu  in pci_dev structure:

> >>

> >> --- a/xen/include/xen/pci.h

> >> +++ b/xen/include/xen/pci.h

> >> @@ -83,6 +83,8 @@ struct pci_dev {

> >>  #define PT_FAULT_THRESHOLD 10

> >>      } fault;

> >>      u64 vf_rlen[6];

> >> +

> >> +   void *iommu; /* No common IOMMU struct so use void pointer */

> 

> Longer term, it would be nice to have concrete IOMMU type with

> implementation specific fields hidden in a union, but void * will work

> for now.


Agree, I think something like below should be the case:

struct arch_pci_dev {
     vmask_t used_vectors;
+    union {
+        struct acpi_drhd_unit *drhd;
+        /* AMD can add its counterpart here if needed */
+    };
 };

Thanks,
Feng

> 

> ~Andrew
diff mbox

Patch

--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -83,6 +83,8 @@  struct pci_dev {
 #define PT_FAULT_THRESHOLD 10
     } fault;
     u64 vf_rlen[6];
+
+   void *iommu; /* No common IOMMU struct so use void pointer */
 };



2. Save iommu pointer in 'struct pci_dev' when to add device:

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1994,6 +1994,7 @@  static int intel_iommu_enable_device(struct pci_dev *pdev)
     if ( ret <= 0 )
         return ret;

+    pdev->iommu = drhd->iommu;
     ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu);

3. use iommu pointer from pci_dev instead of calling acpi_find_matched_drhd_unit each time (also fix the related code).