Re: [REPOST PATCH v3 5/5] amd_iommu: report x2APIC support to the operating system

[Bui Quang Minh <minhquangbui99@gmail.com>]

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 <minhquangbui99@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.Suthikulpanit@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.wan@amd.com>
>      Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>      Signed-off-by: Joerg Roedel <jroedel@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-3cfceaa208bb@gmail.com/