Re: [PATCH 1/2] arm: smccc: handle SMCs/HVCs according to SMCCC

[Julien Grall <julien.grall@arm.com>]

On 06/19/2017 10:59 PM, Volodymyr Babchuk wrote:
> Hello Julien,

Hi Volodymyr,

[...]

>>> +/**
>>> + * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
>>> + */
>>> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)
>>
>> hsr is already part of regs.
>>
>>> +{
>>> +    bool handled = false;
>>> +
>>
>> I am a bit surprised, I don't see any check to prevent a 32-bit guest 
>> to use SMC64 call.
> Should we return ARM_SMCCC_ERR_UNKNOWN_FUNCTION code in this case? Or 
> inject undefined instruction?

We should follow what the spec says here. Per section 2.7 (ARM DEN 
0028B), you should return ARM_SMCC_ERR_UNKOWN_FUNCTION for SMC64/HVC64 
call from AArch32.

>> Furthermore, based from the SMCCC spec (see 2.9 in ARM DEN 0028B), the 
>> compliant SMC calls should have the immediate value of zero.
> Looks like HSR does not hold immediate value (as if in case of HVC/SVC). 
> That means that I need to map memory at PC and decode instruction 
> manually. It is a bit complex, I think. Should we do this?

Well, the immediate is present for the ISS encoding for exception from 
SMC executed in AArch64 state. So you can decode the immediate here.

For SMC executed in AArch32 state, indeed the immediate is not present.

I had a quick look at the arm trusted firmware. I haven't spot any 
decoding of the immediate from an instruction.

My main concern is any non-zero value are reserved. If they are used in 
the future, we may emulate them by mistake. So we should definitely 
handle it for AArch64. For AArch32, we could decode the instruction, but 
that would be time consuming for the benefits of future extension but 
add overhead on the SMC call. So I would just consider it as always 0 
with a suitable comment on top explaining why.

[...]

>>> +
>>> +#include <xen/types.h>
>>> +
>>> +/*
>>> + * This file provides common defines for ARM SMC Calling Convention as
>>> + * specified in
>>> + * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>>> + */
>>> +
>>> +#define ARM_SMCCC_STD_CALL        0
>>
>> Is this file coming from Linux? If so, it should be mention. If not, 
>> please use soft tab and not hard tab. This is valid in all this 
>> file.Actually, part of this defines are from Linux, another defines 
>> was added 
> by myself. What I should to in this case?

Using Xen coding style always (see CODING_STYLE).

Cheers,

-- 
Julien Grall

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