From: Greg Kurz <groug@kaod.org>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: qemu-ppc@nongnu.org, "Cédric Le Goater" <clg@kaod.org>,
"Nicholas Piggin" <npiggin@gmail.com>,
qemu-devel@nongnu.org
Subject: Re: [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub
Date: Mon, 11 May 2020 11:30:58 +0200 [thread overview]
Message-ID: <20200511113058.6032a285@bahia.lan> (raw)
In-Reply-To: <20200511014348.GN2183@umbus.fritz.box>
[-- Attachment #1: Type: text/plain, Size: 7763 bytes --]
On Mon, 11 May 2020 11:43:48 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Thu, May 07, 2020 at 07:27:15PM +0200, Greg Kurz wrote:
> > gdbstub shouldn't silently change guest visible state when doing address
> > translation. While here drop a not very useful comment.
> >
> > This was found while reading the code. I could verify that this affects
> > both powernv and pseries, but I failed to observe any actual bug.
> >
> > Fixes: d04ea940c597 "target/ppc: Add support for Radix partition-scoped translation"
> > Signed-off-by: Greg Kurz <groug@kaod.org>
>
> It's a real fix. But AFAICT we'll always have cause_excp ==
> cause_rc_update, and I can't see any reason we'd ever them different.
This is definitely true as of today because all memory accesses are
performed by a CPU, but POWER9 has accelerator agents (eg. NPU) that
can also issue load/store operations on the PowerBus.
I'm currently doing some experiments to model the NPU as used with
OpenCAPI (the ultimate goal being to have another user for XIVE).
This requires to be able to do EA->RA translation without a CPU
context, as done by the NestMMU in real HW. This requires quite
some code refactoring in mmu-radix64.c and I opted to keep these
flags separate as a first step... but you're right, since page
faults are always handled on behalf of a CPU, I don't see any
reason for them to be different.
Cc'ing Nick in case I've missed something.
> So I'd prefer to just rename the flag and use it for both tests.
>
> Maybe just 'guest_visible' ?
>
Sounds good.
> > ---
> > target/ppc/mmu-radix64.c | 36 ++++++++++++++++++++++++------------
> > 1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> > index ceeb3dfe2d49..bc51cd89a079 100644
> > --- a/target/ppc/mmu-radix64.c
> > +++ b/target/ppc/mmu-radix64.c
> > @@ -270,7 +270,8 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx,
> > ppc_v3_pate_t pate,
> > hwaddr *h_raddr, int *h_prot,
> > int *h_page_size, bool pde_addr,
> > - bool cause_excp)
> > + bool cause_excp,
> > + bool cause_rc_update)
> > {
> > int fault_cause = 0;
> > hwaddr pte_addr;
> > @@ -291,8 +292,9 @@ static int ppc_radix64_partition_scoped_xlate(PowerPCCPU *cpu, int rwx,
> > return 1;
> > }
> >
> > - /* Update Reference and Change Bits */
> > - ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot);
> > + if (cause_rc_update) {
> > + ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, h_prot);
> > + }
> >
> > return 0;
> > }
> > @@ -301,7 +303,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
> > vaddr eaddr, uint64_t pid,
> > ppc_v3_pate_t pate, hwaddr *g_raddr,
> > int *g_prot, int *g_page_size,
> > - bool cause_excp)
> > + bool cause_excp,
> > + bool cause_rc_update)
> > {
> > CPUState *cs = CPU(cpu);
> > CPUPPCState *env = &cpu->env;
> > @@ -336,7 +339,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
> > ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, prtbe_addr,
> > pate, &h_raddr, &h_prot,
> > &h_page_size, true,
> > - cause_excp);
> > + cause_excp,
> > + cause_rc_update);
> > if (ret) {
> > return ret;
> > }
> > @@ -376,7 +380,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
> > ret = ppc_radix64_partition_scoped_xlate(cpu, 0, eaddr, pte_addr,
> > pate, &h_raddr, &h_prot,
> > &h_page_size, true,
> > - cause_excp);
> > + cause_excp,
> > + cause_rc_update);
> > if (ret) {
> > return ret;
> > }
> > @@ -408,7 +413,9 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
> > return 1;
> > }
> >
> > - ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
> > + if (cause_rc_update) {
> > + ppc_radix64_set_rc(cpu, rwx, pte, pte_addr, g_prot);
> > + }
> >
> > return 0;
> > }
> > @@ -433,7 +440,8 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu, int rwx,
> > static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> > bool relocation,
> > hwaddr *raddr, int *psizep, int *protp,
> > - bool cause_excp)
> > + bool cause_excp,
> > + bool cause_rc_update)
> > {
> > CPUPPCState *env = &cpu->env;
> > uint64_t lpid, pid;
> > @@ -483,7 +491,9 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> > if (relocation) {
> > int ret = ppc_radix64_process_scoped_xlate(cpu, rwx, eaddr, pid,
> > pate, &g_raddr, &prot,
> > - &psize, cause_excp);
> > + &psize,
> > + cause_excp,
> > + cause_rc_update);
> > if (ret) {
> > return ret;
> > }
> > @@ -506,7 +516,9 @@ static int ppc_radix64_xlate(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> >
> > ret = ppc_radix64_partition_scoped_xlate(cpu, rwx, eaddr, g_raddr,
> > pate, raddr, &prot, &psize,
> > - 0, cause_excp);
> > + 0,
> > + cause_excp,
> > + cause_rc_update);
> > if (ret) {
> > return ret;
> > }
> > @@ -562,7 +574,7 @@ int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> >
> > /* Translate eaddr to raddr (where raddr is addr qemu needs for access) */
> > if (ppc_radix64_xlate(cpu, eaddr, rwx, relocation, &raddr,
> > - &page_size, &prot, true)) {
> > + &page_size, &prot, true, true)) {
> > return 1;
> > }
> >
> > @@ -584,7 +596,7 @@ hwaddr ppc_radix64_get_phys_page_debug(PowerPCCPU *cpu, target_ulong eaddr)
> > }
> >
> > if (ppc_radix64_xlate(cpu, eaddr, 0, msr_dr, &raddr, &psize,
> > - &prot, false)) {
> > + &prot, false, false)) {
> > return -1;
> > }
> >
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-05-11 9:31 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-07 17:26 [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 Greg Kurz
2020-05-07 17:26 ` [PATCH 1/6] target/ppc: Pass const pointer to ppc_radix64_get_prot_amr() Greg Kurz
2020-05-07 17:26 ` [PATCH 2/6] target/ppc: Pass const pointer to ppc_radix64_get_fully_qualified_addr() Greg Kurz
2020-05-07 17:26 ` [PATCH 3/6] target/ppc: Don't initialize some local variables in ppc_radix64_xlate() Greg Kurz
2020-05-11 9:07 ` Cédric Le Goater
2020-05-11 10:12 ` Greg Kurz
2020-05-07 17:27 ` [PATCH 4/6] target/ppc: Add missing braces in ppc_radix64_partition_scoped_xlate() Greg Kurz
2020-05-07 17:27 ` [PATCH 5/6] target/ppc: Fix arguments to ppc_radix64_partition_scoped_xlate() Greg Kurz
2020-05-07 17:27 ` [PATCH 6/6] target/ppc: Don't update radix PTE R/C bits with gdbstub Greg Kurz
2020-05-11 1:43 ` David Gibson
2020-05-11 9:30 ` Greg Kurz [this message]
2020-05-11 1:44 ` [PATCH 0/6] target/ppc: Various clean-up and fixes for radix64 David Gibson
2020-05-11 16:55 ` Greg Kurz
2020-05-12 0:00 ` David Gibson
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=20200511113058.6032a285@bahia.lan \
--to=groug@kaod.org \
--cc=clg@kaod.org \
--cc=david@gibson.dropbear.id.au \
--cc=npiggin@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/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 NNTP newsgroup(s).