Re: [Xen-devel] [PATCH v2] xen/arm:alternative: Register re-mapped Xen area as a temporary virtual region

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

On 2017/3/24 3:19, Julien Grall wrote:
> Hi Wei,
>
> NIT: title: please add a space between : and alternative.
>

Ok.

> On 23/03/17 09:43, Wei Chen wrote:
>> While I was using the alternative patching in the SErrors patch series [1].
>> I used a branch instruction as alternative instruction.
>>
>>         ALTERNATIVE("nop",
>>                     "b skip_check",
>>                     SKIP_CHECK_PENDING_VSERROR)
>>
>> unfortunately, I got a system panic message with this code:
>
> NIT: s/unfortunately/Unfortunately/
>

Got it.

>>
>> (XEN) build-id: f64081d86e7e88504b7d00e1486f25751c004e39
>> (XEN) alternatives: Patching with alt table 100b9480 -> 100b9498
>> (XEN) Xen BUG at alternative.c:61
>> (XEN) ----[ Xen-4.9-unstable  arm32  debug=y   Tainted:  C   ]----
>> (XEN) CPU:    0
>> (XEN) PC:     00252b68 alternative.c#__apply_alternatives+0x128/0x1d4
>> (XEN) CPSR:   800000da MODE:Hypervisor
>> (XEN)      R0: 00000000 R1: 00000000 R2: 100b9490 R3: 100b949c
>> (XEN)      R4: eafeff84 R5: 00000000 R6: 100b949c R7: 10079290
>> (XEN)      R8: 100792ac R9: 00000001 R10:100b948c R11:002cfe04 R12:002932c0
>> (XEN) HYP: SP: 002cfdc4 LR: 00239128
>> (XEN)
>> (XEN)   VTCR_EL2: 80003558
>> (XEN)  VTTBR_EL2: 0000000000000000
>> (XEN)
>> (XEN)  SCTLR_EL2: 30cd187f
>> (XEN)    HCR_EL2: 000000000038663f
>> (XEN)  TTBR0_EL2: 00000000bff09000
>> (XEN)
>> (XEN)    ESR_EL2: 00000000
>> (XEN)  HPFAR_EL2: 0000000000000000
>> (XEN)      HDFAR: 00000000
>> (XEN)      HIFAR: 00000000
>> (XEN)
>> (XEN) Xen stack trace from sp=002cfdc4:
>> (XEN)    00000000 00294328 002e0004 00000001 10079290 002cfe14 100b9490 
>> 00000000
>> (XEN)    10010000 10122700 00200000 002cfe1c 00000080 00252c14 00000000 
>> 002cfe64
>> (XEN)    00252dd8 00000007 00000000 000bfe00 100b9480 100b9498 002cfe1c 
>> 002cfe1c
>> (XEN)    10010000 10122700 00000000 00000000 00000000 00000000 00000000 
>> 00000000
>> (XEN)    00000000 00000000 00000000 002ddf30 00000000 003113e8 0030f018 
>> 002cfe9c
>> (XEN)    00238914 00000002 00000000 00000000 00000000 0028b000 00000002 
>> 00293800
>> (XEN)    00000002 0030f238 00000002 00290640 00000001 002cfea4 002a2840 
>> 002cff54
>> (XEN)    002a65fc 11112131 10011142 00000000 0028d194 00000000 00000000 
>> 00000000
>> (XEN)    bdffb000 80000000 00000000 c0000000 00000000 00000002 00000000 
>> c0000000
>> (XEN)    002b8060 00002000 002b8040 00000000 c0000000 bc000000 00000000 
>> c0000000
>> (XEN)    00000000 be000000 00000000 00112701 00000000 bff12701 00000000 
>> 00000000
>> (XEN)    00000000 00000000 00000000 00000000 00000018 00000000 00000001 
>> 00000000
>> (XEN)    9fece000 80200000 80000000 00400000 00200550 00000000 00000000 
>> 00000000
>> (XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 
>> 00000000
>> (XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 
>> 00000000
>> (XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 
>> 00000000
>> (XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000 
>> 00000000
>> (XEN)    00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> (XEN) Xen call trace:
>> (XEN)    [<00252b68>] alternative.c#__apply_alternatives+0x128/0x1d4 (PC)
>> (XEN)    [<00239128>] is_active_kernel_text+0x10/0x28 (LR)
>> (XEN)    [<00252dd8>] 
>> alternative.c#__apply_alternatives_multi_stop+0x1c4/0x204
>> (XEN)    [<00238914>] stop_machine_run+0x1e8/0x254
>> (XEN)    [<002a2840>] apply_alternatives_all+0x38/0x54
>> (XEN)    [<002a65fc>] start_xen+0xcf4/0xf88
>> (XEN)    [<00200550>] arm32/head.o#paging+0x94/0xd8
>> (XEN)
>> (XEN)
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) Xen BUG at alternative.c:61
>> (XEN) ****************************************
>>
>> This panic was triggered by the BUG(); in branch_insn_requires_update.
>> That's because in this case the alternative patching needs to update the
>> offset of the branch instruction. But the new target address of the branch
>> instruction could not pass the check of is_active_kernel_text();
>>
>> The reason is that, we haven't registered this the re-mapped Xen area as
>> an active kernel text virtual region. In order to update branch instruction
>> offset, we have to update the Xen text section. But Xen text section is
>> marked as read-only and we configure the hardware to not allow a region
>> to be writable and executable at the same time. So re-map Xen in a temporary
>> area for writing.
>>
>> The calculation of the new target address of the branch instruction is based
>> on this re-mapped area. The new target address will point to a value in the
>> re-mapped area. We haven't registered this area as an active kernel text. So
>> the check of is_active_kernel_text will always return false.
>>
>> We have to register the re-mapped Xen area as a virtual region temporarily to
>> solve this problem,
>
> s/,/./

Ok.

>
> The commit message is much better. However, you don't explain that this
> is only happening when booting Xen (apply_alternatives_all) and not
> livepatching or else.
>

Yes, I hadn't missed that. I will explain it in next version.

> So someone may wonder why you only register the Xen area in
> apply_alternatives_all.
>
>>
>> Notes:
>> This bug will affect the staging, staging-4.8 and stable-4.8 source trees.
>
> NIT: this is not necessary to keep in the commit message after the
> merge. So this should go after ---.
>

Ok, I will place the notes after ---.

> The code looks good to me.
>
> Cheers,
>


-- 
Regards,
Wei Chen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel