lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 29 Mar 2017 16:50:43 +0100
From:   Suzuki K Poulose <Suzuki.Poulose@....com>
To:     Leo Yan <leo.yan@...aro.org>
Cc:     Jonathan Corbet <corbet@....net>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Wei Xu <xuwei5@...ilicon.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Guodong Xu <guodong.xu@...aro.org>,
        John Stultz <john.stultz@...aro.org>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, mike.leach@...aro.org,
        sudeep.holla@....com
Subject: Re: [PATCH v5 6/9] coresight: add support for CPU debug module

On 29/03/17 11:37, Leo Yan wrote:
> On Wed, Mar 29, 2017 at 11:31:03AM +0100, Suzuki K Poulose wrote:
>> On 29/03/17 11:27, Leo Yan wrote:
>>> On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote:
>>>
>>> [...]
>>>
>>>>>>> +	if (mode == EDDEVID_IMPL_NONE) {
>>>>>>> +		drvdata->edpcsr_present  = false;
>>>>>>> +		drvdata->edcidsr_present = false;
>>>>>>> +		drvdata->edvidsr_present = false;
>>>>>>> +	} else if (mode == EDDEVID_IMPL_EDPCSR) {
>>>>>>> +		drvdata->edpcsr_present  = true;
>>>>>>> +		drvdata->edcidsr_present = false;
>>>>>>> +		drvdata->edvidsr_present = false;
>>>>>>> +	} else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) {
>>>>>>> +		if (!IS_ENABLED(CONFIG_64BIT) &&
>>>>>>> +			(pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32))
>>>>>>> +			drvdata->edpcsr_present = false;
>>>>>>> +		else
>>>>>>> +			drvdata->edpcsr_present = true;
>>>>>>
>>>>>> Sorry, I forgot why we do this check only in this mode. Shouldn't this be
>>>>>> common to all modes (of course which implies PCSR is present) ?
>>>>>
>>>>> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we
>>>>> simplize PCSROffset value :
>>>>> 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0])
>>>>> 0001 - No offset applies.
>>>>> 0010 - No offset applies, but do not use in AArch32 mode!
>>>>>
>>>>> So we need handle the corner case is when CPU runs AArch32 mode and
>>>>> PCSRoffset = 'b0010. Other cases the pcsr should be present.
>>>>
>>>> I understand that reasoning. But my question is, why do we check for PCSROffset
>>>> only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == EDDEVID_IMPL_EDPCSR or
>>>> any other mode where PCSR is present.
>>>
>>> Sorry I misunderstood your question.
>>>
>>> I made mistake when I analyzed the possbile combination for mode and
>>> PCSROffset so I thought it's the only case should handle:
>>> { EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
>>>
>>> Below three combinations are possible to exist; so you are right, I
>>> should move this out for the checking:
>>> { EDDEVID_IMPL_NONE,           EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }
>>
>> That need not be covered, as IMPL_NONE says PCSR is not implemented hence you
>> don't worry about anything as the functionality is missing. This should rather be:
>> EDDEVID_IMPL_EDPCSR, where only PCSR is implemented.
>
> I think below combination doesn't really exist:
> { EDDEVID_IMPL_EDPCSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 };
>
> EDDEVID_IMPL_EDPCSR is only defined in ARMv7 ARM, and
> EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 is only defined in ARMv8 ARM.

It is not wrong to check the PCSROffset in all cases where PCSR is available, as if
we hit PCSR on ARMv7 then PCSROffset shouldn't be DIS_AARCH32. And in fact that
would make the code a bit more cleaner. Anyways, I am not particular about this.

Suzuki

Powered by blists - more mailing lists