John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31917
Change subject: soc/intel/cnl: Generate ACPI DMAR table ......................................................................
soc/intel/cnl: Generate ACPI DMAR table
SoC is detected Vt-d capable. ACPI DMAR table is created and DMA-remapping hardware units entries are programmed. Device scope like IOAPIC and HPET will be added once cnl fsp upd update is available.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 8 files changed, 157 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/1
diff --git a/src/soc/intel/cannonlake/acpi.c b/src/soc/intel/cannonlake/acpi.c index 639f6c6..864cbe9 100644 --- a/src/soc/intel/cannonlake/acpi.c +++ b/src/soc/intel/cannonlake/acpi.c @@ -17,19 +17,22 @@
#include <arch/acpi.h> #include <arch/acpigen.h> -#include <device/mmio.h> #include <arch/smp/mpspec.h> #include <cbmem.h> #include <chip.h> +#include <device/mmio.h> +#include <device/pci_ops.h> #include <ec/google/chromeec/ec.h> #include <intelblocks/cpulib.h> #include <intelblocks/pmclib.h> #include <intelblocks/acpi.h> +#include <intelblocks/p2sb.h> #include <soc/cpu.h> #include <soc/iomap.h> #include <soc/nvs.h> #include <soc/pci_devs.h> #include <soc/pm.h> +#include <soc/systemagent.h> #include <string.h> #include <vendorcode/google/chromeos/gnvs.h> #include <wrdd.h> @@ -285,3 +288,78 @@ { return acpigen_soc_gpio_op("\_SB.PCI0.CTXS", gpio_num); } + +static unsigned long acpi_fill_dmar(unsigned long current) +{ + struct device *const igfx_dev = dev_find_slot(0, SA_DEVFN_IGD); + const u32 gfx_vtbar = MCHBAR32(GFXVTBAR) & ~0xfff; + const bool gfxvten = MCHBAR32(GFXVTBAR) & 1; + + /* iGFX has to be enabled, GFXVTBAR set and in 32-bit space. */ + if (igfx_dev && igfx_dev->enabled && gfxvten && + gfx_vtbar && !MCHBAR32(GFXVTBAR + 4)) { + unsigned long tmp = current; + + current += acpi_create_dmar_drhd(current, 0, 0, gfx_vtbar); + current += acpi_create_dmar_ds_pci(current, 0, 2, 0); + + acpi_dmar_drhd_fixup(tmp, current); + + /* Add RMRR entry */ + tmp = current; + + current += acpi_create_dmar_rmrr(current, 0, + sa_get_gsm_base(), sa_get_tolud_base() - 1); + current += acpi_create_dmar_ds_pci(current, 0, 2, 0); + acpi_dmar_rmrr_fixup(tmp, current); + } + + struct device *const p2sb_dev = dev_find_slot(0, PCH_DEVFN_P2SB); + const u32 vtvc0bar = MCHBAR32(VTVC0BAR) & ~0xfff; + const bool vtvc0en = MCHBAR32(VTVC0BAR) & 1; + + /* General VTBAR has to be set and in 32-bit space. */ + if (p2sb_dev && vtvc0bar && vtvc0en && !MCHBAR32(VTVC0BAR + 4)) { + const unsigned long tmp = current; + + /* P2SB may already be hidden. There's no clear rule, when. */ + const u8 p2sb_hidden = + pci_read_config8(p2sb_dev, PCH_P2SB_E0 + 1); + pci_write_config8(p2sb_dev, PCH_P2SB_E0 + 1, 0); + + const u16 ibdf = pci_read_config16(p2sb_dev, PCH_P2SB_IBDF); + const u16 hbdf = pci_read_config16(p2sb_dev, PCH_P2SB_HBDF); + + pci_write_config8(p2sb_dev, PCH_P2SB_E0 + 1, p2sb_hidden); + current += acpi_create_dmar_drhd(current, + DRHD_INCLUDE_PCI_ALL, 0, vtvc0bar); + current += acpi_create_dmar_ds_ioapic(current, + 2, ibdf >> 8, PCI_SLOT(ibdf), PCI_FUNC(ibdf)); + current += acpi_create_dmar_ds_msi_hpet(current, + 0, hbdf >> 8, PCI_SLOT(hbdf), PCI_FUNC(hbdf)); + + acpi_dmar_drhd_fixup(tmp, current); + } + + return current; +} + +unsigned long sa_write_acpi_tables(struct device *dev, + unsigned long current, + struct acpi_rsdp *rsdp) +{ + const struct soc_intel_cannonlake_config *const config = dev->chip_info; + acpi_dmar_t *const dmar = (acpi_dmar_t *)current; + + /* Create DMAR table only if we have Vt-d capability */ + if ((config && config->ignore_vtd) || !soc_is_vtd_capable()) + return current; + + printk(BIOS_DEBUG, "ACPI: * DMAR\n"); + acpi_create_dmar(dmar, DMAR_INTR_REMAP, acpi_fill_dmar); + current += dmar->header.length; + current = acpi_align_current(current); + acpi_add_table(rsdp, dmar); + + return current; +} diff --git a/src/soc/intel/cannonlake/chip.c b/src/soc/intel/cannonlake/chip.c index 993e7f3..050e026 100644 --- a/src/soc/intel/cannonlake/chip.c +++ b/src/soc/intel/cannonlake/chip.c @@ -196,6 +196,7 @@ .set_resources = &pci_domain_set_resources, .scan_bus = &pci_domain_scan_bus, #if CONFIG(HAVE_ACPI_TABLES) + .write_acpi_tables = &sa_write_acpi_tables, .acpi_name = &soc_acpi_name, #endif }; diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 3e4bafc..1574b8e 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -221,6 +221,9 @@ /* Estimated maximum platform power in Watts */ uint16_t psys_pmax;
+ /* Whether to ignore VT-d support of the SKU */ + int ignore_vtd; + /* Intel Speed Shift Technology */ uint8_t speed_shift_enable; /* Enable VR specific mailbox command diff --git a/src/soc/intel/cannonlake/include/soc/iomap.h b/src/soc/intel/cannonlake/include/soc/iomap.h index ed9b29f..100bd11 100644 --- a/src/soc/intel/cannonlake/include/soc/iomap.h +++ b/src/soc/intel/cannonlake/include/soc/iomap.h @@ -52,6 +52,15 @@ #define EDRAM_BASE_ADDRESS 0xfed80000 #define EDRAM_BASE_SIZE 0x4000
+#define GFXVT_BASE_ADDRESS 0xfed90000 +#define GFXVT_BASE_SIZE 0x1000 + +#define IPUVT_BASE_ADDRESS 0xfed92000 +#define IPUVT_BASE_SIZE 0x1000 + +#define VTVC0_BASE_ADDRESS 0xfed91000 +#define VTVC0_BASE_SIZE 0x1000 + #define REG_BASE_ADDRESS 0xfc000000 #define REG_BASE_SIZE 0x1000
diff --git a/src/soc/intel/cannonlake/include/soc/systemagent.h b/src/soc/intel/cannonlake/include/soc/systemagent.h index da83c38..390bd56 100644 --- a/src/soc/intel/cannonlake/include/soc/systemagent.h +++ b/src/soc/intel/cannonlake/include/soc/systemagent.h @@ -30,10 +30,15 @@ #define D_LCK (1 << 4) #define G_SMRAME (1 << 3) #define C_BASE_SEG ((0 << 2) | (1 << 1) | (0 << 0)) +#define CAPID0_A 0xe4 +#define VTD_DISABLE (1 << 23)
-#define BIOS_RESET_CPL 0x5da8 -#define EDRAMBAR 0x5408 -#define REGBAR 0x5420 +#define BIOS_RESET_CPL 0x5da8 +#define GFXVTBAR 0x5400 +#define EDRAMBAR 0x5408 +#define IPUVTBAR 0x7884 +#define VTVC0BAR 0x5410 +#define REGBAR 0x5420
#define MCH_PKG_POWER_LIMIT_LO 0x59a0 #define MCH_PKG_POWER_LIMIT_HI 0x59a4 @@ -43,4 +48,11 @@ #define IMRBASE 0x6A40 #define IMRLIMIT 0x6A48
+bool soc_is_vtd_capable(void); + +static const struct sa_mmio_descriptor soc_vtd_resources[] = { + { GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, "GFXVTBAR" }, + { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" }, + { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, "VTVC0BAR" }, +}; #endif diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 5597c4f..04a24a1 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -98,6 +98,7 @@ assert(dev != NULL); const config_t *config = dev->chip_info; FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; + FSP_M_TEST_CONFIG *t_cfg = &mupd->FspmTestConfig;
soc_memory_init_params(m_cfg, config);
@@ -109,6 +110,15 @@ /* Set debug probe type */ m_cfg->PlatformDebugConsent = config->DebugConsent;
+ /* Enable VT-d and X2APIC */ + if (!config->ignore_vtd) { + m_cfg->VtdBaseAddress[0] = GFXVT_BASE_ADDRESS; + m_cfg->VtdBaseAddress[1] = IPUVT_BASE_ADDRESS; + m_cfg->VtdBaseAddress[2] = VTVC0_BASE_ADDRESS; + m_cfg->X2ApicOptOut = 0; + t_cfg->VtdDisable = 0; + } + mainboard_memory_init_params(mupd); }
diff --git a/src/soc/intel/cannonlake/romstage/systemagent.c b/src/soc/intel/cannonlake/romstage/systemagent.c index 61db22e..6fb3917 100644 --- a/src/soc/intel/cannonlake/romstage/systemagent.c +++ b/src/soc/intel/cannonlake/romstage/systemagent.c @@ -16,10 +16,31 @@ */
#include <device/device.h> +#include <device/pci_ops.h> #include <intelblocks/systemagent.h> #include <soc/iomap.h> +#include <soc/pci_devs.h> #include <soc/romstage.h> #include <soc/systemagent.h> +#include "chip.h" + +static void systemagent_vtd_init(void) +{ + const struct device *const dev = dev_find_slot(0, SA_DEVFN_ROOT); + const struct soc_intel_cannonlake_config *config = NULL; + + if (dev) + config = dev->chip_info; + if (config && config->ignore_vtd) + return; + + const bool vtd_capable = + !(pci_read_config32(SA_DEV_ROOT, CAPID0_A) & VTD_DISABLE); + if (!vtd_capable) + return; + + sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources)); +}
void systemagent_early_init(void) { @@ -40,6 +61,9 @@ /* Set Fixed MMIO address into MCH base address */ sa_set_mch_bar(soc_fixed_mch_resources, ARRAY_SIZE(soc_fixed_mch_resources)); + + systemagent_vtd_init(); + /* Enable PAM registers */ enable_pam_region(); } diff --git a/src/soc/intel/cannonlake/systemagent.c b/src/soc/intel/cannonlake/systemagent.c index 9c8d761..4ff4c1c 100644 --- a/src/soc/intel/cannonlake/systemagent.c +++ b/src/soc/intel/cannonlake/systemagent.c @@ -18,10 +18,19 @@ #include <device/device.h> #include <delay.h> #include <device/pci.h> +#include <device/pci_ops.h> #include <intelblocks/systemagent.h> #include <soc/cpu.h> #include <soc/iomap.h> #include <soc/systemagent.h> +#include "chip.h" + +bool soc_is_vtd_capable(void) +{ + struct device *const root_dev = SA_DEV_ROOT; + return root_dev && + !(pci_read_config32(root_dev, CAPID0_A) & VTD_DISABLE); +}
/* * SoC implementation @@ -31,6 +40,8 @@ */ void soc_add_fixed_mmio_resources(struct device *dev, int *index) { + const struct soc_intel_cannonlake_config *const config = dev->chip_info; + static const struct sa_mmio_descriptor soc_fixed_resources[] = { { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, "PCIEXBAR" }, @@ -54,6 +65,11 @@
sa_add_fixed_mmio_resources(dev, index, soc_fixed_resources, ARRAY_SIZE(soc_fixed_resources)); + + if (!(config && config->ignore_vtd) && soc_is_vtd_capable()) { + sa_add_fixed_mmio_resources(dev, index, soc_vtd_resources, + ARRAY_SIZE(soc_vtd_resources)); + } }
/*
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate ACPI DMAR table ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31917/1/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/1/src/soc/intel/cannonlake/include/soc... PS1, Line 36: 0x5da8 : #define GFXVTBAR 0x5400 : #define EDRAMBAR 0x5408 : #define IPUVTBAR 0x7884 : #define VTVC0BAR 0x5410 : #define REGBAR maybe use tab ?
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#2).
Change subject: soc/intel/cnl: Generate ACPI DMAR table ......................................................................
soc/intel/cnl: Generate ACPI DMAR table
SoC is detected Vt-d capable. ACPI DMAR table is created and DMA-remapping hardware units entries are programmed. Device scope like IOAPIC and HPET will be added once cnl fsp upd update is available.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/chip.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 8 files changed, 154 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/2
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate ACPI DMAR table ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31917/1/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/1/src/soc/intel/cannonlake/include/soc... PS1, Line 36: 0x5da8 : #define GFXVTBAR 0x5400 : #define EDRAMBAR 0x5408 : #define IPUVTBAR 0x7884 : #define VTVC0BAR 0x5410 : #define REGBAR
maybe use tab ?
done
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Set Ready For Review
Hello Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, Lijian Zhao, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#10).
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
TEST=Booted kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 5 files changed, 151 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/10
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 10:
(12 comments)
https://review.coreboot.org/#/c/31917/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31917/3//COMMIT_MSG@8 PS3, Line 8: : SoC is detected Vt-d capable. ACPI DMAR table is : created and DMA-remapping hardware units entries : are programmed. Device scope like IOAPIC and HPET : will be added once cnl fsp upd update is available.
done
Paul, why? also, the usual width for Git commit messages is 72 chars.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@298 PS10, Line 298: and in 64-bit space It's a 64-bit register and the mask is even narrower, so it's always in 64-bit space.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@315 PS10, Line 315: in 64-bit space. see above
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@329 PS10, Line 329: p2sb_dev This doesn't seem to be used?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@333 PS10, Line 333: and in 64-bit space see above
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@337 PS10, Line 337: const u16 ibdf = (V_P2SB_CFG_IBDF_BUS << 8) | : (V_P2SB_CFG_IBDF_DEV << 3) | V_P2SB_CFG_IBDF_FUNC; : const u16 hbdf = (V_P2SB_CFG_HBDF_BUS << 8) | : (V_P2SB_CFG_HBDF_DEV << 3) | V_P2SB_CFG_HBDF_FUNC; : Why not just read the registers instead? That would save us from any doubts if the constants are in sync with them.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@362 PS10, Line 362: !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED) Why this check? are there dependencies between the vtd units?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 43: 0xfffffff000ull The mask seems one bit too long. I guess it should be
0x7ffffff000ull
i.e. bits 38:12 set.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 55: { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" }, I guess it won't hurt but this resource doesn't seem to exist on CFL.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 59: #define V_P2SB_CFG_IBDF_BUS 0 : #define V_P2SB_CFG_IBDF_DEV 30 : #define V_P2SB_CFG_IBDF_FUNC 7 : #define V_P2SB_CFG_HBDF_BUS 0 : #define V_P2SB_CFG_HBDF_DEV 30 : #define V_P2SB_CFG_HBDF_FUNC 6 Where do these numbers come from? When are the respective registers set?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; : Why have a `VtdDisable` option at all? Nobody seems to use it and what could be the reason?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources)); The BWG mentions another register to be programmed first in the IPU's PCI configuration space. Is this already handled?
Hello Aaron Durbin, Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, Lijian Zhao, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#11).
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
TEST=Booted kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 5 files changed, 157 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/11
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 11:
(11 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@298 PS10, Line 298: and in 64-bit space
It's a 64-bit register and the mask is even narrower, so it's always in 64-bit space.
done.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@315 PS10, Line 315: in 64-bit space.
see above
done
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@329 PS10, Line 329: p2sb_dev
This doesn't seem to be used?
Agreed as p2sb_dev was intended to be referred while trying to read ibdf and hbdf. Removed p2sb_dev here.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@333 PS10, Line 333: and in 64-bit space
see above
done
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@337 PS10, Line 337: const u16 ibdf = (V_P2SB_CFG_IBDF_BUS << 8) | : (V_P2SB_CFG_IBDF_DEV << 3) | V_P2SB_CFG_IBDF_FUNC; : const u16 hbdf = (V_P2SB_CFG_HBDF_BUS << 8) | : (V_P2SB_CFG_HBDF_DEV << 3) | V_P2SB_CFG_HBDF_FUNC; :
Why not just read the registers instead? That would save us from any […]
p2sb access is blocked after POSTBOOT_SAI, directly dreading of ibdf and hbdf would get invalid values 0xffff. ibdf=pci_read_config16(p2sb_dev, PCH_P2SB_IBDF) hbdf=pci_read_config16(p2sb_dev, PCH_P2SB_HBDF) so we sync with fsp and program ibdf & hbdf here.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@362 PS10, Line 362: !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED)
Why this check? are there dependencies between the vtd units?
If VtdDisable is false, sa_set_mch_bar(soc_vtd_resources, ...) would program the base and set the enable bit. Apart from iGFX, I guess the additional devices enable bit check would help NOT generating dmar table if VtdDisable is set true.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 43: 0xfffffff000ull
The mask seems one bit too long. I guess it should be […]
done
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 55: { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" },
I guess it won't hurt but this resource doesn't seem to exist on CFL.
It is updated to differentiate platforms.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 59: #define V_P2SB_CFG_IBDF_BUS 0 : #define V_P2SB_CFG_IBDF_DEV 30 : #define V_P2SB_CFG_IBDF_FUNC 7 : #define V_P2SB_CFG_HBDF_BUS 0 : #define V_P2SB_CFG_HBDF_DEV 30 : #define V_P2SB_CFG_HBDF_FUNC 6
Where do these numbers come from? When are the respective registers set?
Those numbers are referred from platforms(cml/cnl/whl/cfl) fsp setting.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
Why have a `VtdDisable` option at all? Nobody seems to use it and what […]
Platform could refer to this VtdDisalble option through devicetree to disable vtd setting and vmx enable/lock configuration.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
The BWG mentions another register to be programmed first in the IPU's […]
I referred to the bios specification which states the IPU device (B0:D5:F0). It would be assumed that fsp configure properly for platforms like Cannonlake. 1. Verify if IPU device is enabled by reading D0F0RE8h[31] and proceed to 2 if this is cleared. 2. If it is found that IPU is disabled or BIOS desires to disable it, then system bios must clear D0.F0.R54h[10]. 3. System bios must set B0.D0.F5R2Ch bits [31:16] and [15:0] to a value of 0x0000. This operation locks this register. 4. System bios must configure B0.D0.F5.R3Ch bits[7:0] to a suitable value for IPU interrupt line routine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@337 PS10, Line 337: const u16 ibdf = (V_P2SB_CFG_IBDF_BUS << 8) | : (V_P2SB_CFG_IBDF_DEV << 3) | V_P2SB_CFG_IBDF_FUNC; : const u16 hbdf = (V_P2SB_CFG_HBDF_BUS << 8) | : (V_P2SB_CFG_HBDF_DEV << 3) | V_P2SB_CFG_HBDF_FUNC; :
p2sb access is blocked after POSTBOOT_SAI, directly dreading of ibdf and hbdf would get invalid valu […]
We are amidst early boot stages, why is POSTBOOT_SAI already effective?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 59: #define V_P2SB_CFG_IBDF_BUS 0 : #define V_P2SB_CFG_IBDF_DEV 30 : #define V_P2SB_CFG_IBDF_FUNC 7 : #define V_P2SB_CFG_HBDF_BUS 0 : #define V_P2SB_CFG_HBDF_DEV 30 : #define V_P2SB_CFG_HBDF_FUNC 6
Those numbers are referred from platforms(cml/cnl/whl/cfl) fsp setting.
From where? is it document somewhere? or only in FSP source code?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
Platform could refer to this VtdDisalble option through devicetree to disable vtd setting and vmx en […]
But why? the devicetree is about the *device* i.e. hardware. How could the hardware design not support VT-d?
And what has this to do with VMX???
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
I referred to the bios specification which states the IPU device (B0:D5:F0). […]
I was refering to the Cannon Lake BWG. AFAICS, this code here runs before FSP, so any prerequisite has to be handled here too. It's probably just one line.
Hello Aaron Durbin, Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#12).
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
TEST=Booted kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/fsp_params.c M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 6 files changed, 161 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/12
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@337 PS10, Line 337: const u16 ibdf = (V_P2SB_CFG_IBDF_BUS << 8) | : (V_P2SB_CFG_IBDF_DEV << 3) | V_P2SB_CFG_IBDF_FUNC; : const u16 hbdf = (V_P2SB_CFG_HBDF_BUS << 8) | : (V_P2SB_CFG_HBDF_DEV << 3) | V_P2SB_CFG_HBDF_FUNC; :
We are amidst early boot stages, why is POSTBOOT_SAI already effective?
P2sbconfigure/P2sbLock/P2sbSaiLock have been executed in silicon initialization. It appears relative late stage while we try to fill acpi table.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 59: #define V_P2SB_CFG_IBDF_BUS 0 : #define V_P2SB_CFG_IBDF_DEV 30 : #define V_P2SB_CFG_IBDF_FUNC 7 : #define V_P2SB_CFG_HBDF_BUS 0 : #define V_P2SB_CFG_HBDF_DEV 30 : #define V_P2SB_CFG_HBDF_FUNC 6
From where? is it document somewhere? or only in FSP source code?
From both of Cannon Lake PCH bios specification and fsp source code. IOxAPIC: IBDF(0x6c), the static BDF is 0:30:7 HPET: HBDF (0x70), the static BDF is 0:30:6
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
But why? the devicetree is about the *device* i.e. hardware. How […]
Along with other fsp upd elements in the devicetree, VT-d can be optionally configured with VtdDisable flag. If VtdDisable is set true, fsp would skip VT-d configuration and no DMAR table generation. It appears properly to associate vmx with VT-d setting. We have Vtd test application running at uefi shell which validate all relevant VT-d configuration along with vmx enable/lock features.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
I was refering to the Cannon Lake BWG. AFAICS, this code here […]
It is assumed you refer to the following: DMA Remapping hardware unit #2, IPUVTDBAR (for IPU) – BIOS must provide a 4KB memory region with the base address programmed in IPU Configuration register B0.D5.F0.R0F4h[31:0] and MCHBAR IPUVTDBAR register offset 0x7884. The IPU configuration register should be programmed first and BIOS should write to higher offset F4h first and then write to lower offset F0h. Next the MCHBAR TBD offset must be programmed with higher DW offset first and then write lower DW offset followed by enable bit. BIOS must make sure IPU DMA remapping engine base address values in IPU Configuration space and MCHBAR are the same.
Coreboot configures IPUVTDBAR (offset 0x7884) first. fsp code implements the following:
PciSegmentWrite32 (PCI_SEGMENT_LIB_ADDRESS (SA_SEG_NUM, SA_IPU_BUS_NUM, SA_IPU_DEV_NUM, SA_IPU_FUN_NUM, R_SA_VTD_IPU_UBAR), 0);
PciSegmentWrite32 (PCI_SEGMENT_LIB_ADDRESS (SA_SEG_NUM, SA_IPU_BUS_NUM, SA_IPU_DEV_NUM, SA_IPU_FUN_NUM, R_SA_VTD_IPU_LBAR), Data32Or);
MmioWrite32 (MchBar + R_SA_MCHBAR_VTD2_HIGH_OFFSET, 0); MmioWrite32 (MchBar + R_SA_MCHBAR_VTD2_LOW_OFFSET, Data32Or);
It seems the configuration has been properly handled.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 12:
(11 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@329 PS10, Line 329: p2sb_dev
Agreed as p2sb_dev was intended to be referred while trying to read ibdf and hbdf. […]
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@337 PS10, Line 337: const u16 ibdf = (V_P2SB_CFG_IBDF_BUS << 8) | : (V_P2SB_CFG_IBDF_DEV << 3) | V_P2SB_CFG_IBDF_FUNC; : const u16 hbdf = (V_P2SB_CFG_HBDF_BUS << 8) | : (V_P2SB_CFG_HBDF_DEV << 3) | V_P2SB_CFG_HBDF_FUNC; :
P2sbconfigure/P2sbLock/P2sbSaiLock have been executed in silicon initialization. […]
Well, I have to admit silicon initialization ends in coreboot before filling the tables. But the whole thing was designed before Intel started to hide registers. I think things like POSTBOOT_SAI should only be set right before we hand over execution to the OS, it definitely shouldn't be treated as part of the silicon init. If it's done too early, it makes the whole firmware more fragile, and harder to write anyway, slowing the whole process down. Like in this case, we have to manually check that the code parts are in sync. Seems like a pretty bad design choice.
If you agree to my assessment, please file a bug for FSP that it shouldn't hide registers before the `ready to boot` notifi- cation phase.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@345 PS10, Line 345: ibdf >> 8 Please get rid of the `ibdf` and `hbdf` variables. Forging the register value just to disassemble it again, doesn't make the code very readable.
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/acpi.c@298 PS12, Line 298: and in 32-bit space This makes no sense either. If you don't have the time to read what the code does, just remove that part of the comments.
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/include/so... PS12, Line 53: #if IS_ENABLED(CONFG_SOC_INTEL_CANNONLAKE) SOC_INTEL_CANNONLAKE is always selected when the code in soc/intel/cannonlake/ is used, so you have to check on the more specific configs SOC_INTEL_WHISKEYLAKE and SOC_INTEL_ COFFEELAKE (I'm not sure about Comet Lake?).
Also, IS_ENABLED() is deprecated and there is a typo: CONF*I*G_. Anyway, please use the new CONFIG() macro, e.g.:
#if CONFIG(SOC_INTEL_COFFEELAKE) || CONFIG(SOC_INTEL_WHISKEYLAKE)
Note that this negates the condition.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 43: 0xfffffff000ull
done
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 55: { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" },
It is updated to differentiate platforms.
Done
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/include/so... PS10, Line 59: #define V_P2SB_CFG_IBDF_BUS 0 : #define V_P2SB_CFG_IBDF_DEV 30 : #define V_P2SB_CFG_IBDF_FUNC 7 : #define V_P2SB_CFG_HBDF_BUS 0 : #define V_P2SB_CFG_HBDF_DEV 30 : #define V_P2SB_CFG_HBDF_FUNC 6
From both of Cannon Lake PCH bios specification and fsp source code. […]
Ack
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/romstage/f... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/romstage/f... PS12, Line 119: tconfig->VtdDisable = config->VtdDisable; This is part of FSP-S configuration for CFL/WHL.
We should also set `VtdBaseAddress[3]`, right?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
It appears properly to associate vmx with VT-d setting.
Why? Sorry to bother again and again, I just don't see the relation. VMX usually refers to VT-x. Maybe we just talk about different things here. So let's answer this first: * Does the VmxEnable UPD refer to VT-x?
We have Vtd test application running at uefi shell which validate all relevant VT-d configuration along with vmx enable/lock features.
Would that validation fail if VT-d is disabled, no DMAR tables are present but VMX is enabled?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources)); Thanks for the quote (I'm bound by NDA, so I can't quote these documents before anybody @intel did). It says there:
The IPU configuration register should be programmed first
But you say:
Coreboot configures IPUVTDBAR (offset 0x7884) first.
Do you see the contradiction?
Hello Aaron Durbin, Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#13).
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
TEST=Booted kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 5 files changed, 152 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/13
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 13:
(7 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@337 PS10, Line 337: const u16 ibdf = (V_P2SB_CFG_IBDF_BUS << 8) | : (V_P2SB_CFG_IBDF_DEV << 3) | V_P2SB_CFG_IBDF_FUNC; : const u16 hbdf = (V_P2SB_CFG_HBDF_BUS << 8) | : (V_P2SB_CFG_HBDF_DEV << 3) | V_P2SB_CFG_HBDF_FUNC; :
Well, I have to admit silicon initialization ends in coreboot […]
Literally it was referred to fsp_silicon_init. P2sb access blocked was a known issue and a HSD had been filed a while ago.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@345 PS10, Line 345: ibdf >> 8
Please get rid of the `ibdf` and `hbdf` variables. Forging […]
ibdf and hbdf usage was intended to mirror fsp settings for both of them in the scenario they can be directly retrieved from fsp. Updated.
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/acpi.c@298 PS12, Line 298: and in 32-bit space
This makes no sense either. If you don't have the time to read […]
done
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/include/so... PS12, Line 53: #if IS_ENABLED(CONFG_SOC_INTEL_CANNONLAKE)
SOC_INTEL_CANNONLAKE is always selected when the code in […]
done
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/romstage/f... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/romstage/f... PS12, Line 119: tconfig->VtdDisable = config->VtdDisable;
This is part of FSP-S configuration for CFL/WHL. […]
VtdDisable and VtdBaseAddress[3] had been deprecated from FSP-S. They are in FSP-M. There had been intention to address both of VtDisable and VtdBaseAddress in fsp_memory_init. But it causes build failure because of corresponding upd change has not been landed in https://github.com/intelfsp/fsp. Issue had been raised a while ago. fsp refers default base address values which match coreboot setting. VtdBaseAddress and VtdDisable can be optionally configured when github intelfsp upd has been updated.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
It appears properly to associate vmx with VT-d setting. […]
VmxEnable(VT-x) and VtdDisable (VT-d) are in FSP-M. The Vtd test validation fails if VT-d is enabled, but vmx is not enabled/locked.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
Thanks for the quote (I'm bound by NDA, so I can't quote these […]
If SA device IPU is not fused off, coreboot updates SaIpuEnable flag and fsp configures IPU. Besides the configurable SaIpuConfiguration, I did not realize other settings needed by coreboot. Please share your finding.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/31917/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31917/3//COMMIT_MSG@8 PS3, Line 8: : SoC is detected Vt-d capable. ACPI DMAR table is : created and DMA-remapping hardware units entries : are programmed. Device scope like IOAPIC and HPET : will be added once cnl fsp upd update is available. Because that’s in the documentation [1].
The third and any number of following lines contain a longer description of the commit as is neccessary, including relevant background information and quite possibly rationale for why the issue was solved in this particular way. These lines should never be longer than 75 characters.
The page cannot be edited, so it has been this way since the beginning.
[1]: https://www.coreboot.org/Git#Commit_messages
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 13:
(9 comments)
https://review.coreboot.org/#/c/31917/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31917/3//COMMIT_MSG@8 PS3, Line 8: : SoC is detected Vt-d capable. ACPI DMAR table is : created and DMA-remapping hardware units entries : are programmed. Device scope like IOAPIC and HPET : will be added once cnl fsp upd update is available.
Because that’s in the documentation [1]. […]
Well, it says 75 is the maximum, not that it's the minimum... Narrower columns are generally easier to read for the eyes, so if somebody already formatted things nicely, please don't make them change it.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@298 PS10, Line 298: and in 64-bit space
done.
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@315 PS10, Line 315: in 64-bit space.
done
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@333 PS10, Line 333: and in 64-bit space
done
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@345 PS10, Line 345: ibdf >> 8
ibdf and hbdf usage was intended to mirror fsp settings for both of them in the scenario they can be […]
Ack
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/acpi.c@298 PS12, Line 298: and in 32-bit space
done
Ack
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/include/so... PS12, Line 53: #if IS_ENABLED(CONFG_SOC_INTEL_CANNONLAKE)
done
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
VmxEnable(VT-x) and VtdDisable (VT-d) are in FSP-M. […]
Well, IIRC, the current logic ensures the opposite: disabling VT-x when VT-d is disabled. What we'd need to ensure that this test always passes is to disable VT-d if VT-x is disabled.
But, I'm questioning the test anyway. Where does that requirement come from?
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
If SA device IPU is not fused off, coreboot updates SaIpuEnable flag and fsp configures IPU. […]
I fear, we are talking past each other. I'm merely concerned about the order of the writes to B0.D5.F0+0xf0 and MCHBAR+0x7884.
The BWG says write B0.D5.F0+0xf0 first, but we are writing to MCHBAR+0x7884 first and call FSP then to write the other (and MCHBAR+0x7884 again).
Also, now that I typed this a few times. Is 0x7884 the correct offset? shouldn't it be 8-byte aligned?
Hello Aaron Durbin, Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#14).
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
TEST=Booted kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 5 files changed, 143 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/14
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
Well, IIRC, the current logic ensures the opposite: disabling VT-x when […]
Along with cpu/memory/graphics virtualization, VT-d is one of virtual-machine extension(VMX). I only run the test application at uefi shell. It seems applicable to check the vmx enabled/locked along with virtualization accomplishment like VT-d.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
I fear, we are talking past each other. I'm merely concerned about the […]
Sorry, I was lost and considered others. vtd2_low (0x7880) and vtd2_high(0x7884) are categorized as MCHBAR offset. I do not see alignment issue. The bios specification states base address programmed in B0.D5.F0.R0F4 and MCHBASE offset 0x7884, which appears contradictory to fsp implementation. Literally fsp clears both of R0F4 and 0x7884 and it programs base address into R0F0 and 0x7880 orderly. Agreed with you about the order issue. We just had further update to drop the programming for 0x7884 and change IPUVTBAR value from 0x7884 to 0x7880 to sync with fsp code.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/#/c/31917/14/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/14/src/soc/intel/cannonlake/acpi.c@314 PS14, Line 314: struct device *const ipu_dev = dev_find_slot(0, SA_DEVFN_IPU); : uint64_t ipuvtbar = MCHBAR64(IPUVTBAR) & VTBAR_MASK; : bool ipuvten = MCHBAR32(IPUVTBAR) & VTBAR_ENABLED; : : if (ipu_dev && ipu_dev->enabled && ipuvtbar && ipuvten) { : unsigned long tmp = current; : : current += acpi_create_dmar_drhd(current, 0, 0, ipuvtbar); : current += acpi_create_dmar_ds_pci(current, 0, 5, 0); : : acpi_dmar_drhd_fixup(tmp, current); : } Does this even exist for CFL or CML?
https://review.coreboot.org/#/c/31917/14/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/14/src/soc/intel/cannonlake/include/so... PS14, Line 42: #define VTBAR_ENABLED 0x01 : #define VTBAR_MASK 0x7ffffff000ull These should be placed under GFXVTBAR as they are fields within that register.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31917/14/src/soc/intel/cannonlake/include/so... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/#/c/31917/14/src/soc/intel/cannonlake/include/so... PS14, Line 42: #define VTBAR_ENABLED 0x01 : #define VTBAR_MASK 0x7ffffff000ull
These should be placed under GFXVTBAR as they are fields within that register.
Nvm. I see you are using it both for GFXVTBAR and VTVC0BAR.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
VT-d is one of virtual-machine extension(VMX)
Where can I read about that? Where is the dependency documented?
I only run the test application at uefi shell. It seems applicable to check the vmx enabled/locked along with virtualization accomplishment like VT-d.
I'll try to run my own tests, when I have the time.
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/#/c/31917/14/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/14/src/soc/intel/cannonlake/acpi.c@314 PS14, Line 314: struct device *const ipu_dev = dev_find_slot(0, SA_DEVFN_IPU); : uint64_t ipuvtbar = MCHBAR64(IPUVTBAR) & VTBAR_MASK; : bool ipuvten = MCHBAR32(IPUVTBAR) & VTBAR_ENABLED; : : if (ipu_dev && ipu_dev->enabled && ipuvtbar && ipuvten) { : unsigned long tmp = current; : : current += acpi_create_dmar_drhd(current, 0, 0, ipuvtbar); : current += acpi_create_dmar_ds_pci(current, 0, 5, 0); : : acpi_dmar_drhd_fixup(tmp, current); : }
Does this even exist for CFL or CML?
There would be no impact for CFL and CML as the readings of ipuvtbar & ipuvten are 0.
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
VT-d is one of virtual-machine extension(VMX) […]
Wrong statement. Along with cpu/memory/graphics, VT-d is one set of virtualization features. vmx refers to processor.
Hello Aaron Durbin, Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#15).
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
BUG=b:130351429 TEST=Booted kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 5 files changed, 143 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/15
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/#/c/31917/15/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/15/src/soc/intel/cannonlake/acpi.c@355 PS15, Line 355: !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED)) The early code always sets this bit if the chipset is capable. If this should stay as a way to check if FSP overrode this choice, please add a comment.
But I'd actually prefer that we force any VtdDisable UPD to 0, to make sure that this doesn't happen.
Hello Aaron Durbin, Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, build bot (Jenkins), Lijian Zhao, Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#16).
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c 5 files changed, 145 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/16
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/#/c/31917/15/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/15/src/soc/intel/cannonlake/acpi.c@355 PS15, Line 355: !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED))
The early code always sets this bit if the chipset is capable. […]
Just added additional comments. Yes, VtdDisable (value 0) will be passed from coreboot to fsp once FSP upd change has been integrated to https://github.com/intelfsp/fsp.
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 16: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 16:
(5 comments)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/acpi.c@362 PS10, Line 362: !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED)
If VtdDisable is false, sa_set_mch_bar(soc_vtd_resources, ... […]
Done
https://review.coreboot.org/#/c/31917/15/src/soc/intel/cannonlake/acpi.c File src/soc/intel/cannonlake/acpi.c:
https://review.coreboot.org/#/c/31917/15/src/soc/intel/cannonlake/acpi.c@355 PS15, Line 355: !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED))
Just added additional comments. […]
Ack
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/romstage/f... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/31917/12/src/soc/intel/cannonlake/romstage/f... PS12, Line 119: tconfig->VtdDisable = config->VtdDisable;
VtdDisable and VtdBaseAddress[3] had been deprecated from FSP-S. They are in FSP-M. […]
Ack
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 42: sa_set_mch_bar(soc_vtd_resources, ARRAY_SIZE(soc_vtd_resources));
Sorry, I was lost and considered others. […]
Ah, I've realized now, that you dropped the entry in soc_vtd_resources[]. That fixes the ordering problem, but now the resource won't be reserved.
https://review.coreboot.org/#/c/31917/16/src/soc/intel/cannonlake/systemagen... File src/soc/intel/cannonlake/systemagent.c:
https://review.coreboot.org/#/c/31917/16/src/soc/intel/cannonlake/systemagen... PS16, Line 68: ARRAY_SIZE(soc_vtd_resources)); Here, the resources get reserved, but the IPU resource is missing now.
As we know that FSP will properly set the MCHBAR (and IPU config) registers, maybe we should only reserve soc_vtd_resources[] here and never set them in romstage?
Hello Aaron Durbin, Pratikkumar V Prajapati, Patrick Rudolph, Shamile Khan, Lijian Zhao, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31917
to look at the new patch set (#17).
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/systemagent.c 4 files changed, 130 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/31917/17
John Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/#/c/31917/16/src/soc/intel/cannonlake/systemagen... File src/soc/intel/cannonlake/systemagent.c:
https://review.coreboot.org/#/c/31917/16/src/soc/intel/cannonlake/systemagen... PS16, Line 68: ARRAY_SIZE(soc_vtd_resources));
Here, the resources get reserved, but the IPU resource is missing now. […]
Agree and updated accordingly.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 17: Code-Review+2
(1 comment)
Thanks. Overall, this looks much cleaner now.
I still wonder why we have a VtdDisable in `chip.h`, but that's not added by this patch anyway :)
https://review.coreboot.org/#/c/31917/16/src/soc/intel/cannonlake/systemagen... File src/soc/intel/cannonlake/systemagent.c:
https://review.coreboot.org/#/c/31917/16/src/soc/intel/cannonlake/systemagen... PS16, Line 68: ARRAY_SIZE(soc_vtd_resources));
Agree and updated accordingly.
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... File src/soc/intel/cannonlake/romstage/systemagent.c:
https://review.coreboot.org/#/c/31917/10/src/soc/intel/cannonlake/romstage/s... PS10, Line 32: if (dev) : config = dev->chip_info; : if (config && config->VtdDisable) : return; :
Wrong statement. Along with cpu/memory/graphics, VT-d is one set of virtualization features. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 17: Code-Review+2
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
Patch Set 17: Code-Review+2
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31917 )
Change subject: soc/intel/cnl: Generate DMAR ACPI table ......................................................................
soc/intel/cnl: Generate DMAR ACPI table
The platform supports Virtualization Technology for Directed I/O. Generate DMAR acpi table if VT-d feature is enabled.
BUG=b:130351429 TEST=Booted to kernel and verified the DMAR table contents.
Change-Id: I4e1ee5244c67affb13947436d81628c5dc665c9e Signed-off-by: John Zhao john.zhao@intel.com Signed-off-by: Pratik Prajapati pratikkumar.v.prajapati@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31917 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Lijian Zhao lijian.zhao@intel.com --- M src/soc/intel/cannonlake/acpi.c M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/systemagent.c 4 files changed, 130 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Lijian Zhao: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/acpi.c b/src/soc/intel/cannonlake/acpi.c index 639f6c6..43d91d3 100644 --- a/src/soc/intel/cannonlake/acpi.c +++ b/src/soc/intel/cannonlake/acpi.c @@ -17,19 +17,22 @@
#include <arch/acpi.h> #include <arch/acpigen.h> -#include <device/mmio.h> #include <arch/smp/mpspec.h> #include <cbmem.h> #include <chip.h> +#include <device/mmio.h> +#include <device/pci_ops.h> #include <ec/google/chromeec/ec.h> #include <intelblocks/cpulib.h> #include <intelblocks/pmclib.h> #include <intelblocks/acpi.h> +#include <intelblocks/p2sb.h> #include <soc/cpu.h> #include <soc/iomap.h> #include <soc/nvs.h> #include <soc/pci_devs.h> #include <soc/pm.h> +#include <soc/systemagent.h> #include <string.h> #include <vendorcode/google/chromeos/gnvs.h> #include <wrdd.h> @@ -285,3 +288,80 @@ { return acpigen_soc_gpio_op("\_SB.PCI0.CTXS", gpio_num); } + +static unsigned long soc_fill_dmar(unsigned long current) +{ + struct device *const igfx_dev = dev_find_slot(0, SA_DEVFN_IGD); + uint64_t gfxvtbar = MCHBAR64(GFXVTBAR) & VTBAR_MASK; + bool gfxvten = MCHBAR32(GFXVTBAR) & VTBAR_ENABLED; + + if (igfx_dev && igfx_dev->enabled && gfxvtbar && gfxvten) { + unsigned long tmp = current; + + current += acpi_create_dmar_drhd(current, 0, 0, gfxvtbar); + current += acpi_create_dmar_ds_pci(current, 0, 2, 0); + + acpi_dmar_drhd_fixup(tmp, current); + + /* Add RMRR entry */ + tmp = current; + current += acpi_create_dmar_rmrr(current, 0, + sa_get_gsm_base(), sa_get_tolud_base() - 1); + current += acpi_create_dmar_ds_pci(current, 0, 2, 0); + acpi_dmar_rmrr_fixup(tmp, current); + } + + struct device *const ipu_dev = dev_find_slot(0, SA_DEVFN_IPU); + uint64_t ipuvtbar = MCHBAR64(IPUVTBAR) & VTBAR_MASK; + bool ipuvten = MCHBAR32(IPUVTBAR) & VTBAR_ENABLED; + + if (ipu_dev && ipu_dev->enabled && ipuvtbar && ipuvten) { + unsigned long tmp = current; + + current += acpi_create_dmar_drhd(current, 0, 0, ipuvtbar); + current += acpi_create_dmar_ds_pci(current, 0, 5, 0); + + acpi_dmar_drhd_fixup(tmp, current); + } + + uint64_t vtvc0bar = MCHBAR64(VTVC0BAR) & VTBAR_MASK; + bool vtvc0en = MCHBAR32(VTVC0BAR) & VTBAR_ENABLED; + + if (vtvc0bar && vtvc0en) { + const unsigned long tmp = current; + + current += acpi_create_dmar_drhd(current, + DRHD_INCLUDE_PCI_ALL, 0, vtvc0bar); + current += acpi_create_dmar_ds_ioapic(current, + 2, V_P2SB_CFG_IBDF_BUS, V_P2SB_CFG_IBDF_DEV, + V_P2SB_CFG_IBDF_FUNC); + current += acpi_create_dmar_ds_msi_hpet(current, + 0, V_P2SB_CFG_HBDF_BUS, V_P2SB_CFG_HBDF_DEV, + V_P2SB_CFG_HBDF_FUNC); + + acpi_dmar_drhd_fixup(tmp, current); + } + + return current; +} + +unsigned long sa_write_acpi_tables(struct device *dev, unsigned long current, + struct acpi_rsdp *rsdp) +{ + acpi_dmar_t *const dmar = (acpi_dmar_t *)current; + + /* Create DMAR table only if we have VT-d capability + * and FSP does not override its feature. + */ + if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE) || + !(MCHBAR32(VTVC0BAR) & VTBAR_ENABLED)) + return current; + + printk(BIOS_DEBUG, "ACPI: * DMAR\n"); + acpi_create_dmar(dmar, DMAR_INTR_REMAP, soc_fill_dmar); + current += dmar->header.length; + current = acpi_align_current(current); + acpi_add_table(rsdp, dmar); + + return current; +} diff --git a/src/soc/intel/cannonlake/include/soc/iomap.h b/src/soc/intel/cannonlake/include/soc/iomap.h index ed9b29f..100bd11 100644 --- a/src/soc/intel/cannonlake/include/soc/iomap.h +++ b/src/soc/intel/cannonlake/include/soc/iomap.h @@ -52,6 +52,15 @@ #define EDRAM_BASE_ADDRESS 0xfed80000 #define EDRAM_BASE_SIZE 0x4000
+#define GFXVT_BASE_ADDRESS 0xfed90000 +#define GFXVT_BASE_SIZE 0x1000 + +#define IPUVT_BASE_ADDRESS 0xfed92000 +#define IPUVT_BASE_SIZE 0x1000 + +#define VTVC0_BASE_ADDRESS 0xfed91000 +#define VTVC0_BASE_SIZE 0x1000 + #define REG_BASE_ADDRESS 0xfc000000 #define REG_BASE_SIZE 0x1000
diff --git a/src/soc/intel/cannonlake/include/soc/systemagent.h b/src/soc/intel/cannonlake/include/soc/systemagent.h index da83c38..3bda9e8 100644 --- a/src/soc/intel/cannonlake/include/soc/systemagent.h +++ b/src/soc/intel/cannonlake/include/soc/systemagent.h @@ -30,10 +30,17 @@ #define D_LCK (1 << 4) #define G_SMRAME (1 << 3) #define C_BASE_SEG ((0 << 2) | (1 << 1) | (0 << 0)) +#define CAPID0_A 0xe4 +#define VTD_DISABLE (1 << 23)
#define BIOS_RESET_CPL 0x5da8 +#define GFXVTBAR 0x5400 #define EDRAMBAR 0x5408 +#define IPUVTBAR 0x7880 +#define VTVC0BAR 0x5410 #define REGBAR 0x5420 +#define VTBAR_ENABLED 0x01 +#define VTBAR_MASK 0x7ffffff000ull
#define MCH_PKG_POWER_LIMIT_LO 0x59a0 #define MCH_PKG_POWER_LIMIT_HI 0x59a4 @@ -43,4 +50,24 @@ #define IMRBASE 0x6A40 #define IMRLIMIT 0x6A48
+#if CONFIG(SOC_INTEL_COFFEELAKE) || CONFIG(SOC_INTEL_WHISKEYLAKE) \ + || CONFIG(SOC_INTEL_COMETLAKE) +static const struct sa_mmio_descriptor soc_vtd_resources[] = { + { GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, "GFXVTBAR" }, + { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, "VTVC0BAR" }, +}; +#else +static const struct sa_mmio_descriptor soc_vtd_resources[] = { + { GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, "GFXVTBAR" }, + { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" }, + { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, "VTVC0BAR" }, +}; +#endif + +#define V_P2SB_CFG_IBDF_BUS 0 +#define V_P2SB_CFG_IBDF_DEV 30 +#define V_P2SB_CFG_IBDF_FUNC 7 +#define V_P2SB_CFG_HBDF_BUS 0 +#define V_P2SB_CFG_HBDF_DEV 30 +#define V_P2SB_CFG_HBDF_FUNC 6 #endif diff --git a/src/soc/intel/cannonlake/systemagent.c b/src/soc/intel/cannonlake/systemagent.c index 9c8d761..d850b15 100644 --- a/src/soc/intel/cannonlake/systemagent.c +++ b/src/soc/intel/cannonlake/systemagent.c @@ -18,10 +18,12 @@ #include <device/device.h> #include <delay.h> #include <device/pci.h> +#include <device/pci_ops.h> #include <intelblocks/systemagent.h> #include <soc/cpu.h> #include <soc/iomap.h> #include <soc/systemagent.h> +#include "chip.h"
/* * SoC implementation @@ -31,6 +33,8 @@ */ void soc_add_fixed_mmio_resources(struct device *dev, int *index) { + const struct soc_intel_cannonlake_config *const config = dev->chip_info; + static const struct sa_mmio_descriptor soc_fixed_resources[] = { { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, "PCIEXBAR" }, @@ -54,6 +58,15 @@
sa_add_fixed_mmio_resources(dev, index, soc_fixed_resources, ARRAY_SIZE(soc_fixed_resources)); + + /* Add Vt-d resources if VT-d is enabled. */ + if ((pci_read_config32(dev, CAPID0_A) & VTD_DISABLE)) + return; + + if (!(config && config->VtdDisable)) { + sa_add_fixed_mmio_resources(dev, index, soc_vtd_resources, + ARRAY_SIZE(soc_vtd_resources)); + } }
/*
-
Furquan Shaikh (Code Review) -
HAOUAS Elyes (Code Review)
-
John Zhao (Code Review)
-
Lijian Zhao (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)