KVM: arm64: vgic: fix GICR_STATUSR in vgic_v3_rd_registers

[PATCH] KVM: arm64: vgic: fix GICR_STATUSR in vgic_v3_rd_registers

Posted by jiaqingtong97@gmail.com 1 year, 2 months ago

From: Jia Qingtong <jiaqingtong@huawei.com>

vgic_uaccess use bsearch search regs in vgic_io_device.regions, but the
GICR_STATUSR have wrong order in vgic_v3_rd_registers.
When check all vgic_register_region, it turned out that only
vgic_v3_rd_registers has this problem.

It's harmless since vgic_uaccess behaves as RAZ&WI when it can't find the
specified reg. This is exactly the same as the behavior of the GICR_STATUSR
register.

So just move GICR_STATUSR to the right place.

Signed-off-by: Jia Qingtong <jiaqingtong@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 9e50928f5d7d..822b4c1a01dc 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -651,9 +651,6 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GICR_CTLR,
 		vgic_mmio_read_v3r_ctlr, vgic_mmio_write_v3r_ctlr, 4,
 		VGIC_ACCESS_32bit),
-	REGISTER_DESC_WITH_LENGTH(GICR_STATUSR,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
-		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICR_IIDR,
 		vgic_mmio_read_v3r_iidr, vgic_mmio_write_wi, 4,
 		VGIC_ACCESS_32bit),
@@ -661,6 +658,9 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
 		vgic_mmio_read_v3r_typer, vgic_mmio_write_wi,
 		NULL, vgic_mmio_uaccess_write_wi, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH(GICR_STATUSR,
+		vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
+		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
 		vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
 		VGIC_ACCESS_32bit),

base-commit: 17a0005644994087794f6552d7a5e105d6976184
-- 
2.46.0

Re: [PATCH] KVM: arm64: vgic: fix GICR_STATUSR in vgic_v3_rd_registers

Posted by Marc Zyngier 1 year, 2 months ago

On Sun, 29 Sep 2024 05:39:35 +0100,
jiaqingtong97@gmail.com wrote:
> 
> From: Jia Qingtong <jiaqingtong@huawei.com>
> 
> vgic_uaccess use bsearch search regs in vgic_io_device.regions, but the
> GICR_STATUSR have wrong order in vgic_v3_rd_registers.
> When check all vgic_register_region, it turned out that only
> vgic_v3_rd_registers has this problem.
> 
> It's harmless since vgic_uaccess behaves as RAZ&WI when it can't find the
> specified reg. This is exactly the same as the behavior of the GICR_STATUSR
> register.
>
> So just move GICR_STATUSR to the right place.

That looks correct, but I think we should have some code that ensures
that these tables are correct at boot time, just like we're doing for
the system registers. Or completely remove our reliance on bsearch().

Another thing is that GICD_STATUSR looks pretty wrong. It is handled
as RAO, but we never clear any "error" (it is WI). This has been buggy
since GICv3 save/restore was added, 7 years ago.

Do you mind spinning a series fixing this up?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: arm64: vgic: fix GICR_STATUSR in vgic_v3_rd_registers

Posted by Jia Qingtong 1 year, 2 months ago

On 29 Sep 10:38, Marc Zyngier wrote:
> On Sun, 29 Sep 2024 05:39:35 +0100,
> jiaqingtong97@gmail.com wrote:
> > 
> > From: Jia Qingtong <jiaqingtong@huawei.com>
> > 
> > vgic_uaccess use bsearch search regs in vgic_io_device.regions, but the
> > GICR_STATUSR have wrong order in vgic_v3_rd_registers.
> > When check all vgic_register_region, it turned out that only
> > vgic_v3_rd_registers has this problem.
> > 
> > It's harmless since vgic_uaccess behaves as RAZ&WI when it can't find the
> > specified reg. This is exactly the same as the behavior of the GICR_STATUSR
> > register.
> >
> > So just move GICR_STATUSR to the right place.
> 
> That looks correct, but I think we should have some code that ensures
> that these tables are correct at boot time, just like we're doing for
> the system registers. Or completely remove our reliance on bsearch().
> 
struct vgic_register_region was defined in vgic-{its,mmio-v2,mmio-v3},
do you think it's appropriate to extern and check tables's item order in
vgic-init.c's kvm_vgic_hyp_init?.

> Another thing is that GICD_STATUSR looks pretty wrong. It is handled
> as RAO, but we never clear any "error" (it is WI). This has been buggy
> since GICv3 save/restore was added, 7 years ago.
> 
Let's change it to RAZ? We will implement the complete logic when someone
really needs this feature.

> Do you mind spinning a series fixing this up?
> 
Sure. 

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

--
Thanks,
Qingtong

Re: [PATCH] KVM: arm64: vgic: fix GICR_STATUSR in vgic_v3_rd_registers

Posted by Marc Zyngier 1 year, 2 months ago

On Mon, 30 Sep 2024 07:20:35 +0100,
Jia Qingtong <jiaqingtong97@gmail.com> wrote:
> 
> On 29 Sep 10:38, Marc Zyngier wrote:
> > On Sun, 29 Sep 2024 05:39:35 +0100,
> > jiaqingtong97@gmail.com wrote:
> > > 
> > > From: Jia Qingtong <jiaqingtong@huawei.com>
> > > 
> > > vgic_uaccess use bsearch search regs in vgic_io_device.regions, but the
> > > GICR_STATUSR have wrong order in vgic_v3_rd_registers.
> > > When check all vgic_register_region, it turned out that only
> > > vgic_v3_rd_registers has this problem.
> > > 
> > > It's harmless since vgic_uaccess behaves as RAZ&WI when it can't find the
> > > specified reg. This is exactly the same as the behavior of the GICR_STATUSR
> > > register.
> > >
> > > So just move GICR_STATUSR to the right place.
> > 
> > That looks correct, but I think we should have some code that ensures
> > that these tables are correct at boot time, just like we're doing for
> > the system registers. Or completely remove our reliance on bsearch().
> > 
> struct vgic_register_region was defined in vgic-{its,mmio-v2,mmio-v3},
> do you think it's appropriate to extern and check tables's item order in
> vgic-init.c's kvm_vgic_hyp_init?.

I'd rather we have local functions performing the check, one in each
of the GIC "modules", calling a global helper taking a pointer to the
register array as a parameter.

> 
> > Another thing is that GICD_STATUSR looks pretty wrong. It is handled
> > as RAO, but we never clear any "error" (it is WI). This has been buggy
> > since GICv3 save/restore was added, 7 years ago.
> > 
> Let's change it to RAZ? We will implement the complete logic when someone
> really needs this feature.

Exactly. Which is probably *never*.

> 
> > Do you mind spinning a series fixing this up?
> > 
> Sure. 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.