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

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