| Message ID | 4E75E7B0.9080104@web.de |
|---|---|
| State | New |
| Headers | show |
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?
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
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.
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
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).
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(), };