[2/2] vga: Fix portio list conversion fallouts

Submitted by Jan Kiszka on Sept. 18, 2011, 12:44 p.m.

Details

Message ID 4E75E7B0.9080104@web.de
State New
Headers show

Commit Message

Jan Kiszka Sept. 18, 2011, 12:44 p.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

Fix copy&paste errors and reduce duplications of the BOCHS VBE ranges.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

This and the previous patch unbreaks VGA over memory/master.

 hw/vga.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

Comments

Avi Kivity Sept. 18, 2011, 3:51 p.m.
On 09/18/2011 03:44 PM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> Fix copy&paste errors and reduce duplications of the BOCHS VBE ranges.
>
> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> ---
>
>   #ifdef CONFIG_BOCHS_VBE
>   static const MemoryRegionPortio vbe_portio_list[] = {
> -# ifdef TARGET_I386
>       { 0, 1, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index },
> +# ifdef TARGET_I386

Strange how the card knows what target it's plugged into.  Fixing this 
is going to involve major pain - need to update the bios and need to 
provide backwards compatibility.  All this assuming guests don't 
hardcode the address.

Anyway I find the deduplication makes the code harder to read.

>       { 1, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
>   # else
> -    { 0, 2, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index },
> -    { 2, 2, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
> +    { 2, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },

Is this correct?  length 1 region with size 2?
Jan Kiszka Sept. 18, 2011, 4:28 p.m.
On 2011-09-18 17:51, Avi Kivity wrote:
> On 09/18/2011 03:44 PM, Jan Kiszka wrote:
>> From: Jan Kiszka<jan.kiszka@siemens.com>
>>
>> Fix copy&paste errors and reduce duplications of the BOCHS VBE ranges.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>
>>   #ifdef CONFIG_BOCHS_VBE
>>   static const MemoryRegionPortio vbe_portio_list[] = {
>> -# ifdef TARGET_I386
>>       { 0, 1, 2, .read = vbe_ioport_read_index, .write =
>> vbe_ioport_write_index },
>> +# ifdef TARGET_I386
> 
> Strange how the card knows what target it's plugged into.  Fixing this
> is going to involve major pain - need to update the bios and need to
> provide backwards compatibility.  All this assuming guests don't
> hardcode the address.
> 
> Anyway I find the deduplication makes the code harder to read.

That line is rather tricky to deduplicate in mind. So it's not nice for
our readers keeping it duplicated around.

> 
>>       { 1, 1, 2, .read = vbe_ioport_read_data, .write =
>> vbe_ioport_write_data },
>>   # else
>> -    { 0, 2, 2, .read = vbe_ioport_read_index, .write =
>> vbe_ioport_write_index },
>> -    { 2, 2, 2, .read = vbe_ioport_read_data, .write =
>> vbe_ioport_write_data },
>> +    { 2, 1, 2, .read = vbe_ioport_read_data, .write =
>> vbe_ioport_write_data },
> 
> Is this correct?  length 1 region with size 2?

Yes, it prevents accesses on base+1.

Jan
Avi Kivity Sept. 18, 2011, 4:51 p.m.
On 09/18/2011 07:28 PM, Jan Kiszka wrote:
> On 2011-09-18 17:51, Avi Kivity wrote:
> >  On 09/18/2011 03:44 PM, Jan Kiszka wrote:
> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
> >>
> >>  Fix copy&paste errors and reduce duplications of the BOCHS VBE ranges.
> >>
> >>  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>  ---
> >>
> >>    #ifdef CONFIG_BOCHS_VBE
> >>    static const MemoryRegionPortio vbe_portio_list[] = {
> >>  -# ifdef TARGET_I386
> >>        { 0, 1, 2, .read = vbe_ioport_read_index, .write =
> >>  vbe_ioport_write_index },
> >>  +# ifdef TARGET_I386
> >
> >  Strange how the card knows what target it's plugged into.  Fixing this
> >  is going to involve major pain - need to update the bios and need to
> >  provide backwards compatibility.  All this assuming guests don't
> >  hardcode the address.
> >
> >  Anyway I find the deduplication makes the code harder to read.
>
> That line is rather tricky to deduplicate in mind. So it's not nice for
> our readers keeping it duplicated around.

I don't think it should be deduplicated.  The device is providing two 
separate ABIs.

> >
> >>        { 1, 1, 2, .read = vbe_ioport_read_data, .write =
> >>  vbe_ioport_write_data },
> >>    # else
> >>  -    { 0, 2, 2, .read = vbe_ioport_read_index, .write =
> >>  vbe_ioport_write_index },
> >>  -    { 2, 2, 2, .read = vbe_ioport_read_data, .write =
> >>  vbe_ioport_write_data },
> >>  +    { 2, 1, 2, .read = vbe_ioport_read_data, .write =
> >>  vbe_ioport_write_data },
> >
> >  Is this correct?  length 1 region with size 2?
>
> Yes, it prevents accesses on base+1.
>

Why disallow it?

For the general case or a 2-byte access near the end of a region, we 
need to provide a general fix.
Jan Kiszka Sept. 18, 2011, 7:07 p.m.
On 2011-09-18 18:51, Avi Kivity wrote:
> On 09/18/2011 07:28 PM, Jan Kiszka wrote:
>> On 2011-09-18 17:51, Avi Kivity wrote:
>> >  On 09/18/2011 03:44 PM, Jan Kiszka wrote:
>> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
>> >>
>> >>  Fix copy&paste errors and reduce duplications of the BOCHS VBE
>> ranges.
>> >>
>> >>  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> >>  ---
>> >>
>> >>    #ifdef CONFIG_BOCHS_VBE
>> >>    static const MemoryRegionPortio vbe_portio_list[] = {
>> >>  -# ifdef TARGET_I386
>> >>        { 0, 1, 2, .read = vbe_ioport_read_index, .write =
>> >>  vbe_ioport_write_index },
>> >>  +# ifdef TARGET_I386
>> >
>> >  Strange how the card knows what target it's plugged into.  Fixing this
>> >  is going to involve major pain - need to update the bios and need to
>> >  provide backwards compatibility.  All this assuming guests don't
>> >  hardcode the address.
>> >
>> >  Anyway I find the deduplication makes the code harder to read.
>>
>> That line is rather tricky to deduplicate in mind. So it's not nice for
>> our readers keeping it duplicated around.
> 
> I don't think it should be deduplicated.  The device is providing two
> separate ABIs.

Yes, two ABIs, and the only difference is the offset of the data register.

> 
>> >
>> >>        { 1, 1, 2, .read = vbe_ioport_read_data, .write =
>> >>  vbe_ioport_write_data },
>> >>    # else
>> >>  -    { 0, 2, 2, .read = vbe_ioport_read_index, .write =
>> >>  vbe_ioport_write_index },
>> >>  -    { 2, 2, 2, .read = vbe_ioport_read_data, .write =
>> >>  vbe_ioport_write_data },
>> >>  +    { 2, 1, 2, .read = vbe_ioport_read_data, .write =
>> >>  vbe_ioport_write_data },
>> >
>> >  Is this correct?  length 1 region with size 2?
>>
>> Yes, it prevents accesses on base+1.
>>
> 
> Why disallow it?

Did anyone check that something useful or at least valid comes out of
the handlers when doing this so far impossible access?

Jan
Avi Kivity Sept. 19, 2011, 12:22 p.m.
On 09/18/2011 10:07 PM, Jan Kiszka wrote:
> >  I don't think it should be deduplicated.  The device is providing two
> >  separate ABIs.
>
> Yes, two ABIs, and the only difference is the offset of the data register.

How about

#ifdef TARGET_I386
# define VBE_DATA_REG 1
#else
# define VBE_DATA_REG 2
#endif


> >
> >  Why disallow it?
>
> Did anyone check that something useful or at least valid comes out of
> the handlers when doing this so far impossible access?
>

It's a general problem.  It can't be fixed in devices, the core has to 
handle this (and devices have to tell it how to react to such accesses).

Patch hide | download patch | download mbox

diff --git a/hw/vga.c b/hw/vga.c
index 513a5f6..22160e8 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2244,19 +2244,18 @@  static const MemoryRegionPortio vga_portio_list[] = {
     { 0x04,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3b4 */
     { 0x0a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3ba */
     { 0x10, 16, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3c0 */
-    { 0x14,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3d4 */
-    { 0x1a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3da */
+    { 0x24,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3d4 */
+    { 0x2a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3da */
     PORTIO_END_OF_LIST(),
 };
 
 #ifdef CONFIG_BOCHS_VBE
 static const MemoryRegionPortio vbe_portio_list[] = {
-# ifdef TARGET_I386
     { 0, 1, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index },
+# ifdef TARGET_I386
     { 1, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
 # else
-    { 0, 2, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index },
-    { 2, 2, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
+    { 2, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
 # endif
     PORTIO_END_OF_LIST(),
 };