linux-kernel - Re: [PATCH 1/2] iommu/amd: Add HATDis feature support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <m5ageqedj5otmfo4yuld4es72esfmlc7vb5htolj5pcffurjl5@mi5xemcd4fgi>
Date: Mon, 12 May 2025 06:30:16 +0000
From: Ankit Soni <Ankit.Soni@....com>
To: Joao Martins <joao.m.martins@...cle.com>
CC: <iommu@...ts.linux.dev>, <suravee.suthikulpanit@....com>,
	<joro@...tes.org>, <will@...nel.org>, <robin.murphy@....com>,
	<linux-kernel@...r.kernel.org>, Alejandro Jimenez
	<alejandro.j.jimenez@...cle.com>, David Woodhouse <dwmw2@...radead.org>,
	<vasant.hegde@....com>
Subject: Re: [PATCH 1/2] iommu/amd: Add HATDis feature support

Hi,

On Thu, May 08, 2025 at 06:03:44PM +0100, Joao Martins wrote:
> On 06/05/2025 06:12, Ankit Soni wrote:
> > On Wed, Apr 30, 2025 at 12:41:04PM +0100, Joao Martins wrote:
> >>> With intel patch you mentioned above, it seems that it is mostly handling 
> >>> "second stage translation support" disable, which will eventually disable dma 
> >>> translation. And in AMD case, HATDis bit indicates host(v1) translation is not
> >>> available, then attempt to use guest(v2) translation, and if both page
> >>> table modes are not available then disable dma tranlation.
> >>
> >> OK, I guess it makes sense if HATDis is v1 only.
> >>
> >> My other call out was that when we disable dma-translation all together (aka
> >> both modes), then we shouldn't advertise the IOMMU groups (internally and to
> >> userspace) by not calling iommu_device_register()/iommu_device_sysfs_add().
> >>
> > 
> > Sorry for the late reply. I had cross-checked it; if the probe fails,
> > then IOMMU groups will not be populated, and eventually, it will not
> > have significance for calling iommu_device_register()/iommu_device_sysfs_add().
> > 
> 
> It would nonetheless populate a ivhd entry in sysfs needlessly but with an empty
> devices list (qemu diff at the tail end for how I checked it; it's only missing
> the ILLEGAL_DEVICE_TABLE_ENTRY event being generated, but enough to check the
> first patch with sw iommu) e.g. as far as I checked:
> 
> $ find /sys | grep ivhd
> /sys/class/iommu/ivhd0
> /sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0
> /sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/uevent
> /sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/amd-iommu
> /sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/amd-iommu/cap
> /sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/amd-iommu/features
> /sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/devices
> /sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/device
> /sys/devices/pci0000:00/0000:00:05.0/iommu/ivhd0/subsystem
> 

I was assuming, since iommu is still active for interrupt remapping,
user may need info for cap and feature using /sys fs.
@vasant: can you please suggest on this?

> -->8--
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 59593582be23..78801672ee3d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2326,7 +2326,11 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker,
> const char *oem_id,
>      /* IOMMU info */
>      build_append_int_noprefix(table_data, 0, 2);
>      /* IOMMU Attributes */
> -    build_append_int_noprefix(table_data, 0, 4);
> +    if (!s->dma_translation) {
> +        build_append_int_noprefix(table_data, (1UL << 0) /* HATDis */, 4);
> +    } else {
> +        build_append_int_noprefix(table_data, 0, 4);
> +    }
>      /* EFR Register Image */
>      build_append_int_noprefix(table_data,
>                                amdvi_extended_feature_register(s),
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 34abb61e092e..d033d309a210 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -83,6 +83,9 @@ uint64_t amdvi_extended_feature_register(AMDVIState *s)
>      if (s->xtsup) {
>          feature |= AMDVI_FEATURE_XT;
>      }
> +    if (!s->dma_translation) {
> +        feature |= AMDVI_HATS_MODE_RESERVED;
> +    }
> 
>      return feature;
>  }
> @@ -1665,6 +1668,7 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error
> **errp)
>  }
> 
>  static Property amdvi_properties[] = {
> +    DEFINE_PROP_BOOL("dma-translation", AMDVIState, dma_translation, true),
>      DEFINE_PROP_BOOL("xtsup", AMDVIState, xtsup, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> index e0dac4d9a96c..b5358dc30533 100644
> --- a/hw/i386/amd_iommu.h
> +++ b/hw/i386/amd_iommu.h
> @@ -169,6 +169,7 @@
>  /* AMDVI paging mode */
>  #define AMDVI_GATS_MODE                 (2ULL <<  12)
>  #define AMDVI_HATS_MODE                 (2ULL <<  10)
> +#define AMDVI_HATS_MODE_RESERVED        (2ULL <<  11)
> 
>  /* IOTLB */
>  #define AMDVI_IOTLB_MAX_SIZE 1024
> @@ -371,6 +372,7 @@ struct AMDVIState {
>      /* Interrupt remapping */
>      bool ga_enabled;
>      bool xtsup;
> +    bool dma_translation;
>  };
> 
>  uint64_t amdvi_extended_feature_register(AMDVIState *s);

Thank you for QEMU patch, i also had similar patch ready, i was waiting
for kernel patch to be submitted.

- Ankit