hw/arm/tegra241-cmdqv: Do not sync GERRORN on VCMDQ allocation

[PATCH] hw/arm/tegra241-cmdqv: Do not sync GERRORN on VCMDQ allocation

Posted by Shameer Kolothum 2 weeks, 4 days ago

When a guest hits a command error, the hardware sets GERROR and the guest
acknowledges it by writing GERRORN to match. These registers use toggle
semantics, and since GERROR is read-only, both GERROR and GERRORN may
remain set after an error has been handled. As long as they stay equal
there is no pending error, so a subsequent VM is unaffected.

When a new VM starts, QEMU allocates the hardware VCMDQ and flushes its
cached GERRORN of zero, while the hardware may still have both GERROR and
GERRORN set from an error the previous guest acknowledged. This makes them
differ again, so the hardware sees a pending error and stall the VCMDQ,
leading to CMD_SYNC timeouts.

Do not sync GERRORN when allocating a hardware VCMDQ.

Fixes: 1d0f77535417 ("hw/arm/tegra241-cmdqv: Route allocated VCMDQ Page0 accesses to the mmap'd host VINTF page0")
Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 hw/arm/tegra241-cmdqv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
index 29c488e0e4..fe88bec41f 100644
--- a/hw/arm/tegra241-cmdqv.c
+++ b/hw/arm/tegra241-cmdqv.c
@@ -212,7 +212,9 @@ static inline uint32_t *tegra241_cmdqv_vintf_lvcmdq_ptr(Tegra241CMDQV *cmdqv,
 /*
  * Flush cached register writes into the mmap'd host VINTF page0 after a
  * successful HW_QUEUE_ALLOC, so the guest's earlier writes survive
- * the cache-to-hardware transition.
+ * the cache-to-hardware transition. GERRORN is intentionally not synced,
+ * as overwriting it with the cached value could recreate a GERROR != GERRORN
+ * mismatch and stall the VCMDQ.
  */
 static void tegra241_cmdqv_sync_vcmdq(Tegra241CMDQV *cmdqv, int index)
 {
@@ -229,9 +231,6 @@ static void tegra241_cmdqv_sync_vcmdq(Tegra241CMDQV *cmdqv, int index)
 
     ptr = tegra241_cmdqv_vintf_lvcmdq_ptr(cmdqv, index, A_VCMDQ0_CONFIG);
     *ptr = cmdqv->vcmdq_config[index];
-
-    ptr = tegra241_cmdqv_vintf_lvcmdq_ptr(cmdqv, index, A_VCMDQ0_GERRORN);
-    *ptr = cmdqv->vcmdq_gerrorn[index];
 }
 
 /*
-- 
2.43.0

Re: [PATCH] hw/arm/tegra241-cmdqv: Do not sync GERRORN on VCMDQ allocation

Posted by Eric Auger 2 weeks, 2 days ago

Hi Shameer,

On 7/7/26 1:24 PM, Shameer Kolothum wrote:
> When a guest hits a command error, the hardware sets GERROR and the guest
> acknowledges it by writing GERRORN to match. These registers use toggle
> semantics, and since GERROR is read-only, both GERROR and GERRORN may
> remain set after an error has been handled. As long as they stay equal
> there is no pending error, so a subsequent VM is unaffected.
>
> When a new VM starts, QEMU allocates the hardware VCMDQ and flushes its
> cached GERRORN of zero, while the hardware may still have both GERROR and
> GERRORN set from an error the previous guest acknowledged. This makes them
> differ again, so the hardware sees a pending error and stall the VCMDQ,
> leading to CMD_SYNC timeouts.
>
> Do not sync GERRORN when allocating a hardware VCMDQ.

One question: I see cmdqv->vcmdq_gerror[index] is likely to be set in
tegra241_cmdqv_setup_vcmdq(). Don't we have a risk to silence one
pre-existing error?

Thanks

Eric

>
> Fixes: 1d0f77535417 ("hw/arm/tegra241-cmdqv: Route allocated VCMDQ Page0 accesses to the mmap'd host VINTF page0")
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> ---
>  hw/arm/tegra241-cmdqv.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
> index 29c488e0e4..fe88bec41f 100644
> --- a/hw/arm/tegra241-cmdqv.c
> +++ b/hw/arm/tegra241-cmdqv.c
> @@ -212,7 +212,9 @@ static inline uint32_t *tegra241_cmdqv_vintf_lvcmdq_ptr(Tegra241CMDQV *cmdqv,
>  /*
>   * Flush cached register writes into the mmap'd host VINTF page0 after a
>   * successful HW_QUEUE_ALLOC, so the guest's earlier writes survive
> - * the cache-to-hardware transition.
> + * the cache-to-hardware transition. GERRORN is intentionally not synced,
> + * as overwriting it with the cached value could recreate a GERROR != GERRORN
> + * mismatch and stall the VCMDQ.
>   */
>  static void tegra241_cmdqv_sync_vcmdq(Tegra241CMDQV *cmdqv, int index)
>  {
> @@ -229,9 +231,6 @@ static void tegra241_cmdqv_sync_vcmdq(Tegra241CMDQV *cmdqv, int index)
>  
>      ptr = tegra241_cmdqv_vintf_lvcmdq_ptr(cmdqv, index, A_VCMDQ0_CONFIG);
>      *ptr = cmdqv->vcmdq_config[index];
> -
> -    ptr = tegra241_cmdqv_vintf_lvcmdq_ptr(cmdqv, index, A_VCMDQ0_GERRORN);
> -    *ptr = cmdqv->vcmdq_gerrorn[index];
>  }
>  
>  /*

RE: [PATCH] hw/arm/tegra241-cmdqv: Do not sync GERRORN on VCMDQ allocation

Posted by Shameer Kolothum Thodi 2 weeks, 2 days ago

Hi Eric,

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 09 July 2026 15:51
> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>; qemu-
> arm@nongnu.org; qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; Nicolin Chen <nicolinc@nvidia.com>; Nathan
> Chen <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>; Jiandi An
> <jan@nvidia.com>
> Subject: Re: [PATCH] hw/arm/tegra241-cmdqv: Do not sync GERRORN on
> VCMDQ allocation
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Shameer,
> 
> On 7/7/26 1:24 PM, Shameer Kolothum wrote:
> > When a guest hits a command error, the hardware sets GERROR and the
> guest
> > acknowledges it by writing GERRORN to match. These registers use toggle
> > semantics, and since GERROR is read-only, both GERROR and GERRORN may
> > remain set after an error has been handled. As long as they stay equal
> > there is no pending error, so a subsequent VM is unaffected.
> >
> > When a new VM starts, QEMU allocates the hardware VCMDQ and flushes
> its
> > cached GERRORN of zero, while the hardware may still have both GERROR
> and
> > GERRORN set from an error the previous guest acknowledged. This makes
> them
> > differ again, so the hardware sees a pending error and stall the VCMDQ,
> > leading to CMD_SYNC timeouts.
> >
> > Do not sync GERRORN when allocating a hardware VCMDQ.
> 
> One question: I see cmdqv->vcmdq_gerror[index] is likely to be set in
> tegra241_cmdqv_setup_vcmdq(). Don't we have a risk to silence one
> pre-existing error?

Hmm..don't think so.

vcmdq_gerror[index] is set only to report CMDQ_INIT_ERR when hardware
queue allocation fails, meaning no hardware VCMDQ exists at that point.
It is intentionally cleared after a successful allocation because the
allocation error no longer applies.

Once the hardware VCMDQ is allocated, GERROR and GERRORN accesses are
backed directly by hardware. Since QEMU no longer overwrites GERRORN,
any pending hardware error where GERROR and GERRORN differ remains
visible to the guest.

I can’t think of any relevant corner case issue here, though I may be missing
something.

Thanks,
Shameer

Re: [PATCH] hw/arm/tegra241-cmdqv: Do not sync GERRORN on VCMDQ allocation

Posted by Eric Auger 2 weeks, 2 days ago


On 7/9/26 5:15 PM, Shameer Kolothum Thodi wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: 09 July 2026 15:51
>> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>; qemu-
>> arm@nongnu.org; qemu-devel@nongnu.org
>> Cc: peter.maydell@linaro.org; Nicolin Chen <nicolinc@nvidia.com>; Nathan
>> Chen <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>; Jiandi An
>> <jan@nvidia.com>
>> Subject: Re: [PATCH] hw/arm/tegra241-cmdqv: Do not sync GERRORN on
>> VCMDQ allocation
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hi Shameer,
>>
>> On 7/7/26 1:24 PM, Shameer Kolothum wrote:
>>> When a guest hits a command error, the hardware sets GERROR and the
>> guest
>>> acknowledges it by writing GERRORN to match. These registers use toggle
>>> semantics, and since GERROR is read-only, both GERROR and GERRORN may
>>> remain set after an error has been handled. As long as they stay equal
>>> there is no pending error, so a subsequent VM is unaffected.
>>>
>>> When a new VM starts, QEMU allocates the hardware VCMDQ and flushes
>> its
>>> cached GERRORN of zero, while the hardware may still have both GERROR
>> and
>>> GERRORN set from an error the previous guest acknowledged. This makes
>> them
>>> differ again, so the hardware sees a pending error and stall the VCMDQ,
>>> leading to CMD_SYNC timeouts.
>>>
>>> Do not sync GERRORN when allocating a hardware VCMDQ.
>> One question: I see cmdqv->vcmdq_gerror[index] is likely to be set in
>> tegra241_cmdqv_setup_vcmdq(). Don't we have a risk to silence one
>> pre-existing error?
> Hmm..don't think so.
>
> vcmdq_gerror[index] is set only to report CMDQ_INIT_ERR when hardware
> queue allocation fails, meaning no hardware VCMDQ exists at that point.
> It is intentionally cleared after a successful allocation because the
> allocation error no longer applies.

OK. thanks for checking. Effectively we return and do not sync in that
case. there is no other cache sync call site so that's OK.

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
>
> Once the hardware VCMDQ is allocated, GERROR and GERRORN accesses are
> backed directly by hardware. Since QEMU no longer overwrites GERRORN,
> any pending hardware error where GERROR and GERRORN differ remains
> visible to the guest.
>
> I can’t think of any relevant corner case issue here, though I may be missing
> something.

Sounds good.

Eric


>
> Thanks,
> Shameer

Re: [PATCH] hw/arm/tegra241-cmdqv: Do not sync GERRORN on VCMDQ allocation

Posted by Peter Maydell 2 weeks, 2 days ago

On Tue, 7 Jul 2026 at 12:25, Shameer Kolothum <skolothumtho@nvidia.com> wrote:
>
> When a guest hits a command error, the hardware sets GERROR and the guest
> acknowledges it by writing GERRORN to match. These registers use toggle
> semantics, and since GERROR is read-only, both GERROR and GERRORN may
> remain set after an error has been handled. As long as they stay equal
> there is no pending error, so a subsequent VM is unaffected.
>
> When a new VM starts, QEMU allocates the hardware VCMDQ and flushes its
> cached GERRORN of zero, while the hardware may still have both GERROR and
> GERRORN set from an error the previous guest acknowledged. This makes them
> differ again, so the hardware sees a pending error and stall the VCMDQ,
> leading to CMD_SYNC timeouts.
>
> Do not sync GERRORN when allocating a hardware VCMDQ.
>
> Fixes: 1d0f77535417 ("hw/arm/tegra241-cmdqv: Route allocated VCMDQ Page0 accesses to the mmap'd host VINTF page0")
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>



Applied to target-arm.next, thanks.

-- PMM

Re: [PATCH] hw/arm/tegra241-cmdqv: Do not sync GERRORN on VCMDQ allocation

Posted by Nicolin Chen 2 weeks, 4 days ago

On Tue, Jul 07, 2026 at 12:24:50PM +0100, Shameer Kolothum wrote:
> When a guest hits a command error, the hardware sets GERROR and the guest
> acknowledges it by writing GERRORN to match. These registers use toggle
> semantics, and since GERROR is read-only, both GERROR and GERRORN may
> remain set after an error has been handled. As long as they stay equal
> there is no pending error, so a subsequent VM is unaffected.
> 
> When a new VM starts, QEMU allocates the hardware VCMDQ and flushes its
> cached GERRORN of zero, while the hardware may still have both GERROR and
> GERRORN set from an error the previous guest acknowledged. This makes them
> differ again, so the hardware sees a pending error and stall the VCMDQ,
> leading to CMD_SYNC timeouts.
> 
> Do not sync GERRORN when allocating a hardware VCMDQ.
> 
> Fixes: 1d0f77535417 ("hw/arm/tegra241-cmdqv: Route allocated VCMDQ Page0 accesses to the mmap'd host VINTF page0")
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
 
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Tested-by: Nicolin Chen <nicolinc@nvidia.com>