| Message ID | tencent_6FF30F7E2E640BEE260FD6523B6BA5486908@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | include virtualization mode as part of priv | expand |
On Thu, Nov 28, 2024 at 12:09 AM Yanfeng <yfliu2008@qq.com> wrote: > > > When debugging hypervisor extension based programs, it is convenient to see the > current virtualization mode from GDB debugger. > > This patch shares the virtualization mode as part of the existing "priv" virtual > register, or more specifically via bit(8). Interesting concept. This seems fine to me, I don't think this will break any existing user. Another option though is to add a "virt" virtual register, which might be easier for people to figure out as it isn't obvious from GDB what bit 8 means. That might be a better approach as then it's really clear what the register means. > > > >From 0d82561b11e1c2835f14ba5460cfff52f0087530 Mon Sep 17 00:00:00 2001 > From: Yanfeng Liu <yfliu2008@qq.com> > Date: Mon, 18 Nov 2024 08:03:15 +0800 > Subject: [PATCH] riscv/gdb: share virt mode via priv register It seems like something strange happened when you submitted the patch. Can you fix this up? Alistair > > This shares virtualization mode together with privilege mode > via the `priv` virtual register over the debug interface. > > Check logs with gdb-multiarch 12.1: > > ``` > (gdb) info registers priv > priv 0x101 prv:1 [Supervisor] > (gdb) set $priv = 1 > (gdb) info registers priv > priv 0x1 prv:1 [Supervisor] > (gdb) set $priv = 0x101 > (gdb) info registers priv > priv 0x101 prv:1 [Supervisor] > (gdb) > ``` > > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com> > --- > target/riscv/cpu_bits.h | 4 ++++ > target/riscv/gdbstub.c | 15 +++++++++++++-- > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > index 385a2c67c2..cc6dece51a 100644 > --- a/target/riscv/cpu_bits.h > +++ b/target/riscv/cpu_bits.h > @@ -623,6 +623,10 @@ typedef enum { > #define PRV_RESERVED 2 > #define PRV_M 3 > > +/* Share virtualization mode as part of priv register */ > +#define PRV_V (1 << 8) > + > + > /* RV32 satp CSR field masks */ > #define SATP32_MODE 0x80000000 > #define SATP32_ASID 0x7fc00000 > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c > index c07df972f1..d9e6ad969a 100644 > --- a/target/riscv/gdbstub.c > +++ b/target/riscv/gdbstub.c > @@ -212,8 +212,14 @@ static int riscv_gdb_get_virtual(CPUState *cs, GByteArray > *buf, int n) > #else > RISCVCPU *cpu = RISCV_CPU(cs); > CPURISCVState *env = &cpu->env; > + target_ulong ret = env->priv; > > - return gdb_get_regl(buf, env->priv); > + /* include virtualization mode */ > + > + if (env->virt_enabled) { > + ret |= PRV_V; > + } > + return gdb_get_regl(buf, ret); > #endif > } > return 0; > @@ -225,11 +231,16 @@ static int riscv_gdb_set_virtual(CPUState *cs, uint8_t > *mem_buf, int n) > #ifndef CONFIG_USER_ONLY > RISCVCPU *cpu = RISCV_CPU(cs); > CPURISCVState *env = &cpu->env; > + target_ulong val = ldtul_p(mem_buf); > > - env->priv = ldtul_p(mem_buf) & 0x3; > + env->priv = val & 0x3; > if (env->priv == PRV_RESERVED) { > env->priv = PRV_S; > } > + > + /* Update virtualization mode */ > + > + env->virt_enabled = (env->priv != PRV_M && (val & PRV_V) != 0); > #endif > return sizeof(target_ulong); > } > -- > 2.34.1 > > > >
On Thu, 2024-11-28 at 10:39 +1000, Alistair Francis wrote: > On Thu, Nov 28, 2024 at 12:09 AM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > When debugging hypervisor extension based programs, it is convenient to see > > the > > current virtualization mode from GDB debugger. > > > > This patch shares the virtualization mode as part of the existing "priv" > > virtual > > register, or more specifically via bit(8). > > Interesting concept. This seems fine to me, I don't think this will > break any existing user. > > Another option though is to add a "virt" virtual register, which might > be easier for people to figure out as it isn't obvious from GDB what > bit 8 means. That might be a better approach as then it's really clear > what the register means. > > > > > > > > From 0d82561b11e1c2835f14ba5460cfff52f0087530 Mon Sep 17 00:00:00 2001 > > From: Yanfeng Liu <yfliu2008@qq.com> > > Date: Mon, 18 Nov 2024 08:03:15 +0800 > > Subject: [PATCH] riscv/gdb: share virt mode via priv register > > It seems like something strange happened when you submitted the patch. > Can you fix this up? > I prepared a patch file via the following steps: - Did `git format-patch --stdout -1 > /tmp/patch`, - Pasted the /tmp/patch content to email composer window, - Filled in email receipts, subject line and a few lines before the patch context in composer window. I am wondering if the lines added before the formatted patch content in last step caused trouble? When resending email, should I use [patch v2] in subject line? I guess it is unnecessary as it is the same patch. I included it as attachment here as well, hoping it helps. Regards, yf > Alistair > > > > > This shares virtualization mode together with privilege mode > > via the `priv` virtual register over the debug interface. > > > > Check logs with gdb-multiarch 12.1: > > > > ``` > > (gdb) info registers priv > > priv 0x101 prv:1 [Supervisor] > > (gdb) set $priv = 1 > > (gdb) info registers priv > > priv 0x1 prv:1 [Supervisor] > > (gdb) set $priv = 0x101 > > (gdb) info registers priv > > priv 0x101 prv:1 [Supervisor] > > (gdb) > > ``` > > > > Signed-off-by: Yanfeng Liu <yfliu2008@qq.com> > > --- > > target/riscv/cpu_bits.h | 4 ++++ > > target/riscv/gdbstub.c | 15 +++++++++++++-- > > 2 files changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > > index 385a2c67c2..cc6dece51a 100644 > > --- a/target/riscv/cpu_bits.h > > +++ b/target/riscv/cpu_bits.h > > @@ -623,6 +623,10 @@ typedef enum { > > #define PRV_RESERVED 2 > > #define PRV_M 3 > > > > +/* Share virtualization mode as part of priv register */ > > +#define PRV_V (1 << 8) > > + > > + > > /* RV32 satp CSR field masks */ > > #define SATP32_MODE 0x80000000 > > #define SATP32_ASID 0x7fc00000 > > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c > > index c07df972f1..d9e6ad969a 100644 > > --- a/target/riscv/gdbstub.c > > +++ b/target/riscv/gdbstub.c > > @@ -212,8 +212,14 @@ static int riscv_gdb_get_virtual(CPUState *cs, > > GByteArray > > *buf, int n) > > #else > > RISCVCPU *cpu = RISCV_CPU(cs); > > CPURISCVState *env = &cpu->env; > > + target_ulong ret = env->priv; > > > > - return gdb_get_regl(buf, env->priv); > > + /* include virtualization mode */ > > + > > + if (env->virt_enabled) { > > + ret |= PRV_V; > > + } > > + return gdb_get_regl(buf, ret); > > #endif > > } > > return 0; > > @@ -225,11 +231,16 @@ static int riscv_gdb_set_virtual(CPUState *cs, uint8_t > > *mem_buf, int n) > > #ifndef CONFIG_USER_ONLY > > RISCVCPU *cpu = RISCV_CPU(cs); > > CPURISCVState *env = &cpu->env; > > + target_ulong val = ldtul_p(mem_buf); > > > > - env->priv = ldtul_p(mem_buf) & 0x3; > > + env->priv = val & 0x3; > > if (env->priv == PRV_RESERVED) { > > env->priv = PRV_S; > > } > > + > > + /* Update virtualization mode */ > > + > > + env->virt_enabled = (env->priv != PRV_M && (val & PRV_V) != 0); > > #endif > > return sizeof(target_ulong); > > } > > -- > > 2.34.1 > > > > > >
On Thu, Nov 28, 2024 at 11:43 AM Yanfeng <yfliu2008@qq.com> wrote: > > On Thu, 2024-11-28 at 10:39 +1000, Alistair Francis wrote: > > On Thu, Nov 28, 2024 at 12:09 AM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > > > > When debugging hypervisor extension based programs, it is convenient to see > > > the > > > current virtualization mode from GDB debugger. > > > > > > This patch shares the virtualization mode as part of the existing "priv" > > > virtual > > > register, or more specifically via bit(8). > > > > Interesting concept. This seems fine to me, I don't think this will > > break any existing user. > > > > Another option though is to add a "virt" virtual register, which might > > be easier for people to figure out as it isn't obvious from GDB what > > bit 8 means. That might be a better approach as then it's really clear > > what the register means. > > > > > > > > > > > > From 0d82561b11e1c2835f14ba5460cfff52f0087530 Mon Sep 17 00:00:00 2001 > > > From: Yanfeng Liu <yfliu2008@qq.com> > > > Date: Mon, 18 Nov 2024 08:03:15 +0800 > > > Subject: [PATCH] riscv/gdb: share virt mode via priv register > > > > It seems like something strange happened when you submitted the patch. > > Can you fix this up? > > > I prepared a patch file via the following steps: > > - Did `git format-patch --stdout -1 > /tmp/patch`, > - Pasted the /tmp/patch content to email composer window, > - Filled in email receipts, subject line and a few lines before the patch > context in composer window. You should use `git send-email` instead, see the QEMU documentation for more details: https://www.qemu.org/docs/master/devel/submitting-a-patch.html#submitting-your-patches > > I am wondering if the lines added before the formatted patch content in last > step caused trouble? > When resending email, should I use [patch v2] in subject line? I guess it is > unnecessary as it is the same patch. Ideally I think a "virt" virtual register would be a better approach, so if you could do that instead and send a v2 that would be great :) Alistair
Alistair. My initial `git send-email" on Ubuntu 22.04 wasn't lukcy: ```shell $ git send-email git: 'send-email' is not a git command. See 'git --help'. ``` I need time to find a solution. As for seperate `virt` thing, my concern is that though the V and P status looks orthogonal, they are not so independent (e.g. `P=3` implies `V=0`). Having them in one `priv` register tells user that that they should be operated together using one command. On Thu, 2024-11-28 at 12:43 +1000, Alistair Francis wrote: > On Thu, Nov 28, 2024 at 11:43 AM Yanfeng <yfliu2008@qq.com> wrote: > > > > On Thu, 2024-11-28 at 10:39 +1000, Alistair Francis wrote: > > > On Thu, Nov 28, 2024 at 12:09 AM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > > > > > > > When debugging hypervisor extension based programs, it is convenient to > > > > see > > > > the > > > > current virtualization mode from GDB debugger. > > > > > > > > This patch shares the virtualization mode as part of the existing "priv" > > > > virtual > > > > register, or more specifically via bit(8). > > > > > > Interesting concept. This seems fine to me, I don't think this will > > > break any existing user. > > > > > > Another option though is to add a "virt" virtual register, which might > > > be easier for people to figure out as it isn't obvious from GDB what > > > bit 8 means. That might be a better approach as then it's really clear > > > what the register means. > > > > > > > > > > > > > > > > From 0d82561b11e1c2835f14ba5460cfff52f0087530 Mon Sep 17 00:00:00 2001 > > > > From: Yanfeng Liu <yfliu2008@qq.com> > > > > Date: Mon, 18 Nov 2024 08:03:15 +0800 > > > > Subject: [PATCH] riscv/gdb: share virt mode via priv register > > > > > > It seems like something strange happened when you submitted the patch. > > > Can you fix this up? > > > > > I prepared a patch file via the following steps: > > > > - Did `git format-patch --stdout -1 > /tmp/patch`, > > - Pasted the /tmp/patch content to email composer window, > > - Filled in email receipts, subject line and a few lines before the patch > > context in composer window. > > You should use `git send-email` instead, see the QEMU documentation > for more details: > https://www.qemu.org/docs/master/devel/submitting-a-patch.html#submitting-your-patches > > > > > I am wondering if the lines added before the formatted patch content in last > > step caused trouble? > > When resending email, should I use [patch v2] in subject line? I guess it is > > unnecessary as it is the same patch. > > Ideally I think a "virt" virtual register would be a better approach, > so if you could do that instead and send a v2 that would be great :) > > Alistair
On Thu, Nov 28, 2024 at 2:05 PM Yanfeng <yfliu2008@qq.com> wrote: > > Alistair. > > My initial `git send-email" on Ubuntu 22.04 wasn't lukcy: > > ```shell > $ git send-email > git: 'send-email' is not a git command. See 'git --help'. > ``` > > I need time to find a solution. You will need to install git-send-email (or something like that) > > As for seperate `virt` thing, my concern is that though the V and P status looks > orthogonal, they are not so independent (e.g. `P=3` implies `V=0`). Having them > in one `priv` register tells user that that they should be operated together > using one command. True, but how can a user know what bit 8 means? Alistair > > On Thu, 2024-11-28 at 12:43 +1000, Alistair Francis wrote: > > On Thu, Nov 28, 2024 at 11:43 AM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > On Thu, 2024-11-28 at 10:39 +1000, Alistair Francis wrote: > > > > On Thu, Nov 28, 2024 at 12:09 AM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > > > > > > > > > > When debugging hypervisor extension based programs, it is convenient to > > > > > see > > > > > the > > > > > current virtualization mode from GDB debugger. > > > > > > > > > > This patch shares the virtualization mode as part of the existing "priv" > > > > > virtual > > > > > register, or more specifically via bit(8). > > > > > > > > Interesting concept. This seems fine to me, I don't think this will > > > > break any existing user. > > > > > > > > Another option though is to add a "virt" virtual register, which might > > > > be easier for people to figure out as it isn't obvious from GDB what > > > > bit 8 means. That might be a better approach as then it's really clear > > > > what the register means. > > > > > > > > > > > > > > > > > > > > From 0d82561b11e1c2835f14ba5460cfff52f0087530 Mon Sep 17 00:00:00 2001 > > > > > From: Yanfeng Liu <yfliu2008@qq.com> > > > > > Date: Mon, 18 Nov 2024 08:03:15 +0800 > > > > > Subject: [PATCH] riscv/gdb: share virt mode via priv register > > > > > > > > It seems like something strange happened when you submitted the patch. > > > > Can you fix this up? > > > > > > > I prepared a patch file via the following steps: > > > > > > - Did `git format-patch --stdout -1 > /tmp/patch`, > > > - Pasted the /tmp/patch content to email composer window, > > > - Filled in email receipts, subject line and a few lines before the patch > > > context in composer window. > > > > You should use `git send-email` instead, see the QEMU documentation > > for more details: > > https://www.qemu.org/docs/master/devel/submitting-a-patch.html#submitting-your-patches > > > > > > > > I am wondering if the lines added before the formatted patch content in last > > > step caused trouble? > > > When resending email, should I use [patch v2] in subject line? I guess it is > > > unnecessary as it is the same patch. > > > > Ideally I think a "virt" virtual register would be a better approach, > > so if you could do that instead and send a v2 that would be great :) > > > > Alistair >
On Thu, 2024-11-28 at 14:10 +1000, Alistair Francis wrote: > On Thu, Nov 28, 2024 at 2:05 PM Yanfeng <yfliu2008@qq.com> wrote: > > > > Alistair. > > > > My initial `git send-email" on Ubuntu 22.04 wasn't lukcy: > > > > ```shell > > $ git send-email > > git: 'send-email' is not a git command. See 'git --help'. > > ``` > > > > I need time to find a solution. > > You will need to install git-send-email (or something like that) Yes, now the "git send-email" exists and I will see how to use it correctly. > > > > > As for seperate `virt` thing, my concern is that though the V and P status > > looks > > orthogonal, they are not so independent (e.g. `P=3` implies `V=0`). Having > > them > > in one `priv` register tells user that that they should be operated together > > using one command. > > True, but how can a user know what bit 8 means? Good point. Can we mention it in the user document for `priv` register? I used bit(8) at my first try and it worked from GDB command line. Actually I also tried bit(4) but found GDB command takes that bit as part of P value so I withdrawed back to bit(8). > > Alistair > > > > > On Thu, 2024-11-28 at 12:43 +1000, Alistair Francis wrote: > > > On Thu, Nov 28, 2024 at 11:43 AM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > > > On Thu, 2024-11-28 at 10:39 +1000, Alistair Francis wrote: > > > > > On Thu, Nov 28, 2024 at 12:09 AM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > > > > > > > > > > > > > When debugging hypervisor extension based programs, it is convenient > > > > > > to > > > > > > see > > > > > > the > > > > > > current virtualization mode from GDB debugger. > > > > > > > > > > > > This patch shares the virtualization mode as part of the existing > > > > > > "priv" > > > > > > virtual > > > > > > register, or more specifically via bit(8). > > > > > > > > > > Interesting concept. This seems fine to me, I don't think this will > > > > > break any existing user. > > > > > > > > > > Another option though is to add a "virt" virtual register, which might > > > > > be easier for people to figure out as it isn't obvious from GDB what > > > > > bit 8 means. That might be a better approach as then it's really clear > > > > > what the register means. > > > > > > > > > > > > > > > > > > > > > > > > From 0d82561b11e1c2835f14ba5460cfff52f0087530 Mon Sep 17 00:00:00 > > > > > > > 2001 > > > > > > From: Yanfeng Liu <yfliu2008@qq.com> > > > > > > Date: Mon, 18 Nov 2024 08:03:15 +0800 > > > > > > Subject: [PATCH] riscv/gdb: share virt mode via priv register > > > > > > > > > > It seems like something strange happened when you submitted the patch. > > > > > Can you fix this up? > > > > > > > > > I prepared a patch file via the following steps: > > > > > > > > - Did `git format-patch --stdout -1 > /tmp/patch`, > > > > - Pasted the /tmp/patch content to email composer window, > > > > - Filled in email receipts, subject line and a few lines before the > > > > patch > > > > context in composer window. > > > > > > You should use `git send-email` instead, see the QEMU documentation > > > for more details: > > > https://www.qemu.org/docs/master/devel/submitting-a-patch.html#submitting-your-patches > > > > > > > > > > > I am wondering if the lines added before the formatted patch content in > > > > last > > > > step caused trouble? > > > > When resending email, should I use [patch v2] in subject line? I guess > > > > it is > > > > unnecessary as it is the same patch. > > > > > > Ideally I think a "virt" virtual register would be a better approach, > > > so if you could do that instead and send a v2 that would be great :) > > > > > > Alistair
On Thu, Nov 28, 2024 at 2:27 PM Yanfeng <yfliu2008@qq.com> wrote: > > On Thu, 2024-11-28 at 14:10 +1000, Alistair Francis wrote: > > On Thu, Nov 28, 2024 at 2:05 PM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > Alistair. > > > > > > My initial `git send-email" on Ubuntu 22.04 wasn't lukcy: > > > > > > ```shell > > > $ git send-email > > > git: 'send-email' is not a git command. See 'git --help'. > > > ``` > > > > > > I need time to find a solution. > > > > You will need to install git-send-email (or something like that) > > Yes, now the "git send-email" exists and I will see how to use it correctly. > > > > > > > > > As for seperate `virt` thing, my concern is that though the V and P status > > > looks > > > orthogonal, they are not so independent (e.g. `P=3` implies `V=0`). Having > > > them > > > in one `priv` register tells user that that they should be operated together > > > using one command. > > > > True, but how can a user know what bit 8 means? > > Good point. Can we mention it in the user document for `priv` register? AFAIK we don't really have a place to document that. Which is why a "virt" register would help as it's self documenting. Alistair > > I used bit(8) at my first try and it worked from GDB command line. Actually I > also tried bit(4) but found GDB command takes that bit as part of P value so I > withdrawed back to bit(8). > > >
On Thu, 2024-11-28 at 14:46 +1000, Alistair Francis wrote: > On Thu, Nov 28, 2024 at 2:27 PM Yanfeng <yfliu2008@qq.com> wrote: > > > > On Thu, 2024-11-28 at 14:10 +1000, Alistair Francis wrote: > > > On Thu, Nov 28, 2024 at 2:05 PM Yanfeng <yfliu2008@qq.com> wrote: > > > > > > > > Alistair. > > > > > > > > My initial `git send-email" on Ubuntu 22.04 wasn't lukcy: > > > > > > > > ```shell > > > > $ git send-email > > > > git: 'send-email' is not a git command. See 'git --help'. > > > > ``` > > > > > > > > I need time to find a solution. > > > > > > You will need to install git-send-email (or something like that) > > > > Yes, now the "git send-email" exists and I will see how to use it correctly. > > > > > > > > > > > > > As for seperate `virt` thing, my concern is that though the V and P > > > > status > > > > looks > > > > orthogonal, they are not so independent (e.g. `P=3` implies `V=0`). > > > > Having > > > > them > > > > in one `priv` register tells user that that they should be operated > > > > together > > > > using one command. > > > > > > True, but how can a user know what bit 8 means? > > > > Good point. Can we mention it in the user document for `priv` register? > > AFAIK we don't really have a place to document that. Which is why a > "virt" register would help as it's self documenting. Okay, I will try adding a `virt` virtual register in a [PATCH v2] later. > > Alistair > > > > > I used bit(8) at my first try and it worked from GDB command line. Actually > > I > > also tried bit(4) but found GDB command takes that bit as part of P value so > > I > > withdrawed back to bit(8). > > > >
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index 385a2c67c2..cc6dece51a 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -623,6 +623,10 @@ typedef enum { #define PRV_RESERVED 2 #define PRV_M 3 +/* Share virtualization mode as part of priv register */ +#define PRV_V (1 << 8) + + /* RV32 satp CSR field masks */ #define SATP32_MODE 0x80000000 #define SATP32_ASID 0x7fc00000 diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c index c07df972f1..d9e6ad969a 100644 --- a/target/riscv/gdbstub.c +++ b/target/riscv/gdbstub.c @@ -212,8 +212,14 @@ static int riscv_gdb_get_virtual(CPUState *cs, GByteArray *buf, int n) #else RISCVCPU *cpu = RISCV_CPU(cs); CPURISCVState *env = &cpu->env; + target_ulong ret = env->priv; - return gdb_get_regl(buf, env->priv); + /* include virtualization mode */ + + if (env->virt_enabled) { + ret |= PRV_V; + } + return gdb_get_regl(buf, ret); #endif }