[Qemu-devel] [PATCH for-2.3 0/3] ioport fixes

[Qemu-devel] [PATCH for-2.3 0/3] ioport fixes

[Paolo Bonzini]

These fix the problem with OpenBIOS's accesses to the VBE DISPI ports.
The problem stems from weird memory regions created by ioport.c.

Patches 1-3 simplify some of the MemoryRegionPortio arrays.  This makes it
easier to ascertain the effect of patch 6, since the patch only affects
MemoryRegionPortio entries whose size is greater than one.

Patch 4 fixes a comment.  ioport.c is crazy enough without wrong
documentation.

The fix is in patch 6; however it makes it possible to trigger some
assertions in ioport.c so patch 5 is needed in order to loosen them.

Peter, if you prefer you can just revert c3c1bb99.

Paolo

Paolo Bonzini (6):
  sb16: remove useless mixer_write_indexw
  gus: clean up MemoryRegionPortio
  ide: there is only one data port
  ioport: remove wrong comment
  ioport: loosen assertions on emulation of 16-bit ports
  ioport: reserve the whole range of an I/O port in the AddressSpace

 hw/audio/gus.c  | 22 ++--------------------
 hw/audio/sb16.c |  7 -------
 hw/ide/core.c   |  4 ++--
 ioport.c        | 30 +++++++++++++++++-------------
 4 files changed, 21 insertions(+), 42 deletions(-)

-- 
2.3.4

[Qemu-devel] [PATCH for-2.3 1/6] sb16: remove useless mixer_write_indexw

[Paolo Bonzini]

ioport.c is already able to split a 16-bit access into two 8-bit
accesses to consecutive ports.  Tested with Epic Pinball.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/sb16.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index 444eb9e..b052de5 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -1121,12 +1121,6 @@ static IO_WRITE_PROTO (mixer_write_datab)
     s->mixer_regs[s->mixer_nreg] = val;
 }
 
-static IO_WRITE_PROTO (mixer_write_indexw)
-{
-    mixer_write_indexb (opaque, nport, val & 0xff);
-    mixer_write_datab (opaque, nport, (val >> 8) & 0xff);
-}
-
 static IO_READ_PROTO (mixer_read)
 {
     SB16State *s = opaque;
@@ -1345,7 +1339,6 @@ static const VMStateDescription vmstate_sb16 = {
 
 static const MemoryRegionPortio sb16_ioport_list[] = {
     {  4, 1, 1, .write = mixer_write_indexb },
-    {  4, 1, 2, .write = mixer_write_indexw },
     {  5, 1, 1, .read = mixer_read, .write = mixer_write_datab },
     {  6, 1, 1, .read = dsp_read, .write = dsp_write },
     { 10, 1, 1, .read = dsp_read },
-- 
2.3.4

Re: [Qemu-devel] [PATCH for-2.3 1/6] sb16: remove useless mixer_write_indexw

[Andreas Färber]

Am 30.03.2015 um 13:45 schrieb Paolo Bonzini:
> ioport.c is already able to split a 16-bit access into two 8-bit
> accesses to consecutive ports.  Tested with Epic Pinball.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/audio/sb16.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
> index 444eb9e..b052de5 100644
> --- a/hw/audio/sb16.c
> +++ b/hw/audio/sb16.c
> @@ -1121,12 +1121,6 @@ static IO_WRITE_PROTO (mixer_write_datab)
>      s->mixer_regs[s->mixer_nreg] = val;
>  }
>  
> -static IO_WRITE_PROTO (mixer_write_indexw)
> -{
> -    mixer_write_indexb (opaque, nport, val & 0xff);
> -    mixer_write_datab (opaque, nport, (val >> 8) & 0xff);
> -}

I assume you've verified the endianness handling, too?

Reviewed-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH for-2.3 1/6] sb16: remove useless mixer_write_indexw

[Paolo Bonzini]

On 31/03/2015 13:19, Andreas Färber wrote:
> Am 30.03.2015 um 13:45 schrieb Paolo Bonzini:
>> ioport.c is already able to split a 16-bit access into two 8-bit
>> accesses to consecutive ports.  Tested with Epic Pinball.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/audio/sb16.c | 7 -------
>>  1 file changed, 7 deletions(-)
>>
>> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
>> index 444eb9e..b052de5 100644
>> --- a/hw/audio/sb16.c
>> +++ b/hw/audio/sb16.c
>> @@ -1121,12 +1121,6 @@ static IO_WRITE_PROTO (mixer_write_datab)
>>      s->mixer_regs[s->mixer_nreg] = val;
>>  }
>>  
>> -static IO_WRITE_PROTO (mixer_write_indexw)
>> -{
>> -    mixer_write_indexb (opaque, nport, val & 0xff);
>> -    mixer_write_datab (opaque, nport, (val >> 8) & 0xff);
>> -}
> 
> I assume you've verified the endianness handling, too?

Yes, it's little-endian in either case:

        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);

for the sb16, index is 0x224 and data is 0x225.

Paolo

[Qemu-devel] [PATCH for-2.3 2/6] gus: clean up MemoryRegionPortio

[Paolo Bonzini]

Remove 16-bit reads/writes, since ioport.c is able to synthesize them.
Remove the two MIDI registers (0x300 and 0x301) from gus_portio_list1,
and add the second MIDI register (0x301) to gus_portio_list2.

Tested with 2nd Reality.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/gus.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/hw/audio/gus.c b/hw/audio/gus.c
index 4a43ce7..772010e 100644
--- a/hw/audio/gus.c
+++ b/hw/audio/gus.c
@@ -71,13 +71,6 @@ IO_READ_PROTO (gus_readb)
     return gus_read (&s->emu, nport, 1);
 }
 
-IO_READ_PROTO (gus_readw)
-{
-    GUSState *s = opaque;
-
-    return gus_read (&s->emu, nport, 2);
-}
-
 IO_WRITE_PROTO (gus_writeb)
 {
     GUSState *s = opaque;
@@ -85,13 +78,6 @@ IO_WRITE_PROTO (gus_writeb)
     gus_write (&s->emu, nport, 1, val);
 }
 
-IO_WRITE_PROTO (gus_writew)
-{
-    GUSState *s = opaque;
-
-    gus_write (&s->emu, nport, 2, val);
-}
-
 static int write_audio (GUSState *s, int samples)
 {
     int net = 0;
@@ -236,17 +222,13 @@ static const VMStateDescription vmstate_gus = {
 
 static const MemoryRegionPortio gus_portio_list1[] = {
     {0x000,  1, 1, .write = gus_writeb },
-    {0x000,  1, 2, .write = gus_writew },
     {0x006, 10, 1, .read = gus_readb, .write = gus_writeb },
-    {0x006, 10, 2, .read = gus_readw, .write = gus_writew },
-    {0x100,  8, 1, .read = gus_readb, .write = gus_writeb },
-    {0x100,  8, 2, .read = gus_readw, .write = gus_writew },
+    {0x102,  6, 1, .read = gus_readb, .write = gus_writeb },
     PORTIO_END_OF_LIST (),
 };
 
 static const MemoryRegionPortio gus_portio_list2[] = {
-    {0, 1, 1, .read = gus_readb },
-    {0, 1, 2, .read = gus_readw },
+    {0, 2, 1, .read = gus_readb },
     PORTIO_END_OF_LIST (),
 };
 
-- 
2.3.4

[Qemu-devel] [PATCH for-2.3 3/6] ide: there is only one data port

[Paolo Bonzini]

IDE PIO data must be written, for example, at 0x1f0.  You cannot
do word or dword writes to 0x1f1..0x1f3 to access the data register.
Adjust the ide_portio_list accordingly.

Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index a895fd8..fcb9080 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2436,8 +2436,8 @@ void ide_init2(IDEBus *bus, qemu_irq irq)
 
 static const MemoryRegionPortio ide_portio_list[] = {
     { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
-    { 0, 2, 2, .read = ide_data_readw, .write = ide_data_writew },
-    { 0, 4, 4, .read = ide_data_readl, .write = ide_data_writel },
+    { 0, 1, 2, .read = ide_data_readw, .write = ide_data_writew },
+    { 0, 1, 4, .read = ide_data_readl, .write = ide_data_writel },
     PORTIO_END_OF_LIST(),
 };
 
-- 
2.3.4

[Qemu-devel] [PATCH for-2.3 4/6] ioport: remove wrong comment

[Paolo Bonzini]

ioport.c has not been using an alias since commit b40acf9 (ioport:
Switch dispatching to memory core layer, 2013-06-24).  Remove the
obsolete comment.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 ioport.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/ioport.c b/ioport.c
index 783a3ae..eb954e3 100644
--- a/ioport.c
+++ b/ioport.c
@@ -239,10 +239,6 @@ static void portio_list_add_1(PortioList *piolist,
         mrpio->ports[i].base = start + off_low;
     }
 
-    /*
-     * Use an alias so that the callback is called with an absolute address,
-     * rather than an offset relative to to start + off_low.
-     */
     memory_region_init_io(&mrpio->mr, piolist->owner, &portio_ops, mrpio,
                           piolist->name, off_high - off_low);
     if (piolist->flush_coalesced_mmio) {
-- 
2.3.4

[Qemu-devel] [PATCH for-2.3 5/6] ioport: loosen assertions on emulation of 16-bit ports

[Paolo Bonzini]

Right now, ioport.c assumes that the entire range specified with
MemoryRegionPortio includes a region with size == 1.  This however
is not true for the VBE DISPI ports, which are 16-bit only.  The
next patch will make these regions' length equal to two, which can
cause the assertions to trigger.  Replace them with simple conditionals.

Also, ioport.c will emulate a 16-bit ioport with two distinct reads
or writes, even if one of the two accesses is out of the bounds given
by the MemoryRegionPortio array.  Do not do this anymore, instead
discard writes to the incorrect register and read it as all-ones.
This ensures that the mrp->read and mrp->write callbacks get an
in-range ioport number.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 ioport.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/ioport.c b/ioport.c
index eb954e3..090c262 100644
--- a/ioport.c
+++ b/ioport.c
@@ -187,9 +187,14 @@ static uint64_t portio_read(void *opaque, hwaddr addr, unsigned size)
         data = mrp->read(mrpio->portio_opaque, mrp->base + addr);
     } else if (size == 2) {
         mrp = find_portio(mrpio, addr, 1, false);
-        assert(mrp);
-        data = mrp->read(mrpio->portio_opaque, mrp->base + addr) |
-                (mrp->read(mrpio->portio_opaque, mrp->base + addr + 1) << 8);
+        if (mrp) {
+            data = mrp->read(mrpio->portio_opaque, mrp->base + addr);
+            if (addr + 1 < mrp->offset + mrp->len) {
+                data |= mrp->read(mrpio->portio_opaque, mrp->base + addr + 1) << 8;
+            } else {
+                data |= 0xff00;
+            }
+        }
     }
     return data;
 }
@@ -204,9 +209,12 @@ static void portio_write(void *opaque, hwaddr addr, uint64_t data,
         mrp->write(mrpio->portio_opaque, mrp->base + addr, data);
     } else if (size == 2) {
         mrp = find_portio(mrpio, addr, 1, true);
-        assert(mrp);
-        mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
-        mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);
+        if (mrp) {
+            mrp->write(mrpio->portio_opaque, mrp->base + addr, data & 0xff);
+            if (addr + 1 < mrp->offset + mrp->len) {
+                mrp->write(mrpio->portio_opaque, mrp->base + addr + 1, data >> 8);
+            }
+        }
     }
 }
 
-- 
2.3.4

[Qemu-devel] [PATCH for-2.3 6/6] ioport: reserve the whole range of an I/O port in the AddressSpace

[Paolo Bonzini]

When an I/O port is more than 1 byte long, ioport.c is currently
creating "short" regions, for example 0x1ce-0x1ce for the 16-bit
Bochs index port.  When I/O ports are memory mapped, and thus
accessed via a subpage_ops memory region, subpage_accepts gets
confused because it finds a hole at 0x1cf and rejects the access.

In order to fix this, modify registration of the region to cover
the whole size of the I/O port.  Attempts to access an invalid
port will be blocked by find_portio returning NULL.

This only affects the VBE DISPI regions.  For all other cases,
the MemoryRegionPortio entries for 2- or 4-byte accesses overlap
an entry for 1-byte accesses, thus the size of the memory region
is not affected.

Reported-by: Zoltan Balaton <balaton@eik.bme.hu>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 ioport.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ioport.c b/ioport.c
index 090c262..304d5d6 100644
--- a/ioport.c
+++ b/ioport.c
@@ -269,7 +269,7 @@ void portio_list_add(PortioList *piolist,
 
     /* Handle the first entry specially.  */
     off_last = off_low = pio_start->offset;
-    off_high = off_low + pio_start->len;
+    off_high = off_low + pio_start->len + pio_start->size - 1;
     count = 1;
 
     for (pio = pio_start + 1; pio->size != 0; pio++, count++) {
@@ -284,10 +284,10 @@ void portio_list_add(PortioList *piolist,
             /* ... and start collecting anew.  */
             pio_start = pio;
             off_low = off_last;
-            off_high = off_low + pio->len;
+            off_high = off_low + pio->len + pio_start->size - 1;
             count = 0;
         } else if (off_last + pio->len > off_high) {
-            off_high = off_last + pio->len;
+            off_high = off_last + pio->len + pio_start->size - 1;
         }
     }
 
-- 
2.3.4

Re: [Qemu-devel] [PATCH for-2.3 0/3] ioport fixes

[Mark Cave-Ayland]

On 30/03/15 12:45, Paolo Bonzini wrote:

> These fix the problem with OpenBIOS's accesses to the VBE DISPI ports.
> The problem stems from weird memory regions created by ioport.c.
> 
> Patches 1-3 simplify some of the MemoryRegionPortio arrays.  This makes it
> easier to ascertain the effect of patch 6, since the patch only affects
> MemoryRegionPortio entries whose size is greater than one.
> 
> Patch 4 fixes a comment.  ioport.c is crazy enough without wrong
> documentation.
> 
> The fix is in patch 6; however it makes it possible to trigger some
> assertions in ioport.c so patch 5 is needed in order to loosen them.
> 
> Peter, if you prefer you can just revert c3c1bb99.
> 
> Paolo
> 
> Paolo Bonzini (6):
>   sb16: remove useless mixer_write_indexw
>   gus: clean up MemoryRegionPortio
>   ide: there is only one data port
>   ioport: remove wrong comment
>   ioport: loosen assertions on emulation of 16-bit ports
>   ioport: reserve the whole range of an I/O port in the AddressSpace
> 
>  hw/audio/gus.c  | 22 ++--------------------
>  hw/audio/sb16.c |  7 -------
>  hw/ide/core.c   |  4 ++--
>  ioport.c        | 30 +++++++++++++++++-------------
>  4 files changed, 21 insertions(+), 42 deletions(-)

While I can't vouch for the effects outside of qemu-system-ppc and
qemu-system-sparc64, I can confirm that this patch series fixes the VGA
VBE register accesses in my testing here. Thanks Paolo!

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH for-2.3 0/3] ioport fixes

[Peter Maydell]

On 30 March 2015 at 12:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
> These fix the problem with OpenBIOS's accesses to the VBE DISPI ports.
> The problem stems from weird memory regions created by ioport.c.
>
> Patches 1-3 simplify some of the MemoryRegionPortio arrays.  This makes it
> easier to ascertain the effect of patch 6, since the patch only affects
> MemoryRegionPortio entries whose size is greater than one.
>
> Patch 4 fixes a comment.  ioport.c is crazy enough without wrong
> documentation.
>
> The fix is in patch 6; however it makes it possible to trigger some
> assertions in ioport.c so patch 5 is needed in order to loosen them.
>
> Peter, if you prefer you can just revert c3c1bb99.

Given the SPARC reset breakage also, I agree with you
(in the other thread) that we should revert c3c1bb99
for 2.3, and fix this all properly in 2.4.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.3 0/3] ioport fixes

[Paolo Bonzini]

On 01/04/2015 11:10, Peter Maydell wrote:
>> > Peter, if you prefer you can just revert c3c1bb99.
> Given the SPARC reset breakage also, I agree with you
> (in the other thread) that we should revert c3c1bb99
> for 2.3, and fix this all properly in 2.4.

Yup, pull request coming.

Paolo