From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/setup: Correct register clobbers for the asm statement when resyncing the stack Date: Tue, 21 Oct 2014 11:03:18 +0100 Message-ID: <54462F66.6050801@citrix.com> References: <1413826207-2974-1-git-send-email-andrew.cooper3@citrix.com> <544639190200007800040901@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <544639190200007800040901@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Daniel Kiper , Keir Fraser , Xen-devel List-Id: xen-devel@lists.xenproject.org On 21/10/14 09:44, Jan Beulich wrote: >>>> On 20.10.14 at 19:30, wrote: >> When resyncing the stack, the asm statement does not identify %rsi, %rdi and >> %rcx as clobbered by the 'rep movsq'. >> >> Luckily, there are no functional problems in the generated code. GCC >> decides >> not to save any of them before calling boostrap_map(), which clobbers them. >> >> Correct the clobbers, by listing them as earlyclobber discarded outputs. >> >> Reported-by: Daniel Kiper >> Signed-off-by: Andrew Cooper >> CC: Keir Fraser >> CC: Jan Beulich >> >> --- >> >> I have done an audit of the other uses of `rep $STRINGOP` and this is the >> only >> asm statement with incorrect clobbers. >> --- >> xen/arch/x86/setup.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index 8c8b91f..f470c4a 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -836,6 +836,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> l2_pgentry_t *pl2e; >> uint64_t load_start; >> int i, j, k; >> + long _discard; > I can't see why you couldn't use i, j, or k for the discarding purposes. A parameter with the name "_discard" is far more explicit about its purpose when read as part of the output parameter list. I could switch to i and leave a comment by the parameters if you insist. > >> @@ -902,11 +903,13 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> "movq %%cr4,%%rsi ; " >> "andb $0x7f,%%sil ; " >> "movq %%rsi,%%cr4 ; " /* CR4.PGE == 0 */ >> - "movq %0,%%cr3 ; " /* CR3 == new pagetables */ >> + "movq %6,%%cr3 ; " /* CR3 == new pagetables */ >> "orb $0x80,%%sil ; " >> "movq %%rsi,%%cr4 " /* CR4.PGE == 1 */ >> - : : "r" (__pa(idle_pg_table)), "S" (cpu0_stack), >> - "D" (__va(__pa(cpu0_stack))), "c" (STACK_SIZE / 8) : "memory" ); >> + : "=&S"(_discard), "=&D"(_discard), "=&c"(_discard) >> + : "0"(cpu0_stack), "1"(__va(__pa(cpu0_stack))), >> + "2"(STACK_SIZE / 8), "r"(__pa(idle_pg_table)) >> + : "memory" ); > Among the inputs, please put the one actively used by number first, > or convert to labeled operands. As far as I can tell, it is impossible to get the idle pagetables to parameter %0 as it is not present in the output list. I have switched to a named parameter. > Also please don't corrupt the > formatting - there ought to be a blank between the closing constraint > quote and the opening parenthesis in all operands. Done ~Andrew