From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=MHMTkUrn; spf=pass (domain: linaro.org, ip: 209.85.128.65, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by groups.io with SMTP; Thu, 19 Sep 2019 03:26:07 -0700 Received: by mail-wm1-f65.google.com with SMTP id a6so3770444wma.5 for ; Thu, 19 Sep 2019 03:26:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VzTAPSUAEEh7qSpeiBvPZ8l5862I/827MTIluy7w5Uk=; b=MHMTkUrnmuWaS5WbVq9cno+zQycWYAzyAUBeJ4GIaH/PTFYoTNqb0CFA8+Ttdg5CCX T15pkD6jyRVGjEtgNEit2iJljhiPp5Iur+KkPIWq5qmuK6lLG7xnMB1TmYNeQHu2xatU M2fZzxgMsTFFQwsWg8MqMdGy3YrZ/m5OdGoYoJiUaDFqJXpYMpn7DexJf40HTKnAXtxx yjQf/ZGbGEHbqv2rUgAVz1rwTIEkY/Iq7WfZB/sJcgGdS+JadjKPV4RBWWIBo6C/oWNv pnYyL1C7Yo+PZmH9QbTjcD09bHjCgxnfNVgeRYf7ljHlX5HYsqNFrwb44VZTQbahLP08 TuJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VzTAPSUAEEh7qSpeiBvPZ8l5862I/827MTIluy7w5Uk=; b=tPw7H7L1yYOCWdU54njRKH+wR3YxIxVhKXsz11WiTIZA7CSV10O4oI1r3s0iEVHLwy V3V02PMKTdcpeMLcRw9k0OgRXPvqqJVZvlb6LO6iz6KTHKg2e/BDROgmyS0XmxMw6ZZT XGwIGh+U1BNMJWFbFrbjgp77xiQahMDRa7LFTPPS1qCWyJuYdZ0YJU6O98yEaIKiB2YI aGfloGeAmvRchuskp09v/6fI39dlWpZkasWbHOVzgJO1m/V537GOLZtplI2H3t0d3LnP wk4qlwu7XfoXL1SmQSLRFgTPKJHiGi8LKnAf2QcbY+W2QxoxtxO9KPiHE7EldC+2a40+ HA6g== X-Gm-Message-State: APjAAAXufLnv8Yuy0nlD0NzPfleaPxCE2AB/Wg8bSnI3P9/5tEYLL1tT 0qRKOoyAURhaf10sxmLN8e7VGiX5A8iL4JJ0pf7u+A== X-Google-Smtp-Source: APXvYqzujTXHxt3bz+S7YJzF38G8eAhyvya/kFMXz/PnB2jcO8WisgIVjEZaqF0CxsslCpI6UwSbXSmTDJmOVh/cVJY= X-Received: by 2002:a1c:e906:: with SMTP id q6mr2008646wmc.136.1568888765397; Thu, 19 Sep 2019 03:26:05 -0700 (PDT) MIME-Version: 1.0 References: <0d024d72b50b7f5a6d3d908d309810f350c5b1f5.1568808805.git.baptiste.gerondeau@linaro.org> <20190919094846.GO28454@bivouac.eciton.net> <20190919100921.GR28454@bivouac.eciton.net> In-Reply-To: <20190919100921.GR28454@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Thu, 19 Sep 2019 13:25:37 +0300 Message-ID: Subject: Re: [PATCH 2/3] ARM/Assembler: Correct syntax from RVCT for MSFT To: Leif Lindholm Cc: Baptiste Gerondeau , edk2-devel-groups-io , "Kinney, Michael D" , "Gao, Liming" , "Zhang, Shenglei" , Baptiste GERONDEAU Content-Type: text/plain; charset="UTF-8" On Thu, 19 Sep 2019 at 13:09, Leif Lindholm wrote: > > On Thu, Sep 19, 2019 at 01:01:04PM +0300, Ard Biesheuvel wrote: > > On Thu, 19 Sep 2019 at 12:48, Leif Lindholm wrote: > > > > > > On Thu, Sep 19, 2019 at 12:32:56PM +0300, Ard Biesheuvel wrote: > > > > On Wed, 18 Sep 2019 at 15:27, Baptiste Gerondeau > > > > wrote: > > > > > > > > > > From: Baptiste GERONDEAU > > > > > > > > > > RVCT and MSFT's ARM assembler share the same file syntax, but some > > > > > instructions use pre-UAL syntax that is not picked up > > > > > by MSFT's ARM assembler, this commit translates those instructions > > > > > into MSFT-buildable ones (subset of UAL/THUMB). > > > > > > > > > > Signed-off-by: Baptiste Gerondeau > > > > > --- > > > > > ArmPkg/Library/ArmExceptionLib/Arm/ExceptionSupport.asm | 30 +++++++++++++++++------------- > > > > > ArmPkg/Library/ArmLib/Arm/ArmV7Support.asm | 6 ++++-- > > > > > MdePkg/Library/BaseIoLibIntrinsic/Arm/ArmVirtMmio.asm | 18 +++++++++--------- > > > > > 3 files changed, 30 insertions(+), 24 deletions(-) > > > > > > > > > > diff --git a/ArmPkg/Library/ArmExceptionLib/Arm/ExceptionSupport.asm b/ArmPkg/Library/ArmExceptionLib/Arm/ExceptionSupport.asm > > > > > index aa0229d2e85f..880246bd6206 100644 > > > > > --- a/ArmPkg/Library/ArmExceptionLib/Arm/ExceptionSupport.asm > > > > > +++ b/ArmPkg/Library/ArmExceptionLib/Arm/ExceptionSupport.asm > > > > > @@ -90,7 +90,7 @@ Fiq > > > > > ResetEntry > > > > > srsfd #0x13! ; Store return state on SVC stack > > > > > ; We are already in SVC mode > > > > > - stmfd SP!,{LR} ; Store the link register for the current mode > > > > > + push {LR} ; Store the link register for the current mode > > > > > sub SP,SP,#0x20 ; Save space for SP, LR, PC, IFAR - CPSR > > > > > stmfd SP!,{R0-R12} ; Store the register state > > > > > > > > > > @@ -102,7 +102,7 @@ UndefinedInstructionEntry > > > > > sub LR, LR, #4 ; Only -2 for Thumb, adjust in CommonExceptionEntry > > > > > srsfd #0x13! ; Store return state on SVC stack > > > > > cps #0x13 ; Switch to SVC for common stack > > > > > - stmfd SP!,{LR} ; Store the link register for the current mode > > > > > + push {LR} ; Store the link register for the current mode > > > > > sub SP,SP,#0x20 ; Save space for SP, LR, PC, IFAR - CPSR > > > > > stmfd SP!,{R0-R12} ; Store the register state > > > > > > > > > > @@ -113,7 +113,7 @@ UndefinedInstructionEntry > > > > > SoftwareInterruptEntry > > > > > srsfd #0x13! ; Store return state on SVC stack > > > > > ; We are already in SVC mode > > > > > - stmfd SP!,{LR} ; Store the link register for the current mode > > > > > + push {LR} ; Store the link register for the current mode > > > > > sub SP,SP,#0x20 ; Save space for SP, LR, PC, IFAR - CPSR > > > > > stmfd SP!,{R0-R12} ; Store the register state > > > > > > > > > > @@ -125,7 +125,7 @@ PrefetchAbortEntry > > > > > sub LR,LR,#4 > > > > > srsfd #0x13! ; Store return state on SVC stack > > > > > cps #0x13 ; Switch to SVC for common stack > > > > > - stmfd SP!,{LR} ; Store the link register for the current mode > > > > > + push {LR} ; Store the link register for the current mode > > > > > sub SP,SP,#0x20 ; Save space for SP, LR, PC, IFAR - CPSR > > > > > stmfd SP!,{R0-R12} ; Store the register state > > > > > > > > > > @@ -137,7 +137,7 @@ DataAbortEntry > > > > > sub LR,LR,#8 > > > > > srsfd #0x13! ; Store return state on SVC stack > > > > > cps #0x13 ; Switch to SVC for common stack > > > > > - stmfd SP!,{LR} ; Store the link register for the current mode > > > > > + push {LR} ; Store the link register for the current mode > > > > > sub SP,SP,#0x20 ; Save space for SP, LR, PC, IFAR - CPSR > > > > > stmfd SP!,{R0-R12} ; Store the register state > > > > > > > > > > @@ -148,7 +148,7 @@ DataAbortEntry > > > > > ReservedExceptionEntry > > > > > srsfd #0x13! ; Store return state on SVC stack > > > > > cps #0x13 ; Switch to SVC for common stack > > > > > - stmfd SP!,{LR} ; Store the link register for the current mode > > > > > + push {LR} ; Store the link register for the current mode > > > > > sub SP,SP,#0x20 ; Save space for SP, LR, PC, IFAR - CPSR > > > > > stmfd SP!,{R0-R12} ; Store the register state > > > > > > > > > > @@ -160,7 +160,7 @@ IrqEntry > > > > > sub LR,LR,#4 > > > > > srsfd #0x13! ; Store return state on SVC stack > > > > > cps #0x13 ; Switch to SVC for common stack > > > > > - stmfd SP!,{LR} ; Store the link register for the current mode > > > > > + push {LR} ; Store the link register for the current mode > > > > > sub SP,SP,#0x20 ; Save space for SP, LR, PC, IFAR - CPSR > > > > > stmfd SP!,{R0-R12} ; Store the register state > > > > > > > > > > @@ -172,7 +172,7 @@ FiqEntry > > > > > sub LR,LR,#4 > > > > > srsfd #0x13! ; Store return state on SVC stack > > > > > cps #0x13 ; Switch to SVC for common stack > > > > > - stmfd SP!,{LR} ; Store the link register for the current mode > > > > > + push {LR} ; Store the link register for the current mode > > > > > sub SP,SP,#0x20 ; Save space for SP, LR, PC, IFAR - CPSR > > > > > stmfd SP!,{R0-R12} ; Store the register state > > > > > ; Since we have already switch to SVC R8_fiq - R12_fiq > > > > > @@ -213,9 +213,11 @@ AsmCommonExceptionEntry > > > > > and R3, R1, #0x1f ; Check CPSR to see if User or System Mode > > > > > cmp R3, #0x1f ; if ((CPSR == 0x10) || (CPSR == 0x1f)) > > > > > cmpne R3, #0x10 ; > > > > > - stmeqed R2, {lr}^ ; save unbanked lr > > > > > + mrseq R8, lr_usr ; save unbanked lr to R8 > > > > > + streq R2, [R8] ; make R2 point to R8 > > > > > ; else > > > > > - stmneed R2, {lr} ; save SVC lr > > > > > + mrsne R8, lr_svc ; save SVC lr to R8 > > > > > + strne R2, [R8] ; make R2 point to R8 > > > > > > > > > > > > > Can you make this str unconditional, and drop the former? > > > > > > Yeah, that would be an improvement. > > > > > > > > > > > > > ldr R5, [SP, #0x58] ; PC is the LR pushed by srsfd > > > > > @@ -280,15 +282,17 @@ CommonCExceptionHandler ( > > > > > and R1, R1, #0x1f ; Check to see if User or System Mode > > > > > cmp R1, #0x1f ; if ((CPSR == 0x10) || (CPSR == 0x1f)) > > > > > cmpne R1, #0x10 ; > > > > > - ldmeqed R2, {lr}^ ; restore unbanked lr > > > > > + ldreq R8, [R2] ; load sys/usr lr from R2 pointer > > > > > + msreq lr_usr, R8 ; restore unbanked lr > > > > > ; else > > > > > - ldmneed R3, {lr} ; restore SVC lr, via ldmfd SP!, {LR} > > > > > + ldrne R8, [R3] ; load SVC lr from R3 pointer > > > > > + msrne lr_svc, R8 ; restore SVC lr, via ldmfd SP!, {LR} > > > > > > > > > > ldmfd SP!,{R0-R12} ; Restore general purpose registers > > > > > ; Exception handler can not change SP > > > > > > > > > > add SP,SP,#0x20 ; Clear out the remaining stack space > > > > > - ldmfd SP!,{LR} ; restore the link register for this context > > > > > + pop {LR} ; restore the link register for this context > > > > > rfefd SP! ; return from exception via srsfd stack slot > > > > > > > > > > END > > > > > diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.asm b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.asm > > > > > index 3146c2b52181..724306399e6c 100644 > > > > > --- a/ArmPkg/Library/ArmLib/Arm/ArmV7Support.asm > > > > > +++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Support.asm > > > > > @@ -200,8 +200,10 @@ Loop2 > > > > > mov R9, R4 ; R9 working copy of the max way size (right aligned) > > > > > > > > > > Loop3 > > > > > - orr R0, R10, R9, LSL R5 ; factor in the way number and cache number into R11 > > > > > - orr R0, R0, R7, LSL R2 ; factor in the index number > > > > > + lsl R8, R9, R5 > > > > > + orr R0, R10, R8 ; factor in the way number and cache number > > > > > + lsl R8, R7, R2 > > > > > + orr R0, R0, R8 ; factor in the index number > > > > > > > > > > > > > What's wrong with this code? > > > > > > Inline barrel shifter is only available in A32, not T32 (and VS seems > > > to love T32). > > > > So is this simply the default of the compiler? I'd prefer it if we > > could add a 'CODE 32' directive instead, that way, we may not need any > > of the other changes to begin with. > > Some of them really weren't supported regardless (and the indentation > change of directives was required). > > This one, possible - Baptiste? > > Oh, and the CODE 32 directive is not supported - it's ARM or THUMB :) > https://docs.microsoft.com/en-us/cpp/assembler/arm/arm-assembler-directives?view=vs-2019 > OK. In any case, I'd strongly prefer it if the .S and .asm files produced identical object code, so please apply the same changes to the sibling .S files as well, please (but only the ones that are really required when building it in ARM mode)