On 5/15/23 03:44, Michael S. Tsirkin wrote:
On Sun, May 14, 2023 at 03:55:11PM +0700, Bui Quang Minh wrote:
On 5/12/23 21:39, Michael S. Tsirkin wrote:
On Tue, Apr 11, 2023 at 09:24:40PM +0700, Bui Quang Minh wrote:
This commit adds XTSup configuration to let user choose to whether enable
this feature or not. When XTSup is enabled, additional bytes in IRTE with
enabled guest virtual VAPIC are used to support 32-bit destination id.

Additionally, this commit changes to use IVHD type 0x11 in ACPI table for
feature report to operating system. This is because Linux does not use
XTSup in IOMMU Feature Reporting field of IVHD type 0x10 but only use XTSup
bit in EFR Register Image of IVHD 0x11 to indicate x2APIC support (see
init_iommu_one in linux/drivers/iommu/amd/init.c)

Signed-off-by: Bui Quang Minh <minhquangbu...@gmail.com>

I'm concerned that switching to type 11 will break some older guests.
It would be better if we could export both type 10 and type 11
ivhd. A question however would be how does this interact
with older guests. For example:
https://lists.linuxfoundation.org/pipermail/iommu/2016-January/015310.html
it looks like linux before 2016 only expected one ivhd entry?

Export both type 0x10 and 0x11 looks reasonable to me. Before the above
commit, I see that Linux still loops through multiple ivhd but only handles
one with type 0x10. On newer kernel, it will choose to handle the type that
appears last corresponding the first devid, which is weird in my opinion.
+static u8 get_highest_supported_ivhd_type(struct acpi_table_header *ivrs)
+{
+       u8 *base = (u8 *)ivrs;
+       struct ivhd_header *ivhd = (struct ivhd_header *)
+                                       (base + IVRS_HEADER_LENGTH);
+       u8 last_type = ivhd->type;
+       u16 devid = ivhd->devid;
+
+       while (((u8 *)ivhd - base < ivrs->length) &&
+              (ivhd->type <= ACPI_IVHD_TYPE_MAX_SUPPORTED)) {
+               u8 *p = (u8 *) ivhd;
+
+               if (ivhd->devid == devid)
+                       last_type = ivhd->type;
+               ivhd = (struct ivhd_header *)(p + ivhd->length);
+       }
+
+       return last_type;
+}

Yes I don't get the logic here either.
Talk to kernel devs who wrote this?

commit 8c7142f56fedfc6824b5bca56fee1f443e01746b
Author: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
Date:   Fri Apr 1 09:05:59 2016 -0400

     iommu/amd: Use the most comprehensive IVHD type that the driver can support
The IVRS in more recent AMD system usually contains multiple
     IVHD block types (e.g. 0x10, 0x11, and 0x40) for each IOMMU.
     The newer IVHD types provide more information (e.g. new features
     specified in the IOMMU spec), while maintain compatibility with
     the older IVHD type.
Having multiple IVHD type allows older IOMMU drivers to still function
     (e.g. using the older IVHD type 0x10) while the newer IOMMU driver can use
     the newer IVHD types (e.g. 0x11 and 0x40). Therefore, the IOMMU driver
     should only make use of the newest IVHD type that it can support.
This patch adds new logic to determine the highest level of IVHD type
     it can support, and use it throughout the to initialize the driver.
     This requires adding another pass to the IVRS parsing to determine
     appropriate IVHD type (see function get_highest_supported_ivhd_type())
     before parsing the contents.
[Vincent: fix the build error of IVHD_DEV_ACPI_HID flag not found] Signed-off-by: Wan Zongshun <vincent....@amd.com>
     Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
     Signed-off-by: Joerg Roedel <jroe...@suse.de>

I've sent a email to talk to kernel developers about this function. Here is the link to the email: https://lore.kernel.org/all/e8a87c2b-a29a-ccf9-49c6-3cfceaa20...@gmail.com/

Reply via email to