* Re: [Qemu-devel] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses
2018-03-15 13:34 [Qemu-devel] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses Peter Maydell
@ 2018-03-22 12:51 ` Peter Maydell
2018-03-22 14:23 ` Andrew Jones
2018-03-23 2:23 ` [Qemu-devel] " Philippe Mathieu-Daudé
2 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2018-03-22 12:51 UTC (permalink / raw)
To: qemu-arm, QEMU Developers; +Cc: patches
Ping for code review -- it would be nice to put this bugfix
into rc1.
thanks
-- PMM
On 15 March 2018 at 13:34, Peter Maydell <peter.maydell@linaro.org> wrote:
> If the GIC has the security extension support enabled, then a
> non-secure access to ICC_PMR must take account of the non-secure
> view of interrupt priorities, where real priorities 0..0x7f
> are secure-only and not visible to the non-secure guest, and
> priorities 0x80..0xff are shown to the guest as if they were
> 0x00..0xff. We had the logic here wrong:
> * on reads, the priority is in the secure range if bit 7
> is clear, not if it is set
> * on writes, we want to set bit 7, not mask everything else
>
> Our ICC_RPR read code had the same error as ICC_PMR.
>
> (Compare the GICv3 spec pseudocode functions ICC_RPR_EL1
> and ICC_PMR_EL1.)
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1748434
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_cpuif.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 5cbafaf497..26f5eeda94 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -836,7 +836,7 @@ static uint64_t icc_pmr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> /* NS access and Group 0 is inaccessible to NS: return the
> * NS view of the current priority
> */
> - if (value & 0x80) {
> + if ((value & 0x80) == 0) {
> /* Secure priorities not visible to NS */
> value = 0;
> } else if (value != 0xff) {
> @@ -871,7 +871,7 @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> /* Current PMR in the secure range, don't allow NS to change it */
> return;
> }
> - value = (value >> 1) & 0x80;
> + value = (value >> 1) | 0x80;
> }
> cs->icc_pmr_el1 = value;
> gicv3_cpuif_update(cs);
> @@ -1609,7 +1609,7 @@ static uint64_t icc_rpr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> if (arm_feature(env, ARM_FEATURE_EL3) &&
> !arm_is_secure(env) && (env->cp15.scr_el3 & SCR_FIQ)) {
> /* NS GIC access and Group 0 is inaccessible to NS */
> - if (prio & 0x80) {
> + if ((prio & 0x80) == 0) {
> /* NS mustn't see priorities in the Secure half of the range */
> prio = 0;
> } else if (prio != 0xff) {
> --
> 2.16.2
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses
2018-03-15 13:34 [Qemu-devel] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses Peter Maydell
2018-03-22 12:51 ` Peter Maydell
@ 2018-03-22 14:23 ` Andrew Jones
2018-03-22 18:29 ` Peter Maydell
2018-03-23 2:23 ` [Qemu-devel] " Philippe Mathieu-Daudé
2 siblings, 1 reply; 8+ messages in thread
From: Andrew Jones @ 2018-03-22 14:23 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, qemu-devel, patches
On Thu, Mar 15, 2018 at 01:34:41PM +0000, Peter Maydell wrote:
> If the GIC has the security extension support enabled, then a
> non-secure access to ICC_PMR must take account of the non-secure
> view of interrupt priorities, where real priorities 0..0x7f
> are secure-only and not visible to the non-secure guest, and
> priorities 0x80..0xff are shown to the guest as if they were
> 0x00..0xff. We had the logic here wrong:
0x00..0x7f
> * on reads, the priority is in the secure range if bit 7
> is clear, not if it is set
> * on writes, we want to set bit 7, not mask everything else
>
> Our ICC_RPR read code had the same error as ICC_PMR.
>
> (Compare the GICv3 spec pseudocode functions ICC_RPR_EL1
> and ICC_PMR_EL1.)
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1748434
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/intc/arm_gicv3_cpuif.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 5cbafaf497..26f5eeda94 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -836,7 +836,7 @@ static uint64_t icc_pmr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> /* NS access and Group 0 is inaccessible to NS: return the
> * NS view of the current priority
> */
> - if (value & 0x80) {
> + if ((value & 0x80) == 0) {
> /* Secure priorities not visible to NS */
> value = 0;
> } else if (value != 0xff) {
> @@ -871,7 +871,7 @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> /* Current PMR in the secure range, don't allow NS to change it */
> return;
> }
> - value = (value >> 1) & 0x80;
> + value = (value >> 1) | 0x80;
> }
> cs->icc_pmr_el1 = value;
> gicv3_cpuif_update(cs);
> @@ -1609,7 +1609,7 @@ static uint64_t icc_rpr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> if (arm_feature(env, ARM_FEATURE_EL3) &&
> !arm_is_secure(env) && (env->cp15.scr_el3 & SCR_FIQ)) {
> /* NS GIC access and Group 0 is inaccessible to NS */
> - if (prio & 0x80) {
> + if ((prio & 0x80) == 0) {
> /* NS mustn't see priorities in the Secure half of the range */
> prio = 0;
> } else if (prio != 0xff) {
> --
> 2.16.2
>
>
Reviewed-by: Andrew Jones <drjones@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses
2018-03-22 14:23 ` Andrew Jones
@ 2018-03-22 18:29 ` Peter Maydell
2018-03-22 20:42 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2018-03-22 18:29 UTC (permalink / raw)
To: Andrew Jones; +Cc: qemu-arm, QEMU Developers, patches
On 22 March 2018 at 14:23, Andrew Jones <drjones@redhat.com> wrote:
> On Thu, Mar 15, 2018 at 01:34:41PM +0000, Peter Maydell wrote:
>> If the GIC has the security extension support enabled, then a
>> non-secure access to ICC_PMR must take account of the non-secure
>> view of interrupt priorities, where real priorities 0..0x7f
>> are secure-only and not visible to the non-secure guest, and
>> priorities 0x80..0xff are shown to the guest as if they were
>> 0x00..0xff. We had the logic here wrong:
>
> 0x00..0x7f
I think 0x00..0xff is correct. The conversion from actual
priority value to the NS-view is
if (prio & 0x80 == 0) {
nsview = 0;
} else {
nsview = (prio << 1) & 0xff;
}
so:
real priority NS view
0x80 0x00
0x90 0x20
0xa0 0x40
0xb0 0x60
0xc0 0x80
0xd0 0xa0
0xe0 0xc0
0xf0 0xe0
the NS view covers the whole 0x00..0xff range, but more sparsely.
(OK, technically you can't ever read 0xff, only 0xfe.)
> Reviewed-by: Andrew Jones <drjones@redhat.com>
Thanks for the review.
-- PMM
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses
2018-03-22 18:29 ` Peter Maydell
@ 2018-03-22 20:42 ` Philippe Mathieu-Daudé
2018-03-23 9:45 ` Peter Maydell
2018-03-23 10:08 ` Andrew Jones
0 siblings, 2 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-03-22 20:42 UTC (permalink / raw)
To: Peter Maydell, Andrew Jones; +Cc: qemu-arm, QEMU Developers, patches
Hi Peter,
On 03/22/2018 03:29 PM, Peter Maydell wrote:
> On 22 March 2018 at 14:23, Andrew Jones <drjones@redhat.com> wrote:
>> On Thu, Mar 15, 2018 at 01:34:41PM +0000, Peter Maydell wrote:
>>> If the GIC has the security extension support enabled, then a
>>> non-secure access to ICC_PMR must take account of the non-secure
>>> view of interrupt priorities, where real priorities 0..0x7f
>>> are secure-only and not visible to the non-secure guest, and
>>> priorities 0x80..0xff are shown to the guest as if they were
>>> 0x00..0xff. We had the logic here wrong:
>>
>> 0x00..0x7f
>
> I think 0x00..0xff is correct.
I guess Andrew only suggested to correct the hex prefix in your comment:
- ... where real priorities 0..0x7f
+ ... where real priorities 0x00..0x7f
> The conversion from actual
> priority value to the NS-view is
> if (prio & 0x80 == 0) {
> nsview = 0;
> } else {
> nsview = (prio << 1) & 0xff;
> }
>
> so:
> real priority NS view
> 0x80 0x00
> 0x90 0x20
> 0xa0 0x40
> 0xb0 0x60
> 0xc0 0x80
> 0xd0 0xa0
> 0xe0 0xc0
> 0xf0 0xe0
>
> the NS view covers the whole 0x00..0xff range, but more sparsely.
> (OK, technically you can't ever read 0xff, only 0xfe.)
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses
2018-03-22 20:42 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
@ 2018-03-23 9:45 ` Peter Maydell
2018-03-23 10:08 ` Andrew Jones
1 sibling, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2018-03-23 9:45 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Andrew Jones, qemu-arm, QEMU Developers, patches
On 22 March 2018 at 20:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 03/22/2018 03:29 PM, Peter Maydell wrote:
>> On 22 March 2018 at 14:23, Andrew Jones <drjones@redhat.com> wrote:
>>> On Thu, Mar 15, 2018 at 01:34:41PM +0000, Peter Maydell wrote:
>>>> If the GIC has the security extension support enabled, then a
>>>> non-secure access to ICC_PMR must take account of the non-secure
>>>> view of interrupt priorities, where real priorities 0..0x7f
>>>> are secure-only and not visible to the non-secure guest, and
>>>> priorities 0x80..0xff are shown to the guest as if they were
>>>> 0x00..0xff. We had the logic here wrong:
>>>
>>> 0x00..0x7f
>>
>> I think 0x00..0xff is correct.
>
> I guess Andrew only suggested to correct the hex prefix in your comment:
> - ... where real priorities 0..0x7f
> + ... where real priorities 0x00..0x7f
Oh, right. Yes, can do that. I was confused by the suggested correction
being directly under "0x00..0xff" in my commit message.
thanks
-- PMM
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses
2018-03-22 20:42 ` [Qemu-devel] [Qemu-arm] " Philippe Mathieu-Daudé
2018-03-23 9:45 ` Peter Maydell
@ 2018-03-23 10:08 ` Andrew Jones
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Jones @ 2018-03-23 10:08 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, qemu-arm, QEMU Developers, patches
On Thu, Mar 22, 2018 at 05:42:02PM -0300, Philippe Mathieu-Daudé wrote:
> Hi Peter,
>
> On 03/22/2018 03:29 PM, Peter Maydell wrote:
> > On 22 March 2018 at 14:23, Andrew Jones <drjones@redhat.com> wrote:
> >> On Thu, Mar 15, 2018 at 01:34:41PM +0000, Peter Maydell wrote:
> >>> If the GIC has the security extension support enabled, then a
> >>> non-secure access to ICC_PMR must take account of the non-secure
> >>> view of interrupt priorities, where real priorities 0..0x7f
> >>> are secure-only and not visible to the non-secure guest, and
> >>> priorities 0x80..0xff are shown to the guest as if they were
> >>> 0x00..0xff. We had the logic here wrong:
> >>
> >> 0x00..0x7f
> >
> > I think 0x00..0xff is correct.
>
> I guess Andrew only suggested to correct the hex prefix in your comment:
> - ... where real priorities 0..0x7f
> + ... where real priorities 0x00..0x7f
Actually I just hadn't thought that through properly. But thanks for
giving me the benefit of the doubt :-)
> >
> > the NS view covers the whole 0x00..0xff range, but more sparsely.
> > (OK, technically you can't ever read 0xff, only 0xfe.)
>
You can read 0xff, because 0xff will be written when either 0xfe or 0xff
are written. Then, when reading, priority == 0xff will fail the != 0xff
test (which is QEMU's implementation of the pseudo code where PRIMask()
is hardcoded to 0xff). In that case it'll just return the priority value
unchanged, which is 0xff.
Thanks,
drew
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses
2018-03-15 13:34 [Qemu-devel] [PATCH for-2.12] hw/intc/arm_gicv3: Fix secure-GIC NS ICC_PMR and ICC_RPR accesses Peter Maydell
2018-03-22 12:51 ` Peter Maydell
2018-03-22 14:23 ` Andrew Jones
@ 2018-03-23 2:23 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-03-23 2:23 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel; +Cc: patches
On 03/15/2018 10:34 AM, Peter Maydell wrote:
> If the GIC has the security extension support enabled, then a
> non-secure access to ICC_PMR must take account of the non-secure
> view of interrupt priorities, where real priorities 0..0x7f
> are secure-only and not visible to the non-secure guest, and
> priorities 0x80..0xff are shown to the guest as if they were
> 0x00..0xff. We had the logic here wrong:
> * on reads, the priority is in the secure range if bit 7
> is clear, not if it is set
> * on writes, we want to set bit 7, not mask everything else
>
> Our ICC_RPR read code had the same error as ICC_PMR.
>
> (Compare the GICv3 spec pseudocode functions ICC_RPR_EL1
> and ICC_PMR_EL1.)
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1748434
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/intc/arm_gicv3_cpuif.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index 5cbafaf497..26f5eeda94 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -836,7 +836,7 @@ static uint64_t icc_pmr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> /* NS access and Group 0 is inaccessible to NS: return the
> * NS view of the current priority
> */
> - if (value & 0x80) {
> + if ((value & 0x80) == 0) {
> /* Secure priorities not visible to NS */
> value = 0;
> } else if (value != 0xff) {
> @@ -871,7 +871,7 @@ static void icc_pmr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> /* Current PMR in the secure range, don't allow NS to change it */
> return;
> }
> - value = (value >> 1) & 0x80;
> + value = (value >> 1) | 0x80;
> }
> cs->icc_pmr_el1 = value;
> gicv3_cpuif_update(cs);
> @@ -1609,7 +1609,7 @@ static uint64_t icc_rpr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> if (arm_feature(env, ARM_FEATURE_EL3) &&
> !arm_is_secure(env) && (env->cp15.scr_el3 & SCR_FIQ)) {
> /* NS GIC access and Group 0 is inaccessible to NS */
> - if (prio & 0x80) {
> + if ((prio & 0x80) == 0) {
> /* NS mustn't see priorities in the Secure half of the range */
> prio = 0;
> } else if (prio != 0xff) {
>
^ permalink raw reply [flat|nested] 8+ messages in thread