Re: [Qemu-devel] [PATCH v2] hw/hppa/dino: mask out lower 2 bits of PCI config addr

[Richard Henderson]

On 2/18/19 10:33 AM, Sven Schnelle wrote:
> some versions of HP-UX 10.20 seems to rely on the fact that DINO
> strips out the lower 2 bits of the PCI configuration address.
> Also update the binary SeaBIOS distributed to the latest version
> from Helge's repository, which is required with that change.
> 
> Signed-off-by: Sven Schnelle <address@hidden>
> ---
>  hw/hppa/dino.c            |  25 ++++++++++++++++++++++---
>  pc-bios/hppa-firmware.img | Bin 215936 -> 760040 bytes
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
> index 360716de57..63441783c4 100644
> --- a/hw/hppa/dino.c
> +++ b/hw/hppa/dino.c
> @@ -178,7 +178,7 @@ static MemTxResult dino_chip_read_with_attrs(void 
> *opaque, hwaddr addr,
>      case DINO_PCI_IO_DATA ... DINO_PCI_IO_DATA + 3:
>          /* Read from PCI IO space. */
>          io = &address_space_io;
> -        ioaddr = s->parent_obj.config_reg;
> +        ioaddr = s->parent_obj.config_reg + (addr & 3);
>          switch (size) {
>          case 1:
>              val = address_space_ldub(io, ioaddr, attrs, &ret);
> @@ -250,7 +250,7 @@ static MemTxResult dino_chip_write_with_attrs(void 
> *opaque, hwaddr addr,
>      case DINO_IO_DATA ... DINO_PCI_IO_DATA + 3:
>          /* Write into PCI IO space.  */
>          io = &address_space_io;
> -        ioaddr = s->parent_obj.config_reg;
> +        ioaddr = s->parent_obj.config_reg + (addr & 3);
>          switch (size) {
>          case 1:
>              address_space_stb(io, ioaddr, val, attrs, &ret);
> @@ -360,6 +360,25 @@ static const MemoryRegionOps dino_config_data_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static uint64_t dino_config_addr_read(void *opaque, hwaddr addr, unsigned 
> len)
> +{
> +    PCIHostState *s = opaque;
> +    return s->config_reg & ~3U;
> +}
> +
> +static void dino_config_addr_write(void *opaque, hwaddr addr,
> +                                   uint64_t val, unsigned len)
> +{
> +    PCIHostState *s = opaque;
> +    s->config_reg = val & ~3U;
> +}
> +
> +static const MemoryRegionOps dino_config_addr_ops = {
> +    .read = dino_config_addr_read,
> +    .write = dino_config_addr_write,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +};
> +

Can you re-test with the following?

If the stores are all aligned, then the reads to not need to re-align.
Also, partial writes to the config register don't make much sense.
Over in hw/pci/pci_host.c, we check addr == 0 && len == 4, which is a
very old fashioned way to do the .valid thing.


r~



diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
index 63441783c4..40f9e1a963 100644
--- a/hw/hppa/dino.c
+++ b/hw/hppa/dino.c
@@ -363,7 +363,7 @@ 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 & ~3U;
+    return s->config_reg;
 }

 static void dino_config_addr_write(void *opaque, hwaddr addr,
@@ -376,6 +376,8 @@ static void dino_config_addr_write(void *opaque, hwaddr 
addr,
 static const MemoryRegionOps dino_config_addr_ops = {
     .read = dino_config_addr_read,
     .write = dino_config_addr_write,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
     .endianness = DEVICE_BIG_ENDIAN,
 };