[Qemu-devel] [PATCH] Allow PIR read in privileged mode

[Qemu-devel] [PATCH] Allow PIR read in privileged mode

[Leandro]

According to PowerISA, the PIR register should be readable in privileged
mode also, not only in hypervisor privileged mode.

The following patch fixes this:

--- qemu-2.12.0-original/target/ppc/translate_init.c    2018-04-24
13:30:47.000000000 -0300
+++ qemu-2.12.0/target/ppc/translate_init.c     2018-04-25
16:40:04.248844415 -0300
@@ -7811,7 +7811,7 @@
     /* 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",

Re: [Qemu-devel] [PATCH] Allow PIR read in privileged mode

[joserz]

> According to PowerISA, the PIR register should be readable in privileged mode also, not only in hypervisor privileged mode.

The code makes sense but it doesn't seem created by git format-patch. If
so, could you please follow the instructions from
https://wiki.qemu.org/Contribute/SubmitAPatch and resend it? (note you
need to cc the maintainers)

In the next version you can include my review.

Reviewed-by: Jose Ricardo Ziviani <joserz@linux.ibm.com>

Thanks

[Qemu-devel] [PATCH] Allow PIR read in privileged mode

[luporl]

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

Re: [Qemu-devel] [Qemu-ppc] [PATCH] Allow PIR read in privileged mode

[Greg Kurz]

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",

[Qemu-devel] [PATCH v3] target/ppc: Allow PIR read in privileged mode

[luporl]

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

Re: [Qemu-devel] [PATCH v3] target/ppc: Allow PIR read in privileged mode

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [PATCH v3] target/ppc: Allow PIR read in privileged mode

[Greg Kurz]

[-- 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 --]

Re: [Qemu-devel] [PATCH v3] target/ppc: Allow PIR read in privileged mode

[David Gibson]

[-- 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 --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] target/ppc: Allow PIR read in privileged mode

[Greg Kurz]

[-- 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 --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3] target/ppc: Allow PIR read in privileged mode

[David Gibson]

[-- 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 --]