Re: [Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[Alex Bennée]

address@hidden writes:

> On 16 September 2013 13:50,  <address@hidden> wrote:
>> From: Alex Bennée <address@hidden>
>>
>> Commit 9b8c69243 broke the ability to boot the kernel as the value
>> returned by unassigned_mem_read returned non-zero and left the kernel
>> looping forever waiting for it to change (see integrator_led_set in
>> the kernel code).
>
> This generally looks OK, but I have a few nits.

OK

>> Relying on a varying implementation detail is incorrect anyway so this
>> introduces a memory region to emulate the debug/led region on the
>> integrator board. It is currently a basic stub as I have no idea what the
>> behaviour of this region should be so for now it simply returns 0's as
>> the old unassigned_mem_read did.
>
> It looks like you need to update the commit message -- you
> looked at the board docs, so we do know the correct behaviour.

True, I shall update.

>> +++ b/hw/misc/arm_intdbg.c
>> @@ -0,0 +1,90 @@
>> +/*
>> + * LED, Switch and Debug control registers for ARM Integrator Boards
>> + *
>> + * This currently is a stub for this functionality written with
>> + * reference to what the Linux kernel looks at. Previously we relied
>> + * on the behaviour of unassigned_mem_read() in the core.
>
> This sounds like the code was written based on the kernel, not
> on the board docs, which is wrong, so I think it could use
> rephrasing. There's no need to mention previous behaviour either
> IMHO -- people who care can look at the commit history.

Sure. I've got a bunch of other review comments to address so I can
clean this up. Unfortunately I've been so busy with other stuff I
haven't had a chance to go through them yet.

As it happens it looks like the integrator image is now running again on
the current HEAD which has made this a little less urgent. However it
does raise an interesting question as to why!

>> + *
>> + * The real h/w is described at:
>> + *  
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
>> + *
>> + * Written by Alex Bennée
>
> Assuming you wrote this with your Linaro hat on, the comment
> should have a
>  * Copyright (c) 2013 Linaro Limited
>
> in it above your 'Written by' line.

Yeah, I was holding off that until I was officially on the clock
although as noticed I have been taking advantage of the email facilities
as a handy place to have the mailing list go through.

<snip>
>> +        /* Nothing interesting implemented yet.  */
>> +        qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of %lx 
>> to %x:%d\n", value, (int)offset, size);
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %lx to 
>> bad offset %x\n", value, (int)offset);
>
> This looks like an overlength line : checkpatch.pl should
> warn if so. (I think there are others too.)

OK thanks.

-- 
Alex Bennée