Re: [Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
On Thu, Sep 13, 2018 at 05:20:34PM -0500, Brijesh Singh wrote: > > > On 09/13/2018 01:18 PM, Michael S. Tsirkin wrote: > ...>> > > > 0x01 00a0 00 00 48 > > > > > > Byte 0: 0x48 (special device) > > > Byte 1 & 2: must be zero > > > Byte 3: 0 (dte setting) > > > Byte 4: 0 (handle) > > > Byte 5 & 6: IOAPIC devfn (14:0.0) > > > > Do you mean *bus* devfn? devfn is 0.0. > > > > Sorry my bad, I was meaning to write devid and not devfn. > > See, > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/amd_iommu_init.c#n2343 > > /* SB IOAPIC is always on this device in AMD systems */ > #define IOAPIC_SB_DEVID ((0x00 << 8) | PCI_DEVFN(0x14, 0)) And devid is bus device function. So in fact it is 0:20.0 So use PCI_BUILD_BDF(0, PCI_DEVFN(0x14, 0)) below > > > > Byte 7: 0x1 (IOAPIC) - See Table 97 in spec > > > > > > Above should go into code comment, along with > > first (oldest) version of spec that has this table. > > Additionally the number is IMHO more readable as: > > (0x1ull << 56) | (PCI_BUILD_BDF(14, 0) << 40) | 0x48 > > > > (assuming I got what it should be). > >
Re: [Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
On 09/13/2018 01:18 PM, Michael S. Tsirkin wrote: ...>> 0x01 00a0 00 00 48 Byte 0: 0x48 (special device) Byte 1 & 2: must be zero Byte 3: 0 (dte setting) Byte 4: 0 (handle) Byte 5 & 6: IOAPIC devfn (14:0.0) Do you mean *bus* devfn? devfn is 0.0. Sorry my bad, I was meaning to write devid and not devfn. See, https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/amd_iommu_init.c#n2343 /* SB IOAPIC is always on this device in AMD systems */ #define IOAPIC_SB_DEVID ((0x00 << 8) | PCI_DEVFN(0x14, 0)) Byte 7: 0x1 (IOAPIC) - See Table 97 in spec Above should go into code comment, along with first (oldest) version of spec that has this table. Additionally the number is IMHO more readable as: (0x1ull << 56) | (PCI_BUILD_BDF(14, 0) << 40) | 0x48 (assuming I got what it should be).
Re: [Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
On Wed, Sep 12, 2018 at 02:24:52PM -0500, Brijesh Singh wrote:
>
>
> On 09/12/2018 11:35 AM, Igor Mammedov wrote:
> ...
>
> > > +/*
> > > + * When interrupt remapping is enabled, Linux IOMMU driver also
> > > checks
> > > + * for special IVHD device (type IO-APIC), which is typically
> > > presented
> > > + * as PCI device 14:00.0.
> > Probably it shouldn't be a 'typically' device from somewhere but rather
> > address
> > fetched from corresponding device model QEMU implements.
> >
>
> IOAPIC is not presented as a true PCI device to guest OS. When IOMMU is
> enabled a pseudo address space to added under root PCI bus. PCI 14:0.0
> presents to this pseudo device.
>
> >
> > > + */
> > > +if (s->intr_enabled) {
> > > +build_append_int_noprefix(table_data, 0x0100a048, 8);
> > ^^ this is
> > incomprehensible,
> > where does this magic number comes from and how was it calculated?
> >
>
> In order to provide interrupt remap support, a special IVHD device need
> to be added, the magic number uses the format defined in Table 95 (IVHD
> device entry type codes).
>
> 0x01 00a0 00 00 48
>
> Byte 0: 0x48 (special device)
> Byte 1 & 2: must be zero
> Byte 3: 0 (dte setting)
> Byte 4: 0 (handle)
> Byte 5 & 6: IOAPIC devfn (14:0.0)
Do you mean *bus* devfn? devfn is 0.0.
> Byte 7: 0x1 (IOAPIC) - See Table 97 in spec
Above should go into code comment, along with
first (oldest) version of spec that has this table.
Additionally the number is IMHO more readable as:
(0x1ull << 56) | (PCI_BUILD_BDF(14, 0) << 40) | 0x48
(assuming I got what it should be).
>
> > > +}
> > > +
> > > build_header(linker, table_data, (void *)(table_data->data +
> > > iommu_start),
> > >"IVRS", table_data->len - iommu_start, 1, NULL, NULL);
> > > }
> >
Re: [Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
On Wed, Sep 12, 2018 at 02:11:10PM -0500, Brijesh Singh wrote:
>
>
> On 09/11/2018 11:35 PM, Peter Xu wrote:
> > On Tue, Sep 11, 2018 at 11:49:47AM -0500, Brijesh Singh wrote:
> > > When interrupt remapping is enabled, add a special IVHD device
> > > (type IOAPIC) -- which is typically PCI device 14:0.0. Linux IOMMU driver
> > > checks for this special device.
> > >
> > > Cc: "Michael S. Tsirkin"
> > > Cc: Paolo Bonzini
> > > Cc: Richard Henderson
> > > Cc: Eduardo Habkost
> > > Cc: Marcel Apfelbaum
> > > Cc: Tom Lendacky
> > > Cc: Suravee Suthikulpanit
> > > Signed-off-by: Brijesh Singh
> > > ---
> > > hw/i386/acpi-build.c | 20 +++-
> > > 1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index e1ee8ae..5c2c638 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -2519,6 +2519,7 @@ build_dmar_q35(GArray *table_data, BIOSLinker
> > > *linker)
> > > static void
> > > build_amd_iommu(GArray *table_data, BIOSLinker *linker)
> > > {
> > > +int ivhd_table_len = 28;
> > > int iommu_start = table_data->len;
> > > AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
> > > @@ -2540,8 +2541,16 @@ build_amd_iommu(GArray *table_data, BIOSLinker
> > > *linker)
> > >(1UL << 6) | /* PrefSup */
> > >(1UL << 7), /* PPRSup */
> > >1);
> > > +
> > > +/*
> > > + * When interrupt remapping is enabled, we add a special IVHD device
> > > + * for type IO-APIC.
> > > + */
> > > +if (s->intr_enabled) {
> > > +ivhd_table_len += 8;
> > > +}
> > > /* IVHD length */
> > > -build_append_int_noprefix(table_data, 28, 2);
> > > +build_append_int_noprefix(table_data, ivhd_table_len, 2);
> > > /* DeviceID */
> > > build_append_int_noprefix(table_data, s->devid, 2);
> > > /* Capability offset */
> > > @@ -2565,6 +2574,15 @@ build_amd_iommu(GArray *table_data, BIOSLinker
> > > *linker)
> > >*/
> > > build_append_int_noprefix(table_data, 0x001, 4);
> > > +/*
> > > + * When interrupt remapping is enabled, Linux IOMMU driver also
> > > checks
> > > + * for special IVHD device (type IO-APIC), which is typically
> > > presented
> > > + * as PCI device 14:00.0.
> > > + */
> > > +if (s->intr_enabled) {
> > > +build_append_int_noprefix(table_data, 0x0100a048, 8);
> >
> > Some comments on the bit definition would be nicer, or "please refer
> > to Table 95 of AMD-Vi spec".
> >
> > Could I ask how come the 14:00.0? Is that in the spec somewhere?
> >
> > And since you explicitly mentioned Linux, then... would it work for
> > Windows too?
> >
>
> The PCI 14:00.0 is SouthBridge IOAPIC device. On bare metal the timer
> subsystem is connected to the SB IOAPIC. The IVRS table must contains
> the entry of SB IOAPIC otherwise Linux will not enable the IR mapping
> while parsing the IVRS.
>
> On bare meta system, IVRS will always have entry for SB IOAPIC. As per
> Windows is concerned, I am not sure if Windows support interrupt remap.
> If it does, adding the SB IOAPIC devid should not cause any problem
> to it because its always available on bare metal system.
>
> Here is linux commit
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059
Thanks for these information. Please feel free to add some of the
information into the comment, that might explain itself better. When
referring to commits, I would suggest just use "Please refer to Linux
commit xxx (xxx)" rather than the link though.
Regards,
--
Peter Xu
Re: [Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
On 09/12/2018 11:35 AM, Igor Mammedov wrote:
...
+/*
+ * When interrupt remapping is enabled, Linux IOMMU driver also checks
+ * for special IVHD device (type IO-APIC), which is typically presented
+ * as PCI device 14:00.0.
Probably it shouldn't be a 'typically' device from somewhere but rather address
fetched from corresponding device model QEMU implements.
IOAPIC is not presented as a true PCI device to guest OS. When IOMMU is
enabled a pseudo address space to added under root PCI bus. PCI 14:0.0
presents to this pseudo device.
+ */
+if (s->intr_enabled) {
+build_append_int_noprefix(table_data, 0x0100a048, 8);
^^ this is incomprehensible,
where does this magic number comes from and how was it calculated?
In order to provide interrupt remap support, a special IVHD device need
to be added, the magic number uses the format defined in Table 95 (IVHD
device entry type codes).
0x01 00a0 00 00 48
Byte 0: 0x48 (special device)
Byte 1 & 2: must be zero
Byte 3: 0 (dte setting)
Byte 4: 0 (handle)
Byte 5 & 6: IOAPIC devfn (14:0.0)
Byte 7: 0x1 (IOAPIC) - See Table 97 in spec
+}
+
build_header(linker, table_data, (void *)(table_data->data + iommu_start),
"IVRS", table_data->len - iommu_start, 1, NULL, NULL);
}
Re: [Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
On 09/11/2018 11:35 PM, Peter Xu wrote:
On Tue, Sep 11, 2018 at 11:49:47AM -0500, Brijesh Singh wrote:
When interrupt remapping is enabled, add a special IVHD device
(type IOAPIC) -- which is typically PCI device 14:0.0. Linux IOMMU driver
checks for this special device.
Cc: "Michael S. Tsirkin"
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
Cc: Marcel Apfelbaum
Cc: Tom Lendacky
Cc: Suravee Suthikulpanit
Signed-off-by: Brijesh Singh
---
hw/i386/acpi-build.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e1ee8ae..5c2c638 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2519,6 +2519,7 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker)
static void
build_amd_iommu(GArray *table_data, BIOSLinker *linker)
{
+int ivhd_table_len = 28;
int iommu_start = table_data->len;
AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
@@ -2540,8 +2541,16 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
(1UL << 6) | /* PrefSup */
(1UL << 7), /* PPRSup */
1);
+
+/*
+ * When interrupt remapping is enabled, we add a special IVHD device
+ * for type IO-APIC.
+ */
+if (s->intr_enabled) {
+ivhd_table_len += 8;
+}
/* IVHD length */
-build_append_int_noprefix(table_data, 28, 2);
+build_append_int_noprefix(table_data, ivhd_table_len, 2);
/* DeviceID */
build_append_int_noprefix(table_data, s->devid, 2);
/* Capability offset */
@@ -2565,6 +2574,15 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
*/
build_append_int_noprefix(table_data, 0x001, 4);
+/*
+ * When interrupt remapping is enabled, Linux IOMMU driver also checks
+ * for special IVHD device (type IO-APIC), which is typically presented
+ * as PCI device 14:00.0.
+ */
+if (s->intr_enabled) {
+build_append_int_noprefix(table_data, 0x0100a048, 8);
Some comments on the bit definition would be nicer, or "please refer
to Table 95 of AMD-Vi spec".
Could I ask how come the 14:00.0? Is that in the spec somewhere?
And since you explicitly mentioned Linux, then... would it work for
Windows too?
The PCI 14:00.0 is SouthBridge IOAPIC device. On bare metal the timer
subsystem is connected to the SB IOAPIC. The IVRS table must contains
the entry of SB IOAPIC otherwise Linux will not enable the IR mapping
while parsing the IVRS.
On bare meta system, IVRS will always have entry for SB IOAPIC. As per
Windows is concerned, I am not sure if Windows support interrupt remap.
If it does, adding the SB IOAPIC devid should not cause any problem
to it because its always available on bare metal system.
Here is linux commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059
Re: [Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
On Tue, 11 Sep 2018 11:49:47 -0500
Brijesh Singh wrote:
> When interrupt remapping is enabled, add a special IVHD device
> (type IOAPIC) -- which is typically PCI device 14:0.0. Linux IOMMU driver
> checks for this special device.
>
> Cc: "Michael S. Tsirkin"
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Cc: Marcel Apfelbaum
> Cc: Tom Lendacky
> Cc: Suravee Suthikulpanit
> Signed-off-by: Brijesh Singh
> ---
> hw/i386/acpi-build.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e1ee8ae..5c2c638 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2519,6 +2519,7 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker)
> static void
> build_amd_iommu(GArray *table_data, BIOSLinker *linker)
> {
> +int ivhd_table_len = 28;
> int iommu_start = table_data->len;
> AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
>
> @@ -2540,8 +2541,16 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
> (1UL << 6) | /* PrefSup */
> (1UL << 7), /* PPRSup */
> 1);
> +
> +/*
> + * When interrupt remapping is enabled, we add a special IVHD device
> + * for type IO-APIC.
> + */
> +if (s->intr_enabled) {
> +ivhd_table_len += 8;
> +}
> /* IVHD length */
> -build_append_int_noprefix(table_data, 28, 2);
> +build_append_int_noprefix(table_data, ivhd_table_len, 2);
> /* DeviceID */
> build_append_int_noprefix(table_data, s->devid, 2);
> /* Capability offset */
> @@ -2565,6 +2574,15 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
> */
> build_append_int_noprefix(table_data, 0x001, 4);
>
> +/*
> + * When interrupt remapping is enabled, Linux IOMMU driver also checks
> + * for special IVHD device (type IO-APIC), which is typically presented
> + * as PCI device 14:00.0.
Probably it shouldn't be a 'typically' device from somewhere but rather address
fetched from corresponding device model QEMU implements.
> + */
> +if (s->intr_enabled) {
> +build_append_int_noprefix(table_data, 0x0100a048, 8);
^^ this is incomprehensible,
where does this magic number comes from and how was it calculated?
> +}
> +
> build_header(linker, table_data, (void *)(table_data->data +
> iommu_start),
> "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
> }
Re: [Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
On Tue, Sep 11, 2018 at 11:49:47AM -0500, Brijesh Singh wrote:
> When interrupt remapping is enabled, add a special IVHD device
> (type IOAPIC) -- which is typically PCI device 14:0.0. Linux IOMMU driver
> checks for this special device.
>
> Cc: "Michael S. Tsirkin"
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Cc: Marcel Apfelbaum
> Cc: Tom Lendacky
> Cc: Suravee Suthikulpanit
> Signed-off-by: Brijesh Singh
> ---
> hw/i386/acpi-build.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e1ee8ae..5c2c638 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2519,6 +2519,7 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker)
> static void
> build_amd_iommu(GArray *table_data, BIOSLinker *linker)
> {
> +int ivhd_table_len = 28;
> int iommu_start = table_data->len;
> AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
>
> @@ -2540,8 +2541,16 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
> (1UL << 6) | /* PrefSup */
> (1UL << 7), /* PPRSup */
> 1);
> +
> +/*
> + * When interrupt remapping is enabled, we add a special IVHD device
> + * for type IO-APIC.
> + */
> +if (s->intr_enabled) {
> +ivhd_table_len += 8;
> +}
> /* IVHD length */
> -build_append_int_noprefix(table_data, 28, 2);
> +build_append_int_noprefix(table_data, ivhd_table_len, 2);
> /* DeviceID */
> build_append_int_noprefix(table_data, s->devid, 2);
> /* Capability offset */
> @@ -2565,6 +2574,15 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
> */
> build_append_int_noprefix(table_data, 0x001, 4);
>
> +/*
> + * When interrupt remapping is enabled, Linux IOMMU driver also checks
> + * for special IVHD device (type IO-APIC), which is typically presented
> + * as PCI device 14:00.0.
> + */
> +if (s->intr_enabled) {
> +build_append_int_noprefix(table_data, 0x0100a048, 8);
Some comments on the bit definition would be nicer, or "please refer
to Table 95 of AMD-Vi spec".
Could I ask how come the 14:00.0? Is that in the spec somewhere?
And since you explicitly mentioned Linux, then... would it work for
Windows too?
Thanks,
> +}
> +
> build_header(linker, table_data, (void *)(table_data->data +
> iommu_start),
> "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
> }
> --
> 2.7.4
>
>
--
Peter Xu
[Qemu-devel] [PATCH 4/6] i386: acpi: add IVHD device entry for IOAPIC
When interrupt remapping is enabled, add a special IVHD device
(type IOAPIC) -- which is typically PCI device 14:0.0. Linux IOMMU driver
checks for this special device.
Cc: "Michael S. Tsirkin"
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
Cc: Marcel Apfelbaum
Cc: Tom Lendacky
Cc: Suravee Suthikulpanit
Signed-off-by: Brijesh Singh
---
hw/i386/acpi-build.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e1ee8ae..5c2c638 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2519,6 +2519,7 @@ build_dmar_q35(GArray *table_data, BIOSLinker *linker)
static void
build_amd_iommu(GArray *table_data, BIOSLinker *linker)
{
+int ivhd_table_len = 28;
int iommu_start = table_data->len;
AMDVIState *s = AMD_IOMMU_DEVICE(x86_iommu_get_default());
@@ -2540,8 +2541,16 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
(1UL << 6) | /* PrefSup */
(1UL << 7), /* PPRSup */
1);
+
+/*
+ * When interrupt remapping is enabled, we add a special IVHD device
+ * for type IO-APIC.
+ */
+if (s->intr_enabled) {
+ivhd_table_len += 8;
+}
/* IVHD length */
-build_append_int_noprefix(table_data, 28, 2);
+build_append_int_noprefix(table_data, ivhd_table_len, 2);
/* DeviceID */
build_append_int_noprefix(table_data, s->devid, 2);
/* Capability offset */
@@ -2565,6 +2574,15 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
*/
build_append_int_noprefix(table_data, 0x001, 4);
+/*
+ * When interrupt remapping is enabled, Linux IOMMU driver also checks
+ * for special IVHD device (type IO-APIC), which is typically presented
+ * as PCI device 14:00.0.
+ */
+if (s->intr_enabled) {
+build_append_int_noprefix(table_data, 0x0100a048, 8);
+}
+
build_header(linker, table_data, (void *)(table_data->data + iommu_start),
"IVRS", table_data->len - iommu_start, 1, NULL, NULL);
}
--
2.7.4
