Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751829AbdHGTA5 (ORCPT ); Mon, 7 Aug 2017 15:00:57 -0400 Received: from szxga04-in.huawei.com ([45.249.212.190]:3018 "EHLO szxga04-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751426AbdHGTAz (ORCPT ); Mon, 7 Aug 2017 15:00:55 -0400 Subject: Re: [PATCH] KVM: arm64: add esr_el2 and far_el2 to sysreg To: Marc Zyngier , , , , , , , , , , James Morse References: <1502122117-6276-1-git-send-email-gengdongjiu@huawei.com> <9fe6aa3a-98f9-6cae-6932-054858b54744@arm.com> From: gengdongjiu Message-ID: <085aaa48-3d8d-5af4-0329-6db8175f7032@huawei.com> Date: Tue, 8 Aug 2017 02:59:44 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.142.68.147] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090203.5988B8D2.0048,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 6bfa0d2158ad1c62fc4e85c2c719a654 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4440 Lines: 108 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 >>>> Signed-off-by: Quanming Wu >>>> --- >>>> 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. >>> >> > >