[PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows

Peter Maydell posted 1 patch 3 years ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210325163315.27724-1-peter.maydell@linaro.org
Maintainers: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Eduardo Habkost <ehabkost@redhat.com>
include/hw/pci-host/gpex.h |  4 +++
hw/core/machine.c          |  1 +
hw/pci-host/gpex.c         | 56 ++++++++++++++++++++++++++++++++++++--
3 files changed, 58 insertions(+), 3 deletions(-)
[PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Peter Maydell 3 years ago
Currently the gpex PCI controller implements no special behaviour for
guest accesses to areas of the PIO and MMIO where it has not mapped
any PCI devices, which means that for Arm you end up with a CPU
exception due to a data abort.

Most host OSes expect "like an x86 PC" behaviour, where bad accesses
like this return -1 for reads and ignore writes.  In the interests of
not being surprising, make host CPU accesses to these windows behave
as -1/discard where there's no mapped PCI device.

The old behaviour generally didn't cause any problems, because
almost always the guest OS will map the PCI devices and then only
access where it has mapped them. One corner case where you will see
this kind of access is if Linux attempts to probe legacy ISA
devices via a PIO window access. So far the only case where we've
seen this has been via the syzkaller fuzzer.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Fixes: https://bugs.launchpad.net/qemu/+bug/1918917
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
v1->v2 changes: put in the hw_compat machinery.

Still not sure if I want to put this in 6.0 or not.

 include/hw/pci-host/gpex.h |  4 +++
 hw/core/machine.c          |  1 +
 hw/pci-host/gpex.c         | 56 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
index d48a020a952..fcf8b638200 100644
--- a/include/hw/pci-host/gpex.h
+++ b/include/hw/pci-host/gpex.h
@@ -49,8 +49,12 @@ struct GPEXHost {
 
     MemoryRegion io_ioport;
     MemoryRegion io_mmio;
+    MemoryRegion io_ioport_window;
+    MemoryRegion io_mmio_window;
     qemu_irq irq[GPEX_NUM_IRQS];
     int irq_num[GPEX_NUM_IRQS];
+
+    bool allow_unmapped_accesses;
 };
 
 struct GPEXConfig {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 257a664ea2e..9750fad7435 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
     { "PIIX4_PM", "smm-compat", "on"},
     { "virtio-blk-device", "report-discard-granularity", "off" },
     { "virtio-net-pci", "vectors", "3"},
+    { "gpex-pcihost", "allow-unmapped-accesses", "false" },
 };
 const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
 
diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
index 2bdbe7b4561..a6752fac5e8 100644
--- a/hw/pci-host/gpex.c
+++ b/hw/pci-host/gpex.c
@@ -83,12 +83,51 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
     int i;
 
     pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
+    sysbus_init_mmio(sbd, &pex->mmio);
+
+    /*
+     * Note that the MemoryRegions io_mmio and io_ioport that we pass
+     * to pci_register_root_bus() are not the same as the
+     * MemoryRegions io_mmio_window and io_ioport_window that we
+     * expose as SysBus MRs. The difference is in the behaviour of
+     * accesses to addresses where no PCI device has been mapped.
+     *
+     * io_mmio and io_ioport are the underlying PCI view of the PCI
+     * address space, and when a PCI device does a bus master access
+     * to a bad address this is reported back to it as a transaction
+     * failure.
+     *
+     * io_mmio_window and io_ioport_window implement "unmapped
+     * addresses read as -1 and ignore writes"; this is traditional
+     * x86 PC behaviour, which is not mandated by the PCI spec proper
+     * but expected by much PCI-using guest software, including Linux.
+     *
+     * In the interests of not being unnecessarily surprising, we
+     * implement it in the gpex PCI host controller, by providing the
+     * _window MRs, which are containers with io ops that implement
+     * the 'background' behaviour and which hold the real PCI MRs as
+     * subregions.
+     */
     memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
     memory_region_init(&s->io_ioport, OBJECT(s), "gpex_ioport", 64 * 1024);
 
-    sysbus_init_mmio(sbd, &pex->mmio);
-    sysbus_init_mmio(sbd, &s->io_mmio);
-    sysbus_init_mmio(sbd, &s->io_ioport);
+    if (s->allow_unmapped_accesses) {
+        memory_region_init_io(&s->io_mmio_window, OBJECT(s),
+                              &unassigned_io_ops, OBJECT(s),
+                              "gpex_mmio_window", UINT64_MAX);
+        memory_region_init_io(&s->io_ioport_window, OBJECT(s),
+                              &unassigned_io_ops, OBJECT(s),
+                              "gpex_ioport_window", 64 * 1024);
+
+        memory_region_add_subregion(&s->io_mmio_window, 0, &s->io_mmio);
+        memory_region_add_subregion(&s->io_ioport_window, 0, &s->io_ioport);
+        sysbus_init_mmio(sbd, &s->io_mmio_window);
+        sysbus_init_mmio(sbd, &s->io_ioport_window);
+    } else {
+        sysbus_init_mmio(sbd, &s->io_mmio);
+        sysbus_init_mmio(sbd, &s->io_ioport);
+    }
+
     for (i = 0; i < GPEX_NUM_IRQS; i++) {
         sysbus_init_irq(sbd, &s->irq[i]);
         s->irq_num[i] = -1;
@@ -108,6 +147,16 @@ static const char *gpex_host_root_bus_path(PCIHostState *host_bridge,
     return "0000:00";
 }
 
+static Property gpex_host_properties[] = {
+    /*
+     * Permit CPU accesses to unmapped areas of the PIO and MMIO windows
+     * (discarding writes and returning -1 for reads) rather than aborting.
+     */
+    DEFINE_PROP_BOOL("allow-unmapped-accesses", GPEXHost,
+                     allow_unmapped_accesses, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void gpex_host_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -117,6 +166,7 @@ static void gpex_host_class_init(ObjectClass *klass, void *data)
     dc->realize = gpex_host_realize;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->fw_name = "pci";
+    device_class_set_props(dc, gpex_host_properties);
 }
 
 static void gpex_host_initfn(Object *obj)
-- 
2.20.1


Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Richard Henderson 3 years ago
On 3/25/21 10:33 AM, Peter Maydell wrote:
> Currently the gpex PCI controller implements no special behaviour for
> guest accesses to areas of the PIO and MMIO where it has not mapped
> any PCI devices, which means that for Arm you end up with a CPU
> exception due to a data abort.
> 
> Most host OSes expect "like an x86 PC" behaviour, where bad accesses
> like this return -1 for reads and ignore writes.  In the interests of
> not being surprising, make host CPU accesses to these windows behave
> as -1/discard where there's no mapped PCI device.
> 
> The old behaviour generally didn't cause any problems, because
> almost always the guest OS will map the PCI devices and then only
> access where it has mapped them. One corner case where you will see
> this kind of access is if Linux attempts to probe legacy ISA
> devices via a PIO window access. So far the only case where we've
> seen this has been via the syzkaller fuzzer.
> 
> Reported-by: Dmitry Vyukov<dvyukov@google.com>
> Fixes:https://bugs.launchpad.net/qemu/+bug/1918917
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> v1->v2 changes: put in the hw_compat machinery.
> 
> Still not sure if I want to put this in 6.0 or not.

I know what you mean.

> 
>   include/hw/pci-host/gpex.h |  4 +++
>   hw/core/machine.c          |  1 +
>   hw/pci-host/gpex.c         | 56 ++++++++++++++++++++++++++++++++++++--
>   3 files changed, 58 insertions(+), 3 deletions(-)

That said, the code looks fine, so,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Michael S. Tsirkin 3 years ago
On Thu, Mar 25, 2021 at 04:33:15PM +0000, Peter Maydell wrote:
> Currently the gpex PCI controller implements no special behaviour for
> guest accesses to areas of the PIO and MMIO where it has not mapped
> any PCI devices, which means that for Arm you end up with a CPU
> exception due to a data abort.
> 
> Most host OSes expect "like an x86 PC" behaviour, where bad accesses
> like this return -1 for reads and ignore writes.  In the interests of
> not being surprising, make host CPU accesses to these windows behave
> as -1/discard where there's no mapped PCI device.
> 
> The old behaviour generally didn't cause any problems, because
> almost always the guest OS will map the PCI devices and then only
> access where it has mapped them. One corner case where you will see
> this kind of access is if Linux attempts to probe legacy ISA
> devices via a PIO window access. So far the only case where we've
> seen this has been via the syzkaller fuzzer.
> 
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1918917
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> v1->v2 changes: put in the hw_compat machinery.
> 
> Still not sure if I want to put this in 6.0 or not.
> 
>  include/hw/pci-host/gpex.h |  4 +++
>  hw/core/machine.c          |  1 +
>  hw/pci-host/gpex.c         | 56 ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
> index d48a020a952..fcf8b638200 100644
> --- a/include/hw/pci-host/gpex.h
> +++ b/include/hw/pci-host/gpex.h
> @@ -49,8 +49,12 @@ struct GPEXHost {
>  
>      MemoryRegion io_ioport;
>      MemoryRegion io_mmio;
> +    MemoryRegion io_ioport_window;
> +    MemoryRegion io_mmio_window;
>      qemu_irq irq[GPEX_NUM_IRQS];
>      int irq_num[GPEX_NUM_IRQS];
> +
> +    bool allow_unmapped_accesses;
>  };
>  
>  struct GPEXConfig {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 257a664ea2e..9750fad7435 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
>      { "PIIX4_PM", "smm-compat", "on"},
>      { "virtio-blk-device", "report-discard-granularity", "off" },
>      { "virtio-net-pci", "vectors", "3"},
> +    { "gpex-pcihost", "allow-unmapped-accesses", "false" },
>  };
>  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>  
> diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
> index 2bdbe7b4561..a6752fac5e8 100644
> --- a/hw/pci-host/gpex.c
> +++ b/hw/pci-host/gpex.c
> @@ -83,12 +83,51 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
>      int i;
>  
>      pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
> +    sysbus_init_mmio(sbd, &pex->mmio);
> +
> +    /*
> +     * Note that the MemoryRegions io_mmio and io_ioport that we pass
> +     * to pci_register_root_bus() are not the same as the
> +     * MemoryRegions io_mmio_window and io_ioport_window that we
> +     * expose as SysBus MRs. The difference is in the behaviour of
> +     * accesses to addresses where no PCI device has been mapped.
> +     *
> +     * io_mmio and io_ioport are the underlying PCI view of the PCI
> +     * address space, and when a PCI device does a bus master access
> +     * to a bad address this is reported back to it as a transaction
> +     * failure.
> +     *
> +     * io_mmio_window and io_ioport_window implement "unmapped
> +     * addresses read as -1 and ignore writes"; this is traditional
> +     * x86 PC behaviour, which is not mandated by the PCI spec proper
> +     * but expected by much PCI-using guest software, including Linux.
> +     *
> +     * In the interests of not being unnecessarily surprising, we
> +     * implement it in the gpex PCI host controller, by providing the
> +     * _window MRs, which are containers with io ops that implement
> +     * the 'background' behaviour and which hold the real PCI MRs as
> +     * subregions.
> +     */
>      memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
>      memory_region_init(&s->io_ioport, OBJECT(s), "gpex_ioport", 64 * 1024);
>  
> -    sysbus_init_mmio(sbd, &pex->mmio);
> -    sysbus_init_mmio(sbd, &s->io_mmio);
> -    sysbus_init_mmio(sbd, &s->io_ioport);
> +    if (s->allow_unmapped_accesses) {
> +        memory_region_init_io(&s->io_mmio_window, OBJECT(s),
> +                              &unassigned_io_ops, OBJECT(s),
> +                              "gpex_mmio_window", UINT64_MAX);
> +        memory_region_init_io(&s->io_ioport_window, OBJECT(s),
> +                              &unassigned_io_ops, OBJECT(s),
> +                              "gpex_ioport_window", 64 * 1024);
> +
> +        memory_region_add_subregion(&s->io_mmio_window, 0, &s->io_mmio);
> +        memory_region_add_subregion(&s->io_ioport_window, 0, &s->io_ioport);
> +        sysbus_init_mmio(sbd, &s->io_mmio_window);
> +        sysbus_init_mmio(sbd, &s->io_ioport_window);
> +    } else {
> +        sysbus_init_mmio(sbd, &s->io_mmio);
> +        sysbus_init_mmio(sbd, &s->io_ioport);
> +    }
> +
>      for (i = 0; i < GPEX_NUM_IRQS; i++) {
>          sysbus_init_irq(sbd, &s->irq[i]);
>          s->irq_num[i] = -1;
> @@ -108,6 +147,16 @@ static const char *gpex_host_root_bus_path(PCIHostState *host_bridge,
>      return "0000:00";
>  }
>  
> +static Property gpex_host_properties[] = {
> +    /*
> +     * Permit CPU accesses to unmapped areas of the PIO and MMIO windows
> +     * (discarding writes and returning -1 for reads) rather than aborting.
> +     */
> +    DEFINE_PROP_BOOL("allow-unmapped-accesses", GPEXHost,
> +                     allow_unmapped_accesses, true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void gpex_host_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -117,6 +166,7 @@ static void gpex_host_class_init(ObjectClass *klass, void *data)
>      dc->realize = gpex_host_realize;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->fw_name = "pci";
> +    device_class_set_props(dc, gpex_host_properties);
>  }
>  
>  static void gpex_host_initfn(Object *obj)
> -- 
> 2.20.1


Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Philippe Mathieu-Daudé 3 years ago
Hi Peter,

On 3/25/21 5:33 PM, Peter Maydell wrote:
> Currently the gpex PCI controller implements no special behaviour for
> guest accesses to areas of the PIO and MMIO where it has not mapped
> any PCI devices, which means that for Arm you end up with a CPU
> exception due to a data abort.
> 
> Most host OSes expect "like an x86 PC" behaviour, where bad accesses
> like this return -1 for reads and ignore writes.  In the interests of
> not being surprising, make host CPU accesses to these windows behave
> as -1/discard where there's no mapped PCI device.
> 
> The old behaviour generally didn't cause any problems, because
> almost always the guest OS will map the PCI devices and then only
> access where it has mapped them. One corner case where you will see
> this kind of access is if Linux attempts to probe legacy ISA
> devices via a PIO window access. So far the only case where we've
> seen this has been via the syzkaller fuzzer.
> 
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1918917
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v1->v2 changes: put in the hw_compat machinery.
> 
> Still not sure if I want to put this in 6.0 or not.
> 
>  include/hw/pci-host/gpex.h |  4 +++
>  hw/core/machine.c          |  1 +
>  hw/pci-host/gpex.c         | 56 ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
> index d48a020a952..fcf8b638200 100644
> --- a/include/hw/pci-host/gpex.h
> +++ b/include/hw/pci-host/gpex.h
> @@ -49,8 +49,12 @@ struct GPEXHost {
>  
>      MemoryRegion io_ioport;
>      MemoryRegion io_mmio;
> +    MemoryRegion io_ioport_window;
> +    MemoryRegion io_mmio_window;
>      qemu_irq irq[GPEX_NUM_IRQS];
>      int irq_num[GPEX_NUM_IRQS];
> +
> +    bool allow_unmapped_accesses;
>  };
>  
>  struct GPEXConfig {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 257a664ea2e..9750fad7435 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
>      { "PIIX4_PM", "smm-compat", "on"},
>      { "virtio-blk-device", "report-discard-granularity", "off" },
>      { "virtio-net-pci", "vectors", "3"},
> +    { "gpex-pcihost", "allow-unmapped-accesses", "false" },
>  };
>  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>  
> diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
> index 2bdbe7b4561..a6752fac5e8 100644
> --- a/hw/pci-host/gpex.c
> +++ b/hw/pci-host/gpex.c
> @@ -83,12 +83,51 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
>      int i;
>  
>      pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
> +    sysbus_init_mmio(sbd, &pex->mmio);
> +
> +    /*
> +     * Note that the MemoryRegions io_mmio and io_ioport that we pass
> +     * to pci_register_root_bus() are not the same as the
> +     * MemoryRegions io_mmio_window and io_ioport_window that we
> +     * expose as SysBus MRs. The difference is in the behaviour of
> +     * accesses to addresses where no PCI device has been mapped.
> +     *
> +     * io_mmio and io_ioport are the underlying PCI view of the PCI
> +     * address space, and when a PCI device does a bus master access
> +     * to a bad address this is reported back to it as a transaction
> +     * failure.
> +     *
> +     * io_mmio_window and io_ioport_window implement "unmapped
> +     * addresses read as -1 and ignore writes"; this is traditional
> +     * x86 PC behaviour, which is not mandated by the PCI spec proper
> +     * but expected by much PCI-using guest software, including Linux.

I suspect PCI-ISA bridges to provide an EISA bus.

The 'IEEE P996' ISA spec doesn't seem public. Per the Intel
ISA spec:
https://archive.org/details/bitsavers_intelbusSpep89_3342148/page/n31/mode/2up
the data lines are tri-stated, but I couldn't find the default logic
when no add-on card owns the bus at the requested address (to confirm
the "read as -1").

> +     * In the interests of not being unnecessarily surprising, we
> +     * implement it in the gpex PCI host controller, by providing the
> +     * _window MRs, which are containers with io ops that implement
> +     * the 'background' behaviour and which hold the real PCI MRs as
> +     * subregions.
> +     */
>      memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
>      memory_region_init(&s->io_ioport, OBJECT(s), "gpex_ioport", 64 * 1024);
>  
> -    sysbus_init_mmio(sbd, &pex->mmio);
> -    sysbus_init_mmio(sbd, &s->io_mmio);
> -    sysbus_init_mmio(sbd, &s->io_ioport);
> +    if (s->allow_unmapped_accesses) {
> +        memory_region_init_io(&s->io_mmio_window, OBJECT(s),
> +                              &unassigned_io_ops, OBJECT(s),
> +                              "gpex_mmio_window", UINT64_MAX);

EISA -> 4 * GiB

unassigned_io_ops allows 64-bit accesses. Here we want up to 32.

Maybe we don't care.

> +        memory_region_init_io(&s->io_ioport_window, OBJECT(s),
> +                              &unassigned_io_ops, OBJECT(s),
> +                              "gpex_ioport_window", 64 * 1024);

Ditto, unassigned_io_ops accepts 64-bit accesses.

> +
> +        memory_region_add_subregion(&s->io_mmio_window, 0, &s->io_mmio);
> +        memory_region_add_subregion(&s->io_ioport_window, 0, &s->io_ioport);
> +        sysbus_init_mmio(sbd, &s->io_mmio_window);
> +        sysbus_init_mmio(sbd, &s->io_ioport_window);
> +    } else {
> +        sysbus_init_mmio(sbd, &s->io_mmio);
> +        sysbus_init_mmio(sbd, &s->io_ioport);
> +    }
> +
>      for (i = 0; i < GPEX_NUM_IRQS; i++) {
>          sysbus_init_irq(sbd, &s->irq[i]);
>          s->irq_num[i] = -1;
> @@ -108,6 +147,16 @@ static const char *gpex_host_root_bus_path(PCIHostState *host_bridge,
>      return "0000:00";
>  }
>  
> +static Property gpex_host_properties[] = {
> +    /*
> +     * Permit CPU accesses to unmapped areas of the PIO and MMIO windows
> +     * (discarding writes and returning -1 for reads) rather than aborting.
> +     */
> +    DEFINE_PROP_BOOL("allow-unmapped-accesses", GPEXHost,
> +                     allow_unmapped_accesses, true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void gpex_host_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -117,6 +166,7 @@ static void gpex_host_class_init(ObjectClass *klass, void *data)
>      dc->realize = gpex_host_realize;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->fw_name = "pci";
> +    device_class_set_props(dc, gpex_host_properties);

IMO this change belongs to the parent bridges,
TYPE_PCI_HOST_BRIDGE and TYPE_PCIE_HOST_BRIDGE.

Again, 6.0 is at the door, so this can be discussed /
updated later.

Regards,

Phil.

Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Peter Maydell 2 years, 11 months ago
On Thu, 25 Mar 2021 at 18:14, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Peter,
>
> On 3/25/21 5:33 PM, Peter Maydell wrote:
> > Currently the gpex PCI controller implements no special behaviour for
> > guest accesses to areas of the PIO and MMIO where it has not mapped
> > any PCI devices, which means that for Arm you end up with a CPU
> > exception due to a data abort.
> >
> > Most host OSes expect "like an x86 PC" behaviour, where bad accesses
> > like this return -1 for reads and ignore writes.  In the interests of
> > not being surprising, make host CPU accesses to these windows behave
> > as -1/discard where there's no mapped PCI device.
> >
> > The old behaviour generally didn't cause any problems, because
> > almost always the guest OS will map the PCI devices and then only
> > access where it has mapped them. One corner case where you will see
> > this kind of access is if Linux attempts to probe legacy ISA
> > devices via a PIO window access. So far the only case where we've
> > seen this has been via the syzkaller fuzzer.
> >
> > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1918917
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > v1->v2 changes: put in the hw_compat machinery.
> >
> > Still not sure if I want to put this in 6.0 or not.
> >
> >  include/hw/pci-host/gpex.h |  4 +++
> >  hw/core/machine.c          |  1 +
> >  hw/pci-host/gpex.c         | 56 ++++++++++++++++++++++++++++++++++++--
> >  3 files changed, 58 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
> > index d48a020a952..fcf8b638200 100644
> > --- a/include/hw/pci-host/gpex.h
> > +++ b/include/hw/pci-host/gpex.h
> > @@ -49,8 +49,12 @@ struct GPEXHost {
> >
> >      MemoryRegion io_ioport;
> >      MemoryRegion io_mmio;
> > +    MemoryRegion io_ioport_window;
> > +    MemoryRegion io_mmio_window;
> >      qemu_irq irq[GPEX_NUM_IRQS];
> >      int irq_num[GPEX_NUM_IRQS];
> > +
> > +    bool allow_unmapped_accesses;
> >  };
> >
> >  struct GPEXConfig {
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 257a664ea2e..9750fad7435 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
> >      { "PIIX4_PM", "smm-compat", "on"},
> >      { "virtio-blk-device", "report-discard-granularity", "off" },
> >      { "virtio-net-pci", "vectors", "3"},
> > +    { "gpex-pcihost", "allow-unmapped-accesses", "false" },
> >  };
> >  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
> >
> > diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
> > index 2bdbe7b4561..a6752fac5e8 100644
> > --- a/hw/pci-host/gpex.c
> > +++ b/hw/pci-host/gpex.c
> > @@ -83,12 +83,51 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
> >      int i;
> >
> >      pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
> > +    sysbus_init_mmio(sbd, &pex->mmio);
> > +
> > +    /*
> > +     * Note that the MemoryRegions io_mmio and io_ioport that we pass
> > +     * to pci_register_root_bus() are not the same as the
> > +     * MemoryRegions io_mmio_window and io_ioport_window that we
> > +     * expose as SysBus MRs. The difference is in the behaviour of
> > +     * accesses to addresses where no PCI device has been mapped.
> > +     *
> > +     * io_mmio and io_ioport are the underlying PCI view of the PCI
> > +     * address space, and when a PCI device does a bus master access
> > +     * to a bad address this is reported back to it as a transaction
> > +     * failure.
> > +     *
> > +     * io_mmio_window and io_ioport_window implement "unmapped
> > +     * addresses read as -1 and ignore writes"; this is traditional
> > +     * x86 PC behaviour, which is not mandated by the PCI spec proper
> > +     * but expected by much PCI-using guest software, including Linux.
>
> I suspect PCI-ISA bridges to provide an EISA bus.

I'm not sure what you mean here -- there isn't an ISA bridge
or an EISA bus involved here. This is purely about the behaviour
of the memory window the PCI host controller exposes to the CPU
(and in particular the window for when a PCI device's BAR is
set to "IO" rather than "MMIO"), though we change both here.

> > +     * In the interests of not being unnecessarily surprising, we
> > +     * implement it in the gpex PCI host controller, by providing the
> > +     * _window MRs, which are containers with io ops that implement
> > +     * the 'background' behaviour and which hold the real PCI MRs as
> > +     * subregions.
> > +     */
> >      memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
> >      memory_region_init(&s->io_ioport, OBJECT(s), "gpex_ioport", 64 * 1024);
> >
> > -    sysbus_init_mmio(sbd, &pex->mmio);
> > -    sysbus_init_mmio(sbd, &s->io_mmio);
> > -    sysbus_init_mmio(sbd, &s->io_ioport);
> > +    if (s->allow_unmapped_accesses) {
> > +        memory_region_init_io(&s->io_mmio_window, OBJECT(s),
> > +                              &unassigned_io_ops, OBJECT(s),
> > +                              "gpex_mmio_window", UINT64_MAX);
>
> EISA -> 4 * GiB
>
> unassigned_io_ops allows 64-bit accesses. Here we want up to 32.
>
> Maybe we don't care.
>
> > +        memory_region_init_io(&s->io_ioport_window, OBJECT(s),
> > +                              &unassigned_io_ops, OBJECT(s),
> > +                              "gpex_ioport_window", 64 * 1024);
>
> Ditto, unassigned_io_ops accepts 64-bit accesses.

These are just using the same sizes as the io_mmio and io_ioport
MRs which the existing code creates.

> >  static void gpex_host_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -117,6 +166,7 @@ static void gpex_host_class_init(ObjectClass *klass, void *data)
> >      dc->realize = gpex_host_realize;
> >      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> >      dc->fw_name = "pci";
> > +    device_class_set_props(dc, gpex_host_properties);
>
> IMO this change belongs to the parent bridges,
> TYPE_PCI_HOST_BRIDGE and TYPE_PCIE_HOST_BRIDGE.

Arnd had a look through the kernel sources and apparently not
all PCI host controllers do this -- there are a few SoCs where the
kernel has to put in special case code to allow for the fact that
it will get a bus error for accesses to unmapped parts of the
window. So I concluded that the specific controller implementation
was the right place for it.

thanks
-- PMM

Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago
On 4/19/21 3:42 PM, Peter Maydell wrote:
> On Thu, 25 Mar 2021 at 18:14, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Peter,
>>
>> On 3/25/21 5:33 PM, Peter Maydell wrote:
>>> Currently the gpex PCI controller implements no special behaviour for
>>> guest accesses to areas of the PIO and MMIO where it has not mapped
>>> any PCI devices, which means that for Arm you end up with a CPU
>>> exception due to a data abort.
>>>
>>> Most host OSes expect "like an x86 PC" behaviour, where bad accesses
>>> like this return -1 for reads and ignore writes.  In the interests of
>>> not being surprising, make host CPU accesses to these windows behave
>>> as -1/discard where there's no mapped PCI device.
>>>
>>> The old behaviour generally didn't cause any problems, because
>>> almost always the guest OS will map the PCI devices and then only
>>> access where it has mapped them. One corner case where you will see
>>> this kind of access is if Linux attempts to probe legacy ISA
>>> devices via a PIO window access. So far the only case where we've
>>> seen this has been via the syzkaller fuzzer.
>>>
>>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>>> Fixes: https://bugs.launchpad.net/qemu/+bug/1918917
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> v1->v2 changes: put in the hw_compat machinery.
>>>
>>> Still not sure if I want to put this in 6.0 or not.
>>>
>>>  include/hw/pci-host/gpex.h |  4 +++
>>>  hw/core/machine.c          |  1 +
>>>  hw/pci-host/gpex.c         | 56 ++++++++++++++++++++++++++++++++++++--
>>>  3 files changed, 58 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
>>> index d48a020a952..fcf8b638200 100644
>>> --- a/include/hw/pci-host/gpex.h
>>> +++ b/include/hw/pci-host/gpex.h
>>> @@ -49,8 +49,12 @@ struct GPEXHost {
>>>
>>>      MemoryRegion io_ioport;
>>>      MemoryRegion io_mmio;
>>> +    MemoryRegion io_ioport_window;
>>> +    MemoryRegion io_mmio_window;
>>>      qemu_irq irq[GPEX_NUM_IRQS];
>>>      int irq_num[GPEX_NUM_IRQS];
>>> +
>>> +    bool allow_unmapped_accesses;
>>>  };
>>>
>>>  struct GPEXConfig {
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index 257a664ea2e..9750fad7435 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
>>>      { "PIIX4_PM", "smm-compat", "on"},
>>>      { "virtio-blk-device", "report-discard-granularity", "off" },
>>>      { "virtio-net-pci", "vectors", "3"},
>>> +    { "gpex-pcihost", "allow-unmapped-accesses", "false" },
>>>  };
>>>  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>>>
>>> diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
>>> index 2bdbe7b4561..a6752fac5e8 100644
>>> --- a/hw/pci-host/gpex.c
>>> +++ b/hw/pci-host/gpex.c
>>> @@ -83,12 +83,51 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
>>>      int i;
>>>
>>>      pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
>>> +    sysbus_init_mmio(sbd, &pex->mmio);
>>> +
>>> +    /*
>>> +     * Note that the MemoryRegions io_mmio and io_ioport that we pass
>>> +     * to pci_register_root_bus() are not the same as the
>>> +     * MemoryRegions io_mmio_window and io_ioport_window that we
>>> +     * expose as SysBus MRs. The difference is in the behaviour of
>>> +     * accesses to addresses where no PCI device has been mapped.
>>> +     *
>>> +     * io_mmio and io_ioport are the underlying PCI view of the PCI
>>> +     * address space, and when a PCI device does a bus master access
>>> +     * to a bad address this is reported back to it as a transaction
>>> +     * failure.
>>> +     *
>>> +     * io_mmio_window and io_ioport_window implement "unmapped
>>> +     * addresses read as -1 and ignore writes"; this is traditional
>>> +     * x86 PC behaviour, which is not mandated by the PCI spec proper
>>> +     * but expected by much PCI-using guest software, including Linux.
>>
>> I suspect PCI-ISA bridges to provide an EISA bus.
> 
> I'm not sure what you mean here -- there isn't an ISA bridge
> or an EISA bus involved here. This is purely about the behaviour
> of the memory window the PCI host controller exposes to the CPU
> (and in particular the window for when a PCI device's BAR is
> set to "IO" rather than "MMIO"), though we change both here.

I guess I always interpreted the IO BAR were here to address ISA
backward compatibility. I don't know well PCI so I'll study it
more. Sorry for my confused comment.

>>> +     * In the interests of not being unnecessarily surprising, we
>>> +     * implement it in the gpex PCI host controller, by providing the
>>> +     * _window MRs, which are containers with io ops that implement
>>> +     * the 'background' behaviour and which hold the real PCI MRs as
>>> +     * subregions.
>>> +     */
>>>      memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
>>>      memory_region_init(&s->io_ioport, OBJECT(s), "gpex_ioport", 64 * 1024);
>>>
>>> -    sysbus_init_mmio(sbd, &pex->mmio);
>>> -    sysbus_init_mmio(sbd, &s->io_mmio);
>>> -    sysbus_init_mmio(sbd, &s->io_ioport);
>>> +    if (s->allow_unmapped_accesses) {
>>> +        memory_region_init_io(&s->io_mmio_window, OBJECT(s),
>>> +                              &unassigned_io_ops, OBJECT(s),
>>> +                              "gpex_mmio_window", UINT64_MAX);
>>
>> EISA -> 4 * GiB
>>
>> unassigned_io_ops allows 64-bit accesses. Here we want up to 32.
>>
>> Maybe we don't care.
>>
>>> +        memory_region_init_io(&s->io_ioport_window, OBJECT(s),
>>> +                              &unassigned_io_ops, OBJECT(s),
>>> +                              "gpex_ioport_window", 64 * 1024);
>>
>> Ditto, unassigned_io_ops accepts 64-bit accesses.
> 
> These are just using the same sizes as the io_mmio and io_ioport
> MRs which the existing code creates.
> 
>>>  static void gpex_host_class_init(ObjectClass *klass, void *data)
>>>  {
>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -117,6 +166,7 @@ static void gpex_host_class_init(ObjectClass *klass, void *data)
>>>      dc->realize = gpex_host_realize;
>>>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>      dc->fw_name = "pci";
>>> +    device_class_set_props(dc, gpex_host_properties);
>>
>> IMO this change belongs to the parent bridges,
>> TYPE_PCI_HOST_BRIDGE and TYPE_PCIE_HOST_BRIDGE.
> 
> Arnd had a look through the kernel sources and apparently not
> all PCI host controllers do this -- there are a few SoCs where the
> kernel has to put in special case code to allow for the fact that
> it will get a bus error for accesses to unmapped parts of the
> window. So I concluded that the specific controller implementation
> was the right place for it.

Yes the changes are simple. I'm certainly not NAcking the patch,
but can't review it neither :( So please ignore my comments.

Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Arnd Bergmann 2 years, 11 months ago
On Tue, Apr 20, 2021 at 1:52 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 4/19/21 3:42 PM, Peter Maydell wrote:
> >>
> >> I suspect PCI-ISA bridges to provide an EISA bus.
> >
> > I'm not sure what you mean here -- there isn't an ISA bridge
> > or an EISA bus involved here. This is purely about the behaviour
> > of the memory window the PCI host controller exposes to the CPU
> > (and in particular the window for when a PCI device's BAR is
> > set to "IO" rather than "MMIO"), though we change both here.
>
> I guess I always interpreted the IO BAR were here to address ISA
> backward compatibility. I don't know well PCI so I'll study it
> more. Sorry for my confused comment.

It is mostly for compatibility, but there are many layers of it:

- PCI supports actual ISA/EISA/VLB/PCMCIA/LPC/PC104/... style
  devices behind a bridge, using I/O ports at their native address.

- PCI devices themselves can have fixed I/O ports at well-known
  addresses, e.g. VGA or IDE/ATA adapters

- PCI devices can behave like legacy devices using port I/O
  but use PCI resource assignment to pick an arbitrary port
  number outside of the legacy range

- PCIe can support all of the above by virtue of being backwards
  compatible with PCI and allowing PCI buses behind bridges,
  though port I/O is deprecated here and often not supported at all

The first two are very rare these days, but Linux still support them
in order to run on old hardware, and any driver for these that
assumes a hardcoded port number can crash the kernel if the
PCI host bridge causes an asynchronous external abort or
similar.

       Arnd

Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago
On 4/20/21 2:26 PM, Arnd Bergmann wrote:
> On Tue, Apr 20, 2021 at 1:52 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 4/19/21 3:42 PM, Peter Maydell wrote:
>>>>
>>>> I suspect PCI-ISA bridges to provide an EISA bus.
>>>
>>> I'm not sure what you mean here -- there isn't an ISA bridge
>>> or an EISA bus involved here. This is purely about the behaviour
>>> of the memory window the PCI host controller exposes to the CPU
>>> (and in particular the window for when a PCI device's BAR is
>>> set to "IO" rather than "MMIO"), though we change both here.
>>
>> I guess I always interpreted the IO BAR were here to address ISA
>> backward compatibility. I don't know well PCI so I'll study it
>> more. Sorry for my confused comment.
> 
> It is mostly for compatibility, but there are many layers of it:
> 
> - PCI supports actual ISA/EISA/VLB/PCMCIA/LPC/PC104/... style
>   devices behind a bridge, using I/O ports at their native address.
> 
> - PCI devices themselves can have fixed I/O ports at well-known
>   addresses, e.g. VGA or IDE/ATA adapters
> 
> - PCI devices can behave like legacy devices using port I/O
>   but use PCI resource assignment to pick an arbitrary port
>   number outside of the legacy range
> 
> - PCIe can support all of the above by virtue of being backwards
>   compatible with PCI and allowing PCI buses behind bridges,
>   though port I/O is deprecated here and often not supported at all
> 
> The first two are very rare these days, but Linux still support them
> in order to run on old hardware, and any driver for these that
> assumes a hardcoded port number can crash the kernel if the
> PCI host bridge causes an asynchronous external abort or
> similar.

Thanks for the clarification and enumeration Arnd!

Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Michael S. Tsirkin 2 years, 11 months ago
On Thu, Mar 25, 2021 at 04:33:15PM +0000, Peter Maydell wrote:
> Currently the gpex PCI controller implements no special behaviour for
> guest accesses to areas of the PIO and MMIO where it has not mapped
> any PCI devices, which means that for Arm you end up with a CPU
> exception due to a data abort.
> 
> Most host OSes expect "like an x86 PC" behaviour, where bad accesses
> like this return -1 for reads and ignore writes.  In the interests of
> not being surprising, make host CPU accesses to these windows behave
> as -1/discard where there's no mapped PCI device.
> 
> The old behaviour generally didn't cause any problems, because
> almost always the guest OS will map the PCI devices and then only
> access where it has mapped them. One corner case where you will see
> this kind of access is if Linux attempts to probe legacy ISA
> devices via a PIO window access. So far the only case where we've
> seen this has been via the syzkaller fuzzer.
> 
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1918917
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Looks ok superficially

Acked-by: Michael S. Tsirkin <mst@redhat.com>

Peter pls merge if appropriate.

> ---
> v1->v2 changes: put in the hw_compat machinery.
> 
> Still not sure if I want to put this in 6.0 or not.
> 
>  include/hw/pci-host/gpex.h |  4 +++
>  hw/core/machine.c          |  1 +
>  hw/pci-host/gpex.c         | 56 ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h
> index d48a020a952..fcf8b638200 100644
> --- a/include/hw/pci-host/gpex.h
> +++ b/include/hw/pci-host/gpex.h
> @@ -49,8 +49,12 @@ struct GPEXHost {
>  
>      MemoryRegion io_ioport;
>      MemoryRegion io_mmio;
> +    MemoryRegion io_ioport_window;
> +    MemoryRegion io_mmio_window;
>      qemu_irq irq[GPEX_NUM_IRQS];
>      int irq_num[GPEX_NUM_IRQS];
> +
> +    bool allow_unmapped_accesses;
>  };
>  
>  struct GPEXConfig {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 257a664ea2e..9750fad7435 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -41,6 +41,7 @@ GlobalProperty hw_compat_5_2[] = {
>      { "PIIX4_PM", "smm-compat", "on"},
>      { "virtio-blk-device", "report-discard-granularity", "off" },
>      { "virtio-net-pci", "vectors", "3"},
> +    { "gpex-pcihost", "allow-unmapped-accesses", "false" },
>  };
>  const size_t hw_compat_5_2_len = G_N_ELEMENTS(hw_compat_5_2);
>  
> diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c
> index 2bdbe7b4561..a6752fac5e8 100644
> --- a/hw/pci-host/gpex.c
> +++ b/hw/pci-host/gpex.c
> @@ -83,12 +83,51 @@ static void gpex_host_realize(DeviceState *dev, Error **errp)
>      int i;
>  
>      pcie_host_mmcfg_init(pex, PCIE_MMCFG_SIZE_MAX);
> +    sysbus_init_mmio(sbd, &pex->mmio);
> +
> +    /*
> +     * Note that the MemoryRegions io_mmio and io_ioport that we pass
> +     * to pci_register_root_bus() are not the same as the
> +     * MemoryRegions io_mmio_window and io_ioport_window that we
> +     * expose as SysBus MRs. The difference is in the behaviour of
> +     * accesses to addresses where no PCI device has been mapped.
> +     *
> +     * io_mmio and io_ioport are the underlying PCI view of the PCI
> +     * address space, and when a PCI device does a bus master access
> +     * to a bad address this is reported back to it as a transaction
> +     * failure.
> +     *
> +     * io_mmio_window and io_ioport_window implement "unmapped
> +     * addresses read as -1 and ignore writes"; this is traditional
> +     * x86 PC behaviour, which is not mandated by the PCI spec proper
> +     * but expected by much PCI-using guest software, including Linux.
> +     *
> +     * In the interests of not being unnecessarily surprising, we
> +     * implement it in the gpex PCI host controller, by providing the
> +     * _window MRs, which are containers with io ops that implement
> +     * the 'background' behaviour and which hold the real PCI MRs as
> +     * subregions.
> +     */
>      memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
>      memory_region_init(&s->io_ioport, OBJECT(s), "gpex_ioport", 64 * 1024);
>  
> -    sysbus_init_mmio(sbd, &pex->mmio);
> -    sysbus_init_mmio(sbd, &s->io_mmio);
> -    sysbus_init_mmio(sbd, &s->io_ioport);
> +    if (s->allow_unmapped_accesses) {
> +        memory_region_init_io(&s->io_mmio_window, OBJECT(s),
> +                              &unassigned_io_ops, OBJECT(s),
> +                              "gpex_mmio_window", UINT64_MAX);
> +        memory_region_init_io(&s->io_ioport_window, OBJECT(s),
> +                              &unassigned_io_ops, OBJECT(s),
> +                              "gpex_ioport_window", 64 * 1024);
> +
> +        memory_region_add_subregion(&s->io_mmio_window, 0, &s->io_mmio);
> +        memory_region_add_subregion(&s->io_ioport_window, 0, &s->io_ioport);
> +        sysbus_init_mmio(sbd, &s->io_mmio_window);
> +        sysbus_init_mmio(sbd, &s->io_ioport_window);
> +    } else {
> +        sysbus_init_mmio(sbd, &s->io_mmio);
> +        sysbus_init_mmio(sbd, &s->io_ioport);
> +    }
> +
>      for (i = 0; i < GPEX_NUM_IRQS; i++) {
>          sysbus_init_irq(sbd, &s->irq[i]);
>          s->irq_num[i] = -1;
> @@ -108,6 +147,16 @@ static const char *gpex_host_root_bus_path(PCIHostState *host_bridge,
>      return "0000:00";
>  }
>  
> +static Property gpex_host_properties[] = {
> +    /*
> +     * Permit CPU accesses to unmapped areas of the PIO and MMIO windows
> +     * (discarding writes and returning -1 for reads) rather than aborting.
> +     */
> +    DEFINE_PROP_BOOL("allow-unmapped-accesses", GPEXHost,
> +                     allow_unmapped_accesses, true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void gpex_host_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -117,6 +166,7 @@ static void gpex_host_class_init(ObjectClass *klass, void *data)
>      dc->realize = gpex_host_realize;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->fw_name = "pci";
> +    device_class_set_props(dc, gpex_host_properties);
>  }
>  
>  static void gpex_host_initfn(Object *obj)
> -- 
> 2.20.1


Re: [PATCH v2 for-6.0?] hw/pci-host/gpex: Don't fault for unmapped parts of MMIO and PIO windows
Posted by Peter Maydell 2 years, 11 months ago
On Tue, 20 Apr 2021 at 11:24, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Mar 25, 2021 at 04:33:15PM +0000, Peter Maydell wrote:
> > Currently the gpex PCI controller implements no special behaviour for
> > guest accesses to areas of the PIO and MMIO where it has not mapped
> > any PCI devices, which means that for Arm you end up with a CPU
> > exception due to a data abort.
> >
> > Most host OSes expect "like an x86 PC" behaviour, where bad accesses
> > like this return -1 for reads and ignore writes.  In the interests of
> > not being surprising, make host CPU accesses to these windows behave
> > as -1/discard where there's no mapped PCI device.
> >
> > The old behaviour generally didn't cause any problems, because
> > almost always the guest OS will map the PCI devices and then only
> > access where it has mapped them. One corner case where you will see
> > this kind of access is if Linux attempts to probe legacy ISA
> > devices via a PIO window access. So far the only case where we've
> > seen this has been via the syzkaller fuzzer.
> >
> > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1918917
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>
> Looks ok superficially
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Peter pls merge if appropriate.

Thanks; I'll take it via target-arm.next for 6.1 (it'll need
a tweak to use hw_compat_6_0 rather than hw_compat_5_2 so it might
need to wait until the patch adding hw_compat_6_0 hits master.)

-- PMM