diff mbox series

[v2,6/8] x86/MCE/AMD: Drop tmp variable in translation code

Message ID 20200903200144.310991-7-Yazen.Ghannam@amd.com (mailing list archive)
State New, archived
Headers show
Series AMD MCA Address Translation Updates | expand

Commit Message

Yazen Ghannam Sept. 3, 2020, 8:01 p.m. UTC 
From: Yazen Ghannam <yazen.ghannam@amd.com>

Remove the "tmp" variable used to save register values. Save the values
in existing variables, if possible.

The register values are 32 bits. Use separate "reg_" variables to hold
the register values if the existing variable sizes doesn't match, or if
no bitfields in a register share the same name as the register.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
Link:
https://lkml.kernel.org/r/20200814191449.183998-3-Yazen.Ghannam@amd.com

v1 -> v2:
* New patch based on comments for v1 Patch 2.

 arch/x86/kernel/cpu/mce/amd.c | 56 +++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 26 deletions(-)

Comments

Borislav Petkov Sept. 23, 2020, 8:05 a.m. UTC | #1 
On Thu, Sep 03, 2020 at 08:01:42PM +0000, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Remove the "tmp" variable used to save register values. Save the values
> in existing variables, if possible.
> 
> The register values are 32 bits. Use separate "reg_" variables to hold
> the register values if the existing variable sizes doesn't match, or if
> no bitfields in a register share the same name as the register.

So I'm missing the "why" in the commit message. Why are you doing this?

Is there some reason which I'll find out later? If not, then this is
just unnecessary churn.

Thx.
Yazen Ghannam Sept. 23, 2020, 4:05 p.m. UTC | #2 
On Wed, Sep 23, 2020 at 10:05:56AM +0200, Borislav Petkov wrote:
> On Thu, Sep 03, 2020 at 08:01:42PM +0000, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > 
> > Remove the "tmp" variable used to save register values. Save the values
> > in existing variables, if possible.
> > 
> > The register values are 32 bits. Use separate "reg_" variables to hold
> > the register values if the existing variable sizes doesn't match, or if
> > no bitfields in a register share the same name as the register.
> 
> So I'm missing the "why" in the commit message. Why are you doing this?
> 
> Is there some reason which I'll find out later? If not, then this is
> just unnecessary churn.
>

I don't have a strong reason other than trying to address a comment in
the first version. I can drop this patch if you prefer.

Thanks,
Yazen
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/mce/amd.c b/arch/x86/kernel/cpu/mce/amd.c
index 90c3ad61ae19..5a18937ff7cd 100644
--- a/arch/x86/kernel/cpu/mce/amd.c
+++ b/arch/x86/kernel/cpu/mce/amd.c
@@ -688,11 +688,14 @@  void mce_amd_feature_init(struct cpuinfo_x86 *c)
 
 int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 {
-	u64 dram_base_addr, dram_limit_addr, dram_hole_base;
 	/* We start from the normalized address */
 	u64 ret_addr = norm_addr;
 
-	u32 tmp;
+	u64 dram_base_addr, dram_limit_addr;
+	u32 dram_hole_base;
+
+	u32 reg_dram_base_addr, reg_dram_limit_addr;
+	u32 reg_dram_offset;
 
 	u8 die_id_shift, die_id_mask, socket_id_shift, socket_id_mask;
 	u8 intlv_num_dies, intlv_num_chan, intlv_num_sockets;
@@ -702,12 +705,12 @@  int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 	u8 cs_mask, cs_id = 0;
 	bool hash_enabled = false;
 
-	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMOFFSET, umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMOFFSET, umc, &reg_dram_offset))
 		goto out_err;
 
 	/* Remove HiAddrOffset from normalized address, if enabled: */
-	if (tmp & BIT(0)) {
-		u64 hi_addr_offset = get_bits(tmp, 31, 20) << 28;
+	if (reg_dram_offset & BIT(0)) {
+		u64 hi_addr_offset = get_bits(reg_dram_offset, 31, 20) << 28;
 
 		/* Check if base 1 is used. */
 		if (norm_addr >= hi_addr_offset) {
@@ -716,20 +719,20 @@  int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		}
 	}
 
-	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMBASEADDR + (8 * base), umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMBASEADDR + (8 * base), umc, &reg_dram_base_addr))
 		goto out_err;
 
 	/* Check if address range is valid. */
-	if (!(tmp & BIT(0))) {
+	if (!(reg_dram_base_addr & BIT(0))) {
 		pr_err("%s: Invalid DramBaseAddress range: 0x%x.\n",
-			__func__, tmp);
+			__func__, reg_dram_base_addr);
 		goto out_err;
 	}
 
-	lgcy_mmio_hole_en = get_bit(tmp, 1);
-	intlv_num_chan	  = get_bits(tmp, 7, 4);
-	intlv_addr_sel	  = get_bits(tmp, 10, 8);
-	dram_base_addr	  = get_bits(tmp, 31, 12) << 28;
+	lgcy_mmio_hole_en = get_bit(reg_dram_base_addr, 1);
+	intlv_num_chan	  = get_bits(reg_dram_base_addr, 7, 4);
+	intlv_addr_sel	  = get_bits(reg_dram_base_addr, 10, 8);
+	dram_base_addr	  = get_bits(reg_dram_base_addr, 31, 12) << 28;
 
 	/* {0, 1, 2, 3} map to address bits {8, 9, 10, 11} respectively */
 	if (intlv_addr_sel > 3) {
@@ -738,12 +741,12 @@  int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		goto out_err;
 	}
 
-	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMLIMITADDR + (8 * base), umc, &tmp))
+	if (amd_df_indirect_read(nid, 0, DF_F0_DRAMLIMITADDR + (8 * base), umc, &reg_dram_limit_addr))
 		goto out_err;
 
-	intlv_num_sockets = get_bit(tmp, 8);
-	intlv_num_dies	  = get_bits(tmp, 11, 10);
-	dram_limit_addr	  = (get_bits(tmp, 31, 12) << 28) | GENMASK_ULL(27, 0);
+	intlv_num_sockets = get_bit(reg_dram_limit_addr, 8);
+	intlv_num_dies	  = get_bits(reg_dram_limit_addr, 11, 10);
+	dram_limit_addr	  = (get_bits(reg_dram_limit_addr, 31, 12) << 28) | GENMASK_ULL(27, 0);
 
 	intlv_addr_bit = intlv_addr_sel + 8;
 
@@ -786,17 +789,18 @@  int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 
 	if (num_intlv_bits > 0) {
 		u64 temp_addr_x, temp_addr_i, temp_addr_y;
-		u8 die_id_bit, sock_id_bit, cs_fabric_id;
+		u32 reg_sys_fabric_id, cs_fabric_id;
+		u8 die_id_bit, sock_id_bit;
 
 		/*
 		 * This is the fabric id for this coherent slave. Use
 		 * umc/channel# as instance id of the coherent slave
 		 * for FICAA.
 		 */
-		if (amd_df_indirect_read(nid, 0, DF_F0_FABRICINSTINFO3, umc, &tmp))
+		if (amd_df_indirect_read(nid, 0, DF_F0_FABRICINSTINFO3, umc, &cs_fabric_id))
 			goto out_err;
 
-		cs_fabric_id = get_bits(tmp, 15, 8);
+		cs_fabric_id = get_bits(cs_fabric_id, 15, 8);
 		die_id_bit   = 0;
 
 		/* If interleaved over more than 1 channel: */
@@ -809,22 +813,22 @@  int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 		sock_id_bit = die_id_bit;
 
 		if (intlv_num_dies || intlv_num_sockets)
-			if (amd_df_indirect_read(nid, 1, DF_F1_SYSFABRICID, umc, &tmp))
+			if (amd_df_indirect_read(nid, 1, DF_F1_SYSFABRICID, umc, &reg_sys_fabric_id))
 				goto out_err;
 
 		/* If interleaved over more than 1 die. */
 		if (intlv_num_dies) {
 			sock_id_bit  = die_id_bit + intlv_num_dies;
-			die_id_shift = get_bits(tmp, 27, 24);
-			die_id_mask  = get_bits(tmp, 15, 8);
+			die_id_shift = get_bits(reg_sys_fabric_id, 27, 24);
+			die_id_mask  = get_bits(reg_sys_fabric_id, 15, 8);
 
 			cs_id |= ((cs_fabric_id & die_id_mask) >> die_id_shift) << die_id_bit;
 		}
 
 		/* If interleaved over more than 1 socket. */
 		if (intlv_num_sockets) {
-			socket_id_shift	= get_bits(tmp, 31, 28);
-			socket_id_mask	= get_bits(tmp, 23, 16);
+			socket_id_shift	= get_bits(reg_sys_fabric_id, 31, 28);
+			socket_id_mask	= get_bits(reg_sys_fabric_id, 23, 16);
 
 			cs_id |= ((cs_fabric_id & socket_id_mask) >> socket_id_shift) << sock_id_bit;
 		}
@@ -848,10 +852,10 @@  int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *sys_addr)
 
 	/* If legacy MMIO hole enabled */
 	if (lgcy_mmio_hole_en) {
-		if (amd_df_indirect_read(nid, 0, DF_F0_MMIOHOLE, umc, &tmp))
+		if (amd_df_indirect_read(nid, 0, DF_F0_MMIOHOLE, umc, &dram_hole_base))
 			goto out_err;
 
-		dram_hole_base = tmp & GENMASK(31, 24);
+		dram_hole_base &= GENMASK(31, 24);
 		if (ret_addr >= dram_hole_base)
 			ret_addr += (BIT_ULL(32) - dram_hole_base);
 	}