| Message ID | 504F273B.4050602@siemens.com |
|---|---|
| State | New |
| Headers | show |
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.kiszka@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.kiszka@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. -- Julien Grall
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.kiszka@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.kiszka@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
On 09/11/2012 02:57 PM, Jan Kiszka wrote: > 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.kiszka@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]) { > @@ -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); > Why not drop DEBUG_BIOS completely? If you want to debug the bios, use debugcon.
On 2012-09-11 14:53, Avi Kivity wrote: > On 09/11/2012 02:57 PM, Jan Kiszka wrote: > >> 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.kiszka@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]) { >> @@ -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); >> > > > Why not drop DEBUG_BIOS completely? If you want to debug the bios, use > debugcon. Probably true. There is more practically dead stuff below this that just prints if DEBUG_BIOS is enabled. Jan
On 09/11/2012 05:11 PM, Jan Kiszka wrote: > On 2012-09-11 14:53, Avi Kivity wrote: >> On 09/11/2012 02:57 PM, Jan Kiszka wrote: >> >>> 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.kiszka@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]) { >>> @@ -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); >>> >> >> >> Why not drop DEBUG_BIOS completely? If you want to debug the bios, use >> debugcon. > > Probably true. There is more practically dead stuff below this that just > prints if DEBUG_BIOS is enabled. Actually it is autotest that is at fault here. It is installing a debugcon with non-standard iobase atop a builtin device. Pre-memory-API, we did not detect that. We can subclass isa-debugcon as bochs-debugcon, change the default ioport to 0x402, and use that instead of the code in pc.c. How does that sound? Autotest will need to be changed to use the new device type.
On 2012-09-11 16:19, Avi Kivity wrote: > On 09/11/2012 05:11 PM, Jan Kiszka wrote: >> On 2012-09-11 14:53, Avi Kivity wrote: >>> On 09/11/2012 02:57 PM, Jan Kiszka wrote: >>> >>>> 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.kiszka@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]) { >>>> @@ -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); >>>> >>> >>> >>> Why not drop DEBUG_BIOS completely? If you want to debug the bios, use >>> debugcon. >> >> Probably true. There is more practically dead stuff below this that just >> prints if DEBUG_BIOS is enabled. > > Actually it is autotest that is at fault here. It is installing a > debugcon with non-standard iobase atop a builtin device. > Pre-memory-API, we did not detect that. Well, the cruft in pc.c was disabled in practice, just leaving useless /dev/null-like ioports behind. I still think we should drop all of them, they have no meaning. > > We can subclass isa-debugcon as bochs-debugcon, change the default > ioport to 0x402, and use that instead of the code in pc.c. How does > that sound? > > Autotest will need to be changed to use the new device type. > That is another story: providing a debugcon that defaults to 0x402, the port I always forgot when I want to debug the BIOS. We could provide bochs-debugcon in addition to the existing interface, avoiding autotest breakage and still making the usage smoother. Jan
Am 11.09.2012 16:26, schrieb Jan Kiszka: > On 2012-09-11 16:19, Avi Kivity wrote: >> On 09/11/2012 05:11 PM, Jan Kiszka wrote: >>> On 2012-09-11 14:53, Avi Kivity wrote: >>>> On 09/11/2012 02:57 PM, Jan Kiszka wrote: >>>> >>>>> 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.kiszka@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]) { >>>>> @@ -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); >>>>> >>>> >>>> >>>> Why not drop DEBUG_BIOS completely? If you want to debug the bios, use >>>> debugcon. >>> >>> Probably true. There is more practically dead stuff below this that just >>> prints if DEBUG_BIOS is enabled. >> >> Actually it is autotest that is at fault here. It is installing a >> debugcon with non-standard iobase atop a builtin device. >> Pre-memory-API, we did not detect that. > > Well, the cruft in pc.c was disabled in practice, just leaving useless > /dev/null-like ioports behind. I still think we should drop all of them, > they have no meaning. > >> >> We can subclass isa-debugcon as bochs-debugcon, change the default >> ioport to 0x402, and use that instead of the code in pc.c. How does >> that sound? >> >> Autotest will need to be changed to use the new device type. >> > > That is another story: providing a debugcon that defaults to 0x402, the > port I always forgot when I want to debug the BIOS. We could provide > bochs-debugcon in addition to the existing interface, avoiding autotest > breakage and still making the usage smoother. Hervé and Anthony had discussed that for qemu-test and I thought their conclusion was to use debug-con with the iobase parameter? CC'ing. Andreas
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]) { @@ -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);