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

2023-11-09 Thread Bui Quang Minh 

On 11/9/23 02:44, Michael S. Tsirkin wrote:

On Wed, Nov 08, 2023 at 09:22:18PM +0700, Bui Quang Minh wrote:

On 11/7/23 07:39, Michael S. Tsirkin wrote:

On Tue, Oct 24, 2023 at 10:21:05PM +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 exports IVHD type 0x11 besides the old IVHD type
0x10 in ACPI table. IVHD type 0x10 does not report full set of IOMMU
features only the legacy ones, so operating system (e.g. Linux) may only
detects x2APIC support if IVHD type 0x11 is available. The IVHD type 0x10
is kept so that old operating system that only parses type 0x10 can detect
the IOMMU device.

Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Bui Quang Minh 



changes IVRS without updating expected files for tests.
result seems to be CI failures:
https://gitlab.com/mstredhat/qemu/-/jobs/5470533834



Thanks Michael, I am preparing the fix in the next version. I've read the
instructions to update the test data in bios-tables-test.c. It says I need
to create some separate patches to update the test data. Are there any
reasons for this? I intend to change the binary and include the ASL diff
into the commit message. Is it enough?


No, not enough.  No, do not ignore the rules please.  Yes, there's a
reason.  The reason is that I need to be able to rebase your patches.  I
then regenerate the binaries. If the patch includes binaries it won't
rebase.


Okay, I got it. I will prepare the fix in the next version.

Thanks,
Quang Minh.

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

2023-11-08 Thread Michael S. Tsirkin 
On Wed, Nov 08, 2023 at 09:22:18PM +0700, Bui Quang Minh wrote:
> On 11/7/23 07:39, Michael S. Tsirkin wrote:
> > On Tue, Oct 24, 2023 at 10:21:05PM +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 exports IVHD type 0x11 besides the old IVHD type
> > > 0x10 in ACPI table. IVHD type 0x10 does not report full set of IOMMU
> > > features only the legacy ones, so operating system (e.g. Linux) may only
> > > detects x2APIC support if IVHD type 0x11 is available. The IVHD type 0x10
> > > is kept so that old operating system that only parses type 0x10 can detect
> > > the IOMMU device.
> > > 
> > > Reviewed-by: Michael S. Tsirkin 
> > > Signed-off-by: Bui Quang Minh 
> > 
> > 
> > changes IVRS without updating expected files for tests.
> > result seems to be CI failures:
> > https://gitlab.com/mstredhat/qemu/-/jobs/5470533834
> 
> 
> Thanks Michael, I am preparing the fix in the next version. I've read the
> instructions to update the test data in bios-tables-test.c. It says I need
> to create some separate patches to update the test data. Are there any
> reasons for this? I intend to change the binary and include the ASL diff
> into the commit message. Is it enough?

No, not enough.  No, do not ignore the rules please.  Yes, there's a
reason.  The reason is that I need to be able to rebase your patches.  I
then regenerate the binaries. If the patch includes binaries it won't
rebase.

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

2023-11-08 Thread Bui Quang Minh 

On 11/7/23 07:39, Michael S. Tsirkin wrote:

On Tue, Oct 24, 2023 at 10:21:05PM +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 exports IVHD type 0x11 besides the old IVHD type
0x10 in ACPI table. IVHD type 0x10 does not report full set of IOMMU
features only the legacy ones, so operating system (e.g. Linux) may only
detects x2APIC support if IVHD type 0x11 is available. The IVHD type 0x10
is kept so that old operating system that only parses type 0x10 can detect
the IOMMU device.

Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Bui Quang Minh 



changes IVRS without updating expected files for tests.
result seems to be CI failures:
https://gitlab.com/mstredhat/qemu/-/jobs/5470533834



Thanks Michael, I am preparing the fix in the next version. I've read 
the instructions to update the test data in bios-tables-test.c. It says 
I need to create some separate patches to update the test data. Are 
there any reasons for this? I intend to change the binary and include 
the ASL diff into the commit message. Is it enough?

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

2023-11-06 Thread Michael S. Tsirkin 
On Tue, Oct 24, 2023 at 10:21:05PM +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 exports IVHD type 0x11 besides the old IVHD type
> 0x10 in ACPI table. IVHD type 0x10 does not report full set of IOMMU
> features only the legacy ones, so operating system (e.g. Linux) may only
> detects x2APIC support if IVHD type 0x11 is available. The IVHD type 0x10
> is kept so that old operating system that only parses type 0x10 can detect
> the IOMMU device.
> 
> Reviewed-by: Michael S. Tsirkin 
> Signed-off-by: Bui Quang Minh 


changes IVRS without updating expected files for tests.
result seems to be CI failures:
https://gitlab.com/mstredhat/qemu/-/jobs/5470533834

> ---
>  hw/i386/acpi-build.c | 129 +++
>  hw/i386/amd_iommu.c  |  29 +-
>  hw/i386/amd_iommu.h  |  16 --
>  3 files changed, 117 insertions(+), 57 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 3f2b27cf75..8069971e54 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2337,30 +2337,23 @@ static void
>  build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
>  const char *oem_table_id)
>  {
> -int ivhd_table_len = 24;
>  AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
>  GArray *ivhd_blob = g_array_new(false, true, 1);
>  AcpiTable table = { .sig = "IVRS", .rev = 1, .oem_id = oem_id,
>  .oem_table_id = oem_table_id };
> +uint64_t feature_report;
>  
>  acpi_table_begin(&table, table_data);
>  /* IVinfo - IO virtualization information common to all
>   * IOMMU units in a system
>   */
> -build_append_int_noprefix(table_data, 40UL << 8/* PASize */, 4);
> +build_append_int_noprefix(table_data,
> + (1UL << 0) | /* EFRSup */
> + (40UL << 8), /* PASize */
> + 4);
>  /* reserved */
>  build_append_int_noprefix(table_data, 0, 8);
>  
> -/* IVHD definition - type 10h */
> -build_append_int_noprefix(table_data, 0x10, 1);
> -/* virtualization flags */
> -build_append_int_noprefix(table_data,
> - (1UL << 0) | /* HtTunEn  */
> - (1UL << 4) | /* iotblSup */
> - (1UL << 6) | /* PrefSup  */
> - (1UL << 7),  /* PPRSup   */
> - 1);
> -
>  /*
>   * A PCI bus walk, for each PCI host bridge, is necessary to create a
>   * complete set of IVHD entries.  Do this into a separate blob so that we
> @@ -2380,56 +2373,94 @@ build_amd_iommu(GArray *table_data, BIOSLinker 
> *linker, const char *oem_id,
>  build_append_int_noprefix(ivhd_blob, 0x001, 4);
>  }
>  
> -ivhd_table_len += ivhd_blob->len;
> -
>  /*
>   * When interrupt remapping is supported, we add a special IVHD device
> - * for type IO-APIC.
> - */
> -if (x86_iommu_ir_supported(x86_iommu_get_default())) {
> -ivhd_table_len += 8;
> -}
> -
> -/* IVHD length */
> -build_append_int_noprefix(table_data, ivhd_table_len, 2);
> -/* DeviceID */
> -build_append_int_noprefix(table_data,
> -  object_property_get_int(OBJECT(&s->pci), 
> "addr",
> -  &error_abort), 2);
> -/* Capability offset */
> -build_append_int_noprefix(table_data, s->pci.capab_offset, 2);
> -/* IOMMU base address */
> -build_append_int_noprefix(table_data, s->mmio.addr, 8);
> -/* PCI Segment Group */
> -build_append_int_noprefix(table_data, 0, 2);
> -/* IOMMU info */
> -build_append_int_noprefix(table_data, 0, 2);
> -/* IOMMU Feature Reporting */
> -build_append_int_noprefix(table_data,
> - (48UL << 30) | /* HATS   */
> - (48UL << 28) | /* GATS   */
> - (1UL << 2)   | /* GTSup  */
> - (1UL << 6),/* GASup  */
> - 4);
> -
> -/* IVHD entries as found above */
> -g_array_append_vals(table_data, ivhd_blob->data, ivhd_blob->len);
> -g_array_free(ivhd_blob, TRUE);
> -
> -/*
> - * Add a special IVHD device type.
> + * for type IO-APIC
>   * Refer to spec - Table 95: IVHD device entry type codes
>   *
>   * Linux IOMMU driver checks for the special IVHD device (type IO-APIC).
>   * See Linux kernel commit 'c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059'
>   */
>  if (x86_iommu_ir_supported(x86_iommu_get_default())) {
> -build_append_int_noprefix(table_data,
> +build_a

[PATCH v9 5/5] amd_iommu: report x2APIC support to the operating system

2023-10-24 Thread Bui Quang Minh 
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 exports IVHD type 0x11 besides the old IVHD type
0x10 in ACPI table. IVHD type 0x10 does not report full set of IOMMU
features only the legacy ones, so operating system (e.g. Linux) may only
detects x2APIC support if IVHD type 0x11 is available. The IVHD type 0x10
is kept so that old operating system that only parses type 0x10 can detect
the IOMMU device.

Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Bui Quang Minh 
---
 hw/i386/acpi-build.c | 129 +++
 hw/i386/amd_iommu.c  |  29 +-
 hw/i386/amd_iommu.h  |  16 --
 3 files changed, 117 insertions(+), 57 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 3f2b27cf75..8069971e54 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2337,30 +2337,23 @@ static void
 build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
 const char *oem_table_id)
 {
-int ivhd_table_len = 24;
 AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
 GArray *ivhd_blob = g_array_new(false, true, 1);
 AcpiTable table = { .sig = "IVRS", .rev = 1, .oem_id = oem_id,
 .oem_table_id = oem_table_id };
+uint64_t feature_report;
 
 acpi_table_begin(&table, table_data);
 /* IVinfo - IO virtualization information common to all
  * IOMMU units in a system
  */
-build_append_int_noprefix(table_data, 40UL << 8/* PASize */, 4);
+build_append_int_noprefix(table_data,
+ (1UL << 0) | /* EFRSup */
+ (40UL << 8), /* PASize */
+ 4);
 /* reserved */
 build_append_int_noprefix(table_data, 0, 8);
 
-/* IVHD definition - type 10h */
-build_append_int_noprefix(table_data, 0x10, 1);
-/* virtualization flags */
-build_append_int_noprefix(table_data,
- (1UL << 0) | /* HtTunEn  */
- (1UL << 4) | /* iotblSup */
- (1UL << 6) | /* PrefSup  */
- (1UL << 7),  /* PPRSup   */
- 1);
-
 /*
  * A PCI bus walk, for each PCI host bridge, is necessary to create a
  * complete set of IVHD entries.  Do this into a separate blob so that we
@@ -2380,56 +2373,94 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, 
const char *oem_id,
 build_append_int_noprefix(ivhd_blob, 0x001, 4);
 }
 
-ivhd_table_len += ivhd_blob->len;
-
 /*
  * When interrupt remapping is supported, we add a special IVHD device
- * for type IO-APIC.
- */
-if (x86_iommu_ir_supported(x86_iommu_get_default())) {
-ivhd_table_len += 8;
-}
-
-/* IVHD length */
-build_append_int_noprefix(table_data, ivhd_table_len, 2);
-/* DeviceID */
-build_append_int_noprefix(table_data,
-  object_property_get_int(OBJECT(&s->pci), "addr",
-  &error_abort), 2);
-/* Capability offset */
-build_append_int_noprefix(table_data, s->pci.capab_offset, 2);
-/* IOMMU base address */
-build_append_int_noprefix(table_data, s->mmio.addr, 8);
-/* PCI Segment Group */
-build_append_int_noprefix(table_data, 0, 2);
-/* IOMMU info */
-build_append_int_noprefix(table_data, 0, 2);
-/* IOMMU Feature Reporting */
-build_append_int_noprefix(table_data,
- (48UL << 30) | /* HATS   */
- (48UL << 28) | /* GATS   */
- (1UL << 2)   | /* GTSup  */
- (1UL << 6),/* GASup  */
- 4);
-
-/* IVHD entries as found above */
-g_array_append_vals(table_data, ivhd_blob->data, ivhd_blob->len);
-g_array_free(ivhd_blob, TRUE);
-
-/*
- * Add a special IVHD device type.
+ * for type IO-APIC
  * Refer to spec - Table 95: IVHD device entry type codes
  *
  * Linux IOMMU driver checks for the special IVHD device (type IO-APIC).
  * See Linux kernel commit 'c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059'
  */
 if (x86_iommu_ir_supported(x86_iommu_get_default())) {
-build_append_int_noprefix(table_data,
+build_append_int_noprefix(ivhd_blob,
  (0x1ull << 56) |   /* type IOAPIC */
  (IOAPIC_SB_DEVID << 40) |  /* IOAPIC devid */
  0x48,  /* special device 
*/
  8);
 }
+
+/* IVHD definition - type 10h */
+build_append_int_noprefix(table_data, 0x10, 1);
+/* virtualization