Re: [PATCH 2/6] amd_iommu: Fix Device ID decoding for INVALIDATE_IOTLB_PAGES command

[Vasant Hegde <vasant.hegde@amd.com>]

Alejandro,

On 3/11/2025 8:54 PM, Alejandro Jimenez wrote:
> The DeviceID bits are extracted using an incorrect offset in the call to
> amdvi_iotlb_remove_page(). This field is read (correctly) earlier, so use
> the value already retrieved for devid.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: d29a09ca6842 ("hw/i386: Introduce AMD IOMMU")
> Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

Fix looks good.

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

FYI. I think we need to reconsider IOTLB invalidation stuff. Its suppose to
invalidate the device side TLB, not IOMMU one. Before that we need to fix the
way we generate hash it. Ideally we should switch to domain ID and fix the size
as well. We are looking into this.

-Vasant


> ---
>  hw/i386/amd_iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 5b21cf134a..068eeb0cae 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -508,7 +508,7 @@ static void amdvi_inval_inttable(AMDVIState *s, uint64_t *cmd)
>  static void iommu_inval_iotlb(AMDVIState *s, uint64_t *cmd)
>  {
>  
> -    uint16_t devid = extract64(cmd[0], 0, 16);
> +    uint16_t devid = cpu_to_le16(extract64(cmd[0], 0, 16));
>      if (extract64(cmd[1], 1, 1) || extract64(cmd[1], 3, 1) ||
>          extract64(cmd[1], 6, 6)) {
>          amdvi_log_illegalcom_error(s, extract64(cmd[0], 60, 4),
> @@ -521,7 +521,7 @@ static void iommu_inval_iotlb(AMDVIState *s, uint64_t *cmd)
>                                      &devid);
>      } else {
>          amdvi_iotlb_remove_page(s, cpu_to_le64(extract64(cmd[1], 12, 52)) << 12,
> -                                cpu_to_le16(extract64(cmd[1], 0, 16)));
> +                                devid);
>      }
>      trace_amdvi_iotlb_inval();
>  }