| Message ID | 20240929043937.242769-2-jiaqingtong97@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic: fix GICR_STATUSR in vgic_v3_rd_registers | expand |
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.
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
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.
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),