Re: [PATCH V2] x86/AMD: Apply workaround for AMD F16h Erratum792

Re: [PATCH V2] x86/AMD: Apply workaround for AMD F16h Erratum792

[Andrew Cooper]

On 05/02/2014 21:43, Aravind Gopalakrishnan wrote:
> Workaround for the Erratum will be in BIOSes spun only after
> Jan 2014 onwards. But initial production parts shipped in 2013
> itself. Since there is a coverage hole, we should carry this fix
> in software in case BIOS does not do the right thing or someone
> is using old BIOS.
>
> Refer to Revision Guide for AMD F16h models 00h-0fh, document 51810
> Rev. 3.04, November2013 for details on the Erratum.
>
> Tested the patch on Fam16h server platform and it works fine.
>
> Changes in V2: (per Andrew.C comments)
> 	- Move pci_val into same scope
> 	- rework indentation to match linux style
>
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/cpu/amd.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index 3307141..703bbda 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -477,6 +477,36 @@ static void __devinit init_amd(struct cpuinfo_x86 *c)
>  		       " all your (PV) guest kernels. ***\n");
>  
>  	if (c->x86 == 0x16 && c->x86_model <= 0xf) {
> +		/*
> +		 * Apply workaround for erratum 792
> +		 * Description:
> +		 * Processor does not ensure DRAM scrub read/write sequence
> +		 * is atomic wrt accesses to CC6 save state area. Therefore
> +		 * if a concurrent scrub read/write access is to same address
> +		 * the entry may appear as if it is not written. This quirk
> +		 * applies to Fam16h models 00h-0Fh
> +		 *
> +		 * See "Revision Guide" for AMD F16h models 00h-0fh,
> +		 * document 51810 rev. 3.04, Nov 2013
> +		 *
> +		 * Equivalent Linux patch link:
> +		 * http://marc.info/?l=linux-kernel&m=139066012217149&w=2
> +		 */
> +		if (smp_processor_id() == 0) {
> +			u32 pci_val;
> +			pci_val = pci_conf_read32(0, 0, 0x18, 0x3, 0x58);
> +			if (pci_val & 0x1f) {
> +				pci_val &= ~0x1fu;
> +				pci_conf_write32(0, 0, 0x18, 0x3, 0x58, pci_val);
> +			}
> +
> +			pci_val = pci_conf_read32(0, 0, 0x18, 0x3, 0x5c);
> +			if (pci_val & 0x1) {
> +				pci_val &= ~0x1u;
> +				pci_conf_write32(0, 0, 0x18, 0x3, 0x5c, pci_val);
> +			}
> +		}
> +
>  		rdmsrl(MSR_AMD64_LS_CFG, value);
>  		if (!(value & (1 << 15))) {
>  			static bool_t warned;

[PATCH V2] x86/AMD: Apply workaround for AMD F16h Erratum792

[Aravind Gopalakrishnan]

Workaround for the Erratum will be in BIOSes spun only after
Jan 2014 onwards. But initial production parts shipped in 2013
itself. Since there is a coverage hole, we should carry this fix
in software in case BIOS does not do the right thing or someone
is using old BIOS.

Refer to Revision Guide for AMD F16h models 00h-0fh, document 51810
Rev. 3.04, November2013 for details on the Erratum.

Tested the patch on Fam16h server platform and it works fine.

Changes in V2: (per Andrew.C comments)
	- Move pci_val into same scope
	- rework indentation to match linux style

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/cpu/amd.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 3307141..703bbda 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -477,6 +477,36 @@ static void __devinit init_amd(struct cpuinfo_x86 *c)
 		       " all your (PV) guest kernels. ***\n");
 
 	if (c->x86 == 0x16 && c->x86_model <= 0xf) {
+		/*
+		 * Apply workaround for erratum 792
+		 * Description:
+		 * Processor does not ensure DRAM scrub read/write sequence
+		 * is atomic wrt accesses to CC6 save state area. Therefore
+		 * if a concurrent scrub read/write access is to same address
+		 * the entry may appear as if it is not written. This quirk
+		 * applies to Fam16h models 00h-0Fh
+		 *
+		 * See "Revision Guide" for AMD F16h models 00h-0fh,
+		 * document 51810 rev. 3.04, Nov 2013
+		 *
+		 * Equivalent Linux patch link:
+		 * http://marc.info/?l=linux-kernel&m=139066012217149&w=2
+		 */
+		if (smp_processor_id() == 0) {
+			u32 pci_val;
+			pci_val = pci_conf_read32(0, 0, 0x18, 0x3, 0x58);
+			if (pci_val & 0x1f) {
+				pci_val &= ~0x1fu;
+				pci_conf_write32(0, 0, 0x18, 0x3, 0x58, pci_val);
+			}
+
+			pci_val = pci_conf_read32(0, 0, 0x18, 0x3, 0x5c);
+			if (pci_val & 0x1) {
+				pci_val &= ~0x1u;
+				pci_conf_write32(0, 0, 0x18, 0x3, 0x5c, pci_val);
+			}
+		}
+
 		rdmsrl(MSR_AMD64_LS_CFG, value);
 		if (!(value & (1 << 15))) {
 			static bool_t warned;
-- 
1.8.1.2

Re: [PATCH V2] x86/AMD: Apply workaround for AMD F16h Erratum792

[Jan Beulich]

>>> On 05.02.14 at 22:43, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -477,6 +477,36 @@ static void __devinit init_amd(struct cpuinfo_x86 *c)
>  		       " all your (PV) guest kernels. ***\n");
>  
>  	if (c->x86 == 0x16 && c->x86_model <= 0xf) {
> +		/*
> +		 * Apply workaround for erratum 792
> +		 * Description:
> +		 * Processor does not ensure DRAM scrub read/write sequence
> +		 * is atomic wrt accesses to CC6 save state area. Therefore
> +		 * if a concurrent scrub read/write access is to same address
> +		 * the entry may appear as if it is not written. This quirk
> +		 * applies to Fam16h models 00h-0Fh
> +		 *
> +		 * See "Revision Guide" for AMD F16h models 00h-0fh,
> +		 * document 51810 rev. 3.04, Nov 2013
> +		 *
> +		 * Equivalent Linux patch link:
> +		 * http://marc.info/?l=linux-kernel&m=139066012217149&w=2 
> +		 */
> +		if (smp_processor_id() == 0) {
> +			u32 pci_val;
> +			pci_val = pci_conf_read32(0, 0, 0x18, 0x3, 0x58);
> +			if (pci_val & 0x1f) {
> +				pci_val &= ~0x1fu;
> +				pci_conf_write32(0, 0, 0x18, 0x3, 0x58, pci_val);
> +			}
> +
> +			pci_val = pci_conf_read32(0, 0, 0x18, 0x3, 0x5c);
> +			if (pci_val & 0x1) {
> +				pci_val &= ~0x1u;
> +				pci_conf_write32(0, 0, 0x18, 0x3, 0x5c, pci_val);
> +			}
> +		}
> +
>  		rdmsrl(MSR_AMD64_LS_CFG, value);
>  		if (!(value & (1 << 15))) {
>  			static bool_t warned;

The patch context even shows what is missing: A diagnostic
message making it possible to know that the workaround was
applied. Of course you don't need two separate messages for
the two parts of the workaround, but indicating in the message
which of them was applied would seem desirable.

Furthermore, I don't see why you would need a new local
variable here at all - there are two suitable variables available
throughout the entire function (l and h).

Jan

Re: [PATCH V2] x86/AMD: Apply workaround for AMD F16h Erratum792

[Aravind Gopalakrishnan]

On 2/6/2014 4:24 AM, Jan Beulich wrote:
>>>> On 05.02.14 at 22:43, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
>> +
>>   		rdmsrl(MSR_AMD64_LS_CFG, value);
>>   		if (!(value & (1 << 15))) {
>>   			static bool_t warned;
> The patch context even shows what is missing: A diagnostic
> message making it possible to know that the workaround was
> applied. Of course you don't need two separate messages for
> the two parts of the workaround, but indicating in the message
> which of them was applied would seem desirable.
>
> Furthermore, I don't see why you would need a new local
> variable here at all - there are two suitable variables available
> throughout the entire function (l and h).
>
>
Okay, corrected the patch as per your comments.
Sending it out as V3.

-Aravind