From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
To: "Alvin Che-Chia Chang(張哲嘉)" <alvinga@andestech.com>,
"qemu-riscv@nongnu.org" <qemu-riscv@nongnu.org>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "alistair.francis@wdc.com" <alistair.francis@wdc.com>,
"bin.meng@windriver.com" <bin.meng@windriver.com>,
"liwei1518@gmail.com" <liwei1518@gmail.com>,
"zhiwei_liu@linux.alibaba.com" <zhiwei_liu@linux.alibaba.com>
Subject: Re: [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint
Date: Thu, 22 Feb 2024 09:37:39 -0300 [thread overview]
Message-ID: <eb13a04e-420a-4db7-a324-25520bcc32ea@ventanamicro.com> (raw)
In-Reply-To: <SEYPR03MB6700A34DA4F22756C3C72CF0A8562@SEYPR03MB6700.apcprd03.prod.outlook.com>
On 2/21/24 22:46, Alvin Che-Chia Chang(張哲嘉) wrote:
> Hi Daniel,
>
>> -----Original Message-----
>> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Sent: Thursday, February 22, 2024 1:26 AM
>> To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>;
>> qemu-riscv@nongnu.org; qemu-devel@nongnu.org
>> Cc: alistair.francis@wdc.com; bin.meng@windriver.com;
>> liwei1518@gmail.com; zhiwei_liu@linux.alibaba.com
>> Subject: Re: [PATCH 2/4] target/riscv: Apply modularized matching conditions
>> for breakpoint
>>
>>
>>
>> On 2/19/24 00:25, Alvin Chang wrote:
>>> We have implemented trigger_common_match(), which checks if the
>>> enabled privilege levels of the trigger match CPU's current privilege level.
>>> Remove the related code in riscv_cpu_debug_check_breakpoint() and
>>> invoke
>>> trigger_common_match() to check the privilege levels of the type 2 and
>>> type 6 triggers for the breakpoints.
>>>
>>> Only the execution bit and the executed PC should be futher checked in
>>
>> typo: further
>
> Thanks! Will fix it.
>
>>
>>> riscv_cpu_debug_check_breakpoint().
>>>
>>> Signed-off-by: Alvin Chang <alvinga@andestech.com>
>>> ---
>>> target/riscv/debug.c | 26 ++++++--------------------
>>> 1 file changed, 6 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/target/riscv/debug.c b/target/riscv/debug.c index
>>> 7932233073..b971ed5d7a 100644
>>> --- a/target/riscv/debug.c
>>> +++ b/target/riscv/debug.c
>>> @@ -855,21 +855,17 @@ bool riscv_cpu_debug_check_breakpoint(CPUState
>> *cs)
>>> for (i = 0; i < RV_MAX_TRIGGERS; i++) {
>>> trigger_type = get_trigger_type(env, i);
>>>
>>> + if (!trigger_common_match(env, trigger_type, i)) {
>>> + continue;
>>> + }
>>> +
>>
>> I believe this will change how the function behaves. Before this patch, we
>> would 'return false' if we have a TRIGGER_TYPE_AD_MATCH and
>> env->virt_enabled is true.
>>
>> Now, for the same case, we'll keep looping until we found a match, or return
>> 'false'
>> if we run out of triggers.
>
> Oh! I didn't notice that the behavior is changed.. thank you.
>
> Image that CPU supports both type 2 and type 6 triggers, and the debugger sets two identical breakpoints.(this should be a mistake, but we should not restrict the debugger)
> One of them is type 2 breakpoint at trigger index 0, and the other is type 6 breakpoint at trigger index 1.
> Now if the system runs in VS/VU modes, it could match type 6 breakpoint, so the looping is necessary.
> If the system runs in M/HS/U modes, it could match type 2 breakpoint.
> Is my understanding correct?
It looks correct to me. We just need to mention in the commit msg that the behavior
change is intentional, not an accident.
Thanks,
Daniel
>
>
> Sincerely,
> Alvin
>
>>
>> This seems ok to do, but we should state in the commit msg that we're
>> intentionally change how the function works. If that's not the idea and we want
>> to preserve the existing behavior, we would need to do:
>>
>>>
>>> + if (!trigger_common_match(env, trigger_type, i)) {
>>> + return false;
>>> + }
>>
>> Instead of just continue looping. Thanks,
>>
>>
>> Daniel
>>
>>> switch (trigger_type) {
>>> case TRIGGER_TYPE_AD_MATCH:
>>> - /* type 2 trigger cannot be fired in VU/VS mode */
>>> - if (env->virt_enabled) {
>>> - return false;
>>> - }
>>> -
>>> ctrl = env->tdata1[i];
>>> pc = env->tdata2[i];
>>>
>>> if ((ctrl & TYPE2_EXEC) && (bp->pc == pc)) {
>>> - /* check U/S/M bit against current privilege level
>> */
>>> - if ((ctrl >> 3) & BIT(env->priv)) {
>>> - return true;
>>> - }
>>> + return true;
>>> }
>>> break;
>>> case TRIGGER_TYPE_AD_MATCH6:
>>> @@ -877,17 +873,7 @@ bool riscv_cpu_debug_check_breakpoint(CPUState
>> *cs)
>>> pc = env->tdata2[i];
>>>
>>> if ((ctrl & TYPE6_EXEC) && (bp->pc == pc)) {
>>> - if (env->virt_enabled) {
>>> - /* check VU/VS bit against current privilege
>> level */
>>> - if ((ctrl >> 23) & BIT(env->priv)) {
>>> - return true;
>>> - }
>>> - } else {
>>> - /* check U/S/M bit against current privilege
>> level */
>>> - if ((ctrl >> 3) & BIT(env->priv)) {
>>> - return true;
>>> - }
>>> - }
>>> + return true;
>>> }
>>> break;
>>> default:
next prev parent reply other threads:[~2024-02-22 12:38 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-19 3:25 [PATCH 0/4] RISC-V: Modularize common match conditions for trigger Alvin Chang via
2024-02-19 3:25 ` [PATCH 1/4] target/riscv: Add functions for common matching conditions of trigger Alvin Chang via
2024-02-21 17:09 ` Daniel Henrique Barboza
2024-02-19 3:25 ` [PATCH 2/4] target/riscv: Apply modularized matching conditions for breakpoint Alvin Chang via
2024-02-21 17:25 ` Daniel Henrique Barboza
2024-02-22 1:46 ` Alvin Che-Chia Chang(張哲嘉)
2024-02-22 12:37 ` Daniel Henrique Barboza [this message]
2024-02-19 3:25 ` [PATCH 3/4] target/riscv: Apply modularized matching conditions for watchpoint Alvin Chang via
2024-02-21 17:31 ` Daniel Henrique Barboza
2024-02-19 3:25 ` [PATCH 4/4] target/riscv: Apply modularized matching conditions for icount trigger Alvin Chang via
2024-02-21 18:05 ` Daniel Henrique Barboza
2024-02-22 2:05 ` Alvin Che-Chia Chang(張哲嘉)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eb13a04e-420a-4db7-a324-25520bcc32ea@ventanamicro.com \
--to=dbarboza@ventanamicro.com \
--cc=alistair.francis@wdc.com \
--cc=alvinga@andestech.com \
--cc=bin.meng@windriver.com \
--cc=liwei1518@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=zhiwei_liu@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).