From: "Michael S. Tsirkin" <mst@redhat.com>
To: Bui Quang Minh <minhquangbui99@gmail.com>
Cc: qemu-devel@nongnu.org, "David Woodhouse" <dwmw2@infradead.org>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Richard Henderson" <richard.henderson@linaro.org>,
"Eduardo Habkost" <eduardo@habkost.net>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Igor Mammedov" <imammedo@redhat.com>,
"Alex Bennée" <alex.bennee@linaro.org>
Subject: Re: [PATCH v4 5/5] amd_iommu: report x2APIC support to the operating system
Date: Mon, 10 Jul 2023 14:37:18 -0400 [thread overview]
Message-ID: <20230710143523-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <bfa5991d-07d0-84ae-d29c-d467c091343d@gmail.com>
On Fri, Jun 23, 2023 at 10:28:43PM +0700, Bui Quang Minh wrote:
> On 6/23/23 03:26, Michael S. Tsirkin wrote:
> > On Mon, May 22, 2023 at 11:31:57PM +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.
> > >
> > > Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > > ---
> > > hw/i386/acpi-build.c | 127 ++++++++++++++++++++++++++-----------------
> > > hw/i386/amd_iommu.c | 21 ++++++-
> > > hw/i386/amd_iommu.h | 16 ++++--
> > > 3 files changed, 108 insertions(+), 56 deletions(-)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 512162003b..4459122e56 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -2339,30 +2339,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
> > > @@ -2382,56 +2375,92 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker, const char *oem_id,
> > > build_append_int_noprefix(ivhd_blob, 0x0000001, 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 flags */
> > > + build_append_int_noprefix(table_data,
> > > + (1UL << 0) | /* HtTunEn */
> > > + (1UL << 4) | /* iotblSup */
> > > + (1UL << 6) | /* PrefSup */
> > > + (1UL << 7), /* PPRSup */
> > > + 1);
> > > +
> > > + /* IVHD length */
> > > + build_append_int_noprefix(table_data, ivhd_blob->len + 24, 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 */
> > > + feature_report = (48UL << 30) | /* HATS */
> > > + (48UL << 28) | /* GATS */
> > > + (1UL << 2) | /* GTSup */
> > > + (1UL << 6); /* GASup */
> > > + if (s->xtsup) {
> > > + feature_report |= (1UL << 0); /* XTSup */
> > > + }
> > > + build_append_int_noprefix(table_data, feature_report, 4);
> > > +
> > > + /* IVHD entries as found above */
> > > + g_array_append_vals(table_data, ivhd_blob->data, ivhd_blob->len);
> > > +
> > > + /* IVHD definition - type 11h */
> > > + build_append_int_noprefix(table_data, 0x11, 1);
> > > + /* virtualization flags */
> > > + build_append_int_noprefix(table_data,
> > > + (1UL << 0) | /* HtTunEn */
> > > + (1UL << 4), /* iotblSup */
> > > + 1);
> > > +
> > > + /* IVHD length */
> > > + build_append_int_noprefix(table_data, ivhd_blob->len + 40, 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 Attributes */
> > > + build_append_int_noprefix(table_data, 0, 4);
> > > + /* EFR Register Image */
> > > + build_append_int_noprefix(table_data, s->efr_reg, 8);
> > > + /* EFR Register Image 2 */
> > > + build_append_int_noprefix(table_data, 0, 8);
> > > +
> > > + /* IVHD entries as found above */
> > > + g_array_append_vals(table_data, ivhd_blob->data, ivhd_blob->len);
> > > +
> > > + g_array_free(ivhd_blob, TRUE);
> > > acpi_table_end(linker, &table);
> > > }
> > > diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> > > index 9c77304438..0e308184d7 100644
> > > --- a/hw/i386/amd_iommu.c
> > > +++ b/hw/i386/amd_iommu.c
> > > @@ -31,6 +31,7 @@
> > > #include "hw/i386/apic_internal.h"
> > > #include "trace.h"
> > > #include "hw/i386/apic-msidef.h"
> > > +#include "hw/qdev-properties.h"
> > > /* used AMD-Vi MMIO registers */
> > > const char *amdvi_mmio_low[] = {
> > > @@ -1155,7 +1156,12 @@ static int amdvi_int_remap_ga(AMDVIState *iommu,
> > > irq->vector = irte.hi.fields.vector;
> > > irq->dest_mode = irte.lo.fields_remap.dm;
> > > irq->redir_hint = irte.lo.fields_remap.rq_eoi;
> > > - irq->dest = irte.lo.fields_remap.destination;
> > > + if (iommu->xtsup) {
> > > + irq->dest = irte.lo.fields_remap.destination |
> > > + (irte.hi.fields.destination_hi << 24);
> > > + } else {
> > > + irq->dest = irte.lo.fields_remap.destination & 0xff;
> > > + }
> > > return 0;
> > > }
> > > @@ -1503,10 +1509,15 @@ static void amdvi_init(AMDVIState *s)
> > > s->enabled = false;
> > > s->ats_enabled = false;
> > > s->cmdbuf_enabled = false;
> > > + s->efr_reg = AMDVI_DEFAULT_EXT_FEATURES;
> > > +
> > > + if (s->xtsup) {
> > > + s->efr_reg |= AMDVI_FEATURE_XT;
> > > + }
> > > /* reset MMIO */
> > > memset(s->mmior, 0, AMDVI_MMIO_SIZE);
> > > - amdvi_set_quad(s, AMDVI_MMIO_EXT_FEATURES, AMDVI_EXT_FEATURES,
> > > + amdvi_set_quad(s, AMDVI_MMIO_EXT_FEATURES, s->efr_reg,
> > > 0xffffffffffffffef, 0);
> > > amdvi_set_quad(s, AMDVI_MMIO_STATUS, 0, 0x98, 0x67);
> > > }
> > > @@ -1591,6 +1602,11 @@ static void amdvi_sysbus_realize(DeviceState *dev, Error **errp)
> > > amdvi_init(s);
> > > }
> > > +static Property amdvi_properties[] = {
> > > + DEFINE_PROP_BOOL("xtsup", AMDVIState, xtsup, false),
> > > + DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> > > static const VMStateDescription vmstate_amdvi_sysbus = {
> > > .name = "amd-iommu",
> > > .unmigratable = 1
> > > @@ -1617,6 +1633,7 @@ static void amdvi_sysbus_class_init(ObjectClass *klass, void *data)
> > > dc->user_creatable = true;
> > > set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > > dc->desc = "AMD IOMMU (AMD-Vi) DMA Remapping device";
> > > + device_class_set_props(dc, amdvi_properties);
> > > }
> > > static const TypeInfo amdvi_sysbus = {
> > > diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
> > > index 6da893ee57..f3730db990 100644
> > > --- a/hw/i386/amd_iommu.h
> > > +++ b/hw/i386/amd_iommu.h
> > > @@ -154,6 +154,7 @@
> > > #define AMDVI_FEATURE_PREFETCH (1ULL << 0) /* page prefetch */
> > > #define AMDVI_FEATURE_PPR (1ULL << 1) /* PPR Support */
> > > +#define AMDVI_FEATURE_XT (1ULL << 2) /* x2APIC Support */
> > > #define AMDVI_FEATURE_GT (1ULL << 4) /* Guest Translation */
> > > #define AMDVI_FEATURE_IA (1ULL << 6) /* inval all support */
> > > #define AMDVI_FEATURE_GA (1ULL << 7) /* guest VAPIC support */
> > > @@ -173,8 +174,9 @@
> > > #define AMDVI_IOTLB_MAX_SIZE 1024
> > > #define AMDVI_DEVID_SHIFT 36
> > > -/* extended feature support */
> > > -#define AMDVI_EXT_FEATURES (AMDVI_FEATURE_PREFETCH | AMDVI_FEATURE_PPR | \
> > > +/* default extended feature */
> > > +#define AMDVI_DEFAULT_EXT_FEATURES \
> > > + (AMDVI_FEATURE_PREFETCH | AMDVI_FEATURE_PPR | \
> > > AMDVI_FEATURE_IA | AMDVI_FEATURE_GT | AMDVI_FEATURE_HE | \
> > > AMDVI_GATS_MODE | AMDVI_HATS_MODE | AMDVI_FEATURE_GA)
> > > @@ -278,8 +280,8 @@ union irte_ga_lo {
> > > dm:1,
> > > /* ------ */
> > > guest_mode:1,
> > > - destination:8,
> > > - rsvd_1:48;
> > > + destination:24,
> > > + rsvd_1:32;
> > > } fields_remap;
> > > };
> > > @@ -287,7 +289,8 @@ union irte_ga_hi {
> > > uint64_t val;
> > > struct {
> > > uint64_t vector:8,
> > > - rsvd_2:56;
> > > + rsvd_2:48,
> > > + destination_hi:8;
> > > } fields;
> > > };
> > > @@ -366,6 +369,9 @@ struct AMDVIState {
> > > /* Interrupt remapping */
> > > bool ga_enabled;
> > > + bool xtsup;
> > > +
> > > + uint64_t efr_reg; /* extended feature register */
> > > };
> >
> > It would be cleaner to have efr_reg as a function and just call it
> > when needed.
> >
> > With that addressed:
> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> You mean function like this
>
> uint64_t amdvi_extended_feature_register(AMDVIState *s)
> {
> uint64_t feature = AMDVI_DEFAULT_EXT_FEATURES;
> if (s->xtsup)
> feature |= AMDVI_FEATURE_XT;
>
> return feature;
> }
Yes exactly. going to do that?
> > > #endif
> > > --
> > > 2.25.1
> >
next prev parent reply other threads:[~2023-07-10 18:38 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-22 16:31 [PATCH v4 0/5] Support x2APIC mode with TCG accelerator Bui Quang Minh
2023-05-22 16:31 ` [PATCH v4 1/5] i386/tcg: implement x2APIC registers MSR access Bui Quang Minh
2023-05-22 16:31 ` [PATCH v4 2/5] apic: add support for x2APIC mode Bui Quang Minh
2023-05-22 16:31 ` [PATCH v4 3/5] apic, i386/tcg: add x2apic transitions Bui Quang Minh
2023-05-22 16:31 ` [PATCH v4 4/5] intel_iommu: allow Extended Interrupt Mode when using userspace APIC Bui Quang Minh
2023-05-22 16:31 ` [PATCH v4 5/5] amd_iommu: report x2APIC support to the operating system Bui Quang Minh
2023-06-22 20:26 ` Michael S. Tsirkin
2023-06-23 15:28 ` Bui Quang Minh
2023-07-10 18:37 ` Michael S. Tsirkin [this message]
2023-07-12 14:38 ` Bui Quang Minh
2023-06-22 20:30 ` [PATCH v4 0/5] Support x2APIC mode with TCG accelerator Michael S. Tsirkin
2023-07-10 18:39 ` Michael S. Tsirkin
2023-07-12 14:57 ` Bui Quang Minh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230710143523-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=alex.bennee@linaro.org \
--cc=dwmw2@infradead.org \
--cc=eduardo@habkost.net \
--cc=imammedo@redhat.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=minhquangbui99@gmail.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.