On 07.12.2021 09:37, Michal Orzel wrote: > On 06.12.2021 16:29, Julien Grall wrote: >> On 06/12/2021 14:20, Michal Orzel wrote: >>> to hypervisor when switching to AArch32 state. >>> > I will change to "from AArch32 state". >>> According to section D1.20.2 of Arm Arm(DDI 0487A.j): >>> "If the general-purpose register was accessible from AArch32 state the >>> upper 32 bits either become zero, or hold the value that the same >>> architectural register held before any AArch32 execution. >>> The choice between these two options is IMPLEMENTATIONDEFINED" >> >> Typo: Missing space between IMPLEMENTATION and DEFINED. >> > Ok. >>> >>> Currently Xen does not ensure that the top 32 bits are zeroed and this >>> needs to be fixed. >> >> Can you outline why this is a problem and why we need to protect? IIRC, the >> main concern is Xen may misinterpret what the guest requested but we are not >> concerned about Xen using wrong value. >> > I would say: > " > The reason why this is a problem is that there are places in Xen where we > assume that top 32bits are zero for AArch32 guests. > If they are not, this can lead to misinterpretation of Xen regarding what the > guest requested. > For example hypercalls returning an error encoded in a signed long like > do_sched_op, do_hmv_op, do_memory_op would return -ENOSYS > if the command passed as the first argument was clobbered, > " >>> >>> Fix this bug by zeroing the upper 32 bits of these registers on an >>> entry to hypervisor when switching to AArch32 state. >>> >>> Set default value of parameter compat of macro entry to 0 (AArch64 mode >>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank >>> when not passed. >> >> Which error do you see otherwise? Is it a compilation error? >> > Yes, this is a compilation error. The errors appear at each line when "entry" > is called without passing value for "compat". > So basically in all the places where entry is called with hyp=1. > When taking the current patch and removing default value for compat you will > get: > ``` > entry.S:254: Error: ".endif" without ".if" > entry.S:258: Error: symbol `.if' is already defined > entry.S:258: Error: ".endif" without ".if" > entry.S:262: Error: symbol `.if' is already defined > entry.S:262: Error: ".endif" without ".if" > entry.S:266: Error: symbol `.if' is already defined > entry.S:266: Error: ".endif" without ".if" > entry.S:278: Error: symbol `.if' is already defined > entry.S:278: Error: ".endif" without ".if" > entry.S:292: Error: symbol `.if' is already defined > entry.S:292: Error: ".endif" without ".if" > entry.S:317: Error: symbol `.if' is already defined > entry.S:317: Error: ".endif" without ".if" > ```
An alternative might be to use .if 0\compat >>> --- a/xen/arch/arm/arm64/entry.S >>> +++ b/xen/arch/arm/arm64/entry.S >>> @@ -109,8 +109,16 @@ >>> * If 0, we rely on the on x0/x1 to have been saved at the correct >>> * position on the stack before. >>> */ >>> - .macro entry, hyp, compat, save_x0_x1=1 >>> + .macro entry, hyp, compat=0, save_x0_x1=1 >>> sub sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR >>> */ >>> + >>> + /* Zero the upper 32 bits of the registers when switching to >>> AArch32 */ >>> + .if \compat == 1 /* AArch32 mode */ >>> + .irp >>> nr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29 >>> + mov w\nr, w\nr >>> + .endr >>> + .endif >> >> So Jan mentioned, the x0/x1 may have already been saved. So you may need to >> fetch them from the stack and then clobber the top 32-bit. >> > So I would do the following: > -fetch x0/x1 from the stack > -clobber them > -store them again on the stack > > /* > * Zero the upper 32 bits of the gp registers when switching > * from AArch32. > */ > .if \compat == 1 /* AArch32 mode */ > > /* x0/x1 have already been saved so fetch them to zero top 32 bits */ > .if \save_x0_x1 == 0 > ldp x0, x1, [sp], #(UREGS_kernel_sizeof - UREGS_X0) > .endif > > .irp > nr,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29 > mov w\nr, w\nr > .endr > > .if \save_x0_x1 == 0 > stp x0, x1, [sp, #-(UREGS_kernel_sizeof - UREGS_X0)] > .endif > > .endif Wouldn't it be more efficient to store 32 bits of zero each into the high halves of the respective stack slots? Afaict same code size, but less memory / cache traffic. Plus it would avoid the latent issue of a user of the macro actually expecting the two registers to retain their values across the macro invocation. I'm also puzzled by the two different memory addressing forms, but I can easily see that I may be lacking enough Arm knowledge there. Jan
