From: Alexandru Elisei <hidden> Date: 2021-09-27 12:49:49
What sparked these two small patches is the series that fixed the PMU reset
values and their visibility from userspace, more specifically the
discussion around the patch that removed the PMSWINC_EL0 shadow register
[1].
The patches are straightforward cleanups without any changes in
functionality.
Tested on a rockpro64, by running kvm-unit-tests under qemu.
[1] https://www.spinics.net/lists/kvm-arm/msg47976.html
Alexandru Elisei (2):
KVM: arm64: Return early from read_id_reg() if register is RAZ
KVM: arm64: Use get_raz_reg() for userspace reads of PMSWINC_EL0
arch/arm64/kvm/sys_regs.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
--
2.33.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Alexandru Elisei <hidden> Date: 2021-09-27 12:49:50
If read_id_reg() is called for an ID register which is Read-As-Zero
(RAZ), it initializes the return value to zero, then goes through a list
of registers which require special handling.
By not returning as soon as it tests if the register is RAZ, it creates
the opportunity for bugs, if a patch changes a register to RAZ (like has
happened with PMSWINC_EL0 in commit 11663111cd49), but doesn't remove the
special handling from read_id_reg(); or if a register is RAZ in certain
situations, and readable in others.
Return early as to make it impossible for a RAZ register to be anything
other than zero.
Signed-off-by: Alexandru Elisei <redacted>
---
arch/arm64/kvm/sys_regs.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Alexandru Elisei <hidden> Date: 2021-09-27 12:50:00
PMSWINC_EL0 is a write-only register and was initially part of the VCPU
register state, but was later removed in commit 7a3ba3095a32 ("KVM:
arm64: Remove PMSWINC_EL0 shadow register"). To prevent regressions, the
register was kept accessible from userspace as Read-As-Zero (RAZ).
The read function that is used to handle userspace reads of this
register is get_raz_id_reg(), which, while technically correct, as it
returns 0, it is not semantically correct, as PMSWINC_EL0 is not an ID
register as the function name suggests.
Add a new function, get_raz_reg(), to use it as the accessor for
PMSWINC_EL0, as to not conflate get_raz_id_reg() to handle other types
of registers.
No functional change intended.
Signed-off-by: Alexandru Elisei <redacted>
---
arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
From: Andrew Jones <hidden> Date: 2021-09-30 13:29:25
On Mon, Sep 27, 2021 at 01:49:10PM +0100, Alexandru Elisei wrote:
quoted hunk
If read_id_reg() is called for an ID register which is Read-As-Zero
(RAZ), it initializes the return value to zero, then goes through a list
of registers which require special handling.
By not returning as soon as it tests if the register is RAZ, it creates
the opportunity for bugs, if a patch changes a register to RAZ (like has
happened with PMSWINC_EL0 in commit 11663111cd49), but doesn't remove the
special handling from read_id_reg(); or if a register is RAZ in certain
situations, and readable in others.
Return early as to make it impossible for a RAZ register to be anything
other than zero.
Signed-off-by: Alexandru Elisei <redacted>
---
arch/arm64/kvm/sys_regs.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Andrew Jones <hidden> Date: 2021-09-30 13:31:43
On Mon, Sep 27, 2021 at 01:49:11PM +0100, Alexandru Elisei wrote:
quoted hunk
PMSWINC_EL0 is a write-only register and was initially part of the VCPU
register state, but was later removed in commit 7a3ba3095a32 ("KVM:
arm64: Remove PMSWINC_EL0 shadow register"). To prevent regressions, the
register was kept accessible from userspace as Read-As-Zero (RAZ).
The read function that is used to handle userspace reads of this
register is get_raz_id_reg(), which, while technically correct, as it
returns 0, it is not semantically correct, as PMSWINC_EL0 is not an ID
register as the function name suggests.
Add a new function, get_raz_reg(), to use it as the accessor for
PMSWINC_EL0, as to not conflate get_raz_id_reg() to handle other types
of registers.
No functional change intended.
Signed-off-by: Alexandru Elisei <redacted>
---
arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
What about replacing get_raz_id_reg() with this new function? Do really need
both?
Thanks,
drew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Alexandru Elisei <hidden> Date: 2021-10-06 14:49:55
Hi Drew,
Thank you for the review!
On Thu, Sep 30, 2021 at 03:29:15PM +0200, Andrew Jones wrote:
On Mon, Sep 27, 2021 at 01:49:11PM +0100, Alexandru Elisei wrote:
quoted
PMSWINC_EL0 is a write-only register and was initially part of the VCPU
register state, but was later removed in commit 7a3ba3095a32 ("KVM:
arm64: Remove PMSWINC_EL0 shadow register"). To prevent regressions, the
register was kept accessible from userspace as Read-As-Zero (RAZ).
The read function that is used to handle userspace reads of this
register is get_raz_id_reg(), which, while technically correct, as it
returns 0, it is not semantically correct, as PMSWINC_EL0 is not an ID
register as the function name suggests.
Add a new function, get_raz_reg(), to use it as the accessor for
PMSWINC_EL0, as to not conflate get_raz_id_reg() to handle other types
of registers.
No functional change intended.
Signed-off-by: Alexandru Elisei <redacted>
---
arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
What about replacing get_raz_id_reg() with this new function? Do really need
both?
I thought about that when writing this patch. I ultimately decided against it
because changing the get_user accessor to be get_raz_reg() instead of
get_raz_id_reg() would break the symmetry with set_user, which needs to stay
set_raz_id_reg(), and cannot be substituted with set_wi_reg() because that would
be a change in behaviour (set_raz_id_reg() checks that val == 0, set_wi_reg()
doesn't).
I do agree that get_raz_id_reg() does the exact same thing as get_raz_reg(), but
in a more roundabout manner. So if you still feel that I should use
get_raz_reg() instead, I'll do that for the next iteration of the series. What
do you think?
Thanks,
Alex
From: Andrew Jones <hidden> Date: 2021-10-06 15:25:10
On Wed, Oct 06, 2021 at 03:49:19PM +0100, Alexandru Elisei wrote:
Hi Drew,
Thank you for the review!
On Thu, Sep 30, 2021 at 03:29:15PM +0200, Andrew Jones wrote:
quoted
On Mon, Sep 27, 2021 at 01:49:11PM +0100, Alexandru Elisei wrote:
quoted
PMSWINC_EL0 is a write-only register and was initially part of the VCPU
register state, but was later removed in commit 7a3ba3095a32 ("KVM:
arm64: Remove PMSWINC_EL0 shadow register"). To prevent regressions, the
register was kept accessible from userspace as Read-As-Zero (RAZ).
The read function that is used to handle userspace reads of this
register is get_raz_id_reg(), which, while technically correct, as it
returns 0, it is not semantically correct, as PMSWINC_EL0 is not an ID
register as the function name suggests.
Add a new function, get_raz_reg(), to use it as the accessor for
PMSWINC_EL0, as to not conflate get_raz_id_reg() to handle other types
of registers.
No functional change intended.
Signed-off-by: Alexandru Elisei <redacted>
---
arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
What about replacing get_raz_id_reg() with this new function? Do really need
both?
I thought about that when writing this patch. I ultimately decided against it
because changing the get_user accessor to be get_raz_reg() instead of
get_raz_id_reg() would break the symmetry with set_user, which needs to stay
set_raz_id_reg(), and cannot be substituted with set_wi_reg() because that would
be a change in behaviour (set_raz_id_reg() checks that val == 0, set_wi_reg()
doesn't).
I do agree that get_raz_id_reg() does the exact same thing as get_raz_reg(), but
in a more roundabout manner. So if you still feel that I should use
get_raz_reg() instead, I'll do that for the next iteration of the series. What
do you think?
I'd prefer we avoid maintaining two implementations of the same
functionality. If we want to keep the symmetry with set_raz_id_reg,
then we could implement get_raz_id_reg as 'return get_raz_reg()'.
Thanks,
drew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Alexandru Elisei <hidden> Date: 2021-10-06 15:36:13
Hi Drew,
On Wed, Oct 06, 2021 at 05:23:02PM +0200, Andrew Jones wrote:
On Wed, Oct 06, 2021 at 03:49:19PM +0100, Alexandru Elisei wrote:
quoted
Hi Drew,
Thank you for the review!
On Thu, Sep 30, 2021 at 03:29:15PM +0200, Andrew Jones wrote:
quoted
On Mon, Sep 27, 2021 at 01:49:11PM +0100, Alexandru Elisei wrote:
quoted
PMSWINC_EL0 is a write-only register and was initially part of the VCPU
register state, but was later removed in commit 7a3ba3095a32 ("KVM:
arm64: Remove PMSWINC_EL0 shadow register"). To prevent regressions, the
register was kept accessible from userspace as Read-As-Zero (RAZ).
The read function that is used to handle userspace reads of this
register is get_raz_id_reg(), which, while technically correct, as it
returns 0, it is not semantically correct, as PMSWINC_EL0 is not an ID
register as the function name suggests.
Add a new function, get_raz_reg(), to use it as the accessor for
PMSWINC_EL0, as to not conflate get_raz_id_reg() to handle other types
of registers.
No functional change intended.
Signed-off-by: Alexandru Elisei <redacted>
---
arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
What about replacing get_raz_id_reg() with this new function? Do really need
both?
I thought about that when writing this patch. I ultimately decided against it
because changing the get_user accessor to be get_raz_reg() instead of
get_raz_id_reg() would break the symmetry with set_user, which needs to stay
set_raz_id_reg(), and cannot be substituted with set_wi_reg() because that would
be a change in behaviour (set_raz_id_reg() checks that val == 0, set_wi_reg()
doesn't).
I do agree that get_raz_id_reg() does the exact same thing as get_raz_reg(), but
in a more roundabout manner. So if you still feel that I should use
get_raz_reg() instead, I'll do that for the next iteration of the series. What
do you think?
I'd prefer we avoid maintaining two implementations of the same
functionality. If we want to keep the symmetry with set_raz_id_reg,
then we could implement get_raz_id_reg as 'return get_raz_reg()'.
Agreed, I'll replace get_raz_id_reg() with get_raz_reg().
Thanks,
Alex
From: Marc Zyngier <maz@kernel.org> Date: 2021-10-11 10:00:58
Hi Alexandru,
On Mon, 27 Sep 2021 13:49:09 +0100,
Alexandru Elisei [off-list ref] wrote:
What sparked these two small patches is the series that fixed the PMU reset
values and their visibility from userspace, more specifically the
discussion around the patch that removed the PMSWINC_EL0 shadow register
[1].
The patches are straightforward cleanups without any changes in
functionality.
Tested on a rockpro64, by running kvm-unit-tests under qemu.
[1] https://www.spinics.net/lists/kvm-arm/msg47976.html
Alexandru Elisei (2):
KVM: arm64: Return early from read_id_reg() if register is RAZ
KVM: arm64: Use get_raz_reg() for userspace reads of PMSWINC_EL0
arch/arm64/kvm/sys_regs.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
If you are going to respin this one, now would be the time! :-)
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel