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.
--
Julien Grall
