Re: [PATCH v4 01/13] xen/arm64: head.S: Introduce enable_{boot,secondary}_cpu_mm

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Aug 9, 2023, at 19:59, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Henry,
> 
> Title: NIT: Add () after _mm to stay consistent with the rest.

Yes sure, I will add “()” in v5.

> 
> On 01/08/2023 04:44, Henry Wang wrote:
>> From: Wei Chen <wei.chen@xxxxxxx>
>> 
>> +enable_secondary_cpu_mm:
>> +        mov   x5, lr
>> +
>> +        load_paddr x0, init_ttbr
>> +        ldr   x0, [x0]
>> +
>> +        bl    enable_mmu
>> +        mov   lr, x5
>> +
>> +        /* return to secondary_switched */
> 
> Technically, you will return to the virtual address set in 'lr'. This is 
> 'secondary_switched' today but this could change.
> 
> So it would be better to have a more generic comment like:
> 
> Return to the virtual address requested by the caller.

Sure, and...

> 
>> +        ret
>> +ENDPROC(enable_secondary_cpu_mm)
>> +
>> 
>> +1:
>> +        /*
>> +         * The 1:1 map may clash with other parts of the Xen virtual memory
>> +         * layout. As it is not used anymore, remove it completely to
>> +         * avoid having to worry about replacing existing mapping
>> +         * afterwards. Function will return to primary_switched.
> 
> Same remark here.

…same here.

> 
>> +         */
>> +        b     remove_identity_mapping
>> +
>> +        /*
>> +         * Below is supposed to be unreachable code, as "ret" in
>> +         * remove_identity_mapping will use the return address in LR in 
>> advance.
>> +         */
>> +        b     fail
> 
> Looking at this again, I am not entirely sure how this could reached if 
> remove_identity_mapping use 'ret' and you call it with 'b'. So I would 
> suggest to drop it and move 'mov lr, x5' closer to 'b 
> remove_identity_mapping'. So it is clearer that it will return.

Ok, I’ve addressed this locally and tested the change, Xen and Dom0 boot
fine with the changes that you suggested. Will send v5 soon after fixing
all your comments. Thanks!

Kind regards,
Henry

> 
>> +ENDPROC(enable_boot_cpu_mm)
>> +
>>  /*
>>   * Remove the 1:1 map from the page-tables. It is not easy to keep track
>>   * where the 1:1 map was mapped, so we will look for the top-level entry
> 
> Cheers,
> 
> -- 
> Julien Grall