Re: [PATCH v2] xen/arm: Correctly support WARN_ON

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 08/09/14 06:01, Ian Campbell wrote:
>> +    /* PC is always 4-byte align, as Xen is using ARM instruction set */
>
> "aligned"

Ok.

> Is it worth a check here? I presume the nested fault if PC were
> misaligned would be pretty exciting, print+goto die would seem
> appropriate.

I don't think so. The "undefined instruction" is only used when the 
processor is unable to decode the instruction. If it fails to load the 
instruction (because a wrong memory address, pc misaligned), Xen would 
have received a "prefetch abort".

>> +    instr = *((uint32_t *)regs->pc);
>> +    if ( instr != BUG_OPCODE )
>> +        goto die;
>> +
>> +    if ( do_bug_frame(regs, regs->pc) )
>> +        goto die;
>> +
>> +    regs->pc += 4;
>> +    return;
>> +
>> +die:
>> +    do_unexpected_trap("undefined instruction", regs);
>
> No need to change the case of this message.

Ok.

>>   }> +#ifdef CONFIG_ARM_64
>> +static void do_trap_brk(struct cpu_user_regs *regs, union hsr hsr)
>> +{
>> +    /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
>> +     * software breakpoint exception for EL1 and EL0 here
>> +     */
>
> BUG_ON? ;-O

Sounds a good plan.

>> +die:
>> +        do_unexpected_trap("undefined breakpoint value", regs);
>
> Please can you capitilise this to match the other callers.

Will do.

>
>> +/* Many version of GCC doesn't support the asm %c parameter which would
>
> "versions".
>
>> + * be preferable to this unpleasantness. We use mergeable string
>> + * sections to avoid multiple copies of the string appearing in the
>> + * Xen image.
>
> OOI what is the size increase of the final (stripped) binary with this patch?
	
I compared the file xen/xen and on both arm32/arm64 the binary is 
smaller of about 2%.

I think this is because previously gcc wasn't merge the string when 
BUG_ON was used in inline function.

Regards,

-- 
Julien Grall