Re: [PATCH v3 03/17] xen/arm: vsmc: Implement SMCCC_ARCH_WORKAROUND_1 BP hardening support

[Andre Przywara <andre.przywara@linaro.org>]

Hi,

On 21/02/18 16:41, Julien Grall wrote:
> 
> 
> On 21/02/18 16:34, Andre Przywara wrote:
>> Hi,
> 
> Hi,
> 
>> On 15/02/18 15:02, Julien Grall wrote:
>>> SMCCC 1.1 offers firmware-based CPU workarounds. In particular,
>>> SMCCC_ARCH_WORKAROUND_1 provides BP hardening for variant 2 of XSA-254
>>> (CVE-2017-5715).
>>>
>>> If the hypervisor has some mitigation for this issue, report that we
>>> deal with it using SMCCC_ARCH_WORKAROUND_1, as we apply the hypervisor
>>> workaround on every guest exit.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>
>>>
>>> ---
>>>      Changes in v3:
>>>          - Fix minor conflict during rebase
>>>
>>>      Changes in v2:
>>>          - Add Volodymyr's reviewed-by
>>> ---
>>>   xen/arch/arm/vsmc.c         | 22 ++++++++++++++++++++--
>>>   xen/include/asm-arm/smccc.h |  6 ++++++
>>>   2 files changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>>> index 7ec492741b..40a80d5760 100644
>>> --- a/xen/arch/arm/vsmc.c
>>> +++ b/xen/arch/arm/vsmc.c
>>> @@ -18,6 +18,7 @@
>>>   #include <xen/lib.h>
>>>   #include <xen/types.h>
>>>   #include <public/arch-arm/smccc.h>
>>> +#include <asm/cpufeature.h>
>>>   #include <asm/monitor.h>
>>>   #include <asm/regs.h>
>>>   #include <asm/smccc.h>
>>> @@ -93,8 +94,25 @@ static bool handle_arch(struct cpu_user_regs *regs)
>>>           return true;
>>>         case ARM_SMCCC_ARCH_FEATURES_FID:
>>> -        /* Nothing supported yet */
>>> -        set_user_reg(regs, 0, ARM_SMCCC_NOT_SUPPORTED);
>>> +    {
>>> +        uint32_t arch_func_id = get_user_reg(regs, 1);
>>
>> Shouldn't the function identifier be in x0/r0?
> 
> x0/r0 contain the function identifier of the actual function (i.e
> ARM_SMCCC_ARCH_FEATURES_FID). What we want here is the arch_func_id to
> check if the firmware implement it. This is the first parameter of the
> function, according to the calling convention this will be stored in x1/r1.

Ah, right. I guess the missing context in this patch confused me.
Looks alright then:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel