From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Sven Schnelle <svens@stackframe.org>,
Richard Henderson <rth@twiddle.net>
Cc: Helge Deller <deller@gmx.de>, qemu-devel@nongnu.org
Subject: Re: [PATCH v5 1/6] hw/hppa/dino.c: Improve emulation of Dino PCI chip
Date: Thu, 13 Feb 2020 00:37:25 +0100 [thread overview]
Message-ID: <779e7a6e-00e1-1f51-7132-1cd389bbd921@redhat.com> (raw)
In-Reply-To: <20191220211512.3289-2-svens@stackframe.org>
Hi Sven, Helge.
On 12/20/19 10:15 PM, Sven Schnelle wrote:
> From: Helge Deller <deller@gmx.de>
>
> The tests of the dino chip with the Online-diagnostics CD
> ("ODE DINOTEST") now succeeds.
> Additionally add some qemu trace events.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> MAINTAINERS | 2 +-
> hw/hppa/dino.c | 97 +++++++++++++++++++++++++++++++++++++-------
> hw/hppa/trace-events | 5 +++
> 3 files changed, 89 insertions(+), 15 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 387879aebc..e333bc67a4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -876,7 +876,7 @@ F: hw/*/etraxfs_*.c
>
> HP-PARISC Machines
> ------------------
> -Dino
> +HP B160L
> M: Richard Henderson <rth@twiddle.net>
> R: Helge Deller <deller@gmx.de>
> S: Odd Fixes
> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
> index ab6969b45f..9797a7f0d9 100644
> --- a/hw/hppa/dino.c
> +++ b/hw/hppa/dino.c
> @@ -1,7 +1,7 @@
> /*
> - * HP-PARISC Dino PCI chipset emulation.
> + * HP-PARISC Dino PCI chipset emulation, as in B160L and similiar machines
> *
> - * (C) 2017 by Helge Deller <deller@gmx.de>
> + * (C) 2017-2019 by Helge Deller <deller@gmx.de>
> *
> * This work is licensed under the GNU GPL license version 2 or later.
> *
> @@ -21,6 +21,7 @@
> #include "migration/vmstate.h"
> #include "hppa_sys.h"
> #include "exec/address-spaces.h"
> +#include "trace.h"
>
>
> #define TYPE_DINO_PCI_HOST_BRIDGE "dino-pcihost"
> @@ -82,11 +83,28 @@
> #define DINO_PCI_HOST_BRIDGE(obj) \
> OBJECT_CHECK(DinoState, (obj), TYPE_DINO_PCI_HOST_BRIDGE)
>
> +#define DINO800_REGS ((DINO_TLTIM - DINO_GMASK) / 4)
Coverity noticed (CID 1419394) this should be '(1 + (DINO_TLTIM -
DINO_GMASK) / 4)' (13 registers).
> +static const uint32_t reg800_keep_bits[DINO800_REGS] = {
> + MAKE_64BIT_MASK(0, 1),
> + MAKE_64BIT_MASK(0, 7),
> + MAKE_64BIT_MASK(0, 7),
> + MAKE_64BIT_MASK(0, 8),
> + MAKE_64BIT_MASK(0, 7),
> + MAKE_64BIT_MASK(0, 9),
> + MAKE_64BIT_MASK(0, 32),
Looking at the datasheet pp. 30, this register is 'Undefined'.
We should report GUEST_ERROR if it is accessed.
> + MAKE_64BIT_MASK(0, 8),
> + MAKE_64BIT_MASK(0, 30),
> + MAKE_64BIT_MASK(0, 25),
Still looking at the datasheet (pp. 37) PCIROR is 24-bit (not 25).
> + MAKE_64BIT_MASK(0, 22),
Here you are missing register 0x82c...
> + MAKE_64BIT_MASK(0, 9),
> +};
> +
Altogether:
static const uint32_t reg800_keep_bits[DINO800_REGS] = {
MAKE_64BIT_MASK(0, 1), /* GMASK */
MAKE_64BIT_MASK(0, 7), /* PAMR */
MAKE_64BIT_MASK(0, 7), /* PAPR */
MAKE_64BIT_MASK(0, 8), /* DAMODE */
MAKE_64BIT_MASK(0, 7), /* PCICMD */
MAKE_64BIT_MASK(0, 9), /* PCISTS */
MAKE_64BIT_MASK(0, 0), /* Undefined */
MAKE_64BIT_MASK(0, 8), /* MLTIM */
MAKE_64BIT_MASK(0, 30), /* BRDG_FEAT */
MAKE_64BIT_MASK(0, 24), /* PCIROR */
MAKE_64BIT_MASK(0, 22), /* PCIWOR */
MAKE_64BIT_MASK(0, 0), /* Undocumented */
MAKE_64BIT_MASK(0, 9), /* TLTIM */
};
> typedef struct DinoState {
> PCIHostState parent_obj;
>
> /* PCI_CONFIG_ADDR is parent_obj.config_reg, via pci_host_conf_be_ops,
> so that we can map PCI_CONFIG_DATA to pci_host_data_be_ops. */
> + uint32_t config_reg_dino; /* keep original copy, including 2 lowest bits */
>
> uint32_t iar0;
> uint32_t iar1;
> @@ -94,8 +112,12 @@ typedef struct DinoState {
> uint32_t ipr;
> uint32_t icr;
> uint32_t ilr;
> + uint32_t io_fbb_en;
> uint32_t io_addr_en;
> uint32_t io_control;
> + uint32_t toc_addr;
> +
> + uint32_t reg800[DINO800_REGS];
>
> MemoryRegion this_mem;
> MemoryRegion pci_mem;
> @@ -106,8 +128,6 @@ typedef struct DinoState {
> MemoryRegion bm_ram_alias;
> MemoryRegion bm_pci_alias;
> MemoryRegion bm_cpu_alias;
> -
> - MemoryRegion cpu0_eir_mem;
> } DinoState;
>
> /*
> @@ -122,6 +142,8 @@ static void gsc_to_pci_forwarding(DinoState *s)
> tmp = extract32(s->io_control, 7, 2);
> enabled = (tmp == 0x01);
> io_addr_en = s->io_addr_en;
> + /* Mask out first (=firmware) and last (=Dino) areas. */
> + io_addr_en &= ~(BIT(31) | BIT(0));
>
> memory_region_transaction_begin();
> for (i = 1; i < 31; i++) {
> @@ -142,6 +164,8 @@ static bool dino_chip_mem_valid(void *opaque, hwaddr addr,
> unsigned size, bool is_write,
> MemTxAttrs attrs)
> {
> + bool ret = false;
> +
> switch (addr) {
> case DINO_IAR0:
> case DINO_IAR1:
> @@ -152,16 +176,22 @@ static bool dino_chip_mem_valid(void *opaque, hwaddr addr,
> case DINO_ICR:
> case DINO_ILR:
> case DINO_IO_CONTROL:
> + case DINO_IO_FBB_EN:
> case DINO_IO_ADDR_EN:
> case DINO_PCI_IO_DATA:
> - return true;
> + case DINO_TOC_ADDR:
> + case DINO_GMASK ... DINO_TLTIM:
> + ret = true;
> + break;
> case DINO_PCI_IO_DATA + 2:
> - return size <= 2;
> + ret = (size <= 2);
> + break;
> case DINO_PCI_IO_DATA + 1:
> case DINO_PCI_IO_DATA + 3:
> - return size == 1;
> + ret = (size == 1);
> }
> - return false;
> + trace_dino_chip_mem_valid(addr, ret);
> + return ret;
> }
>
> static MemTxResult dino_chip_read_with_attrs(void *opaque, hwaddr addr,
> @@ -194,6 +224,9 @@ static MemTxResult dino_chip_read_with_attrs(void *opaque, hwaddr addr,
> }
> break;
>
> + case DINO_IO_FBB_EN:
> + val = s->io_fbb_en;
> + break;
> case DINO_IO_ADDR_EN:
> val = s->io_addr_en;
> break;
> @@ -227,12 +260,28 @@ static MemTxResult dino_chip_read_with_attrs(void *opaque, hwaddr addr,
> case DINO_IRR1:
> val = s->ilr & s->imr & s->icr;
> break;
> + case DINO_TOC_ADDR:
> + val = s->toc_addr;
> + break;
> + case DINO_GMASK ... DINO_TLTIM:
> + val = s->reg800[(addr - DINO_GMASK) / 4];
> + if (addr == DINO_PAMR) {
> + val &= ~0x01; /* LSB is hardwired to 0 */
> + }
> + if (addr == DINO_MLTIM) {
> + val &= ~0x07; /* 3 LSB are hardwired to 0 */
> + }
> + if (addr == DINO_BRDG_FEAT) {
> + val &= ~(0x10710E0ul | 8); /* bits 5-7, 24 & 15 reserved */
> + }
> + break;
>
> default:
> /* Controlled by dino_chip_mem_valid above. */
> g_assert_not_reached();
> }
>
> + trace_dino_chip_read(addr, val);
> *data = val;
> return ret;
> }
> @@ -245,6 +294,9 @@ static MemTxResult dino_chip_write_with_attrs(void *opaque, hwaddr addr,
> AddressSpace *io;
> MemTxResult ret;
> uint16_t ioaddr;
> + int i;
> +
> + trace_dino_chip_write(addr, val);
>
> switch (addr) {
> case DINO_IO_DATA ... DINO_PCI_IO_DATA + 3:
> @@ -266,9 +318,11 @@ static MemTxResult dino_chip_write_with_attrs(void *opaque, hwaddr addr,
> }
> return ret;
>
> + case DINO_IO_FBB_EN:
> + s->io_fbb_en = val & 0x03;
> + break;
> case DINO_IO_ADDR_EN:
> - /* Never allow first (=firmware) and last (=Dino) areas. */
> - s->io_addr_en = val & 0x7ffffffe;
> + s->io_addr_en = val;
> gsc_to_pci_forwarding(s);
> break;
> case DINO_IO_CONTROL:
> @@ -292,6 +346,10 @@ static MemTxResult dino_chip_write_with_attrs(void *opaque, hwaddr addr,
> /* Any write to IPR clears the register. */
> s->ipr = 0;
> break;
> + case DINO_TOC_ADDR:
> + /* IO_COMMAND of CPU with client_id bits */
> + s->toc_addr = 0xFFFA0030 | (val & 0x1e000);
> + break;
>
> case DINO_ILR:
> case DINO_IRR0:
> @@ -299,6 +357,12 @@ static MemTxResult dino_chip_write_with_attrs(void *opaque, hwaddr addr,
> /* These registers are read-only. */
> break;
>
> + case DINO_GMASK ... DINO_TLTIM:
> + i = (addr - DINO_GMASK) / 4;
> + val &= reg800_keep_bits[i];
Due to the missing register, Coverity reports:
>>> CID 1419394: Memory - illegal accesses (OVERRUN)
>>> Overrunning array "reg800_keep_bits" of 12 4-byte elements at
element index 12 (byte offset 48) using index "i" (which evaluates to 12).
> + s->reg800[i] = val;
> + break; > +
> default:
> /* Controlled by dino_chip_mem_valid above. */
> g_assert_not_reached();
> @@ -323,7 +387,7 @@ static const MemoryRegionOps dino_chip_ops = {
>
> static const VMStateDescription vmstate_dino = {
> .name = "Dino",
> - .version_id = 1,
> + .version_id = 2,
> .minimum_version_id = 1,
> .fields = (VMStateField[]) {
> VMSTATE_UINT32(iar0, DinoState),
> @@ -332,13 +396,14 @@ static const VMStateDescription vmstate_dino = {
> VMSTATE_UINT32(ipr, DinoState),
> VMSTATE_UINT32(icr, DinoState),
> VMSTATE_UINT32(ilr, DinoState),
> + VMSTATE_UINT32(io_fbb_en, DinoState),
> VMSTATE_UINT32(io_addr_en, DinoState),
> VMSTATE_UINT32(io_control, DinoState),
> + VMSTATE_UINT32(toc_addr, DinoState),
> VMSTATE_END_OF_LIST()
> }
> };
>
> -
> /* Unlike pci_config_data_le_ops, no check of high bit set in config_reg. */
>
> static uint64_t dino_config_data_read(void *opaque, hwaddr addr, unsigned len)
> @@ -362,14 +427,16 @@ static const MemoryRegionOps dino_config_data_ops = {
>
> static uint64_t dino_config_addr_read(void *opaque, hwaddr addr, unsigned len)
> {
> - PCIHostState *s = opaque;
> - return s->config_reg;
> + DinoState *s = opaque;
> + return s->config_reg_dino;
> }
>
> static void dino_config_addr_write(void *opaque, hwaddr addr,
> uint64_t val, unsigned len)
> {
> PCIHostState *s = opaque;
> + DinoState *ds = opaque;
> + ds->config_reg_dino = val; /* keep a copy of original value */
> s->config_reg = val & ~3U;
> }
>
> @@ -453,6 +520,8 @@ PCIBus *dino_init(MemoryRegion *addr_space,
>
> dev = qdev_create(NULL, TYPE_DINO_PCI_HOST_BRIDGE);
> s = DINO_PCI_HOST_BRIDGE(dev);
> + s->iar0 = s->iar1 = CPU_HPA + 3;
> + s->toc_addr = 0xFFFA0030; /* IO_COMMAND of CPU */
>
> /* Dino PCI access from main memory. */
> memory_region_init_io(&s->this_mem, OBJECT(s), &dino_chip_ops,
> diff --git a/hw/hppa/trace-events b/hw/hppa/trace-events
> index 4e2acb6176..f943b16c4e 100644
> --- a/hw/hppa/trace-events
> +++ b/hw/hppa/trace-events
> @@ -2,3 +2,8 @@
>
> # pci.c
> hppa_pci_iack_write(void) ""
> +
> +# dino.c
> +dino_chip_mem_valid(uint64_t addr, uint32_t val) "access to addr 0x%"PRIx64" is %d"
> +dino_chip_read(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%08x"
> +dino_chip_write(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%08x"
>
next prev parent reply other threads:[~2020-02-12 23:38 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-20 21:15 [PATCH v5 0/6] HPPA: i82596, PS/2 and graphics emulation Sven Schnelle
2019-12-20 21:15 ` [PATCH v5 1/6] hw/hppa/dino.c: Improve emulation of Dino PCI chip Sven Schnelle
2020-02-12 23:37 ` Philippe Mathieu-Daudé [this message]
2020-02-13 22:59 ` Philippe Mathieu-Daudé
2019-12-20 21:15 ` [PATCH v5 2/6] hppa: Add support for LASI chip with i82596 NIC Sven Schnelle
2019-12-20 21:15 ` [PATCH v5 3/6] ps2: accept 'Set Key Make and Break' commands Sven Schnelle
2019-12-20 21:15 ` [PATCH v5 4/6] hppa: add emulation of LASI PS2 controllers Sven Schnelle
2020-01-03 6:15 ` Philippe Mathieu-Daudé
2020-01-19 17:22 ` Sven Schnelle
2019-12-20 21:15 ` [PATCH v5 5/6] hppa: Add emulation of Artist graphics Sven Schnelle
2019-12-22 12:37 ` Philippe Mathieu-Daudé
2019-12-23 17:50 ` Sven Schnelle
2019-12-24 0:18 ` Philippe Mathieu-Daudé
2019-12-27 20:57 ` Helge Deller
2019-12-29 23:15 ` Philippe Mathieu-Daudé
2020-02-12 23:55 ` Philippe Mathieu-Daudé
2019-12-20 21:15 ` [PATCH v5 6/6] seabios-hppa: update to latest version Sven Schnelle
2019-12-22 12:33 ` Philippe Mathieu-Daudé
2019-12-21 22:22 ` [PATCH v5 0/6] HPPA: i82596, PS/2 and graphics emulation Helge Deller
2019-12-21 22:24 ` [PATCH 1/2] hppa: Do not enable artist graphics with -nographic option Helge Deller
2019-12-22 8:39 ` Sven Schnelle
2019-12-22 10:22 ` Helge Deller
2019-12-29 1:25 ` Richard Henderson
2019-12-21 22:25 ` [PATCH 2/2] hppa: Switch to tulip NIC by default Helge Deller
2019-12-29 1:25 ` Richard Henderson
2019-12-29 1:25 ` [PATCH v5 0/6] HPPA: i82596, PS/2 and graphics emulation Richard Henderson
2019-12-29 13:08 ` Helge Deller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=779e7a6e-00e1-1f51-7132-1cd389bbd921@redhat.com \
--to=philmd@redhat.com \
--cc=deller@gmx.de \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
--cc=svens@stackframe.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).