|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
On May 11, 2016 5:07 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> On 11.05.16 at 10:35, <quan.xu@xxxxxxxxx> wrote:
> > On May 10, 2016 5:29 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >> >>> On 06.05.16 at 10:54, <quan.xu@xxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |