Re: [Xen-devel] [PATCH v2 06/11] vpci/header: add teardown cleanup

[Jan Beulich]

>>> On 17.07.18 at 11:48,  wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -131,12 +131,15 @@ bool vpci_process_pending(struct vcpu *v)
>  if ( rc == -ERESTART )
>  return true;
>  
> -spin_lock(&v->vpci.pdev->vpci_lock);
> -if ( v->vpci.pdev->vpci )
> -/* Disable memory decoding unconditionally on failure. */
> -modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
> -!rc && v->vpci.rom_only);
> -spin_unlock(&v->vpci.pdev->vpci_lock);
> +if ( v->vpci.pdev )
> +{
> +spin_lock(&v->vpci.pdev->vpci_lock);
> +if ( v->vpci.pdev->vpci )
> +/* Disable memory decoding unconditionally on failure. */
> +modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
> +!rc && v->vpci.rom_only);
> +spin_unlock(&v->vpci.pdev->vpci_lock);
> +}
>  
>  rangeset_destroy(v->vpci.mem);
>  v->vpci.mem = NULL;

A few lines down from here there is

vpci_remove_device(v->vpci.pdev);

which I think has the same issue.

> @@ -560,7 +563,25 @@ static int init_bars(struct pci_dev *pdev)
>  
>  return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, true, false) : 0;
>  }
> -REGISTER_VPCI_INIT(init_bars, NULL, VPCI_PRIORITY_MIDDLE);
> +
> +static void teardown_bars(struct pci_dev *pdev)
> +{
> +uint16_t cmd = pci_conf_read16(pdev->seg, pdev->bus, 
> PCI_SLOT(pdev->devfn),
> +   PCI_FUNC(pdev->devfn), PCI_COMMAND);
> +
> +if ( cmd & PCI_COMMAND_MEMORY )
> +{
> +/* Unmap all BARs from guest p2m. */
> +modify_bars(pdev, false, false);
> +/*
> + * Since this operation is deferred at the point when it finishes the
> + * device might have been removed, so don't attempt to disable memory
> + * decoding afterwards.
> + */
> +current->vpci.pdev = NULL;

Did you not mean to prefix the comment with FIXME: ? Ultimately
device removal should be delayed until all cleanup has been done,
imo.

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2 06/11] vpci/header: add teardown cleanup

[Roger Pau Monne]

In order to unmap the BARs

Signed-off-by: Roger Pau Monné 
---
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
---
Changes since v1:
 - Add comment regarding the fact that memory decoding is not
   disabled when removing a device.
---
 xen/drivers/vpci/header.c | 35 ---
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 4363270a55..17a9dbb0bf 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -131,12 +131,15 @@ bool vpci_process_pending(struct vcpu *v)
 if ( rc == -ERESTART )
 return true;
 
-spin_lock(&v->vpci.pdev->vpci_lock);
-if ( v->vpci.pdev->vpci )
-/* Disable memory decoding unconditionally on failure. */
-modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
-!rc && v->vpci.rom_only);
-spin_unlock(&v->vpci.pdev->vpci_lock);
+if ( v->vpci.pdev )
+{
+spin_lock(&v->vpci.pdev->vpci_lock);
+if ( v->vpci.pdev->vpci )
+/* Disable memory decoding unconditionally on failure. */
+modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
+!rc && v->vpci.rom_only);
+spin_unlock(&v->vpci.pdev->vpci_lock);
+}
 
 rangeset_destroy(v->vpci.mem);
 v->vpci.mem = NULL;
@@ -560,7 +563,25 @@ static int init_bars(struct pci_dev *pdev)
 
 return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, true, false) : 0;
 }
-REGISTER_VPCI_INIT(init_bars, NULL, VPCI_PRIORITY_MIDDLE);
+
+static void teardown_bars(struct pci_dev *pdev)
+{
+uint16_t cmd = pci_conf_read16(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+   PCI_FUNC(pdev->devfn), PCI_COMMAND);
+
+if ( cmd & PCI_COMMAND_MEMORY )
+{
+/* Unmap all BARs from guest p2m. */
+modify_bars(pdev, false, false);
+/*
+ * Since this operation is deferred at the point when it finishes the
+ * device might have been removed, so don't attempt to disable memory
+ * decoding afterwards.
+ */
+current->vpci.pdev = NULL;
+}
+}
+REGISTER_VPCI_INIT(init_bars, teardown_bars, VPCI_PRIORITY_MIDDLE);
 
 /*
  * Local variables:
-- 
2.17.1


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel