Re: [Xen-devel] [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
On May 12, 2016 4:45 PM, Jan Beulich wrote:
> >>> On 12.05.16 at 07:16, wrote:
> > Taken together, there are 3 call trees to me_wifi_quirk():
> >
> > 1).
> > ...--me_wifi_quirk()--domain_context_mapping_one()--
> domain_context_map
> > ping()--se
> > tup_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().
>
> This does not follow from the above. You again should propagate the error in
> all cases (unless it would overwrite an earlier error - as you're doing in
> various
> other places).
>
Sorry, I know the item 2). is tricky, as I am confused about ' while in the
unmap case it should probably stay as is, to fit the "best effort" theme '.
Actually, what I need to enhance the p10 are:
- drop ret
- replace the
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;
+}
TO_
-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);
btw, I found I am struggling in this v4 and I will spend more time fixing.
thanks for your patience.
Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
>>> On 12.05.16 at 07:16, wrote: > Taken together, there are 3 call trees to me_wifi_quirk(): > > 1). > ...--me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--se > tup_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(). This does not follow from the above. You again should propagate the error in all cases (unless it would overwrite an earlier error - as you're doing in various other places). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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 wrote:
> >>> On 11.05.16 at 10:35, wrote:
> > On May 10, 2016 5:29 PM, Jan Beulich wrote:
> >> >>> On 06.05.16 at 10:54, 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 do
Re: [Xen-devel] [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
>>> On 11.05.16 at 10:35, wrote:
> On May 10, 2016 5:29 PM, Jan Beulich wrote:
>> >>> On 06.05.16 at 10:54, 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
On May 10, 2016 5:29 PM, Jan Beulich wrote:
> >>> On 06.05.16 at 10:54, 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
>>> On 06.05.16 at 10:54, 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
