diff mbox series

[v3,3/3] xen/pci: Refactor MSI code that implements MSI functionality within XEN

Message ID 7b6651f10922571a10685dc7652fbce03b6b6e51.1619453100.git.rahul.singh@arm.com (mailing list archive)
State Superseded
Headers show
Series xen/pci: Make PCI passthrough code non-x86 specific | expand

Commit Message

Rahul Singh April 26, 2021, 4:21 p.m. UTC
MSI code that implements MSI functionality to support MSI within XEN is
not usable on ARM. Move the code under CONFIG_PCI_MSI_INTERCEPT flag to
gate the code for ARM.

Currently, we have no idea how MSI functionality will be supported for
other architecture therefore we have decided to move the code under
CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
code but to avoid an extra flag we decided to use this.

No functional change intended.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes since v2:
 - This patch is added in this version.
---
 xen/drivers/passthrough/msi-intercept.c | 41 +++++++++++++++++++++++++
 xen/drivers/passthrough/pci.c           | 34 ++++----------------
 xen/include/xen/msi-intercept.h         |  7 +++++
 xen/include/xen/pci.h                   | 11 ++++---
 4 files changed, 61 insertions(+), 32 deletions(-)

Comments

Roger Pau Monné April 28, 2021, 11:06 a.m. UTC | #1
On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
> MSI code that implements MSI functionality to support MSI within XEN is
> not usable on ARM. Move the code under CONFIG_PCI_MSI_INTERCEPT flag to
> gate the code for ARM.
> 
> Currently, we have no idea how MSI functionality will be supported for
> other architecture therefore we have decided to move the code under
> CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
> code but to avoid an extra flag we decided to use this.
> 
> No functional change intended.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

I think this is fine, as we don't really want to add another Kconfig
option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
code.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Some nits below...

> ---
> Changes since v2:
>  - This patch is added in this version.
> ---
>  xen/drivers/passthrough/msi-intercept.c | 41 +++++++++++++++++++++++++
>  xen/drivers/passthrough/pci.c           | 34 ++++----------------
>  xen/include/xen/msi-intercept.h         |  7 +++++
>  xen/include/xen/pci.h                   | 11 ++++---
>  4 files changed, 61 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/msi-intercept.c b/xen/drivers/passthrough/msi-intercept.c
> index 1edae6d4e8..33ab71514d 100644
> --- a/xen/drivers/passthrough/msi-intercept.c
> +++ b/xen/drivers/passthrough/msi-intercept.c
> @@ -19,6 +19,47 @@
>  #include <asm/msi.h>
>  #include <asm/hvm/io.h>
>  
> +int pdev_msi_init(struct pci_dev *pdev)
> +{
> +    unsigned int pos;
> +
> +    INIT_LIST_HEAD(&pdev->msi_list);
> +
> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
> +    if ( pos )
> +    {
> +        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
> +
> +        pdev->msi_maxvec = multi_msi_capable(ctrl);
> +    }
> +
> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
> +    if ( pos )
> +    {
> +        struct arch_msix *msix = xzalloc(struct arch_msix);
> +        uint16_t ctrl;
> +
> +        if ( !msix )
> +            return -ENOMEM;
> +
> +        spin_lock_init(&msix->table_lock);
> +
> +        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> +        msix->nr_entries = msix_table_size(ctrl);
> +
> +        pdev->msix = msix;
> +    }
> +
> +    return 0;
> +}
> +
> +void pdev_msi_deinit(struct pci_dev *pdev)
> +{
> +    XFREE(pdev->msix);
> +}
> +
>  int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
>  {
>      int rc;
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 298be21b5b..b1e3c711ad 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>  {
>      struct pci_dev *pdev;
>      unsigned int pos;
> +    int rc;
>  
>      list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>          if ( pdev->bus == bus && pdev->devfn == devfn )
> @@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>      *((u8*) &pdev->bus) = bus;
>      *((u8*) &pdev->devfn) = devfn;
>      pdev->domain = NULL;
> -    INIT_LIST_HEAD(&pdev->msi_list);
> -
> -    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> -                              PCI_CAP_ID_MSI);
> -    if ( pos )
> -    {
> -        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>  
> -        pdev->msi_maxvec = multi_msi_capable(ctrl);
> -    }
> -
> -    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> -                              PCI_CAP_ID_MSIX);
> -    if ( pos )
> +    rc = pdev_msi_init(pdev);
> +    if ( rc )
>      {
> -        struct arch_msix *msix = xzalloc(struct arch_msix);
> -        uint16_t ctrl;
> -
> -        if ( !msix )
> -        {
> -            xfree(pdev);
> -            return NULL;
> -        }
> -        spin_lock_init(&msix->table_lock);
> -
> -        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> -        msix->nr_entries = msix_table_size(ctrl);
> -
> -        pdev->msix = msix;
> +        XFREE(pdev);

There's no need to use XFREE here, plain xfree is fine since pdev is a
local variable so makes no sense to assign to NULL before returning.

> +        return NULL;
>      }
>  
>      list_add(&pdev->alldevs_list, &pseg->alldevs_list);
> @@ -449,7 +427,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
>      }
>  
>      list_del(&pdev->alldevs_list);
> -    xfree(pdev->msix);
> +    pdev_msi_deinit(pdev);
>      xfree(pdev);
>  }
>  
> diff --git a/xen/include/xen/msi-intercept.h b/xen/include/xen/msi-intercept.h
> index 77c105e286..38ff7a09e1 100644
> --- a/xen/include/xen/msi-intercept.h
> +++ b/xen/include/xen/msi-intercept.h
> @@ -21,16 +21,23 @@
>  
>  #include <asm/msi.h>
>  
> +int pdev_msi_init(struct pci_dev *pdev);
> +void pdev_msi_deinit(struct pci_dev *pdev);
>  int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
>  void pdev_dump_msi(const struct pci_dev *pdev);
>  
>  #else /* !CONFIG_PCI_MSI_INTERCEPT */
> +static inline int pdev_msi_init(struct pci_dev *pdev)
> +{
> +    return 0;
> +}
>  
>  static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
>  {
>      return 0;
>  }
>  
> +static inline void pdev_msi_deinit(struct pci_dev *pdev) {}
>  static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
>  static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>  
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 8e3d4d9454..f5b57270be 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -79,10 +79,6 @@ struct pci_dev {
>      struct list_head alldevs_list;
>      struct list_head domain_list;
>  
> -    struct list_head msi_list;
> -
> -    struct arch_msix *msix;
> -
>      struct domain *domain;
>  
>      const union {
> @@ -94,7 +90,14 @@ struct pci_dev {
>          pci_sbdf_t sbdf;
>      };
>  
> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> +    struct list_head msi_list;
> +
> +    struct arch_msix *msix;
> +
>      uint8_t msi_maxvec;
> +#endif

This seems to introduce some padding between the sbdf and the msi_list
field. I guess that's better than having two different
CONFIG_PCI_MSI_INTERCEPT guarded regions?

Thanks, Roger.
Jan Beulich April 28, 2021, 2:11 p.m. UTC | #2
On 28.04.2021 13:06, Roger Pau Monné wrote:
> On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
>> MSI code that implements MSI functionality to support MSI within XEN is
>> not usable on ARM. Move the code under CONFIG_PCI_MSI_INTERCEPT flag to
>> gate the code for ARM.
>>
>> Currently, we have no idea how MSI functionality will be supported for
>> other architecture therefore we have decided to move the code under
>> CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
>> code but to avoid an extra flag we decided to use this.
>>
>> No functional change intended.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> I think this is fine, as we don't really want to add another Kconfig
> option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
> code.

While a separate config option may be excessive, keying it to the
wrong one is not desirable imo. If we want to avoid having PCI_MSI,
then keeping respective bits guarded by X86 would look better to me.
I'm not convinced though that doing the separation properly right
away (see also the XSM changes in patch 2 which imo belong here)
isn't going to be better in the long run, and hence introducing
HAS_PCI_MSI right away isn't the way to go.

Jan
Julien Grall April 28, 2021, 2:26 p.m. UTC | #3
On 28/04/2021 12:06, Roger Pau Monné wrote:
> On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
>> MSI code that implements MSI functionality to support MSI within XEN is
>> not usable on ARM. Move the code under CONFIG_PCI_MSI_INTERCEPT flag to
>> gate the code for ARM.
>>
>> Currently, we have no idea how MSI functionality will be supported for
>> other architecture therefore we have decided to move the code under
>> CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
>> code but to avoid an extra flag we decided to use this.
>>
>> No functional change intended.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> I think this is fine, as we don't really want to add another Kconfig
> option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
> code.

+1 This is code and therefore can be revisited once we have more data 
(e.g. a second arch that will re-use it).

> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,
Rahul Singh April 29, 2021, 11:59 a.m. UTC | #4
Hi Roger,

> On 28 Apr 2021, at 12:06 pm, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Mon, Apr 26, 2021 at 05:21:27PM +0100, Rahul Singh wrote:
>> MSI code that implements MSI functionality to support MSI within XEN is
>> not usable on ARM. Move the code under CONFIG_PCI_MSI_INTERCEPT flag to
>> gate the code for ARM.
>> 
>> Currently, we have no idea how MSI functionality will be supported for
>> other architecture therefore we have decided to move the code under
>> CONFIG_PCI_MSI_INTERCEPT. We know this is not the right flag to gate the
>> code but to avoid an extra flag we decided to use this.
>> 
>> No functional change intended.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> I think this is fine, as we don't really want to add another Kconfig
> option (ie: CONFIG_PCI_MSI) for just the non explicitly intercept MSI
> code.
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 

Thanks.

> Some nits below...
> 
>> ---
>> Changes since v2:
>> - This patch is added in this version.
>> ---
>> xen/drivers/passthrough/msi-intercept.c | 41 +++++++++++++++++++++++++
>> xen/drivers/passthrough/pci.c           | 34 ++++----------------
>> xen/include/xen/msi-intercept.h         |  7 +++++
>> xen/include/xen/pci.h                   | 11 ++++---
>> 4 files changed, 61 insertions(+), 32 deletions(-)
>> 
>> diff --git a/xen/drivers/passthrough/msi-intercept.c b/xen/drivers/passthrough/msi-intercept.c
>> index 1edae6d4e8..33ab71514d 100644
>> --- a/xen/drivers/passthrough/msi-intercept.c
>> +++ b/xen/drivers/passthrough/msi-intercept.c
>> @@ -19,6 +19,47 @@
>> #include <asm/msi.h>
>> #include <asm/hvm/io.h>
>> 
>> +int pdev_msi_init(struct pci_dev *pdev)
>> +{
>> +    unsigned int pos;
>> +
>> +    INIT_LIST_HEAD(&pdev->msi_list);
>> +
>> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
>> +    if ( pos )
>> +    {
>> +        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> +
>> +        pdev->msi_maxvec = multi_msi_capable(ctrl);
>> +    }
>> +
>> +    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> +                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
>> +    if ( pos )
>> +    {
>> +        struct arch_msix *msix = xzalloc(struct arch_msix);
>> +        uint16_t ctrl;
>> +
>> +        if ( !msix )
>> +            return -ENOMEM;
>> +
>> +        spin_lock_init(&msix->table_lock);
>> +
>> +        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
>> +        msix->nr_entries = msix_table_size(ctrl);
>> +
>> +        pdev->msix = msix;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +void pdev_msi_deinit(struct pci_dev *pdev)
>> +{
>> +    XFREE(pdev->msix);
>> +}
>> +
>> int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
>> {
>>     int rc;
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 298be21b5b..b1e3c711ad 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -314,6 +314,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>> {
>>     struct pci_dev *pdev;
>>     unsigned int pos;
>> +    int rc;
>> 
>>     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>         if ( pdev->bus == bus && pdev->devfn == devfn )
>> @@ -327,35 +328,12 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>>     *((u8*) &pdev->bus) = bus;
>>     *((u8*) &pdev->devfn) = devfn;
>>     pdev->domain = NULL;
>> -    INIT_LIST_HEAD(&pdev->msi_list);
>> -
>> -    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>> -                              PCI_CAP_ID_MSI);
>> -    if ( pos )
>> -    {
>> -        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
>> 
>> -        pdev->msi_maxvec = multi_msi_capable(ctrl);
>> -    }
>> -
>> -    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>> -                              PCI_CAP_ID_MSIX);
>> -    if ( pos )
>> +    rc = pdev_msi_init(pdev);
>> +    if ( rc )
>>     {
>> -        struct arch_msix *msix = xzalloc(struct arch_msix);
>> -        uint16_t ctrl;
>> -
>> -        if ( !msix )
>> -        {
>> -            xfree(pdev);
>> -            return NULL;
>> -        }
>> -        spin_lock_init(&msix->table_lock);
>> -
>> -        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
>> -        msix->nr_entries = msix_table_size(ctrl);
>> -
>> -        pdev->msix = msix;
>> +        XFREE(pdev);
> 
> There's no need to use XFREE here, plain xfree is fine since pdev is a
> local variable so makes no sense to assign to NULL before returning.

Ok. I will change it to xfree in next version.

> 
>> +        return NULL;
>>     }
>> 
>>     list_add(&pdev->alldevs_list, &pseg->alldevs_list);
>> @@ -449,7 +427,7 @@ static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
>>     }
>> 
>>     list_del(&pdev->alldevs_list);
>> -    xfree(pdev->msix);
>> +    pdev_msi_deinit(pdev);
>>     xfree(pdev);
>> }
>> 
>> diff --git a/xen/include/xen/msi-intercept.h b/xen/include/xen/msi-intercept.h
>> index 77c105e286..38ff7a09e1 100644
>> --- a/xen/include/xen/msi-intercept.h
>> +++ b/xen/include/xen/msi-intercept.h
>> @@ -21,16 +21,23 @@
>> 
>> #include <asm/msi.h>
>> 
>> +int pdev_msi_init(struct pci_dev *pdev);
>> +void pdev_msi_deinit(struct pci_dev *pdev);
>> int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
>> void pdev_dump_msi(const struct pci_dev *pdev);
>> 
>> #else /* !CONFIG_PCI_MSI_INTERCEPT */
>> +static inline int pdev_msi_init(struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +}
>> 
>> static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
>> {
>>     return 0;
>> }
>> 
>> +static inline void pdev_msi_deinit(struct pci_dev *pdev) {}
>> static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
>> static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
>> 
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index 8e3d4d9454..f5b57270be 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -79,10 +79,6 @@ struct pci_dev {
>>     struct list_head alldevs_list;
>>     struct list_head domain_list;
>> 
>> -    struct list_head msi_list;
>> -
>> -    struct arch_msix *msix;
>> -
>>     struct domain *domain;
>> 
>>     const union {
>> @@ -94,7 +90,14 @@ struct pci_dev {
>>         pci_sbdf_t sbdf;
>>     };
>> 
>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>> +    struct list_head msi_list;
>> +
>> +    struct arch_msix *msix;
>> +
>>     uint8_t msi_maxvec;
>> +#endif
> 
> This seems to introduce some padding between the sbdf and the msi_list
> field. I guess that's better than having two different
> CONFIG_PCI_MSI_INTERCEPT guarded regions?

Yes. That’s why I move all the fields related to MSI to one place to avoid the 
extra CONFIG_PCI_MSI_INTERCEPT instance.

Regards,
Rahul
> 
> Thanks, Roger.
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/msi-intercept.c b/xen/drivers/passthrough/msi-intercept.c
index 1edae6d4e8..33ab71514d 100644
--- a/xen/drivers/passthrough/msi-intercept.c
+++ b/xen/drivers/passthrough/msi-intercept.c
@@ -19,6 +19,47 @@ 
 #include <asm/msi.h>
 #include <asm/hvm/io.h>
 
+int pdev_msi_init(struct pci_dev *pdev)
+{
+    unsigned int pos;
+
+    INIT_LIST_HEAD(&pdev->msi_list);
+
+    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSI);
+    if ( pos )
+    {
+        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
+
+        pdev->msi_maxvec = multi_msi_capable(ctrl);
+    }
+
+    pos = pci_find_cap_offset(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+                              PCI_FUNC(pdev->devfn), PCI_CAP_ID_MSIX);
+    if ( pos )
+    {
+        struct arch_msix *msix = xzalloc(struct arch_msix);
+        uint16_t ctrl;
+
+        if ( !msix )
+            return -ENOMEM;
+
+        spin_lock_init(&msix->table_lock);
+
+        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
+        msix->nr_entries = msix_table_size(ctrl);
+
+        pdev->msix = msix;
+    }
+
+    return 0;
+}
+
+void pdev_msi_deinit(struct pci_dev *pdev)
+{
+    XFREE(pdev->msix);
+}
+
 int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
 {
     int rc;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 298be21b5b..b1e3c711ad 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -314,6 +314,7 @@  static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
 {
     struct pci_dev *pdev;
     unsigned int pos;
+    int rc;
 
     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
         if ( pdev->bus == bus && pdev->devfn == devfn )
@@ -327,35 +328,12 @@  static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
     *((u8*) &pdev->bus) = bus;
     *((u8*) &pdev->devfn) = devfn;
     pdev->domain = NULL;
-    INIT_LIST_HEAD(&pdev->msi_list);
-
-    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                              PCI_CAP_ID_MSI);
-    if ( pos )
-    {
-        uint16_t ctrl = pci_conf_read16(pdev->sbdf, msi_control_reg(pos));
 
-        pdev->msi_maxvec = multi_msi_capable(ctrl);
-    }
-
-    pos = pci_find_cap_offset(pseg->nr, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                              PCI_CAP_ID_MSIX);
-    if ( pos )
+    rc = pdev_msi_init(pdev);
+    if ( rc )
     {
-        struct arch_msix *msix = xzalloc(struct arch_msix);
-        uint16_t ctrl;
-
-        if ( !msix )
-        {
-            xfree(pdev);
-            return NULL;
-        }
-        spin_lock_init(&msix->table_lock);
-
-        ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
-        msix->nr_entries = msix_table_size(ctrl);
-
-        pdev->msix = msix;
+        XFREE(pdev);
+        return NULL;
     }
 
     list_add(&pdev->alldevs_list, &pseg->alldevs_list);
@@ -449,7 +427,7 @@  static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
     }
 
     list_del(&pdev->alldevs_list);
-    xfree(pdev->msix);
+    pdev_msi_deinit(pdev);
     xfree(pdev);
 }
 
diff --git a/xen/include/xen/msi-intercept.h b/xen/include/xen/msi-intercept.h
index 77c105e286..38ff7a09e1 100644
--- a/xen/include/xen/msi-intercept.h
+++ b/xen/include/xen/msi-intercept.h
@@ -21,16 +21,23 @@ 
 
 #include <asm/msi.h>
 
+int pdev_msi_init(struct pci_dev *pdev);
+void pdev_msi_deinit(struct pci_dev *pdev);
 int pdev_msix_assign(struct domain *d, struct pci_dev *pdev);
 void pdev_dump_msi(const struct pci_dev *pdev);
 
 #else /* !CONFIG_PCI_MSI_INTERCEPT */
+static inline int pdev_msi_init(struct pci_dev *pdev)
+{
+    return 0;
+}
 
 static inline int pdev_msix_assign(struct domain *d, struct pci_dev *pdev)
 {
     return 0;
 }
 
+static inline void pdev_msi_deinit(struct pci_dev *pdev) {}
 static inline void pdev_dump_msi(const struct pci_dev *pdev) {}
 static inline void pci_cleanup_msi(struct pci_dev *pdev) {}
 
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 8e3d4d9454..f5b57270be 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -79,10 +79,6 @@  struct pci_dev {
     struct list_head alldevs_list;
     struct list_head domain_list;
 
-    struct list_head msi_list;
-
-    struct arch_msix *msix;
-
     struct domain *domain;
 
     const union {
@@ -94,7 +90,14 @@  struct pci_dev {
         pci_sbdf_t sbdf;
     };
 
+#ifdef CONFIG_PCI_MSI_INTERCEPT
+    struct list_head msi_list;
+
+    struct arch_msix *msix;
+
     uint8_t msi_maxvec;
+#endif
+
     uint8_t phantom_stride;
 
     nodeid_t node; /* NUMA node */