[Xen-devel,RFC,for-4.13,09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

Message ID 20190926183808.11630-10-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: XSA-201 and XSA-263 fixes
Related show

Commit Message

Julien Grall Sept. 26, 2019, 6:38 p.m.
Using alternative_if makes the code a bit more streamlined.

Take the opportunity to use the new auto-nop infrastructure to avoid
counting the number of nop in the else part for arch/arm/arm64/entry.S

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This is pretty much a matter of taste, but at least for arm64 this
    allows us to use the auto-nop infrastructure. So the arm32 is more
    to keep inline with arm64.
---
 xen/arch/arm/arm32/entry.S | 9 ++++++---
 xen/arch/arm/arm64/entry.S | 8 +++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

Comments

Volodymyr Babchuk Sept. 27, 2019, 12:11 p.m. | #1
Julien Grall writes:

> Using alternative_if makes the code a bit more streamlined.
>
> Take the opportunity to use the new auto-nop infrastructure to avoid
> counting the number of nop in the else part for arch/arm/arm64/entry.S
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>     This is pretty much a matter of taste, but at least for arm64 this
>     allows us to use the auto-nop infrastructure. So the arm32 is more
>     to keep inline with arm64.
> ---
>  xen/arch/arm/arm32/entry.S | 9 ++++++---
>  xen/arch/arm/arm64/entry.S | 8 +++++---
>  2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 0b4cd19abd..1428cd3583 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -65,9 +65,12 @@ save_guest_regs:
>           * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>           * feature, the checking of pending SErrors will be skipped.
>           */
> -        ALTERNATIVE("nop",
> -                    "b skip_check",
> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        nop
> +        alternative_else
> +        b   skip_check
> +        alternative_endif
> +
for the arm32 code you can have my r-b:
Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

>          /*
>           * Start to check pending virtual abort in the gap of Guest -> HYP
>           * world switch.
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 458d12f188..91cf6ee6f4 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -170,9 +170,11 @@
>           * is not set. If a vSError took place, the initial exception will be
>           * skipped. Exit ASAP
>           */
> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
> -                    "nop; nop",
> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        bl      check_pending_vserror
> +        cbnz    x0, 1f
> +        alternative_else_nop_endif
> +
You asked other people to do not introduce new code in one patch and
rewrite it in the following patch. But there you are doing exactly the
same. I believe, it is possible to move all "alternative" patches to the
very beginning of the patch series and only then introduce macro
guest_vector.

>          mov     x0, sp
>          bl      enter_hypervisor_from_guest_noirq
>          msr     daifclr, \iflags


--
Volodymyr Babchuk at EPAM
Julien Grall Sept. 27, 2019, 12:34 p.m. | #2
Hi,

On 27/09/2019 13:11, Volodymyr Babchuk wrote:
> 
> 
> Julien Grall writes:
> 
>> Using alternative_if makes the code a bit more streamlined.
>>
>> Take the opportunity to use the new auto-nop infrastructure to avoid
>> counting the number of nop in the else part for arch/arm/arm64/entry.S
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      This is pretty much a matter of taste, but at least for arm64 this
>>      allows us to use the auto-nop infrastructure. So the arm32 is more
>>      to keep inline with arm64.
>> ---
>>   xen/arch/arm/arm32/entry.S | 9 ++++++---
>>   xen/arch/arm/arm64/entry.S | 8 +++++---
>>   2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>> index 0b4cd19abd..1428cd3583 100644
>> --- a/xen/arch/arm/arm32/entry.S
>> +++ b/xen/arch/arm/arm32/entry.S
>> @@ -65,9 +65,12 @@ save_guest_regs:
>>            * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>>            * feature, the checking of pending SErrors will be skipped.
>>            */
>> -        ALTERNATIVE("nop",
>> -                    "b skip_check",
>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>> +        nop
>> +        alternative_else
>> +        b   skip_check
>> +        alternative_endif
>> +
> for the arm32 code you can have my r-b:
> Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
>>           /*
>>            * Start to check pending virtual abort in the gap of Guest -> HYP
>>            * world switch.
>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>> index 458d12f188..91cf6ee6f4 100644
>> --- a/xen/arch/arm/arm64/entry.S
>> +++ b/xen/arch/arm/arm64/entry.S
>> @@ -170,9 +170,11 @@
>>            * is not set. If a vSError took place, the initial exception will be
>>            * skipped. Exit ASAP
>>            */
>> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>> -                    "nop; nop",
>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>> +        bl      check_pending_vserror
>> +        cbnz    x0, 1f
>> +        alternative_else_nop_endif
>> +
> You asked other people to do not introduce new code in one patch and
> rewrite it in the following patch. But there you are doing exactly the
> same.

This is a fairly borderline comment knowing that I usually don't request 
clean-up and code consolidation in the same patch.

> I believe, it is possible to move all "alternative" patches to the
> very beginning of the patch series and only then introduce macro
> guest_vector.

For a first, the first patch is definitely not new code. This is code 
consolidation and therefore I don't tend to mix the two for clarity. So this 
should have been a patch before the first patch.

Secondly, the first 4 patches are candidate for backport. The rest of the series 
would be good to backport but I am not aware of a critical issue in previous Xen 
release to strongly push for it.

Cheers,
Volodymyr Babchuk Sept. 27, 2019, 12:46 p.m. | #3
Julien Grall writes:

> Hi,
>
> On 27/09/2019 13:11, Volodymyr Babchuk wrote:
>>
>>
>> Julien Grall writes:
>>
>>> Using alternative_if makes the code a bit more streamlined.
>>>
>>> Take the opportunity to use the new auto-nop infrastructure to avoid
>>> counting the number of nop in the else part for arch/arm/arm64/entry.S
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>      This is pretty much a matter of taste, but at least for arm64 this
>>>      allows us to use the auto-nop infrastructure. So the arm32 is more
>>>      to keep inline with arm64.
>>> ---
>>>   xen/arch/arm/arm32/entry.S | 9 ++++++---
>>>   xen/arch/arm/arm64/entry.S | 8 +++++---
>>>   2 files changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>>> index 0b4cd19abd..1428cd3583 100644
>>> --- a/xen/arch/arm/arm32/entry.S
>>> +++ b/xen/arch/arm/arm32/entry.S
>>> @@ -65,9 +65,12 @@ save_guest_regs:
>>>            * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>>>            * feature, the checking of pending SErrors will be skipped.
>>>            */
>>> -        ALTERNATIVE("nop",
>>> -                    "b skip_check",
>>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>>> +        nop
>>> +        alternative_else
>>> +        b   skip_check
>>> +        alternative_endif
>>> +
>> for the arm32 code you can have my r-b:
>> Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>
>>>           /*
>>>            * Start to check pending virtual abort in the gap of Guest -> HYP
>>>            * world switch.
>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>> index 458d12f188..91cf6ee6f4 100644
>>> --- a/xen/arch/arm/arm64/entry.S
>>> +++ b/xen/arch/arm/arm64/entry.S
>>> @@ -170,9 +170,11 @@
>>>            * is not set. If a vSError took place, the initial exception will be
>>>            * skipped. Exit ASAP
>>>            */
>>> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>>> -                    "nop; nop",
>>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>>> +        bl      check_pending_vserror
>>> +        cbnz    x0, 1f
>>> +        alternative_else_nop_endif
>>> +
>> You asked other people to do not introduce new code in one patch and
>> rewrite it in the following patch. But there you are doing exactly the
>> same.
>
> This is a fairly borderline comment knowing that I usually don't
> request clean-up and code consolidation in the same patch.
I understand this. Also I understand why are you asking for clean-up.
No one likes to review the same code twice.

Anyways, I not wanted to be offensive. Sorry for that.


>> I believe, it is possible to move all "alternative" patches to the
>> very beginning of the patch series and only then introduce macro
>> guest_vector.
>
> For a first, the first patch is definitely not new code. This is code
> consolidation and therefore I don't tend to mix the two for
> clarity. So this should have been a patch before the first patch.
>
> Secondly, the first 4 patches are candidate for backport. The rest of
> the series would be good to backport but I am not aware of a critical
> issue in previous Xen release to strongly push for it.
I see. Yes, I'm always forgetting about backporting :(
So, for the rest of the patch:

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Stefano Stabellini Oct. 1, 2019, 10:19 p.m. | #4
On Thu, 26 Sep 2019, Julien Grall wrote:
> Using alternative_if makes the code a bit more streamlined.
> 
> Take the opportunity to use the new auto-nop infrastructure to avoid
> counting the number of nop in the else part for arch/arm/arm64/entry.S
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     This is pretty much a matter of taste, but at least for arm64 this
>     allows us to use the auto-nop infrastructure. So the arm32 is more
>     to keep inline with arm64.
> ---
>  xen/arch/arm/arm32/entry.S | 9 ++++++---
>  xen/arch/arm/arm64/entry.S | 8 +++++---
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 0b4cd19abd..1428cd3583 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -65,9 +65,12 @@ save_guest_regs:
>           * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
>           * feature, the checking of pending SErrors will be skipped.
>           */
> -        ALTERNATIVE("nop",
> -                    "b skip_check",
> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        nop
> +        alternative_else
> +        b   skip_check
> +        alternative_endif

This could be written as:

alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
b   skip_check
alternative_else_nop_endif


>          /*
>           * Start to check pending virtual abort in the gap of Guest -> HYP
>           * world switch.
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 458d12f188..91cf6ee6f4 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -170,9 +170,11 @@
>           * is not set. If a vSError took place, the initial exception will be
>           * skipped. Exit ASAP
>           */
> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
> -                    "nop; nop",
> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> +        bl      check_pending_vserror
> +        cbnz    x0, 1f
> +        alternative_else_nop_endif
> +
>          mov     x0, sp
>          bl      enter_hypervisor_from_guest_noirq
>          msr     daifclr, \iflags
> -- 
> 2.11.0
>
Julien Grall Oct. 1, 2019, 10:44 p.m. | #5
Hi Stefano,

On 01/10/2019 23:19, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Julien Grall wrote:

>> Using alternative_if makes the code a bit more streamlined.

>>

>> Take the opportunity to use the new auto-nop infrastructure to avoid

>> counting the number of nop in the else part for arch/arm/arm64/entry.S

>>

>> Signed-off-by: Julien Grall <julien.grall@arm.com>

>>

>> ---

>>      This is pretty much a matter of taste, but at least for arm64 this

>>      allows us to use the auto-nop infrastructure. So the arm32 is more

>>      to keep inline with arm64.

>> ---

>>   xen/arch/arm/arm32/entry.S | 9 ++++++---

>>   xen/arch/arm/arm64/entry.S | 8 +++++---

>>   2 files changed, 11 insertions(+), 6 deletions(-)

>>

>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S

>> index 0b4cd19abd..1428cd3583 100644

>> --- a/xen/arch/arm/arm32/entry.S

>> +++ b/xen/arch/arm/arm32/entry.S

>> @@ -65,9 +65,12 @@ save_guest_regs:

>>            * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu

>>            * feature, the checking of pending SErrors will be skipped.

>>            */

>> -        ALTERNATIVE("nop",

>> -                    "b skip_check",

>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)

>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT

>> +        nop

>> +        alternative_else

>> +        b   skip_check

>> +        alternative_endif

> 

> This could be written as:

> 

> alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT

> b   skip_check

> alternative_else_nop_endif


Actually my implementation is wrong :/. We want to skip the check if the 
cap is set. So this should be:

alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
b    skip_check
alternative_else_nop_endif

> 

> 

>>           /*

>>            * Start to check pending virtual abort in the gap of Guest -> HYP

>>            * world switch.

>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S

>> index 458d12f188..91cf6ee6f4 100644

>> --- a/xen/arch/arm/arm64/entry.S

>> +++ b/xen/arch/arm/arm64/entry.S

>> @@ -170,9 +170,11 @@

>>            * is not set. If a vSError took place, the initial exception will be

>>            * skipped. Exit ASAP

>>            */

>> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",

>> -                    "nop; nop",

>> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)

>> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT


This would need to be alternative_if_not as want to call the function 
when the cap is not set.

>> +        bl      check_pending_vserror

>> +        cbnz    x0, 1f

>> +        alternative_else_nop_endif

>> +

>>           mov     x0, sp

>>           bl      enter_hypervisor_from_guest_noirq

>>           msr     daifclr, \iflags

>> -- 

>> 2.11.0

>>


Cheeers,

-- 
Julien Grall
Stefano Stabellini Oct. 1, 2019, 10:52 p.m. | #6
On Tue, 1 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/10/2019 23:19, Stefano Stabellini wrote:
> > On Thu, 26 Sep 2019, Julien Grall wrote:
> >> Using alternative_if makes the code a bit more streamlined.
> >>
> >> Take the opportunity to use the new auto-nop infrastructure to avoid
> >> counting the number of nop in the else part for arch/arm/arm64/entry.S
> >>
> >> Signed-off-by: Julien Grall <julien.grall@arm.com>
> >>
> >> ---
> >>      This is pretty much a matter of taste, but at least for arm64 this
> >>      allows us to use the auto-nop infrastructure. So the arm32 is more
> >>      to keep inline with arm64.
> >> ---
> >>   xen/arch/arm/arm32/entry.S | 9 ++++++---
> >>   xen/arch/arm/arm64/entry.S | 8 +++++---
> >>   2 files changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> >> index 0b4cd19abd..1428cd3583 100644
> >> --- a/xen/arch/arm/arm32/entry.S
> >> +++ b/xen/arch/arm/arm32/entry.S
> >> @@ -65,9 +65,12 @@ save_guest_regs:
> >>            * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
> >>            * feature, the checking of pending SErrors will be skipped.
> >>            */
> >> -        ALTERNATIVE("nop",
> >> -                    "b skip_check",
> >> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> >> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> >> +        nop
> >> +        alternative_else
> >> +        b   skip_check
> >> +        alternative_endif
> > 
> > This could be written as:
> > 
> > alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> > b   skip_check
> > alternative_else_nop_endif
> 
> Actually my implementation is wrong :/. We want to skip the check if the 
> cap is set. So this should be:
> 
> alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> b    skip_check
> alternative_else_nop_endif

Ah, yes of course, like the name suggests


> > 
> >>           /*
> >>            * Start to check pending virtual abort in the gap of Guest -> HYP
> >>            * world switch.
> >> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> >> index 458d12f188..91cf6ee6f4 100644
> >> --- a/xen/arch/arm/arm64/entry.S
> >> +++ b/xen/arch/arm/arm64/entry.S
> >> @@ -170,9 +170,11 @@
> >>            * is not set. If a vSError took place, the initial exception will be
> >>            * skipped. Exit ASAP
> >>            */
> >> -        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
> >> -                    "nop; nop",
> >> -                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> >> +        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
> 
> This would need to be alternative_if_not as want to call the function 
> when the cap is not set.

Yep


> >> +        bl      check_pending_vserror
> >> +        cbnz    x0, 1f
> >> +        alternative_else_nop_endif
> >> +
> >>           mov     x0, sp
> >>           bl      enter_hypervisor_from_guest_noirq
> >>           msr     daifclr, \iflags
> >> -- 
> >> 2.11.0
> >>
> 
> Cheeers,
> 
> -- 
> Julien Grall
>

Patch

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 0b4cd19abd..1428cd3583 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -65,9 +65,12 @@  save_guest_regs:
          * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu
          * feature, the checking of pending SErrors will be skipped.
          */
-        ALTERNATIVE("nop",
-                    "b skip_check",
-                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
+        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
+        nop
+        alternative_else
+        b   skip_check
+        alternative_endif
+
         /*
          * Start to check pending virtual abort in the gap of Guest -> HYP
          * world switch.
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 458d12f188..91cf6ee6f4 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -170,9 +170,11 @@ 
          * is not set. If a vSError took place, the initial exception will be
          * skipped. Exit ASAP
          */
-        ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
-                    "nop; nop",
-                    SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
+        alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
+        bl      check_pending_vserror
+        cbnz    x0, 1f
+        alternative_else_nop_endif
+
         mov     x0, sp
         bl      enter_hypervisor_from_guest_noirq
         msr     daifclr, \iflags