* [Qemu-devel] [PATCH] Allow PIR read in privileged mode
@ 2018-05-07 13:48 luporl
2018-05-07 16:08 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
0 siblings, 1 reply; 8+ messages in thread
From: luporl @ 2018-05-07 13:48 UTC (permalink / raw)
To: qemu-devel; +Cc: luporl, David Gibson, Alexander Graf, qemu-ppc
According to PowerISA, the PIR register should be readable in privileged
mode also, not only in hypervisor privileged mode.
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Leandro Lupori <leandro.lupori@gmail.com>
Reviewed-by: Jose Ricardo Ziviani <joserz@linux.ibm.com>
---
target/ppc/translate_init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index a72be6d121..7b56e3ffb9 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -7816,7 +7816,7 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
/* Processor identification */
spr_register_hv(env, SPR_PIR, "PIR",
SPR_NOACCESS, SPR_NOACCESS,
- SPR_NOACCESS, SPR_NOACCESS,
+ &spr_read_generic, SPR_NOACCESS,
&spr_read_generic, NULL,
0x00000000);
spr_register_hv(env, SPR_HID0, "HID0",
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH] Allow PIR read in privileged mode
2018-05-07 13:48 [Qemu-devel] [PATCH] Allow PIR read in privileged mode luporl
@ 2018-05-07 16:08 ` Greg Kurz
2018-05-07 16:52 ` [Qemu-devel] [PATCH v3] target/ppc: " luporl
0 siblings, 1 reply; 8+ messages in thread
From: Greg Kurz @ 2018-05-07 16:08 UTC (permalink / raw)
To: luporl; +Cc: qemu-devel, qemu-ppc, David Gibson
Hi Leandro,
You seem to be a newcomer to QEMU development. Welcome ! :)
Please find a few remarks below, so that you can improve your patch submission
skills.
First, it is good practice to provide the subsystem name in the subject, as
stated in:
https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message
ie,
[PATCH] target/ppc: Allow PIR read in privileged mode
Then, this is your second shot for this patch, so you should have added:
- a version tag, as stated in:
https://wiki.qemu.org/Contribute/SubmitAPatch#When_resending_patches_add_a_version_tag
ie,
[PATCH v2] target/ppc: Allow PIR read in privileged mode
- a summary of changes since previous versions, as stated in:
https://wiki.qemu.org/Contribute/SubmitAPatch#Include_version_history_in_patchset_revisions
ie,
Signed-off-by: Leandro Lupori <leandro.lupori@gmail.com>
Reviewed-by: Jose Ricardo Ziviani <joserz@linux.ibm.com>
---
Changes in v2:
- added my Signed-off-by, maintainers CC and Jose's Reviewed-by tags
Pay attention that this summary of changes MUST be added below the '---',
because it is only relevant for the review process and we don't want to
record in the git changelog.
On Mon, 7 May 2018 10:48:06 -0300
luporl <leandro.lupori@gmail.com> wrote:
> According to PowerISA, the PIR register should be readable in privileged
> mode also, not only in hypervisor privileged mode.
>
True.
PowerISA 3.0 - 4.3.3 Processor Identification Register
"Read access to the PIR is privileged; write access is not provided."
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Leandro Lupori <leandro.lupori@gmail.com>
> Reviewed-by: Jose Ricardo Ziviani <joserz@linux.ibm.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> target/ppc/translate_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index a72be6d121..7b56e3ffb9 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -7816,7 +7816,7 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
> /* Processor identification */
> spr_register_hv(env, SPR_PIR, "PIR",
> SPR_NOACCESS, SPR_NOACCESS,
> - SPR_NOACCESS, SPR_NOACCESS,
> + &spr_read_generic, SPR_NOACCESS,
> &spr_read_generic, NULL,
> 0x00000000);
> spr_register_hv(env, SPR_HID0, "HID0",
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v3] target/ppc: Allow PIR read in privileged mode
2018-05-07 16:08 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
@ 2018-05-07 16:52 ` luporl
2018-06-04 0:53 ` David Gibson
0 siblings, 1 reply; 8+ messages in thread
From: luporl @ 2018-05-07 16:52 UTC (permalink / raw)
To: qemu-devel; +Cc: luporl, David Gibson, Alexander Graf, qemu-ppc
According to PowerISA, the PIR register should be readable in privileged
mode also, not only in hypervisor privileged mode.
PowerISA 3.0 - 4.3.3 Processor Identification Register
"Read access to the PIR is privileged; write access is not provided."
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Leandro Lupori <leandro.lupori@gmail.com>
Reviewed-by: Jose Ricardo Ziviani <joserz@linux.ibm.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
Changes in v2:
- added my Signed-off-by, maintainers CC and Jose's Reviewed-by tags
Changes in v3:
- added subsystem name, version tag and summary of changes
- added the section of PowerISA that describes PIR access privileges
target/ppc/translate_init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index a72be6d121..7b56e3ffb9 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -7816,7 +7816,7 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
/* Processor identification */
spr_register_hv(env, SPR_PIR, "PIR",
SPR_NOACCESS, SPR_NOACCESS,
- SPR_NOACCESS, SPR_NOACCESS,
+ &spr_read_generic, SPR_NOACCESS,
&spr_read_generic, NULL,
0x00000000);
spr_register_hv(env, SPR_HID0, "HID0",
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3] target/ppc: Allow PIR read in privileged mode
2018-05-07 16:52 ` [Qemu-devel] [PATCH v3] target/ppc: " luporl
@ 2018-06-04 0:53 ` David Gibson
2018-06-05 16:46 ` Greg Kurz
0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2018-06-04 0:53 UTC (permalink / raw)
To: luporl; +Cc: qemu-devel, Alexander Graf, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]
On Mon, May 07, 2018 at 01:52:42PM -0300, luporl wrote:
> According to PowerISA, the PIR register should be readable in privileged
> mode also, not only in hypervisor privileged mode.
>
> PowerISA 3.0 - 4.3.3 Processor Identification Register
>
> "Read access to the PIR is privileged; write access is not
> provided."
Yes... but a little further down it says "The PIR is a hypervisor
resource". Looking at the older 2.07 ISA, it says that
guest-supervisor mode reads to the PIR should be redirected to the
GPIR register, which this change won't accomplish.
So, I'm not sure what to make of this.
>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Leandro Lupori <leandro.lupori@gmail.com>
> Reviewed-by: Jose Ricardo Ziviani <joserz@linux.ibm.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
> Changes in v2:
> - added my Signed-off-by, maintainers CC and Jose's Reviewed-by tags
>
> Changes in v3:
> - added subsystem name, version tag and summary of changes
> - added the section of PowerISA that describes PIR access privileges
>
> target/ppc/translate_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index a72be6d121..7b56e3ffb9 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -7816,7 +7816,7 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
> /* Processor identification */
> spr_register_hv(env, SPR_PIR, "PIR",
> SPR_NOACCESS, SPR_NOACCESS,
> - SPR_NOACCESS, SPR_NOACCESS,
> + &spr_read_generic, SPR_NOACCESS,
> &spr_read_generic, NULL,
> 0x00000000);
> spr_register_hv(env, SPR_HID0, "HID0",
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3] target/ppc: Allow PIR read in privileged mode
2018-06-04 0:53 ` David Gibson
@ 2018-06-05 16:46 ` Greg Kurz
2018-06-06 0:53 ` David Gibson
0 siblings, 1 reply; 8+ messages in thread
From: Greg Kurz @ 2018-06-05 16:46 UTC (permalink / raw)
To: David Gibson; +Cc: luporl, qemu-ppc, qemu-devel, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 2390 bytes --]
On Mon, 4 Jun 2018 10:53:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, May 07, 2018 at 01:52:42PM -0300, luporl wrote:
> > According to PowerISA, the PIR register should be readable in privileged
> > mode also, not only in hypervisor privileged mode.
> >
> > PowerISA 3.0 - 4.3.3 Processor Identification Register
> >
> > "Read access to the PIR is privileged; write access is not
> > provided."
>
> Yes... but a little further down it says "The PIR is a hypervisor
> resource". Looking at the older 2.07 ISA, it says that
> guest-supervisor mode reads to the PIR should be redirected to the
> GPIR register, which this change won't accomplish.
>
Hmmm, there are two definitions for the PIR, one in Book III-S (4.3.3)
and one in Book III-E (5.3.3). It looks like you're referring to the
latter...
[Category:Embedded.Hypervisor]
Read accesses to the PIR in guest supervisor state are
mapped to the GPIR.
The Book III-S definition doesn't mention the GPIR.
> So, I'm not sure what to make of this.
>
> >
> > Cc: David Gibson <david@gibson.dropbear.id.au>
> > Cc: Alexander Graf <agraf@suse.de>
> > Cc: qemu-ppc@nongnu.org
> > Signed-off-by: Leandro Lupori <leandro.lupori@gmail.com>
> > Reviewed-by: Jose Ricardo Ziviani <joserz@linux.ibm.com>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > ---
> > Changes in v2:
> > - added my Signed-off-by, maintainers CC and Jose's Reviewed-by tags
> >
> > Changes in v3:
> > - added subsystem name, version tag and summary of changes
> > - added the section of PowerISA that describes PIR access privileges
> >
> > target/ppc/translate_init.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index a72be6d121..7b56e3ffb9 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -7816,7 +7816,7 @@ static void gen_spr_book3s_ids(CPUPPCState *env)
> > /* Processor identification */
> > spr_register_hv(env, SPR_PIR, "PIR",
> > SPR_NOACCESS, SPR_NOACCESS,
> > - SPR_NOACCESS, SPR_NOACCESS,
> > + &spr_read_generic, SPR_NOACCESS,
> > &spr_read_generic, NULL,
> > 0x00000000);
> > spr_register_hv(env, SPR_HID0, "HID0",
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3] target/ppc: Allow PIR read in privileged mode
2018-06-05 16:46 ` Greg Kurz
@ 2018-06-06 0:53 ` David Gibson
2018-06-06 9:19 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2018-06-06 0:53 UTC (permalink / raw)
To: Greg Kurz; +Cc: luporl, qemu-ppc, qemu-devel, Alexander Graf
[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]
On Tue, Jun 05, 2018 at 06:46:12PM +0200, Greg Kurz wrote:
> On Mon, 4 Jun 2018 10:53:22 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Mon, May 07, 2018 at 01:52:42PM -0300, luporl wrote:
> > > According to PowerISA, the PIR register should be readable in privileged
> > > mode also, not only in hypervisor privileged mode.
> > >
> > > PowerISA 3.0 - 4.3.3 Processor Identification Register
> > >
> > > "Read access to the PIR is privileged; write access is not
> > > provided."
> >
> > Yes... but a little further down it says "The PIR is a hypervisor
> > resource". Looking at the older 2.07 ISA, it says that
> > guest-supervisor mode reads to the PIR should be redirected to the
> > GPIR register, which this change won't accomplish.
> >
>
> Hmmm, there are two definitions for the PIR, one in Book III-S (4.3.3)
> and one in Book III-E (5.3.3). It looks like you're referring to the
> latter...
>
> [Category:Embedded.Hypervisor]
> Read accesses to the PIR in guest supervisor state are
> mapped to the GPIR.
>
> The Book III-S definition doesn't mention the GPIR.
Oops, sorry. Yes the GPIR stuff is only for BookE. The statement
about the PIR being a hypervisor resource is definitely in the BookS
section, however (both 2.07 and 3.0).
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] target/ppc: Allow PIR read in privileged mode
2018-06-06 0:53 ` David Gibson
@ 2018-06-06 9:19 ` Greg Kurz
2018-06-08 9:20 ` David Gibson
0 siblings, 1 reply; 8+ messages in thread
From: Greg Kurz @ 2018-06-06 9:19 UTC (permalink / raw)
To: David Gibson; +Cc: luporl, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]
On Wed, 6 Jun 2018 10:53:17 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Jun 05, 2018 at 06:46:12PM +0200, Greg Kurz wrote:
> > On Mon, 4 Jun 2018 10:53:22 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Mon, May 07, 2018 at 01:52:42PM -0300, luporl wrote:
> > > > According to PowerISA, the PIR register should be readable in privileged
> > > > mode also, not only in hypervisor privileged mode.
> > > >
> > > > PowerISA 3.0 - 4.3.3 Processor Identification Register
> > > >
> > > > "Read access to the PIR is privileged; write access is not
> > > > provided."
> > >
> > > Yes... but a little further down it says "The PIR is a hypervisor
> > > resource". Looking at the older 2.07 ISA, it says that
> > > guest-supervisor mode reads to the PIR should be redirected to the
> > > GPIR register, which this change won't accomplish.
> > >
> >
> > Hmmm, there are two definitions for the PIR, one in Book III-S (4.3.3)
> > and one in Book III-E (5.3.3). It looks like you're referring to the
> > latter...
> >
> > [Category:Embedded.Hypervisor]
> > Read accesses to the PIR in guest supervisor state are
> > mapped to the GPIR.
> >
> > The Book III-S definition doesn't mention the GPIR.
>
> Oops, sorry. Yes the GPIR stuff is only for BookE. The statement
> about the PIR being a hypervisor resource is definitely in the BookS
> section, however (both 2.07 and 3.0).
>
Yes it is, but IIUC, this means that the guest cannot modify it, eg,
do mtspr. Section 4.4.4 in Book III-S has a list of SPRs that seem to
indicate that mfspr doesn't require hypervisor state with the PIR.
FWIW, this can be verified with xmon in a KVM guest:
0:mon> S
...
srr0 = c0000000000cd06c srr1 = 8000000000001033 dsisr = 00000000
dscr = 0000000000000000 ppr = 0010000000000000 pir = 00000020
...
0:mon> Sr 3ff
SPR 0x3ff (1023) = 0x20
but with TCG xmon hits a program check:
0:mon> S
...
srr0 = c0000000000ef204 srr1 = 8000000000041033 dsisr = 40000000
cpu 0x0: Vector: 700 (Program Check) at [c00000003ffdf510]
...
cpu 0x0: Exception 700 (Program Check) in xmon, returning to main loop
...
0:mon> Sr 3ff
SPR 0x3ff (1023) Faulted during read
This patch makes xmon happy under TCG.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] target/ppc: Allow PIR read in privileged mode
2018-06-06 9:19 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
@ 2018-06-08 9:20 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2018-06-08 9:20 UTC (permalink / raw)
To: Greg Kurz; +Cc: luporl, qemu-ppc, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]
On Wed, Jun 06, 2018 at 11:19:22AM +0200, Greg Kurz wrote:
> On Wed, 6 Jun 2018 10:53:17 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Tue, Jun 05, 2018 at 06:46:12PM +0200, Greg Kurz wrote:
> > > On Mon, 4 Jun 2018 10:53:22 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > On Mon, May 07, 2018 at 01:52:42PM -0300, luporl wrote:
> > > > > According to PowerISA, the PIR register should be readable in privileged
> > > > > mode also, not only in hypervisor privileged mode.
> > > > >
> > > > > PowerISA 3.0 - 4.3.3 Processor Identification Register
> > > > >
> > > > > "Read access to the PIR is privileged; write access is not
> > > > > provided."
> > > >
> > > > Yes... but a little further down it says "The PIR is a hypervisor
> > > > resource". Looking at the older 2.07 ISA, it says that
> > > > guest-supervisor mode reads to the PIR should be redirected to the
> > > > GPIR register, which this change won't accomplish.
> > > >
> > >
> > > Hmmm, there are two definitions for the PIR, one in Book III-S (4.3.3)
> > > and one in Book III-E (5.3.3). It looks like you're referring to the
> > > latter...
> > >
> > > [Category:Embedded.Hypervisor]
> > > Read accesses to the PIR in guest supervisor state are
> > > mapped to the GPIR.
> > >
> > > The Book III-S definition doesn't mention the GPIR.
> >
> > Oops, sorry. Yes the GPIR stuff is only for BookE. The statement
> > about the PIR being a hypervisor resource is definitely in the BookS
> > section, however (both 2.07 and 3.0).
> >
>
> Yes it is, but IIUC, this means that the guest cannot modify it, eg,
> do mtspr. Section 4.4.4 in Book III-S has a list of SPRs that seem to
> indicate that mfspr doesn't require hypervisor state with the PIR.
Ah, yes, I was looking for a summary that covered that, but hadn't
found it yet.
The patch doesn't actually apply clean to the current tree any more,
due to a rename. So can you repost, and I'll apply.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-06-08 9:20 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-07 13:48 [Qemu-devel] [PATCH] Allow PIR read in privileged mode luporl
2018-05-07 16:08 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2018-05-07 16:52 ` [Qemu-devel] [PATCH v3] target/ppc: " luporl
2018-06-04 0:53 ` David Gibson
2018-06-05 16:46 ` Greg Kurz
2018-06-06 0:53 ` David Gibson
2018-06-06 9:19 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2018-06-08 9:20 ` David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.