Re: consult a question about 'inject_abt64()'

Re: consult a question about 'inject_abt64()'

[gengdongjiu]

To KVM mailing List about below question, thanks.



On 2017/9/18 21:17, Matt Evans wrote:
> Hi Gengdongjiu,
> 
> 
>> On 18 Sep 2017, at 07:39, gengdongjiu <gengdongjiu@huawei.com> wrote:
>>
>> Hi Matt,
>>   sorry to disturb you, I want to consult you a question about the function inject_abt64();
>> For this function, the abort EC can be ESR_ELx_EC_DABT_LOW or ESR_ELx_EC_DABT_CUR according to *vcpu_cpsr(vcpu);
>> why it always ESR_ELx_EC_DABT_LOW?  thanks!
>>
>> As you can see the Iabort, when injecting the Iabort, it will check the cpsr to decide to inject the ESR_ELx_EC_IABT_LOW or ESR_ELx_EC_IABT_CUR.
>> but I do not know why the Dabort will always be ESR_ELx_EC_DABT_LOW when injecting.
>>
>>        /*
>>         * Here, the guest runs in AArch64 mode when in EL1. If we get
>>         * an AArch32 fault, it means we managed to trap an EL0 fault.
>>         */
>>        if (is_aarch32 || (cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t)
>>                esr |= (ESR_ELx_EC_IABT_LOW << ESR_ELx_EC_SHIFT);
>>        else
>>                esr |= (ESR_ELx_EC_IABT_CUR << ESR_ELx_EC_SHIFT);
> 
> I think you're right.  When I fixed that line I had noticed the lack of left-shift but didn't think about the actual ESR value.
> 
> The injected vector is calculated correctly (between current-EL or lower-EL vectors) by get_except_vector() but the EC should also correspond to current-EL or lower-EL.
> 
> It is worth following this up on the ARM KVM mailing list!  (To be fair, your question should have been sent there too :) )
> 
> Thanks,
> 
> 
> Matt
> 
> 
> 
>>
>>
>>
>> commit e4fe9e7dc3828bf6a5714eb3c55aef6260d823a2
>> Author: Matt Evans <matt.evans@arm.com>
>> Date:   Mon May 16 13:54:56 2016 +0100
>>
>>    kvm: arm64: Fix EC field in inject_abt64
>>
>>    The EC field of the constructed ESR is conditionally modified by ORing in
>>    ESR_ELx_EC_DABT_LOW for a data abort.  However, ESR_ELx_EC_SHIFT is missing
>>    from this condition.
>>
>>    Signed-off-by: Matt Evans <matt.evans@arm.com>
>>    Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>>    Cc: <stable@vger.kernel.org>
>>    Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>
>> diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
>> index 4d1ac81870d2..e9e0e6db73f6 100644
>> --- a/arch/arm64/kvm/inject_fault.c
>> +++ b/arch/arm64/kvm/inject_fault.c
>> @@ -162,7 +162,7 @@ static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addr
>>                esr |= (ESR_ELx_EC_IABT_CUR << ESR_ELx_EC_SHIFT);
>>
>>        if (!is_iabt)
>> -               esr |= ESR_ELx_EC_DABT_LOW;
>> +               esr |= ESR_ELx_EC_DABT_LOW << ESR_ELx_EC_SHIFT;
>>
>>        vcpu_sys_reg(vcpu, ESR_EL1) = esr | ESR_ELx_FSC_EXTABT;
>> }
>>
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 
> 

Re: consult a question about 'inject_abt64()'

[Christoffer Dall]

Hi,

On Mon, Sep 18, 2017 at 09:49:25PM +0800, gengdongjiu wrote:
> To KVM mailing List about below question, thanks.
> 

[Please send properly formatted and written e-mails to this mailing
list instead of just forwarding a conversation withtout futher
explanation.  It would have been more helpful if you had suggested a
patch or at least explained what you concluded from your conversation
with Matt.]

I don't think there's a problem here, because the ESR values are defined
as

#define ESR_ELx_EC_IABT_LOW▸    (0x20)
#define ESR_ELx_EC_IABT_CUR▸    (0x21)
...
#define ESR_ELx_EC_DABT_LOW▸    (0x24)
#define ESR_ELx_EC_DABT_CUR▸    (0x25)

and we just or on the differating bit for a data abort:

if (!is_iabt)
	esr |= ESR_ELx_EC_DABT_LOW << ESR_ELx_EC_SHIFT;

So you'll see that ESR_ELx_EC_IABT_LOW and ESR_ELx_EC_IABT_CUR become
ESR_ELx_EC_DABT_LOW and ESR_ELx_EC_DABT_CUR, respectively.

Thanks,
-Christoffer

> 
> 
> On 2017/9/18 21:17, Matt Evans wrote:
> > Hi Gengdongjiu,
> > 
> > 
> >> On 18 Sep 2017, at 07:39, gengdongjiu <gengdongjiu@huawei.com> wrote:
> >>
> >> Hi Matt,
> >>   sorry to disturb you, I want to consult you a question about the function inject_abt64();
> >> For this function, the abort EC can be ESR_ELx_EC_DABT_LOW or ESR_ELx_EC_DABT_CUR according to *vcpu_cpsr(vcpu);
> >> why it always ESR_ELx_EC_DABT_LOW?  thanks!
> >>
> >> As you can see the Iabort, when injecting the Iabort, it will check the cpsr to decide to inject the ESR_ELx_EC_IABT_LOW or ESR_ELx_EC_IABT_CUR.
> >> but I do not know why the Dabort will always be ESR_ELx_EC_DABT_LOW when injecting.
> >>
> >>        /*
> >>         * Here, the guest runs in AArch64 mode when in EL1. If we get
> >>         * an AArch32 fault, it means we managed to trap an EL0 fault.
> >>         */
> >>        if (is_aarch32 || (cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t)
> >>                esr |= (ESR_ELx_EC_IABT_LOW << ESR_ELx_EC_SHIFT);
> >>        else
> >>                esr |= (ESR_ELx_EC_IABT_CUR << ESR_ELx_EC_SHIFT);
> > 
> > I think you're right.  When I fixed that line I had noticed the lack of left-shift but didn't think about the actual ESR value.
> > 
> > The injected vector is calculated correctly (between current-EL or lower-EL vectors) by get_except_vector() but the EC should also correspond to current-EL or lower-EL.
> > 
> > It is worth following this up on the ARM KVM mailing list!  (To be fair, your question should have been sent there too :) )
> > 
> > Thanks,
> > 
> > 
> > Matt
> > 
> > 
> > 
> >>
> >>
> >>
> >> commit e4fe9e7dc3828bf6a5714eb3c55aef6260d823a2
> >> Author: Matt Evans <matt.evans@arm.com>
> >> Date:   Mon May 16 13:54:56 2016 +0100
> >>
> >>    kvm: arm64: Fix EC field in inject_abt64
> >>
> >>    The EC field of the constructed ESR is conditionally modified by ORing in
> >>    ESR_ELx_EC_DABT_LOW for a data abort.  However, ESR_ELx_EC_SHIFT is missing
> >>    from this condition.
> >>
> >>    Signed-off-by: Matt Evans <matt.evans@arm.com>
> >>    Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> >>    Cc: <stable@vger.kernel.org>
> >>    Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >>
> >> diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
> >> index 4d1ac81870d2..e9e0e6db73f6 100644
> >> --- a/arch/arm64/kvm/inject_fault.c
> >> +++ b/arch/arm64/kvm/inject_fault.c
> >> @@ -162,7 +162,7 @@ static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addr
> >>                esr |= (ESR_ELx_EC_IABT_CUR << ESR_ELx_EC_SHIFT);
> >>
> >>        if (!is_iabt)
> >> -               esr |= ESR_ELx_EC_DABT_LOW;
> >> +               esr |= ESR_ELx_EC_DABT_LOW << ESR_ELx_EC_SHIFT;
> >>
> >>        vcpu_sys_reg(vcpu, ESR_EL1) = esr | ESR_ELx_FSC_EXTABT;
> >> }
> >>
> > 
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > 
> > 
> 

Re: consult a question about 'inject_abt64()'

[gengdongjiu]

Hi, Christoffer

On 2017/9/19 6:32, Christoffer Dall wrote:
> Hi,
> 
> On Mon, Sep 18, 2017 at 09:49:25PM +0800, gengdongjiu wrote:
>> To KVM mailing List about below question, thanks.
>>
> 
> [Please send properly formatted and written e-mails to this mailing
> list instead of just forwarding a conversation withtout futher
> explanation.  It would have been more helpful if you had suggested a
> patch or at least explained what you concluded from your conversation
> with Matt.]
sorry for the mail format. I will pay attention to that next time.

> 
> I don't think there's a problem here, because the ESR values are defined
> as
> 
> #define ESR_ELx_EC_IABT_LOW▸    (0x20)
> #define ESR_ELx_EC_IABT_CUR▸    (0x21)
> ...
> #define ESR_ELx_EC_DABT_LOW▸    (0x24)
> #define ESR_ELx_EC_DABT_CUR▸    (0x25)
> 
> and we just or on the differating bit for a data abort:
> 
> if (!is_iabt)
> 	esr |= ESR_ELx_EC_DABT_LOW << ESR_ELx_EC_SHIFT;
> 
> So you'll see that ESR_ELx_EC_IABT_LOW and ESR_ELx_EC_IABT_CUR become
> ESR_ELx_EC_DABT_LOW and ESR_ELx_EC_DABT_CUR, respectively.

Thanks for the explanation.


> 
> Thanks,
> -Christoffer
> 
>>
>>
>> On 2017/9/18 21:17, Matt Evans wrote:
>>> Hi Gengdongjiu,
>>>
>>>
>>>> On 18 Sep 2017, at 07:39, gengdongjiu <gengdongjiu@huawei.com> wrote:
>>>>
>>>> Hi Matt,
>>>>   sorry to disturb you, I want to consult you a question about the function inject_abt64();
>>>> For this function, the abort EC can be ESR_ELx_EC_DABT_LOW or ESR_ELx_EC_DABT_CUR according to *vcpu_cpsr(vcpu);
>>>> why it always ESR_ELx_EC_DABT_LOW?  thanks!
>>>>
>>>> As you can see the Iabort, when injecting the Iabort, it will check the cpsr to decide to inject the ESR_ELx_EC_IABT_LOW or ESR_ELx_EC_IABT_CUR.
>>>> but I do not know why the Dabort will always be ESR_ELx_EC_DABT_LOW when injecting.
>>>>
>>>>        /*
>>>>         * Here, the guest runs in AArch64 mode when in EL1. If we get
>>>>         * an AArch32 fault, it means we managed to trap an EL0 fault.
>>>>         */
>>>>        if (is_aarch32 || (cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t)
>>>>                esr |= (ESR_ELx_EC_IABT_LOW << ESR_ELx_EC_SHIFT);
>>>>        else
>>>>                esr |= (ESR_ELx_EC_IABT_CUR << ESR_ELx_EC_SHIFT);
>>>
>>> I think you're right.  When I fixed that line I had noticed the lack of left-shift but didn't think about the actual ESR value.
>>>
>>> The injected vector is calculated correctly (between current-EL or lower-EL vectors) by get_except_vector() but the EC should also correspond to current-EL or lower-EL.
>>>
>>> It is worth following this up on the ARM KVM mailing list!  (To be fair, your question should have been sent there too :) )
>>>
>>> Thanks,
>>>
>>>
>>> Matt
>>>
>>>
>>>
>>>>
>>>>
>>>>
>>>> commit e4fe9e7dc3828bf6a5714eb3c55aef6260d823a2
>>>> Author: Matt Evans <matt.evans@arm.com>
>>>> Date:   Mon May 16 13:54:56 2016 +0100
>>>>
>>>>    kvm: arm64: Fix EC field in inject_abt64
>>>>
>>>>    The EC field of the constructed ESR is conditionally modified by ORing in
>>>>    ESR_ELx_EC_DABT_LOW for a data abort.  However, ESR_ELx_EC_SHIFT is missing
>>>>    from this condition.
>>>>
>>>>    Signed-off-by: Matt Evans <matt.evans@arm.com>
>>>>    Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>    Cc: <stable@vger.kernel.org>
>>>>    Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>>>
>>>> diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
>>>> index 4d1ac81870d2..e9e0e6db73f6 100644
>>>> --- a/arch/arm64/kvm/inject_fault.c
>>>> +++ b/arch/arm64/kvm/inject_fault.c
>>>> @@ -162,7 +162,7 @@ static void inject_abt64(struct kvm_vcpu *vcpu, bool is_iabt, unsigned long addr
>>>>                esr |= (ESR_ELx_EC_IABT_CUR << ESR_ELx_EC_SHIFT);
>>>>
>>>>        if (!is_iabt)
>>>> -               esr |= ESR_ELx_EC_DABT_LOW;
>>>> +               esr |= ESR_ELx_EC_DABT_LOW << ESR_ELx_EC_SHIFT;
>>>>
>>>>        vcpu_sys_reg(vcpu, ESR_EL1) = esr | ESR_ELx_FSC_EXTABT;
>>>> }
>>>>
>>>
>>> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>>>
>>>
>>
> 
> .
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: consult a question about 'inject_abt64()'

[Marc Zyngier]

On 19/09/17 03:22, gengdongjiu wrote:
> Hi, Christoffer
> 
> On 2017/9/19 6:32, Christoffer Dall wrote:
>> Hi,
>>
>> On Mon, Sep 18, 2017 at 09:49:25PM +0800, gengdongjiu wrote:
>>> To KVM mailing List about below question, thanks.
>>>
>>
>> [Please send properly formatted and written e-mails to this mailing
>> list instead of just forwarding a conversation withtout futher
>> explanation.  It would have been more helpful if you had suggested a
>> patch or at least explained what you concluded from your conversation
>> with Matt.]
> sorry for the mail format. I will pay attention to that next time.

Please also pay attention to what you are forwarding. Matt's email
contains an explicit confidentiality clause, and the normal course of
action for any reader of the ML would be to *delete* this email.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...