[PATCH v4 0/5] x86/pvh: fix fixes for PVH Dom0

[PATCH v4 0/5] x86/pvh: fix fixes for PVH Dom0

[Roger Pau Monne]

Hello,

The following series contain miscellaneous fixes for a PVH Dom0. I've
found out this issues while trying to boot on an AMD EPYC box.

The series can be found on my git repo:

git://xenbits.xen.org/people/royger/xen.git fixes-pvh-v4

Roger Pau Monne (5):
  vpci: fix updating the command register
  vpci: fix deferral of long operations
  vpci/msix: carve p2m hole for MSIX MMIO regions
  amd/iommu: assign iommu devices to Xen
  amd/iommu: skip bridge devices when updating IOMMU page tables

 xen/arch/x86/hvm/ioreq.c                 |  6 +--
 xen/drivers/passthrough/amd/iommu_init.c | 10 +++++
 xen/drivers/passthrough/amd/iommu_map.c  |  4 ++
 xen/drivers/vpci/header.c                | 50 +++++++++++++++---------
 xen/drivers/vpci/msix.c                  | 49 +++++++++++++++++++++++
 xen/include/xen/vpci.h                   |  5 ++-
 6 files changed, 102 insertions(+), 22 deletions(-)

-- 
2.19.1


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

[PATCH v4 1/5] vpci: fix updating the command register

[Roger Pau Monne]

When switching the memory decoding bit in the command register the
rest of the changes where dropped, leading to only the memory decoding
bit being updated.

Fix this by writing the command register once the guest physmap
manipulations are done if there are changes to the memory decoding
bit.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v3:
 - Only update the command register once after the physmap changes are
   done.
---
 xen/drivers/vpci/header.c | 35 +++++++++++++++++------------------
 xen/include/xen/vpci.h    |  2 +-
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 4573ccadf0..162d51f7e2 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -81,11 +81,12 @@ static int map_range(unsigned long s, unsigned long e, void *data,
  * BAR's enable bit has changed with the memory decoding bit already enabled.
  * If rom_only is not set then it's the memory decoding bit that changed.
  */
-static void modify_decoding(const struct pci_dev *pdev, bool map, bool rom_only)
+static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
+                            bool rom_only)
 {
     struct vpci_header *header = &pdev->vpci->header;
     uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
-    uint16_t cmd;
+    bool map = cmd & PCI_COMMAND_MEMORY;
     unsigned int i;
 
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
@@ -111,9 +112,6 @@ static void modify_decoding(const struct pci_dev *pdev, bool map, bool rom_only)
     }
 
     ASSERT(!rom_only);
-    cmd = pci_conf_read16(pdev->seg, pdev->bus, slot, func, PCI_COMMAND);
-    cmd &= ~PCI_COMMAND_MEMORY;
-    cmd |= map ? PCI_COMMAND_MEMORY : 0;
     pci_conf_write16(pdev->seg, pdev->bus, slot, func, PCI_COMMAND,
                      cmd);
 }
@@ -124,7 +122,7 @@ bool vpci_process_pending(struct vcpu *v)
     {
         struct map_data data = {
             .d = v->domain,
-            .map = v->vpci.map,
+            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
         };
         int rc = rangeset_consume_ranges(v->vpci.mem, map_range, &data);
 
@@ -133,7 +131,8 @@ bool vpci_process_pending(struct vcpu *v)
 
         spin_lock(&v->vpci.pdev->vpci->lock);
         /* Disable memory decoding unconditionally on failure. */
-        modify_decoding(v->vpci.pdev, !rc && v->vpci.map,
+        modify_decoding(v->vpci.pdev,
+                        rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
                         !rc && v->vpci.rom_only);
         spin_unlock(&v->vpci.pdev->vpci->lock);
 
@@ -154,7 +153,7 @@ bool vpci_process_pending(struct vcpu *v)
 }
 
 static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
-                            struct rangeset *mem)
+                            struct rangeset *mem, uint16_t cmd)
 {
     struct map_data data = { .d = d, .map = true };
     int rc;
@@ -163,13 +162,13 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
         process_pending_softirqs();
     rangeset_destroy(mem);
     if ( !rc )
-        modify_decoding(pdev, true, false);
+        modify_decoding(pdev, cmd, false);
 
     return rc;
 }
 
 static void defer_map(struct domain *d, struct pci_dev *pdev,
-                      struct rangeset *mem, bool map, bool rom_only)
+                      struct rangeset *mem, uint16_t cmd, bool rom_only)
 {
     struct vcpu *curr = current;
 
@@ -181,11 +180,11 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
      */
     curr->vpci.pdev = pdev;
     curr->vpci.mem = mem;
-    curr->vpci.map = map;
+    curr->vpci.cmd = cmd;
     curr->vpci.rom_only = rom_only;
 }
 
-static int modify_bars(const struct pci_dev *pdev, bool map, bool rom_only)
+static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 {
     struct vpci_header *header = &pdev->vpci->header;
     struct rangeset *mem = rangeset_new(NULL, NULL, 0);
@@ -305,11 +304,11 @@ static int modify_bars(const struct pci_dev *pdev, bool map, bool rom_only)
          * be called iff the memory decoding bit is enabled, thus the operation
          * will always be to establish mappings and process all the BARs.
          */
-        ASSERT(map && !rom_only);
-        return apply_map(pdev->domain, pdev, mem);
+        ASSERT((cmd & PCI_COMMAND_MEMORY) && !rom_only);
+        return apply_map(pdev->domain, pdev, mem, cmd);
     }
 
-    defer_map(dev->domain, dev, mem, map, rom_only);
+    defer_map(dev->domain, dev, mem, cmd, rom_only);
 
     return 0;
 }
@@ -332,7 +331,7 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
          * memory decoding bit has not been changed, so leave everything as-is,
          * hoping the guest will realize and try again.
          */
-        modify_bars(pdev, cmd & PCI_COMMAND_MEMORY, false);
+        modify_bars(pdev, cmd, false);
     else
         pci_conf_write16(pdev->seg, pdev->bus, slot, func, reg, cmd);
 }
@@ -413,7 +412,7 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
         header->rom_enabled = new_enabled;
         pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
     }
-    else if ( modify_bars(pdev, new_enabled, true) )
+    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
         /*
          * No memory has been added or removed from the p2m (because the actual
          * p2m changes are deferred in defer_map) and the ROM enable bit has
@@ -549,7 +548,7 @@ static int init_bars(struct pci_dev *pdev)
             rom->type = VPCI_BAR_EMPTY;
     }
 
-    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, true, false) : 0;
+    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
 }
 REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
 
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index af2b8580ee..44104b75b6 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -145,7 +145,7 @@ struct vpci_vcpu {
     /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
     struct rangeset *mem;
     struct pci_dev *pdev;
-    bool map      : 1;
+    uint16_t cmd;
     bool rom_only : 1;
 };
 
-- 
2.19.1


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

Re: [PATCH v4 1/5] vpci: fix updating the command register

[Jan Beulich]

>>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> @@ -413,7 +412,7 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>          header->rom_enabled = new_enabled;
>          pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
>      }
> -    else if ( modify_bars(pdev, new_enabled, true) )
> +    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )

Do you really mean to clear all other defined bits of the command
register here?

Jan



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

Re: [PATCH v4 1/5] vpci: fix updating the command register

[Roger Pau Monné]

On Fri, Nov 16, 2018 at 05:00:29AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> > @@ -413,7 +412,7 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
> >          header->rom_enabled = new_enabled;
> >          pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
> >      }
> > -    else if ( modify_bars(pdev, new_enabled, true) )
> > +    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
> 
> Do you really mean to clear all other defined bits of the command
> register here?

This is a ROM BAR write, not a command register write. rom_write
passes PCI_COMMAND_MEMORY merely to signal this is a mapping
operation, but the value would never be written to the command
register, there's an ASSERT(!rom_only) just before the deferred write
of the command register in modify_decoding.

Thanks, Roger.

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

Re: [PATCH v4 1/5] vpci: fix updating the command register

[Jan Beulich]

>>> On 16.11.18 at 15:32, <roger.pau@citrix.com> wrote:
> On Fri, Nov 16, 2018 at 05:00:29AM -0700, Jan Beulich wrote:
>> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
>> > @@ -413,7 +412,7 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>> >          header->rom_enabled = new_enabled;
>> >          pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
>> >      }
>> > -    else if ( modify_bars(pdev, new_enabled, true) )
>> > +    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
>> 
>> Do you really mean to clear all other defined bits of the command
>> register here?
> 
> This is a ROM BAR write, not a command register write. rom_write
> passes PCI_COMMAND_MEMORY merely to signal this is a mapping
> operation, but the value would never be written to the command
> register, there's an ASSERT(!rom_only) just before the deferred write
> of the command register in modify_decoding.

Oh, I see. This is getting more subtle than it already was, so perhaps
worth attaching a brief comment here, the more that if anything was
wrong with the logic bad behavior would result in release builds?

Jan



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

Re: [PATCH v4 1/5] vpci: fix updating the command register

[Roger Pau Monné]

On Mon, Nov 19, 2018 at 01:26:11AM -0700, Jan Beulich wrote:
> >>> On 16.11.18 at 15:32, <roger.pau@citrix.com> wrote:
> > On Fri, Nov 16, 2018 at 05:00:29AM -0700, Jan Beulich wrote:
> >> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> >> > @@ -413,7 +412,7 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
> >> >          header->rom_enabled = new_enabled;
> >> >          pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
> >> >      }
> >> > -    else if ( modify_bars(pdev, new_enabled, true) )
> >> > +    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
> >> 
> >> Do you really mean to clear all other defined bits of the command
> >> register here?
> > 
> > This is a ROM BAR write, not a command register write. rom_write
> > passes PCI_COMMAND_MEMORY merely to signal this is a mapping
> > operation, but the value would never be written to the command
> > register, there's an ASSERT(!rom_only) just before the deferred write
> > of the command register in modify_decoding.
> 
> Oh, I see. This is getting more subtle than it already was, so perhaps
> worth attaching a brief comment here, the more that if anything was
> wrong with the logic bad behavior would result in release builds?

It would, I could change the ASSERT to a BUG in modify_decoding if
that seems more foolproof.

Thanks, Roger.

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

Re: [PATCH v4 1/5] vpci: fix updating the command register

[Jan Beulich]

>>> On 19.11.18 at 12:09, <roger.pau@citrix.com> wrote:
> On Mon, Nov 19, 2018 at 01:26:11AM -0700, Jan Beulich wrote:
>> >>> On 16.11.18 at 15:32, <roger.pau@citrix.com> wrote:
>> > On Fri, Nov 16, 2018 at 05:00:29AM -0700, Jan Beulich wrote:
>> >> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
>> >> > @@ -413,7 +412,7 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>> >> >          header->rom_enabled = new_enabled;
>> >> >          pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
>> >> >      }
>> >> > -    else if ( modify_bars(pdev, new_enabled, true) )
>> >> > +    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
>> >> 
>> >> Do you really mean to clear all other defined bits of the command
>> >> register here?
>> > 
>> > This is a ROM BAR write, not a command register write. rom_write
>> > passes PCI_COMMAND_MEMORY merely to signal this is a mapping
>> > operation, but the value would never be written to the command
>> > register, there's an ASSERT(!rom_only) just before the deferred write
>> > of the command register in modify_decoding.
>> 
>> Oh, I see. This is getting more subtle than it already was, so perhaps
>> worth attaching a brief comment here, the more that if anything was
>> wrong with the logic bad behavior would result in release builds?
> 
> It would, I could change the ASSERT to a BUG in modify_decoding if
> that seems more foolproof.

I intentionally did not suggest such a conversion: I'd like us to
stop bringing down the entire host when a problem related to
just one guest is encountered. That said - as long as vPCI is
for the hardware domain only, the transformation (annotated
to indicate it needs adjustment when widening exposure to
DomU-s) would perhaps be acceptable.

Jan



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

[PATCH v4 2/5] vpci: fix deferral of long operations

[Roger Pau Monne]

Current logic to handle long running operations has two flaws:

 - hvm_io_pending is only used by Intel code, fix this by moving the
   call to vpci_process_pending into handle_hvm_io_completion.
 - Raise a scheduler softirq when preemption is required. The
   do_softirq calls in the SVM/VMX guest entry points will make sure
   the guest vcpu is not restarted until all the pending work is
   finished.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
---
Changes since v3:
 - Don't use a tasklet.
---
 xen/arch/x86/hvm/ioreq.c  | 6 +++---
 xen/drivers/vpci/header.c | 4 ++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index e2e755a8a1..4c4b33e220 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -85,9 +85,6 @@ bool hvm_io_pending(struct vcpu *v)
     struct hvm_ioreq_server *s;
     unsigned int id;
 
-    if ( has_vpci(d) && vpci_process_pending(v) )
-        return true;
-
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
         struct hvm_ioreq_vcpu *sv;
@@ -186,6 +183,9 @@ bool handle_hvm_io_completion(struct vcpu *v)
     enum hvm_io_completion io_completion;
     unsigned int id;
 
+    if ( has_vpci(d) && vpci_process_pending(v) )
+        return false;
+
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
         struct hvm_ioreq_vcpu *sv;
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 162d51f7e2..720dec07fa 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -127,7 +127,10 @@ bool vpci_process_pending(struct vcpu *v)
         int rc = rangeset_consume_ranges(v->vpci.mem, map_range, &data);
 
         if ( rc == -ERESTART )
+        {
+            raise_softirq(SCHEDULE_SOFTIRQ);
             return true;
+        }
 
         spin_lock(&v->vpci.pdev->vpci->lock);
         /* Disable memory decoding unconditionally on failure. */
@@ -182,6 +185,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
     curr->vpci.mem = mem;
     curr->vpci.cmd = cmd;
     curr->vpci.rom_only = rom_only;
+    raise_softirq(SCHEDULE_SOFTIRQ);
 }
 
 static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
-- 
2.19.1


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

Re: [PATCH v4 2/5] vpci: fix deferral of long operations

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> Sent: 14 November 2018 11:58
> To: xen-devel@lists.xenproject.org
> Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Julien
> Grall <julien.grall@arm.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Tim
> (Xen.org) <tim@xen.org>
> Subject: [PATCH v4 2/5] vpci: fix deferral of long operations
> 
> Current logic to handle long running operations has two flaws:
> 
>  - hvm_io_pending is only used by Intel code, fix this by moving the
>    call to vpci_process_pending into handle_hvm_io_completion.
>  - Raise a scheduler softirq when preemption is required. The
>    do_softirq calls in the SVM/VMX guest entry points will make sure
>    the guest vcpu is not restarted until all the pending work is
>    finished.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> Cc: Paul Durrant <paul.durrant@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> ---
> Changes since v3:
>  - Don't use a tasklet.
> ---
>  xen/arch/x86/hvm/ioreq.c  | 6 +++---
>  xen/drivers/vpci/header.c | 4 ++++
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index e2e755a8a1..4c4b33e220 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -85,9 +85,6 @@ bool hvm_io_pending(struct vcpu *v)
>      struct hvm_ioreq_server *s;
>      unsigned int id;
> 
> -    if ( has_vpci(d) && vpci_process_pending(v) )
> -        return true;
> -
>      FOR_EACH_IOREQ_SERVER(d, id, s)
>      {
>          struct hvm_ioreq_vcpu *sv;
> @@ -186,6 +183,9 @@ bool handle_hvm_io_completion(struct vcpu *v)
>      enum hvm_io_completion io_completion;
>      unsigned int id;
> 
> +    if ( has_vpci(d) && vpci_process_pending(v) )
> +        return false;
> +
>      FOR_EACH_IOREQ_SERVER(d, id, s)
>      {
>          struct hvm_ioreq_vcpu *sv;
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 162d51f7e2..720dec07fa 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -127,7 +127,10 @@ bool vpci_process_pending(struct vcpu *v)
>          int rc = rangeset_consume_ranges(v->vpci.mem, map_range, &data);
> 
>          if ( rc == -ERESTART )
> +        {
> +            raise_softirq(SCHEDULE_SOFTIRQ);
>              return true;
> +        }
> 
>          spin_lock(&v->vpci.pdev->vpci->lock);
>          /* Disable memory decoding unconditionally on failure. */
> @@ -182,6 +185,7 @@ static void defer_map(struct domain *d, struct pci_dev
> *pdev,
>      curr->vpci.mem = mem;
>      curr->vpci.cmd = cmd;
>      curr->vpci.rom_only = rom_only;
> +    raise_softirq(SCHEDULE_SOFTIRQ);
>  }
> 
>  static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool
> rom_only)
> --
> 2.19.1

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

Re: [PATCH v4 2/5] vpci: fix deferral of long operations

[Jan Beulich]

>>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> Current logic to handle long running operations has two flaws:
> 
>  - hvm_io_pending is only used by Intel code, fix this by moving the
>    call to vpci_process_pending into handle_hvm_io_completion.

As mentioned before, the reference to Intel code is wrong here.
The main caller is handle_pio(), and the Intel specific one sits in
nested VMX code anyway, so is of basically no interest for the
purposes here.

> @@ -186,6 +183,9 @@ bool handle_hvm_io_completion(struct vcpu *v)
>      enum hvm_io_completion io_completion;
>      unsigned int id;
>  
> +    if ( has_vpci(d) && vpci_process_pending(v) )
> +        return false;

I think this would be the better place to raise the scheduler softirq.
vpci_process_pending() should indicated to its caller just whether
preemption is needed, but should not care how this is actually to
be effected. This is benign now, but might change if a second
caller would need adding.

> @@ -182,6 +185,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>      curr->vpci.mem = mem;
>      curr->vpci.cmd = cmd;
>      curr->vpci.rom_only = rom_only;
> +    raise_softirq(SCHEDULE_SOFTIRQ);
>  }

Why is this needed? The description doesn't mention it.

Jan



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

Re: [PATCH v4 2/5] vpci: fix deferral of long operations

[Roger Pau Monné]

On Fri, Nov 16, 2018 at 05:11:39AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> > Current logic to handle long running operations has two flaws:
> > 
> >  - hvm_io_pending is only used by Intel code, fix this by moving the
> >    call to vpci_process_pending into handle_hvm_io_completion.
> 
> As mentioned before, the reference to Intel code is wrong here.
> The main caller is handle_pio(), and the Intel specific one sits in
> nested VMX code anyway, so is of basically no interest for the
> purposes here.
> 
> > @@ -186,6 +183,9 @@ bool handle_hvm_io_completion(struct vcpu *v)
> >      enum hvm_io_completion io_completion;
> >      unsigned int id;
> >  
> > +    if ( has_vpci(d) && vpci_process_pending(v) )
> > +        return false;
> 
> I think this would be the better place to raise the scheduler softirq.
> vpci_process_pending() should indicated to its caller just whether
> preemption is needed, but should not care how this is actually to
> be effected. This is benign now, but might change if a second
> caller would need adding.

Ack.

> > @@ -182,6 +185,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
> >      curr->vpci.mem = mem;
> >      curr->vpci.cmd = cmd;
> >      curr->vpci.rom_only = rom_only;
> > +    raise_softirq(SCHEDULE_SOFTIRQ);
> >  }
> 
> Why is this needed? The description doesn't mention it.

vPCI needs to raise a shceduler softirq in order to prevent the guest
from resuming execution after deferring a map/unmap operation. Without
this the guest might just resume execution while having pending work
if there's no scheduler softirq already pending.

Thanks, Roger.

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

Re: [PATCH v4 2/5] vpci: fix deferral of long operations

[Jan Beulich]

>>> On 16.11.18 at 15:57, <roger.pau@citrix.com> wrote:
> On Fri, Nov 16, 2018 at 05:11:39AM -0700, Jan Beulich wrote:
>> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
>> > @@ -182,6 +185,7 @@ static void defer_map(struct domain *d, struct pci_dev *pdev,
>> >      curr->vpci.mem = mem;
>> >      curr->vpci.cmd = cmd;
>> >      curr->vpci.rom_only = rom_only;
>> > +    raise_softirq(SCHEDULE_SOFTIRQ);
>> >  }
>> 
>> Why is this needed? The description doesn't mention it.
> 
> vPCI needs to raise a shceduler softirq in order to prevent the guest
> from resuming execution after deferring a map/unmap operation. Without
> this the guest might just resume execution while having pending work
> if there's no scheduler softirq already pending.

Ah yes, but please again attach a brief comment to clarify this.

Jan



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

[PATCH v4 3/5] vpci/msix: carve p2m hole for MSIX MMIO regions

[Roger Pau Monne]

Make sure the MSIX MMIO regions don't have p2m entries setup, so that
accesses to them trap into the hypervisor and can be handled by vpci.

Commit 042678762 ("x86/iommu: add map-reserved dom0-iommu option to
map reserved memory ranges") added mappings for all the reserved
regions into the PVH Dom0 p2m, and some of those reserved regions
might contain MSIX MMIO regions, hence the need to make sure there are
no mappings established.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v3:
 - Allow p2m_invalid for unmapped regions.
 - Add a DomU FIXME comment.
 - Reword commit message.
---
 xen/drivers/vpci/header.c | 11 +++++++++
 xen/drivers/vpci/msix.c   | 49 +++++++++++++++++++++++++++++++++++++++
 xen/include/xen/vpci.h    |  3 +++
 3 files changed, 63 insertions(+)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 720dec07fa..f761804beb 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -89,6 +89,17 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
     bool map = cmd & PCI_COMMAND_MEMORY;
     unsigned int i;
 
+    /*
+     * Make sure there are no mappings in the MSIX MMIO areas, so that accesses
+     * can be trapped (and emulated) by Xen when the memory decoding bit is
+     * enabled.
+     *
+     * FIXME: punching holes after the p2m has been set up might be racy for
+     * DomU usage, needs to be revisited.
+     */
+    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
+        return;
+
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
     {
         if ( !MAPPABLE_BAR(&header->bars[i]) )
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 1960dae123..af3ffa087d 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -21,6 +21,7 @@
 #include <xen/vpci.h>
 
 #include <asm/msi.h>
+#include <asm/p2m.h>
 
 #define VMSIX_SIZE(num) offsetof(struct vpci_msix, entries[num])
 
@@ -395,6 +396,54 @@ static const struct hvm_mmio_ops vpci_msix_table_ops = {
     .write = msix_write,
 };
 
+int vpci_make_msix_hole(const struct pci_dev *pdev)
+{
+    struct domain *d = pdev->domain;
+    unsigned int i;
+
+    if ( !pdev->vpci->msix )
+        return 0;
+
+    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
+    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
+    {
+        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
+        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
+                                     vmsix_table_size(pdev->vpci, i) - 1);
+
+        for ( ; start <= end; start++ )
+        {
+            p2m_type_t t;
+            mfn_t mfn = get_gfn_query(d, start, &t);
+
+            switch ( t )
+            {
+            case p2m_mmio_dm:
+            case p2m_invalid:
+                break;
+            case p2m_mmio_direct:
+                if ( mfn_x(mfn) == start )
+                {
+                    clear_identity_p2m_entry(d, start);
+                    break;
+                }
+                /* fallthrough. */
+            default:
+                put_gfn(d, start);
+                gprintk(XENLOG_WARNING,
+                        "%04x:%02x:%02x.%u: existing mapping (mfn: %" PRI_mfn
+                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
+                        pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
+                        PCI_FUNC(pdev->devfn), mfn_x(mfn), t, start);
+                return -EEXIST;
+            }
+            put_gfn(d, start);
+        }
+    }
+
+    return 0;
+}
+
 static int init_msix(struct pci_dev *pdev)
 {
     struct domain *d = pdev->domain;
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 44104b75b6..4cf233c779 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -152,6 +152,9 @@ struct vpci_vcpu {
 #ifdef __XEN__
 void vpci_dump_msi(void);
 
+/* Make sure there's a hole in the p2m for the MSIX mmio areas. */
+int vpci_make_msix_hole(const struct pci_dev *pdev);
+
 /* Arch-specific vPCI MSI helpers. */
 void vpci_msi_arch_mask(struct vpci_msi *msi, const struct pci_dev *pdev,
                         unsigned int entry, bool mask);
-- 
2.19.1


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

Re: [PATCH v4 3/5] vpci/msix: carve p2m hole for MSIX MMIO regions

[Jan Beulich]

>>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> Make sure the MSIX MMIO regions don't have p2m entries setup, so that
> accesses to them trap into the hypervisor and can be handled by vpci.
> 
> Commit 042678762 ("x86/iommu: add map-reserved dom0-iommu option to
> map reserved memory ranges") added mappings for all the reserved
> regions into the PVH Dom0 p2m, and some of those reserved regions
> might contain MSIX MMIO regions, hence the need to make sure there are
> no mappings established.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
despite ...

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -89,6 +89,17 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
>      bool map = cmd & PCI_COMMAND_MEMORY;
>      unsigned int i;
>  
> +    /*
> +     * Make sure there are no mappings in the MSIX MMIO areas, so that accesses
> +     * can be trapped (and emulated) by Xen when the memory decoding bit is
> +     * enabled.
> +     *
> +     * FIXME: punching holes after the p2m has been set up might be racy for
> +     * DomU usage, needs to be revisited.
> +     */
> +    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
> +        return;

... this still worrying me, as I think it'll need to be re-implemented
altogether at that later point.

Afaict this patch is independent of the earlier two (leaving aside
some fuzz that would be easy to resolve), which means it could
go in ahead of them.

Jan


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

Re: [PATCH v4 3/5] vpci/msix: carve p2m hole for MSIX MMIO regions

[Roger Pau Monné]

On Mon, Nov 19, 2018 at 07:56:18AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> > Make sure the MSIX MMIO regions don't have p2m entries setup, so that
> > accesses to them trap into the hypervisor and can be handled by vpci.
> > 
> > Commit 042678762 ("x86/iommu: add map-reserved dom0-iommu option to
> > map reserved memory ranges") added mappings for all the reserved
> > regions into the PVH Dom0 p2m, and some of those reserved regions
> > might contain MSIX MMIO regions, hence the need to make sure there are
> > no mappings established.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> despite ...
> 
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -89,6 +89,17 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
> >      bool map = cmd & PCI_COMMAND_MEMORY;
> >      unsigned int i;
> >  
> > +    /*
> > +     * Make sure there are no mappings in the MSIX MMIO areas, so that accesses
> > +     * can be trapped (and emulated) by Xen when the memory decoding bit is
> > +     * enabled.
> > +     *
> > +     * FIXME: punching holes after the p2m has been set up might be racy for
> > +     * DomU usage, needs to be revisited.
> > +     */
> > +    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
> > +        return;
> 
> ... this still worrying me, as I think it'll need to be re-implemented
> altogether at that later point.

I expect this whole BAR mapping and unmapping to be limited to Dom0,
DomU should have a much more static physmap so that it can be setup by
the toolstack at creation time and the guest should not be allowed to
modify it.

That being set I also don't specially like this code, it's too
complex and requires iteration over all the devices for every
map/unmap operation. I have plans to improve this, which should be
discussed in a different email, but that's post-4.12.

> Afaict this patch is independent of the earlier two (leaving aside
> some fuzz that would be easy to resolve), which means it could
> go in ahead of them.

I plan to resend the series quite soon with the other comments fixed,
so let me know if I should drop this patch, I will also keep an eye on
the repo.

Thanks, Roger.

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

[PATCH v4 4/5] amd/iommu: assign iommu devices to Xen

[Roger Pau Monne]

AMD IOMMU devices are exposed on the PCI bus, and thus are assigned by
default to the hardware domain. This can cause issues because the
IOMMU devices are not behind an IOMMU, and conceptually it's also wrong
to give the hardware domain ownership of those devices since they are
in use by Xen.

Fix this by assigning the PCI IOMMU devices to Xen.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
---
 xen/drivers/passthrough/amd/iommu_init.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 15c10b0929..c74c1566d9 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -993,6 +993,16 @@ static void * __init allocate_ppr_log(struct amd_iommu *iommu)
 
 static int __init amd_iommu_init_one(struct amd_iommu *iommu)
 {
+    struct pci_dev *pdev;
+
+    pcidevs_lock();
+    pdev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
+                        PCI_DEVFN2(iommu->bdf));
+    if ( pdev )
+        /* Assign the IOMMU PCI device to Xen  */
+        pdev->domain = dom_xen;
+    pcidevs_unlock();
+
     if ( map_iommu_mmio_region(iommu) != 0 )
         goto error_out;
 
-- 
2.19.1


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

Re: [PATCH v4 4/5] amd/iommu: assign iommu devices to Xen

[Andrew Cooper]

On 14/11/2018 11:57, Roger Pau Monne wrote:
> AMD IOMMU devices are exposed on the PCI bus, and thus are assigned by
> default to the hardware domain. This can cause issues because the
> IOMMU devices are not behind an IOMMU, and conceptually it's also wrong
> to give the hardware domain ownership of those devices since they are
> in use by Xen.
>
> Fix this by assigning the PCI IOMMU devices to Xen.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

This is unfortunately a symptom of much more basic bug in Xen.

Particularly on recent server parts, there are many PCI devices which
represent processor internals and aren't safe to give even to dom0.

There should be a whitelist of devices we consider safe, not a blacklist
of those we know to be unsafe.

Most of this can be class based, and perhaps we can default-allow all
devices which are slots in a root port, but I am -1 to this patch
because it is fixing a symptom, not the problem.

~Andrew

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

Re: [PATCH v4 4/5] amd/iommu: assign iommu devices to Xen

[Jan Beulich]

>>> On 14.11.18 at 13:33, <andrew.cooper3@citrix.com> wrote:
> On 14/11/2018 11:57, Roger Pau Monne wrote:
>> AMD IOMMU devices are exposed on the PCI bus, and thus are assigned by
>> default to the hardware domain. This can cause issues because the
>> IOMMU devices are not behind an IOMMU, and conceptually it's also wrong
>> to give the hardware domain ownership of those devices since they are
>> in use by Xen.
>>
>> Fix this by assigning the PCI IOMMU devices to Xen.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> This is unfortunately a symptom of much more basic bug in Xen.
> 
> Particularly on recent server parts, there are many PCI devices which
> represent processor internals and aren't safe to give even to dom0.

Well, yes, some perhaps.

> There should be a whitelist of devices we consider safe, not a blacklist
> of those we know to be unsafe.

Such an approach would, I'm afraid, be workable only if vendors
(pro)actively communicated properties and device IDs, such that
we would not constantly lag behind what's on the market.

Jan


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

Re: [PATCH v4 4/5] amd/iommu: assign iommu devices to Xen

[Roger Pau Monné]

On Wed, Nov 14, 2018 at 12:33:46PM +0000, Andrew Cooper wrote:
> On 14/11/2018 11:57, Roger Pau Monne wrote:
> > AMD IOMMU devices are exposed on the PCI bus, and thus are assigned by
> > default to the hardware domain. This can cause issues because the
> > IOMMU devices are not behind an IOMMU, and conceptually it's also wrong
> > to give the hardware domain ownership of those devices since they are
> > in use by Xen.
> >
> > Fix this by assigning the PCI IOMMU devices to Xen.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> This is unfortunately a symptom of much more basic bug in Xen.
> 
> Particularly on recent server parts, there are many PCI devices which
> represent processor internals and aren't safe to give even to dom0.
> 
> There should be a whitelist of devices we consider safe, not a blacklist
> of those we know to be unsafe.
> 
> Most of this can be class based, and perhaps we can default-allow all
> devices which are slots in a root port, but I am -1 to this patch
> because it is fixing a symptom, not the problem.

While the whitelisting sounds fine to me, I still think we need this
patch anyway.

If we look at the IOMMU specific case, the device class should be 8
(system peripheral) and subclass 6 (IOMMU), but it's quite likely
there are IOMMU devices with class 8 and subclass 0x80 (generic
peripheral).

In the above case we know for sure the sbdf of the IOMMU devices, so I
think it doesn't hurt to assign them to Xen straight away, regardless
of whether we end up doing a whitelisting before assigning devices to
the hardware domain.

Thanks, Roger.

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

Re: [PATCH v4 4/5] amd/iommu: assign iommu devices to Xen

[Jan Beulich]

>>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -993,6 +993,16 @@ static void * __init allocate_ppr_log(struct amd_iommu *iommu)
>  
>  static int __init amd_iommu_init_one(struct amd_iommu *iommu)
>  {
> +    struct pci_dev *pdev;
> +
> +    pcidevs_lock();
> +    pdev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
> +                        PCI_DEVFN2(iommu->bdf));
> +    if ( pdev )
> +        /* Assign the IOMMU PCI device to Xen  */
> +        pdev->domain = dom_xen;
> +    pcidevs_unlock();

Why do you kind of open-code pci_hide_device()? It would need
extending to cope with a non-zero segment number, but I'd much
prefer if there could be one central place where the logic lives.
That way list addition would also not be omitted, like you do.

As to the hiding in general, also considering Andrew's objection:
Are these devices representing the IOMMU and nothing else? As
mentioned by Andrew something similar would be needed on the
VT-d side, but iirc there's less clear of a relationship there in any
event (which causes me to wonder about the situation on the
AMD side). I'm asking not the least because iirc at the time
pci_hide_device() was introduced I think it was considered to
hide the AMD IOMMU devices; I don't recall why we didn't in the
end, though.

Jan



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

Re: [PATCH v4 4/5] amd/iommu: assign iommu devices to Xen

[Roger Pau Monné]

On Thu, Nov 15, 2018 at 08:34:44AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -993,6 +993,16 @@ static void * __init allocate_ppr_log(struct amd_iommu *iommu)
> >  
> >  static int __init amd_iommu_init_one(struct amd_iommu *iommu)
> >  {
> > +    struct pci_dev *pdev;
> > +
> > +    pcidevs_lock();
> > +    pdev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
> > +                        PCI_DEVFN2(iommu->bdf));
> > +    if ( pdev )
> > +        /* Assign the IOMMU PCI device to Xen  */
> > +        pdev->domain = dom_xen;
> > +    pcidevs_unlock();
> 
> Why do you kind of open-code pci_hide_device()? It would need
> extending to cope with a non-zero segment number, but I'd much
> prefer if there could be one central place where the logic lives.
> That way list addition would also not be omitted, like you do.

Sure, expanding pci_hide_device is better, I just didn't realize this
function existed in the first place.

> As to the hiding in general, also considering Andrew's objection:
> Are these devices representing the IOMMU and nothing else? As
> mentioned by Andrew something similar would be needed on the
> VT-d side, but iirc there's less clear of a relationship there in any
> event (which causes me to wonder about the situation on the
> AMD side). I'm asking not the least because iirc at the time
> pci_hide_device() was introduced I think it was considered to
> hide the AMD IOMMU devices; I don't recall why we didn't in the
> end, though.

I think this would be easier if I give more context about the issue
I'm hitting here, which is very similar to the issue patch 5/5
attempts to address.

The problem is that the IOMMU PCI device itself is obviously not
behind an IOMMU, and update_paging_mode will return an error if it
finds any such device in the domain list when attempting to expand the
IOMMU page tables:

...
iommu = find_iommu_for_device(pdev->seg, bdf);
if ( !iommu )
{
    AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
    return -ENODEV;
}
...

Another option is to allow the hardware domain to have assigned
devices that a not behind an IOMMU, but I would consider this more
like a workaround rather than a real fix.

I'm not sure whether the IOMMU AMD PCI device could also represent
something else, Maybe Brian can provide more insight on whether there
might be other platform devices encompassed in the same PCI device as
the IOMMU.

Thanks, Roger.

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

[PATCH v4 5/5] amd/iommu: skip bridge devices when updating IOMMU page tables

[Roger Pau Monne]

Bridges are not behind an IOMMU, and are already special cased and
silently skipped in amd_iommu_add_device. Apply the same special
casing when updating page tables.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
---
 xen/drivers/passthrough/amd/iommu_map.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 6a2c877d34..62292a9015 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -593,6 +593,10 @@ static int update_paging_mode(struct domain *d, unsigned long dfn)
         /* Update device table entries using new root table and paging mode */
         for_each_pdev( d, pdev )
         {
+            if ( is_hardware_domain(d) &&
+                 pdev->type == DEV_TYPE_PCI_HOST_BRIDGE )
+                continue;
+
             bdf = PCI_BDF2(pdev->bus, pdev->devfn);
             iommu = find_iommu_for_device(pdev->seg, bdf);
             if ( !iommu )
-- 
2.19.1


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

Re: [PATCH v4 5/5] amd/iommu: skip bridge devices when updating IOMMU page tables

[Jan Beulich]

>>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> Bridges are not behind an IOMMU, and are already special cased and
> silently skipped in amd_iommu_add_device. Apply the same special
> casing when updating page tables.

But bridges also don't issue I/O on their own if I'm not mistaken. So
what I'm missing here is a word on the benefit of this change. I also
question the "silently" in your wording, seeing the AMD_IOMMU_DEBUG()
there.

The code change itself looks fine to me, albeit personally I'd prefer
if it fully matched the other conditional (i.e. if you flipped the
operands of && ). Of course the special casing of the hardware
domain here is somewhat odd anyway.

Jan



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

Re: [PATCH v4 5/5] amd/iommu: skip bridge devices when updating IOMMU page tables

[Roger Pau Monné]

On Thu, Nov 15, 2018 at 08:40:11AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> > Bridges are not behind an IOMMU, and are already special cased and
> > silently skipped in amd_iommu_add_device. Apply the same special
> > casing when updating page tables.
> 
> But bridges also don't issue I/O on their own if I'm not mistaken. So
> what I'm missing here is a word on the benefit of this change. I also
> question the "silently" in your wording, seeing the AMD_IOMMU_DEBUG()
> there.

I see, by silently I meant without throwing an error, but I think just
using 'skipped' would be clearer.

The benefit is that update_paging_mode doesn't return an error when it
finds a bridge attached to Dom0, which would cause the caller of
update_paging_mode (amd_iommu_{un}map_page) to crash the domain.

Ie: without this change a PVH Dom0 running on AMD hardware crashes
when the IOMMU page table is expanded.

> The code change itself looks fine to me, albeit personally I'd prefer
> if it fully matched the other conditional (i.e. if you flipped the
> operands of && ). Of course the special casing of the hardware
> domain here is somewhat odd anyway.

Do you mean because bridges would only be ever assigned to the
hardware domain?

Thanks, Roger.

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

Re: [PATCH v4 5/5] amd/iommu: skip bridge devices when updating IOMMU page tables

[Jan Beulich]

>>> On 15.11.18 at 16:48, <roger.pau@citrix.com> wrote:
> On Thu, Nov 15, 2018 at 08:40:11AM -0700, Jan Beulich wrote:
>> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
>> > Bridges are not behind an IOMMU, and are already special cased and
>> > silently skipped in amd_iommu_add_device. Apply the same special
>> > casing when updating page tables.
>> 
>> But bridges also don't issue I/O on their own if I'm not mistaken. So
>> what I'm missing here is a word on the benefit of this change. I also
>> question the "silently" in your wording, seeing the AMD_IOMMU_DEBUG()
>> there.
> 
> I see, by silently I meant without throwing an error, but I think just
> using 'skipped' would be clearer.
> 
> The benefit is that update_paging_mode doesn't return an error when it
> finds a bridge attached to Dom0, which would cause the caller of
> update_paging_mode (amd_iommu_{un}map_page) to crash the domain.
> 
> Ie: without this change a PVH Dom0 running on AMD hardware crashes
> when the IOMMU page table is expanded.

You want to say so in the description then.

>> The code change itself looks fine to me, albeit personally I'd prefer
>> if it fully matched the other conditional (i.e. if you flipped the
>> operands of && ). Of course the special casing of the hardware
>> domain here is somewhat odd anyway.
> 
> Do you mean because bridges would only be ever assigned to the
> hardware domain?

No, because even if they were assigned to another domain they still
should be skipped.

Jan



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