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

From: "Xu, Quan" <quan.xu@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"Wu, Feng" <feng.wu@intel.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
Date: Thu, 12 May 2016 05:16:06 +0000	[thread overview]
Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B8AD3F2@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <5733126D02000078000EA7BF@prv-mh.provo.novell.com>

On May 11, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> 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?
> 

---
Good idea.

---

Taken together, there are 3 call trees to me_wifi_quirk():

 1). ...--me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--setup_hwdom_device()

                    There is no use in calling this function if an earlier error occurred. The change can be more lightweight (the detailed change is pending).

2).  me_wifi_quirk()--domain_context_unmap_one()--...

                   As you mentioned,  while in the unmap case it should probably stay as is, to fit the "best effort" theme. 

                  Then I need to remove the  __must_check annotation  of me_wifi_quirk().

3). me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--reassign_device_ownership()
                    This is not an earlier error, so we need propagate the error up to the call tree (the detailed change is pending).



The below is based on previous v4 p1...p9:
---

diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index cbe0286..d4d37c3 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -91,7 +91,7 @@ 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 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..0ac7894 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1429,8 +1429,8 @@ int domain_context_mapping_one(

     unmap_vtd_domain_page(context_entries);

-    if ( !seg )
-        me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+    if ( !seg && !rc )
+        rc = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);

     return rc;
 }
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index 473d1fc..3606b52 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 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)
--
1.9.1


Quan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
