From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57666) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBPRe-0005ce-HF for qemu-devel@nongnu.org; Tue, 11 Sep 2012 08:20:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TBPRT-0000bj-00 for qemu-devel@nongnu.org; Tue, 11 Sep 2012 08:19:58 -0400 Received: from david.siemens.de ([192.35.17.14]:33785) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TBPRS-0000bT-M7 for qemu-devel@nongnu.org; Tue, 11 Sep 2012 08:19:46 -0400 Message-ID: <504F2C5A.3090702@siemens.com> Date: Tue, 11 Sep 2012 14:19:38 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <504F0131.6090501@redhat.com> <504F037D.7020207@redhat.com> <504F2009.9040900@citrix.com> <504F24F8.5080600@siemens.com> <504F273B.4050602@siemens.com> <504F2B30.80306@citrix.com> In-Reply-To: <504F2B30.80306@citrix.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Julien Grall Cc: Stefano Stabellini , "kraxel@redhat.com" , Avi Kivity , "afaerber@suse.de" , "qemu-devel@nongnu.org" 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 >>> >>> 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 >> --- >> 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