Re: [Qemu-devel] [RFC PATCH 10/10] hw/pci-host/gt64120: Clean the decoded address space

[Philippe Mathieu-Daudé]

Cc'ing the kernel folks.

On 6/25/19 12:28 AM, Philippe Mathieu-Daudé wrote:
> The SysAd bus is split in various address spaces.
> Declare the different regions separately, this helps a lot
> while tracing different access while debugging.
> 
> We also add the PCI1 ranges.
> 
> See 'GT-64120A System Controller' datasheet Rev, 1.1,
> "Table 15: CPU and Device Decoder Default Address Mapping"
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
> While this device is modelled toward the Malta board, it is generic.
> ---
>  hw/mips/mips_malta.c  |  6 ------
>  hw/pci-host/gt64120.c | 19 +++++++++++++++++++
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 97f8ffbf1b..d6e4a0dad9 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -53,7 +53,6 @@
>  #include "sysemu/qtest.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> -#include "hw/misc/empty_slot.h"
>  #include "sysemu/kvm.h"
>  #include "hw/semihosting/semihost.h"
>  #include "hw/mips/cps.h"
> @@ -1209,11 +1208,6 @@ void mips_malta_init(MachineState *machine)
>      DeviceState *dev = qdev_create(NULL, TYPE_MIPS_MALTA);
>      MaltaState *s = MIPS_MALTA(dev);
>  
> -    /* The whole address space decoded by the GT-64120A doesn't generate
> -       exception when accessing invalid memory. Create an empty slot to
> -       emulate this feature. */
> -    empty_slot_init("gt64120-ad", 0x00000000, 0x20000000);
> -
>      qdev_init_nofail(dev);
>  
>      /* create CPU */
> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> index 5209038ee5..6eaa571994 100644
> --- a/hw/pci-host/gt64120.c
> +++ b/hw/pci-host/gt64120.c
> @@ -31,6 +31,8 @@
>  #include "hw/pci/pci_host.h"
>  #include "hw/i386/pc.h"
>  #include "exec/address-spaces.h"
> +#include "hw/misc/empty_slot.h"
> +#include "hw/misc/unimp.h"
>  #include "trace.h"
>  
>  #define GT_REGS                 (0x1000 >> 2)
> @@ -1206,6 +1208,23 @@ PCIBus *gt64120_create(qemu_irq *pic, bool 
> target_is_bigendian)
>                            "isd-mem", 0x1000);
>  
>      pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
> +
> +    create_unimplemented_device("gt64120_i2o", 0x14000000, 256);
> +
> +    empty_slot_init("SCS0",     0x00000000, 8 * MiB);
> +    empty_slot_init("SCS1",     0x00800000, 8 * MiB);
> +    empty_slot_init("SCS2",     0x01000000, 8 * MiB);
> +    empty_slot_init("SCS3",     0x01800000, 8 * MiB);

Since it is a bit pointless to alloc 4 regions, I could
simplify those 4 as:

       empty_slot_init("SCS[4]",   0x00000000, 4 * 8 * MiB);

The difference with the previous content is now we have
two new holes:

- 0x02000000-0x10000000
- 0x14001000-0x1c000000

Ralf/Paul/James, what should happen when a guest access these
holes (hardware PoV, no QEMU)?

The address space with this patch is:

(qemu) info mtree
address-space: memory
0000000000000000-0000000007ffffff (prio 0, i/o): alias low_ram
@mips_malta.ram 0000000000000000-0000000007ffffff
0000000000000000-00000000007fffff (prio -10000, i/o): SCS0
0000000000800000-0000000000ffffff (prio -10000, i/o): SCS1
0000000001000000-00000000017fffff (prio -10000, i/o): SCS2
0000000001800000-0000000001ffffff (prio -10000, i/o): SCS3
0000000002000000-000000000fffffff [hole]
0000000010000000-0000000011ffffff (prio 0, i/o): alias pci0-io @io
0000000000000000-0000000001ffffff
0000000012000000-0000000013ffffff (prio 0, i/o): alias pci0-mem0
@pci0-mem 0000000012000000-0000000013ffffff
0000000014000000-0000000014000fff (prio 0, i/o): isd-mem
0000000014000000-00000000140000ff (prio -1000, i/o): gt64120_i2o
0000000014001000-000000001bffffff [hole]
000000001c000000-000000001c7fffff (prio -10000, i/o): CS0
000000001c800000-000000001cffffff (prio -10000, i/o): CS1
000000001d000000-000000001effffff (prio -10000, i/o): CS2
000000001e000000-000000001e3fffff (prio 0, romd): mips_malta.bios
000000001f000000-000000001f0008ff (prio 0, i/o): alias malta-fpga
@malta-fpga 0000000000000000-00000000000008ff
000000001f000000-000000001fbfffff (prio -10000, i/o): CS3
000000001f000900-000000001f00093f (prio 0, i/o): serial
000000001f000a00-000000001f00ffff (prio 0, i/o): alias malta-fpga
@malta-fpga 0000000000000a00-000000000000ffff
000000001fc00000-000000001fffffff (prio 0, rom): bios.1fc
000000001fc00000-000000001fffffff (prio -10000, i/o): BootCS
0000000020000000-0000000021ffffff (prio -1000, i/o): pci1-io
0000000022000000-0000000023ffffff (prio -10000, i/o): pci1-mem0
0000000024000000-0000000025ffffff (prio -10000, i/o): pci1-mem1
0000000080000000-0000000087ffffff (prio 0, ram): mips_malta.ram
00000000f2000000-00000000f3ffffff (prio 0, i/o): alias pci0-mem1
@pci0-mem 00000000f2000000-00000000f3ffffff

> +    empty_slot_init("CS0",      0x1c000000, 8 * MiB);
> +    empty_slot_init("CS1",      0x1c800000, 8 * MiB);
> +    empty_slot_init("CS2",      0x1d000000, 32 * MiB);
> +    empty_slot_init("CS3",      0x1f000000, 12 * MiB);

I'm not very happy to add a non-pow2 range, but this is how
it appears on the datasheet. I suppose the correct range is
16MB with lower priority than the BootCS.

> +    empty_slot_init("BootCS",   0x1fc00000, 4 * MiB);

> +    create_unimplemented_device("pci1-io", 0x20000000, 32 * MiB);
> +    empty_slot_init("pci1-mem0", 0x22000000, 32 * MiB);
> +    empty_slot_init("pci1-mem1", 0x24000000, 32 * MiB);

This part is new, and could go in a separate patch:
Currently, no guest ever accessed this space.

Regards,

Phil.

> +
>      return phb->bus;
>  }
>