diff mbox

[v4,10/10] vt-d: propagate error up to ME phantom function mapping and unmapping

Message ID 1462524880-67205-11-git-send-email-quan.xu@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Quan Xu May 6, 2016, 8:54 a.m. UTC 
Propagate the IOMMU Device-TLB flush error up to ME phantom function
mapping and unmapping.

Signed-off-by: Quan Xu <quan.xu@intel.com>

CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
---
 xen/drivers/passthrough/vtd/extern.h |  3 ++-
 xen/drivers/passthrough/vtd/iommu.c  | 18 ++++++++++++++----
 xen/drivers/passthrough/vtd/quirks.c | 28 ++++++++++++++++++----------
 3 files changed, 34 insertions(+), 15 deletions(-)

Comments

Jan Beulich May 10, 2016, 9:29 a.m. UTC | #1 
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> @@ -1430,7 +1430,12 @@ int domain_context_mapping_one(
>      unmap_vtd_domain_page(context_entries);
>  
>      if ( !seg )
> -        me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> +    {
> +        ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> +
> +        if ( !rc )
> +            rc = ret;
> +    }

Is there any use in calling this function if an earlier error occurred?
If not, the change can be more lightweight (while in the unmap case
it should probably stay as is, to fit the "best effort" theme).

Jan
Quan Xu May 11, 2016, 8:35 a.m. UTC | #2 
On May 10, 2016 5:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > @@ -1430,7 +1430,12 @@ int domain_context_mapping_one(
> >      unmap_vtd_domain_page(context_entries);
> >
> >      if ( !seg )
> > -        me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> > +    {
> > +        ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> > +
> > +        if ( !rc )
> > +            rc = ret;
> > +    }
> 
> Is there any use in calling this function if an earlier error occurred?
> If not,

It is  no use.


We may need to consider this call tree:
   $ me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--reassign_device_ownership()--...

Then, what about dropping this patch? Leave it as is,
 or remove ' __must_check' annotation and propagate error up to the above call tree only?


>  the change can be more lightweight (while in the unmap case it should
> probably stay as is, to fit the "best effort" theme).
> 

Quan
Jan Beulich May 11, 2016, 9:07 a.m. UTC | #3 
>>> On 11.05.16 at 10:35, <quan.xu@intel.com> wrote:
> On May 10, 2016 5:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> > @@ -1430,7 +1430,12 @@ int domain_context_mapping_one(
>> >      unmap_vtd_domain_page(context_entries);
>> >
>> >      if ( !seg )
>> > -        me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
>> > +    {
>> > +        ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
>> > +
>> > +        if ( !rc )
>> > +            rc = ret;
>> > +    }
>> 
>> Is there any use in calling this function if an earlier error occurred?
>> If not,
> 
> It is  no use.

With this I don't understand ...

> We may need to consider this call tree:
>    $ 
> me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--reass
> ign_device_ownership()--...
> 
> Then, what about dropping this patch? Leave it as is,
>  or remove ' __must_check' annotation and propagate error up to the above 
> call tree only?

... this. If calling the function is pointless if an earlier error occurred,
why don't you just check rc to be zero alongside the !seg check?

Jan
diff mbox

Patch

diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index cbe0286..6772839 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -91,7 +91,8 @@  int is_igd_vt_enabled_quirk(void);
 void platform_quirks_init(void);
 void vtd_ops_preamble_quirk(struct iommu* iommu);
 void vtd_ops_postamble_quirk(struct iommu* iommu);
-void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
+int __must_check me_wifi_quirk(struct domain *domain,
+                               u8 bus, u8 devfn, int map);
 void pci_vtd_quirk(const struct pci_dev *);
 bool_t platform_supports_intremap(void);
 bool_t platform_supports_x2apic(void);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 29cf870..ac81c8a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1296,7 +1296,7 @@  int domain_context_mapping_one(
     u64 maddr, pgd_maddr;
     u16 seg = iommu->intel->drhd->segment;
     int agaw;
-    int rc;
+    int rc, ret;
 
     ASSERT(pcidevs_locked());
     spin_lock(&iommu->lock);
@@ -1430,7 +1430,12 @@  int domain_context_mapping_one(
     unmap_vtd_domain_page(context_entries);
 
     if ( !seg )
-        me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+    {
+        ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+
+        if ( !rc )
+            rc = ret;
+    }
 
     return rc;
 }
@@ -1527,7 +1532,7 @@  int domain_context_unmap_one(
     struct context_entry *context, *context_entries;
     u64 maddr;
     int iommu_domid;
-    int rc;
+    int rc, ret;
 
     ASSERT(pcidevs_locked());
     spin_lock(&iommu->lock);
@@ -1575,7 +1580,12 @@  int domain_context_unmap_one(
     unmap_vtd_domain_page(context_entries);
 
     if ( !iommu->intel->drhd->segment )
-        me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC);
+    {
+        ret = me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC);
+
+        if ( !rc )
+            rc = ret;
+    }
 
     return rc;
 }
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index 473d1fc..e37060c 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -331,10 +331,12 @@  void __init platform_quirks_init(void)
  * assigning Intel integrated wifi device to a guest.
  */
 
-static void map_me_phantom_function(struct domain *domain, u32 dev, int map)
+static int __must_check map_me_phantom_function(struct domain *domain,
+                                                u32 dev, int map)
 {
     struct acpi_drhd_unit *drhd;
     struct pci_dev *pdev;
+    int rc;
 
     /* find ME VT-d engine base on a real ME device */
     pdev = pci_get_pdev(0, 0, PCI_DEVFN(dev, 0));
@@ -342,23 +344,27 @@  static void map_me_phantom_function(struct domain *domain, u32 dev, int map)
 
     /* map or unmap ME phantom function */
     if ( map )
-        domain_context_mapping_one(domain, drhd->iommu, 0,
-                                   PCI_DEVFN(dev, 7), NULL);
+        rc = domain_context_mapping_one(domain, drhd->iommu, 0,
+                                        PCI_DEVFN(dev, 7), NULL);
     else
-        domain_context_unmap_one(domain, drhd->iommu, 0,
-                                 PCI_DEVFN(dev, 7));
+        rc = domain_context_unmap_one(domain, drhd->iommu, 0,
+                                      PCI_DEVFN(dev, 7));
+
+    return rc;
 }
 
-void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
+int __must_check me_wifi_quirk(struct domain *domain,
+                               u8 bus, u8 devfn, int map)
 {
     u32 id;
+    int rc = 0;
 
     id = pci_conf_read32(0, 0, 0, 0, 0);
     if ( IS_CTG(id) )
     {
         /* quit if ME does not exist */
         if ( pci_conf_read32(0, 0, 3, 0, 0) == 0xffffffff )
-            return;
+            return 0;
 
         /* if device is WLAN device, map ME phantom device 0:3.7 */
         id = pci_conf_read32(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0);
@@ -372,7 +378,7 @@  void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
             case 0x423b8086:
             case 0x423c8086:
             case 0x423d8086:
-                map_me_phantom_function(domain, 3, map);
+                rc = map_me_phantom_function(domain, 3, map);
                 break;
             default:
                 break;
@@ -382,7 +388,7 @@  void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
     {
         /* quit if ME does not exist */
         if ( pci_conf_read32(0, 0, 22, 0, 0) == 0xffffffff )
-            return;
+            return 0;
 
         /* if device is WLAN device, map ME phantom device 0:22.7 */
         id = pci_conf_read32(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0);
@@ -398,12 +404,14 @@  void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
             case 0x42388086:        /* Puma Peak */
             case 0x422b8086:
             case 0x422c8086:
-                map_me_phantom_function(domain, 22, map);
+                rc = map_me_phantom_function(domain, 22, map);
                 break;
             default:
                 break;
         }
     }
+
+    return rc;
 }
 
 void pci_vtd_quirk(const struct pci_dev *pdev)