On 5/12/23 11:44, Thomas Gleixner wrote:
On Thu, May 04 2023 at 14:50, Ross Philipson wrote:
The routine slaunch_setup is called out of the x86 specific setup_arch
Can you please make functions visible in changelogs by appending (),
i.e. setup_arch() ?
Yes I will.
See https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/maintainer-tip.html__;!!ACWV5N9M2RV99hQ!IpJMDBpAvJRDAh0tZI_nMv0zZqwQDnxFjBEKRitYq4JU-iV-NnXg28lGtTwb1ynVA4XEy5n9aSdIekxkztyZ$
for further hints.
+static u32 sl_flags;
+static struct sl_ap_wake_info ap_wake_info;
+static u64 evtlog_addr;
+static u32 evtlog_size;
+static u64 vtd_pmr_lo_size;
Is any of this modifyable after boot? If not then this wants to be
annotated with __ro_after_init.
I believe you are correct and these are never modified after boot so I
will do this.
+/* This should be plenty of room */
+static u8 txt_dmar[PAGE_SIZE] __aligned(16);
+
+u32 slaunch_get_flags(void)
+{
+ return sl_flags;
+}
+EXPORT_SYMBOL(slaunch_get_flags);
What needs this export? If there is a reason then please EXPORT_SYMBOL_GPL()
I think that may be incorrect. I will look into it.
+struct sl_ap_wake_info *slaunch_get_ap_wake_info(void)
+{
+ return &ap_wake_info;
+}
If you return a pointer, then there is not much of point for encapsulating.
I am sorry, I am not 100% sure what you mean.
+struct acpi_table_header *slaunch_get_dmar_table(struct acpi_table_header *dmar)
Some explanation on public visible functions would be really useful.
I can add that.
+{
+ /* The DMAR is only stashed and provided via TXT on Intel systems */
-ENOPARSE.
I take it you mean you don't understand the comment. I will try to make
it clearer.
+ if (memcmp(txt_dmar, "DMAR", 4))
+ return dmar;
+
+ return (struct acpi_table_header *)(&txt_dmar[0]);
s/&txt_dmar[0]/txt_dmar/ No?
Just an old habit. I can change it.
+}
+void __noreturn slaunch_txt_reset(void __iomem *txt,
+ const char *msg, u64 error)
Please avoid these line breaks. We lifted the 80 character limit quite
some time ago.
Ack
+
+ /* Iterate over heap tables looking for table of "type" */
+ for (i = 0; i < type; i++) {
+ base += offset;
+ heap = early_memremap(base, sizeof(u64));
+ if (!heap)
+ slaunch_txt_reset(txt,
+ "Error early_memremap of heap for heap walk\n",
+ SL_ERROR_HEAP_MAP);
This is horrible to read.
if (!heap) {
slaunch_txt_reset(txt, "Error early_memremap of heap for heap walk\n",
SL_ERROR_HEAP_MAP);
}
See documentation about bracket rules.
Will do.
+
+/*
+ * TXT stashes a safe copy of the DMAR ACPI table to prevent tampering.
+ * It is stored in the TXT heap. Fetch it from there and make it available
+ * to the IOMMU driver.
+ */
+static void __init slaunch_copy_dmar_table(void __iomem *txt)
+{
+ struct txt_sinit_mle_data *sinit_mle_data;
+ u32 field_offset, dmar_size, dmar_offset;
+ void *dmar;
+
+ memset(&txt_dmar, 0, PAGE_SIZE);
txt_dmar is statically allocated so it's already zero, no?
Yes. This may be left over from an older iteration of the patches. I
will ditch it.
+/*
+ * Intel TXT specific late stub setup and validation.
+ */
+void __init slaunch_setup_txt(void)
+{
+ u64 one = TXT_REGVALUE_ONE, val;
+ void __iomem *txt;
+
+ if (!boot_cpu_has(X86_FEATURE_SMX))
+ return;
+
+ /*
+ * If booted through secure launch entry point, the loadflags
+ * option will be set.
+ */
+ if (!(boot_params.hdr.loadflags & SLAUNCH_FLAG))
+ return;
+
+ /*
+ * See if SENTER was done by reading the status register in the
+ * public space. If the public register space cannot be read, TXT may
+ * be disabled.
+ */
+ txt = early_ioremap(TXT_PUB_CONFIG_REGS_BASE,
+ TXT_NR_CONFIG_PAGES * PAGE_SIZE);
+ if (!txt)
+ return;
Wait. You have established above that SMX is available and the boot has
set the SLAUNCH flag.
So if that ioremap() fails then there is an issue with the fixmaps.
How is returning here sensible? The system will just die later on in the
worst case with some undecodable issue.
Good point. I don't think I can do a TXT reset at this point but I could
panic.
Thanks for the review,
Ross
Thanks,
tglx
