On 2012-09-11 14:14, Julien Grall wrote: > On 09/11/2012 12:57 PM, Jan Kiszka wrote: > >> On 2012-09-11 13:48, Jan Kiszka wrote: >>> On 2012-09-11 13:27, Julien Grall wrote: >>>> On 09/11/2012 10:25 AM, Avi Kivity wrote: >>>>> On 09/11/2012 12:15 PM, Avi Kivity wrote: >>>>> >>>>>> On 09/04/2012 06:13 PM, Julien Grall wrote: >>>>>> >>>>>>> This is the nineth version of patch series about ioport registration. >>>>>>> >>>>>>> Some part of QEMU still use register_ioport* functions to register >>>>>>> ioport. >>>>>>> These functions doesn't allow to use Memory Listener on it. >>>>>>> >>>>>> Thanks, applied all (w/ updated patch 4), will push shortly. >>>>>> >>>>>> >>>>>> >>>>> Aborts with the command line >>>>> >>>>> qemu-system-x86_64 -device isa-debugcon,iobase=0x402,chardev=stdio >>>>> -chardev stdio,id=stdio >>>>> >>>>> >>>>> >>>> >>>> I have bisected and found the problem with bochs_bios_init in hw/pc.c. >>>> Bosch already register the iport 0x402. I'm not sure that it's a bug. >>>> In fact register_ioport_read/write check if the current ioport is used >>>> with the opaque variable. >>>> Before my patch, bochs_bios_init registered it's ioport with opaque >>>> NULL, so if someone (like debugcon) wants to use the ioport there is >>>> no problem. But now, I used portio_list_init to register bochs ioport, >>>> so the opaque is not NULL. >>>> I don't really know how to resolve this problem. Perhaps we could >>>> just improve the debug message. >>> >>> My suggestion: >>> >>> pc: Don't listen on debug ports by default >>> >>> Only listen on debug ports when we also handle them. They are better >>> handled by debugcon these days which is runtime configurable. >>> >>> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> >>> >>> diff --git a/hw/pc.c b/hw/pc.c >>> index 112739a..70abf0e 100644 >>> --- a/hw/pc.c >>> +++ b/hw/pc.c >>> @@ -539,9 +539,9 @@ static void bochs_bios_write(void *opaque, uint32_t >>> addr, uint32_t val) >>> case 0x401: >>> /* used to be panic, now unused */ >>> break; >>> +#ifdef DEBUG_BIOS >>> case 0x402: >>> case 0x403: >>> -#ifdef DEBUG_BIOS >>> fprintf(stderr, "%c", val); >>> #endif >>> break; >>> >> >> OK, ket's try properly again: >> >> ----8<----- >> >> Only listen on debug ports when we also handle them. They are better >> handled by debugcon these days which is runtime configurable. >> >> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> >> --- >> hw/pc.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pc.c b/hw/pc.c >> index 112739a..134d5f7 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -539,12 +539,12 @@ static void bochs_bios_write(void *opaque, uint32_t >> addr, uint32_t val) >> case 0x401: >> /* used to be panic, now unused */ >> break; >> +#ifdef DEBUG_BIOS >> case 0x402: >> case 0x403: >> -#ifdef DEBUG_BIOS >> fprintf(stderr, "%c", val); >> -#endif >> break; >> +#endif >> case 0x8900: >> /* same as Bochs power off */ >> if (val == shutdown_str[shutdown_index]) { > > > Is it possible to modify this part: > >> @@ -598,8 +598,10 @@ static void *bochs_bios_init(void) >> >> register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL); >> register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL); >> +#ifdef DEBUG_BIOS >> register_ioport_write(0x402, 1, 1, bochs_bios_write, NULL); >> register_ioport_write(0x403, 1, 1, bochs_bios_write, NULL); >> +#endif >> register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL); >> >> register_ioport_write(0x501, 1, 1, bochs_bios_write, NULL); > > > by something like that: > > -------------------------- > @@ -592,7 +592,9 @@ int e820_add_entry(uint64_t address, uint64_t length, > uint32_t type) > > static const MemoryRegionPortio bochs_bios_portio_list[] = { > { 0x400, 2, 2, .write = bochs_bios_write, }, /* 0x400 */ > +#ifdef DEBUG_BIOS > { 0x402, 2, 1, .write = bochs_bios_write, }, /* 0x402 */ > +#endif > { 0x500, 1, 1, .write = bochs_bios_write, }, /* 0x500 */ > { 0x501, 1, 1, .write = bochs_bios_write, }, /* 0x501 */ > { 0x501, 2, 2, .write = bochs_bios_write, }, /* 0x501 */ > > --------------------------- > So it can be applied just after: "memory: unify ioport registration" > patch series.Otherwise, I will modify my patch series.
No, lets do it again in an order that avoids temporal regressions, specifically as autotests depend on debugcon. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux