Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
[WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0
These platforms return to romstage from FSP only after already having torn CAR down. A copy of the entire CAR region is available and discoverable via HOB.
Previously, CBMEM console detected on-the-fly that CAR migration had happened and relocated cbmem_console_p accoringlin with car_sync_var(). However, if the CAR_GLOBAL pointing to another object inside CAR is a relative offset instead, we have a more generic solution that can be used with timestamps code as well.
Change-Id: Ica877b47e68d56189e9d998b5630019d4328a419 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/arm/include/arch/early_variables.h M src/arch/arm64/include/arch/early_variables.h M src/arch/mips/include/arch/early_variables.h M src/arch/ppc64/include/arch/early_variables.h M src/arch/riscv/include/arch/early_variables.h M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/car.c M src/lib/cbmem_console.c 8 files changed, 49 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35140/1
diff --git a/src/arch/arm/include/arch/early_variables.h b/src/arch/arm/include/arch/early_variables.h index 9f06923..e559e9b 100644 --- a/src/arch/arm/include/arch/early_variables.h +++ b/src/arch/arm/include/arch/early_variables.h @@ -18,7 +18,9 @@
static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) do { (var) = (val); } while (0)
+#define car_get_reloc_ptr car_get_var +#define car_set_reloc_ptr car_set_var + #endif diff --git a/src/arch/arm64/include/arch/early_variables.h b/src/arch/arm64/include/arch/early_variables.h index 9f06923..e559e9b 100644 --- a/src/arch/arm64/include/arch/early_variables.h +++ b/src/arch/arm64/include/arch/early_variables.h @@ -18,7 +18,9 @@
static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) do { (var) = (val); } while (0)
+#define car_get_reloc_ptr car_get_var +#define car_set_reloc_ptr car_set_var + #endif diff --git a/src/arch/mips/include/arch/early_variables.h b/src/arch/mips/include/arch/early_variables.h index 8fc9c19..32f4b6b 100644 --- a/src/arch/mips/include/arch/early_variables.h +++ b/src/arch/mips/include/arch/early_variables.h @@ -19,7 +19,9 @@
static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) { (var) = (val); }
+#define car_get_reloc_ptr car_get_var +#define car_set_reloc_ptr car_set_var + #endif /* __MIPS_ARCH_EARLY_VARIABLES_H */ diff --git a/src/arch/ppc64/include/arch/early_variables.h b/src/arch/ppc64/include/arch/early_variables.h index 22c3750..539e7f9 100644 --- a/src/arch/ppc64/include/arch/early_variables.h +++ b/src/arch/ppc64/include/arch/early_variables.h @@ -19,8 +19,9 @@ #define CAR_MIGRATE(migrate_fn_) static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) - #define car_set_var(var, val) do { (var) = (val); } while (0)
+#define car_get_reloc_ptr car_get_var +#define car_set_reloc_ptr car_set_var + #endif diff --git a/src/arch/riscv/include/arch/early_variables.h b/src/arch/riscv/include/arch/early_variables.h index acc5ac3..69f5f56 100644 --- a/src/arch/riscv/include/arch/early_variables.h +++ b/src/arch/riscv/include/arch/early_variables.h @@ -21,7 +21,9 @@ #define CAR_MIGRATE(migrate_fn_) static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) do { (var) = (val); } while (0)
+#define car_get_reloc_ptr car_get_var +#define car_set_reloc_ptr car_set_var + #endif diff --git a/src/arch/x86/include/arch/early_variables.h b/src/arch/x86/include/arch/early_variables.h index 3910a82..4b29fa3 100644 --- a/src/arch/x86/include/arch/early_variables.h +++ b/src/arch/x86/include/arch/early_variables.h @@ -33,19 +33,23 @@ /* Get the correct pointer for the CAR global variable. */ void *car_get_var_ptr(void *var);
-/* Get and update a CAR_GLOBAL pointing elsewhere in car.global_data*/ -void *car_sync_var_ptr(void *var); - /* Return 1 when currently running with globals in Cache-as-RAM, 0 otherwise. */ int car_active(void);
/* Get and set a primitive type global variable. */ #define car_get_var(var) \ (*(typeof(var) *)car_get_var_ptr(&(var))) -#define car_sync_var(var) \ - (*(typeof(var) *)car_sync_var_ptr(&(var))) #define car_set_var(var, val) car_get_var(var) = (val)
+/* Get and set a CAR_GLOBAL pointing elsewhere inside CAR. */ +#if CONFIG(PLATFORM_USES_FSP1_0) +void *car_get_reloc_ptr(void *var); +void car_set_reloc_ptr(void *var, void *tgt); +#else +#define car_get_reloc_ptr car_get_var +#define car_set_reloc_ptr car_set_var +#endif + static inline size_t car_data_size(void) { size_t car_size = _car_relocatable_data_size; @@ -66,9 +70,11 @@
#define CAR_GLOBAL #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) (var) = (val)
+#define car_get_reloc_ptr car_get_var +#define car_set_reloc_ptr car_set_var + static inline void *car_get_var_ptr(void *var) { return var; diff --git a/src/cpu/x86/car.c b/src/cpu/x86/car.c index 6fc6168..285e29f 100644 --- a/src/cpu/x86/car.c +++ b/src/cpu/x86/car.c @@ -77,42 +77,31 @@ return &migrated_base[offset]; }
+#if CONFIG(PLATFORM_USES_FSP1_0) /* - * Update a CAR_GLOBAL variable var, originally pointing to CAR region, - * with the address in migrated CAR region in DRAM. + * Pointers to CAR_GLOBALs use relative addressing, this way discovery + * of objects that are not CAR_GLOBALs themselves, remain discoverable + * after CAR migration has happened. */ -void *car_sync_var_ptr(void *var) +void car_set_reloc_ptr(void *var, void *dst) { - void **mig_var = car_get_var_ptr(var); - void *_car_start = _car_relocatable_data_start; - void *_car_end = _car_relocatable_data_end; - - /* Not moved or migrated yet. */ - if (mig_var == var) - return mig_var; - - /* - * Migrate the cbmem console pointer for FSP 1.0 platforms. Otherwise, - * keep console buffer in CAR until cbmemc_reinit() moves it. - */ - if (*mig_var == _preram_cbmem_console) { - if (CONFIG(PLATFORM_USES_FSP1_0)) - *mig_var += (char *)mig_var - (char *)var; - return mig_var; - } - - /* It's already pointing outside car.global_data. */ - if (*mig_var < _car_start || *mig_var > _car_end) - return mig_var; - - /* Move the pointer by the same amount the variable storing it was - * moved by. - */ - *mig_var += (char *)mig_var - (char *)var; - - return mig_var; + uintptr_t *mig_var = car_get_var_ptr(var); + if (dst) + *mig_var = (uintptr_t)dst - (uintptr_t)mig_var; + else + *mig_var = 0; }
+void *car_get_reloc_ptr(void *var) +{ + uintptr_t *mig_var = car_get_var_ptr(var); + if (*mig_var) + return (void *)((uintptr_t)mig_var + *mig_var); + else + return NULL; +} +#endif + int car_active(void) { return !car_migrated; diff --git a/src/lib/cbmem_console.c b/src/lib/cbmem_console.c index df2d1ff..1987a99 100644 --- a/src/lib/cbmem_console.c +++ b/src/lib/cbmem_console.c @@ -73,12 +73,12 @@
static struct cbmem_console *current_console(void) { - return car_sync_var(cbmem_console_p); + return car_get_reloc_ptr(cbmem_console_p); }
static void current_console_set(struct cbmem_console *new_console_p) { - car_set_var(cbmem_console_p, new_console_p); + car_set_reloc_ptr(cbmem_console_p, new_console_p); }
static int buffer_valid(struct cbmem_console *cbm_cons_p, u32 total_space)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
Patch Set 1:
This is untested dependency for the followup cleanups on car.ld.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
Patch Set 1: Code-Review+1
Hello Werner Zeh, Aaron Durbin, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35140
to look at the new patch set (#2).
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
[WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0
These platforms return to romstage from FSP only after already having torn CAR down. A copy of the entire CAR region is available and discoverable via HOB.
Previously, CBMEM console detected on-the-fly that CAR migration had happened and relocated cbmem_console_p accoringlin with car_sync_var(). However, if the CAR_GLOBAL pointing to another object inside CAR is a relative offset instead, we have a more generic solution that can be used with timestamps code as well.
Change-Id: Ica877b47e68d56189e9d998b5630019d4328a419 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/arm/include/arch/early_variables.h M src/arch/arm64/include/arch/early_variables.h M src/arch/mips/include/arch/early_variables.h M src/arch/ppc64/include/arch/early_variables.h M src/arch/riscv/include/arch/early_variables.h M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/car.c M src/lib/cbmem_console.c 8 files changed, 50 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35140/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35140/2/src/cpu/x86/car.c File src/cpu/x86/car.c:
https://review.coreboot.org/c/coreboot/+/35140/2/src/cpu/x86/car.c@97 PS2, Line 97: uintptr_t *mig_var = car_get_var_ptr(var); Could you add some comments here?
We're getting the migrated pointer and subtracting the value at this new slot?
I'm commenting my thought process here as it's not immediately obvious to me.
struct foo { void *bar; } baz;
&baz == &baz.bar prior to relocation, say it's address A.
baz.bar points to something else, address is B but let's say just after baz, A + sizeof(struct foo); i.e. B = A + sizeof(struct foo).
Things get moved. A -> A' = A - 0x1000 (arbitrary offset for explanation).
bar.baz should have a value of B' = B - 0x1000 == A' + sizeof(struct foo).
So this code: mig_var = car_get_var_ptr(&bar.baz) /* A' */ if (*mig_var) /* *mig_var = B */ return A' - B;
A - 0x1000 - B = A - 0x1000 - (A + sizeof(struct foo) = -0x1000 - sizeof(struct foo)
That math doesn't work in my mind. Can you explain further?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35140/2/src/cpu/x86/car.c File src/cpu/x86/car.c:
https://review.coreboot.org/c/coreboot/+/35140/2/src/cpu/x86/car.c@97 PS2, Line 97: uintptr_t *mig_var = car_get_var_ptr(var);
Could you add some comments here?
Yes, if patchset #2 actually works with FSP1.0...
We're getting the migrated pointer and subtracting the value at this new slot?
My logic here is that since FSP1.0 migrates the entire _car_region (implicitly), absolute pointers to CAR region become stale, but pointers to CAR that are relative to a CAR_GLOBAL remain valid.
So this code: mig_var = car_get_var_ptr(&bar.baz) /* A' */ if (*mig_var) /* *mig_var = B */ return A' - B;
But value in *mig_var = (A - B) == -sizeof(struct foo).
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35140/2/src/cpu/x86/car.c File src/cpu/x86/car.c:
https://review.coreboot.org/c/coreboot/+/35140/2/src/cpu/x86/car.c@97 PS2, Line 97: uintptr_t *mig_var = car_get_var_ptr(var);
Could you add some comments here?
Yes, if patchset #2 actually works with FSP1.0...
We're getting the migrated pointer and subtracting the value at this new slot?
My logic here is that since FSP1.0 migrates the entire _car_region (implicitly), absolute pointers to CAR region become stale, but pointers to CAR that are relative to a CAR_GLOBAL remain valid.
I get the intent because that was what sync_var_ptr() was previous doing. I do see what you are doing now (it wasn't clear earlier). I have more comments on patchset 4.
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c File src/cpu/x86/car.c:
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c@82 PS4, Line 82: Pointers to CAR_GLOBALs use relative addressing What ensures dst is a location in CAR_GLOBALs region?
I think this comment needs to be expanded upon because it's not immediately clear how deliberately this code is manipulating the values being stored in pointer objects. It's also pretty dangerous to do this as the code utilizing these functions has no idea that a pointer is no longer a pointer.
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c@86 PS4, Line 86: dst This would be clearer if it was 'val' or something like that.
Hello Werner Zeh, Aaron Durbin, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35140
to look at the new patch set (#5).
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
[WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0
These platforms return to romstage from FSP only after already having torn CAR down. A copy of the entire CAR region is available and discoverable via HOB.
Previously, CBMEM console detected on-the-fly that CAR migration had happened and relocated cbmem_console_p accoringlin with car_sync_var(). However, if the CAR_GLOBAL pointing to another object inside CAR is a relative offset instead, we have a more generic solution that can be used with timestamps code as well.
Change-Id: Ica877b47e68d56189e9d998b5630019d4328a419 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/arm/include/arch/early_variables.h M src/arch/arm64/include/arch/early_variables.h M src/arch/mips/include/arch/early_variables.h M src/arch/ppc64/include/arch/early_variables.h M src/arch/riscv/include/arch/early_variables.h M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/car.c M src/lib/cbmem_console.c 8 files changed, 55 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35140/5
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: [WIP] arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35140/2/src/cpu/x86/car.c File src/cpu/x86/car.c:
https://review.coreboot.org/c/coreboot/+/35140/2/src/cpu/x86/car.c@97 PS2, Line 97: uintptr_t *mig_var = car_get_var_ptr(var);
Could you add some comments here? […]
Ack
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c File src/cpu/x86/car.c:
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c@82 PS4, Line 82: Pointers to CAR_GLOBALs use relative addressing
What ensures dst is a location in CAR_GLOBALs region? […]
Changed the text. After CBMEM comes online dst will be outside _car_region. We could try add some assert() here, but this code is doomed with next release and there has been no FSP1.0 development for some 5 years now.
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c@86 PS4, Line 86: dst
This would be clearer if it was 'val' or something like that.
Also s/mig_var/offset, not sure if it made this better or worse.
Hello Werner Zeh, Aaron Durbin, ron minnich, Julius Werner, Marshall Dawson, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35140
to look at the new patch set (#6).
Change subject: arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0
These platforms return to romstage from FSP only after already having torn CAR down. A copy of the entire CAR region is available and discoverable via HOB.
Previously, CBMEM console detected on-the-fly that CAR migration had happened and relocated cbmem_console_p accoringlin with car_sync_var(). However, if the CAR_GLOBAL pointing to another object inside CAR is a relative offset instead, we have a more generic solution that can be used with timestamps code as well.
Change-Id: Ica877b47e68d56189e9d998b5630019d4328a419 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/arm/include/arch/early_variables.h M src/arch/arm64/include/arch/early_variables.h M src/arch/mips/include/arch/early_variables.h M src/arch/ppc64/include/arch/early_variables.h M src/arch/riscv/include/arch/early_variables.h M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/car.c M src/lib/cbmem_console.c 8 files changed, 55 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35140/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
Patch Set 7: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c File src/cpu/x86/car.c:
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c@82 PS4, Line 82: Pointers to CAR_GLOBALs use relative addressing
Changed the text. After CBMEM comes online dst will be outside _car_region. […]
Done
https://review.coreboot.org/c/coreboot/+/35140/4/src/cpu/x86/car.c@86 PS4, Line 86: dst
Also s/mig_var/offset, not sure if it made this better or worse.
Done
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35140 )
Change subject: arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0 ......................................................................
arch/x86: Refactor CAR_GLOBAL quirk for FSP1.0
These platforms return to romstage from FSP only after already having torn CAR down. A copy of the entire CAR region is available and discoverable via HOB.
Previously, CBMEM console detected on-the-fly that CAR migration had happened and relocated cbmem_console_p accoringlin with car_sync_var(). However, if the CAR_GLOBAL pointing to another object inside CAR is a relative offset instead, we have a more generic solution that can be used with timestamps code as well.
Change-Id: Ica877b47e68d56189e9d998b5630019d4328a419 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35140 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/arch/arm/include/arch/early_variables.h M src/arch/arm64/include/arch/early_variables.h M src/arch/mips/include/arch/early_variables.h M src/arch/ppc64/include/arch/early_variables.h M src/arch/riscv/include/arch/early_variables.h M src/arch/x86/include/arch/early_variables.h M src/cpu/x86/car.c M src/lib/cbmem_console.c 8 files changed, 55 insertions(+), 44 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/arch/arm/include/arch/early_variables.h b/src/arch/arm/include/arch/early_variables.h index 9f06923..7002a88 100644 --- a/src/arch/arm/include/arch/early_variables.h +++ b/src/arch/arm/include/arch/early_variables.h @@ -18,7 +18,9 @@
static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) do { (var) = (val); } while (0)
+#define car_get_ptr car_get_var +#define car_set_ptr car_set_var + #endif diff --git a/src/arch/arm64/include/arch/early_variables.h b/src/arch/arm64/include/arch/early_variables.h index 9f06923..7002a88 100644 --- a/src/arch/arm64/include/arch/early_variables.h +++ b/src/arch/arm64/include/arch/early_variables.h @@ -18,7 +18,9 @@
static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) do { (var) = (val); } while (0)
+#define car_get_ptr car_get_var +#define car_set_ptr car_set_var + #endif diff --git a/src/arch/mips/include/arch/early_variables.h b/src/arch/mips/include/arch/early_variables.h index 8fc9c19..6ad8260 100644 --- a/src/arch/mips/include/arch/early_variables.h +++ b/src/arch/mips/include/arch/early_variables.h @@ -19,7 +19,9 @@
static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) { (var) = (val); }
+#define car_get_ptr car_get_var +#define car_set_ptr car_set_var + #endif /* __MIPS_ARCH_EARLY_VARIABLES_H */ diff --git a/src/arch/ppc64/include/arch/early_variables.h b/src/arch/ppc64/include/arch/early_variables.h index 22c3750..05b8bc7 100644 --- a/src/arch/ppc64/include/arch/early_variables.h +++ b/src/arch/ppc64/include/arch/early_variables.h @@ -19,8 +19,9 @@ #define CAR_MIGRATE(migrate_fn_) static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) - #define car_set_var(var, val) do { (var) = (val); } while (0)
+#define car_get_ptr car_get_var +#define car_set_ptr car_set_var + #endif diff --git a/src/arch/riscv/include/arch/early_variables.h b/src/arch/riscv/include/arch/early_variables.h index acc5ac3..a2da5f8 100644 --- a/src/arch/riscv/include/arch/early_variables.h +++ b/src/arch/riscv/include/arch/early_variables.h @@ -21,7 +21,9 @@ #define CAR_MIGRATE(migrate_fn_) static inline void *car_get_var_ptr(void *var) { return var; } #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) do { (var) = (val); } while (0)
+#define car_get_ptr car_get_var +#define car_set_ptr car_set_var + #endif diff --git a/src/arch/x86/include/arch/early_variables.h b/src/arch/x86/include/arch/early_variables.h index 3910a82..8111e7e 100644 --- a/src/arch/x86/include/arch/early_variables.h +++ b/src/arch/x86/include/arch/early_variables.h @@ -33,19 +33,25 @@ /* Get the correct pointer for the CAR global variable. */ void *car_get_var_ptr(void *var);
-/* Get and update a CAR_GLOBAL pointing elsewhere in car.global_data*/ -void *car_sync_var_ptr(void *var); - /* Return 1 when currently running with globals in Cache-as-RAM, 0 otherwise. */ int car_active(void);
/* Get and set a primitive type global variable. */ #define car_get_var(var) \ (*(typeof(var) *)car_get_var_ptr(&(var))) -#define car_sync_var(var) \ - (*(typeof(var) *)car_sync_var_ptr(&(var))) #define car_set_var(var, val) car_get_var(var) = (val)
+/* Get and set a CAR_GLOBAL pointing elsewhere inside CAR. */ +#if !CONFIG(PLATFORM_USES_FSP1_0) +#define car_get_ptr car_get_var +#define car_set_ptr car_set_var +#else +void *car_get_reloc_ptr(void *var); +void car_set_reloc_ptr(void *var, void *val); +#define car_get_ptr(var) car_get_reloc_ptr(&(var)) +#define car_set_ptr(var, val) car_set_reloc_ptr(&(var), (val)) +#endif + static inline size_t car_data_size(void) { size_t car_size = _car_relocatable_data_size; @@ -66,8 +72,9 @@
#define CAR_GLOBAL #define car_get_var(var) (var) -#define car_sync_var(var) (var) #define car_set_var(var, val) (var) = (val) +#define car_get_ptr car_get_var +#define car_set_ptr car_set_var
static inline void *car_get_var_ptr(void *var) { diff --git a/src/cpu/x86/car.c b/src/cpu/x86/car.c index 6fc6168..009ac19 100644 --- a/src/cpu/x86/car.c +++ b/src/cpu/x86/car.c @@ -77,42 +77,37 @@ return &migrated_base[offset]; }
+#if CONFIG(PLATFORM_USES_FSP1_0) /* - * Update a CAR_GLOBAL variable var, originally pointing to CAR region, - * with the address in migrated CAR region in DRAM. + * When a CAR_GLOBAL points to target object inside CAR, use relative + * addressing. Such CAR_GLOBAL has to be expicitly accessed using + * car_set_reloc_ptr() and car_get_reloc_ptr() as the stored value is now + * an offset instead of the absolute address (pointer) of the target. + * + * This way discovery of objects that are not CAR_GLOBALs themselves, + * remain discoverable after CAR migration has implicitly happened. */ -void *car_sync_var_ptr(void *var) +void car_set_reloc_ptr(void *var, void *val) { - void **mig_var = car_get_var_ptr(var); - void *_car_start = _car_relocatable_data_start; - void *_car_end = _car_relocatable_data_end; + uintptr_t *offset = car_get_var_ptr(var); + *offset = 0;
- /* Not moved or migrated yet. */ - if (mig_var == var) - return mig_var; - - /* - * Migrate the cbmem console pointer for FSP 1.0 platforms. Otherwise, - * keep console buffer in CAR until cbmemc_reinit() moves it. - */ - if (*mig_var == _preram_cbmem_console) { - if (CONFIG(PLATFORM_USES_FSP1_0)) - *mig_var += (char *)mig_var - (char *)var; - return mig_var; - } - - /* It's already pointing outside car.global_data. */ - if (*mig_var < _car_start || *mig_var > _car_end) - return mig_var; - - /* Move the pointer by the same amount the variable storing it was - * moved by. - */ - *mig_var += (char *)mig_var - (char *)var; - - return mig_var; + if (val) + *offset = (uintptr_t)offset - (uintptr_t)val; }
+void *car_get_reloc_ptr(void *var) +{ + uintptr_t *offset = car_get_var_ptr(var); + void *val = NULL; + + if (*offset) + val = (void *)((uintptr_t)offset - *offset); + + return val; +} +#endif + int car_active(void) { return !car_migrated; diff --git a/src/lib/cbmem_console.c b/src/lib/cbmem_console.c index df2d1ff..18c3b71 100644 --- a/src/lib/cbmem_console.c +++ b/src/lib/cbmem_console.c @@ -73,12 +73,12 @@
static struct cbmem_console *current_console(void) { - return car_sync_var(cbmem_console_p); + return car_get_ptr(cbmem_console_p); }
static void current_console_set(struct cbmem_console *new_console_p) { - car_set_var(cbmem_console_p, new_console_p); + car_set_ptr(cbmem_console_p, new_console_p); }
static int buffer_valid(struct cbmem_console *cbm_cons_p, u32 total_space)
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)