Re: [PATCH] x86/msi: always propagate MSI writes when not in active system mode

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Mar 18, 2025 at 09:29:45AM +0100, Roger Pau Monne wrote:
> Relax the limitation on MSI register writes, and only apply it when the
> system is in active state.  For example AMD IOMMU drivers rely on using
> set_msi_affinity() to force an MSI register write on resume from
> suspension.
> 
> The original patch intention was to reduce the number of MSI register
> writes when the system is in active state.  Leave the other states to
> always perform the writes, as it's safer given the existing code, and it's
> expected to not make a difference performance wise.
> 
> For such propagation to work even when the IRT index is not updated the MSI
> message must be adjusted in all success cases for AMD IOMMU, not just when
> the index has been newly allocated.
> 
> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Fixes: ('8e60d47cf011 x86/iommu: avoid MSI address and data writes if IRT 
> index hasn't changed')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Tested-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

> ---
>  xen/arch/x86/msi.c                       | 9 +++++++++
>  xen/drivers/passthrough/amd/iommu_intr.c | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 163ccf874720..8bb3bb18af61 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -189,6 +189,15 @@ static int write_msi_msg(struct msi_desc *entry, struct 
> msi_msg *msg,
>  {
>      entry->msg = *msg;
>  
> +    if ( unlikely(system_state != SYS_STATE_active) )
> +        /*
> +         * Always propagate writes when not in the 'active' state.  The
> +         * optimization to avoid the MSI address and data registers write is
> +         * only relevant for runtime state, and drivers on resume (at least)
> +         * rely on set_msi_affinity() to update the hardware state.
> +         */
> +        force = true;
> +
>      if ( iommu_intremap != iommu_intremap_off )
>      {
>          int rc;
> diff --git a/xen/drivers/passthrough/amd/iommu_intr.c 
> b/xen/drivers/passthrough/amd/iommu_intr.c
> index 9abdc38053d7..08766122b421 100644
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -546,7 +546,7 @@ int cf_check amd_iommu_msi_msg_update_ire(
>      rc = update_intremap_entry_from_msi_msg(iommu, bdf, nr,
>                                              &msi_desc->remap_index,
>                                              msg, &data);
> -    if ( rc > 0 )
> +    if ( rc >= 0 )
>      {
>          for ( i = 1; i < nr; ++i )
>              msi_desc[i].remap_index = msi_desc->remap_index + i;
> -- 
> 2.48.1
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature