Marc,
On 2017/8/8 0:56, Marc Zyngier wrote:
> On 07/08/17 17:23, gengdongjiu wrote:
>> Hi Marc,
>> As James's suggestion, I move injection SEA Error logic to the user space(Qemu), Qemu sets the related guest OS esr/elr/pstate/spsr
>> through IOCTL KVM_SET_ONE_REG. For the SEA, when Qemu sets the esr_el1.IL bit, it needs to refer to esr_el2.IL, else Qemu does not know the trapped
>> instruction was a 16-bit or a 32-bit instruction, also it needs to set far_el1 using far_el2, because this is synchronization abort.
>
> Usespace may need some fault information, but certainly not the full set
> of FAR_EL2/ESR_EL2. What it needs is a very small set of well defined
> information, properly abstracted, and not data that is completely
> private to the hypervisor.
Marc, just now I update the patch, may be use the vcpu->arch.fault.esr_el2 and vcpu->arch.fault.far_el2 to set the vcpu_sys_reg(vcpu,FAR_EL2)/vcpu_sys_reg(vcpu,ESR_EL2) can be better.
Now the user space can not directly get the vcpu->arch.faul.xxxx value, so need use vcpu_sys_reg to pass.
>
> Thanks,
>
> M.
>>
>>
>>
>>
>> On 2017/8/7 23:57, Marc Zyngier wrote:
>>> +James, since he deals with all things RAS. Please keep him on CC at all
>>> times.
>>>
>>> On 07/08/17 17:08, Dongjiu Geng wrote:
>>>> For the firmware-first RAS solution, SEA and SEI is injected
>>>> by the user space, user space needs to know the esr_el2 and
>>>> far_el2's value, so add them to sysreg. user space uses
>>>> the IOCTL KVM_GET_ONE_REG can get their value.
>>>
>>> No.
>>>
>>> This has zero purpose being exposed to userspace. Userspace sees a VM
>>> that runs at EL1, and nothing else, so exposing EL2 registers doesn't
>>> make *any* sense.
>>>
>>> If you want something to be exposed to userspace, it has to be properly
>>> abstracted and describe something that is relevant to the VM. An EL2
>>> register satisfies none of these conditions.
>>>
>>>>
>>>> Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
>>>> Signed-off-by: Quanming Wu <wuquanming@xxxxxxxxxx>
>>>> ---
>>>> arch/arm64/include/asm/kvm_host.h | 6 ++++--
>>>> arch/arm64/kvm/sys_regs.c | 6 ++++++
>>>> 2 files changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>>> index b6242fb..6063eec 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -103,10 +103,12 @@ enum vcpu_sysreg {
>>>> TTBR0_EL1, /* Translation Table Base Register 0 */
>>>> TTBR1_EL1, /* Translation Table Base Register 1 */
>>>> TCR_EL1, /* Translation Control Register */
>>>> - ESR_EL1, /* Exception Syndrome Register */
>>>> + ESR_EL1, /* Exception Syndrome Register for EL1 */
>>>> + ESR_EL2, /* Exception Syndrome Register for EL2 */
>>>> AFSR0_EL1, /* Auxiliary Fault Status Register 0 */
>>>> AFSR1_EL1, /* Auxiliary Fault Status Register 1 */
>>>> - FAR_EL1, /* Fault Address Register */
>>>> + FAR_EL1, /* Fault Address Register for EL1 */
>>>> + FAR_EL2, /* Fault Address Register for EL2 */
>>>> MAIR_EL1, /* Memory Attribute Indirection Register */
>>>> VBAR_EL1, /* Vector Base Address Register */
>>>> CONTEXTIDR_EL1, /* Context ID Register */
>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>> index 0e26f8c..0c286bf 100644
>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>>> @@ -987,9 +987,15 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>> /* ESR_EL1 */
>>>> { Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0010), Op2(0b000),
>>>> access_vm_reg, reset_unknown, ESR_EL1 },
>>>> + /* ESR_EL2 */
>>>> + { Op0(0b11), Op1(0b100), CRn(0b0101), CRm(0b0010), Op2(0b000),
>>>> + access_vm_reg, reset_unknown, ESR_EL2 },
>>>> /* FAR_EL1 */
>>>> { Op0(0b11), Op1(0b000), CRn(0b0110), CRm(0b0000), Op2(0b000),
>>>> access_vm_reg, reset_unknown, FAR_EL1 },
>>>> + /* FAR_EL2 */
>>>> + { Op0(0b11), Op1(0b100), CRn(0b0110), CRm(0b0000), Op2(0b000),
>>>> + access_vm_reg, reset_unknown, FAR_EL2 },
>>>> /* PAR_EL1 */
>>>> { Op0(0b11), Op1(0b000), CRn(0b0111), CRm(0b0100), Op2(0b000),
>>>> NULL, reset_unknown, PAR_EL1 },
>>>>
>>>
>>> Also, what do you return here? All you're doing to return to userspace
>>> is 0x1de7ec7edbadc0deULL (which perfectly matches this patch).
>>>
>>> So for all intents and purposes, this patch is pretty useless.
>>>
>>> Thanks,
>>>
>>> M.
>>>
>>
>
>
