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.gr...@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_babc...@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_babc...@epam.com>
--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
