[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default |
Date: |
Tue, 11 Sep 2012 14:19:38 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
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 <address@hidden>
>>>
>>> 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 <address@hidden>
>> ---
>> 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
- [Qemu-devel] [PATCH V9 6/8] hw/serial.c: replace register_ioport*, (continued)
- [Qemu-devel] [PATCH V9 6/8] hw/serial.c: replace register_ioport*, Julien Grall, 2012/09/04
- [Qemu-devel] [PATCH V9 8/8] hw/dma.c: replace register_ioport*, Julien Grall, 2012/09/04
- Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration, Julien Grall, 2012/09/04
- [Qemu-devel] [PATCH V9 4/8] hw/acpi_piix4.c: replace register_ioport*, Julien Grall, 2012/09/10
- Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration, Avi Kivity, 2012/09/11
- Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration, Avi Kivity, 2012/09/11
- Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default, Avi Kivity, 2012/09/11
- Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default, Jan Kiszka, 2012/09/11
- Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default, Avi Kivity, 2012/09/11
- Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default, Jan Kiszka, 2012/09/11
- Re: [Qemu-devel] [PATCH] pc: Don't listen on debug ports by default, Andreas Färber, 2012/09/11
Re: [Qemu-devel] [PATCH V9 0/8] memory: unify ioport registration, Avi Kivity, 2012/09/19