[PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors

[PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors

[Andrew Cooper]

The original limit provided wasn't accurate.  Blobs are in fact rather larger.

Fixes: fe36a173d1 ("x86/amd: Initial support for Fam19h processors")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Ian Jackson <iwj@xenproject.org>

For 4.15.  This is a very targetted fix at AMD Zen3 processors.  Without it,
microcode loading doesn't function.
---
 xen/arch/x86/cpu/microcode/amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 5255028af7..c4ab395799 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -111,7 +111,7 @@ static bool verify_patch_size(uint32_t patch_size)
 #define F15H_MPB_MAX_SIZE 4096
 #define F16H_MPB_MAX_SIZE 3458
 #define F17H_MPB_MAX_SIZE 3200
-#define F19H_MPB_MAX_SIZE 4800
+#define F19H_MPB_MAX_SIZE 5568
 
     switch ( boot_cpu_data.x86 )
     {
-- 
2.11.0

Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors

[Jan Beulich]

On 09.02.2021 16:33, Andrew Cooper wrote:
> The original limit provided wasn't accurate.  Blobs are in fact rather larger.
> 
> Fixes: fe36a173d1 ("x86/amd: Initial support for Fam19h processors")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -111,7 +111,7 @@ static bool verify_patch_size(uint32_t patch_size)
>  #define F15H_MPB_MAX_SIZE 4096
>  #define F16H_MPB_MAX_SIZE 3458
>  #define F17H_MPB_MAX_SIZE 3200
> -#define F19H_MPB_MAX_SIZE 4800
> +#define F19H_MPB_MAX_SIZE 5568

How certain is it that there's not going to be another increase?
And in comparison, how bad would it be if we pulled this upper
limit to something that's at least slightly less of an "odd"
number, e.g. 0x1800, and thus provide some headroom?

Jan

Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors"):
> On 09.02.2021 16:33, Andrew Cooper wrote:
> > The original limit provided wasn't accurate.  Blobs are in fact rather larger.
> > 
> > Fixes: fe36a173d1 ("x86/amd: Initial support for Fam19h processors")
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> > --- a/xen/arch/x86/cpu/microcode/amd.c
> > +++ b/xen/arch/x86/cpu/microcode/amd.c
> > @@ -111,7 +111,7 @@ static bool verify_patch_size(uint32_t patch_size)
> >  #define F15H_MPB_MAX_SIZE 4096
> >  #define F16H_MPB_MAX_SIZE 3458
> >  #define F17H_MPB_MAX_SIZE 3200
> > -#define F19H_MPB_MAX_SIZE 4800
> > +#define F19H_MPB_MAX_SIZE 5568
> 
> How certain is it that there's not going to be another increase?
> And in comparison, how bad would it be if we pulled this upper
> limit to something that's at least slightly less of an "odd"
> number, e.g. 0x1800, and thus provide some headroom?

5568 does seem really an excessively magic number...

Ian.

Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors

[Andrew Cooper]

On 09/02/2021 17:17, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors"):
>> On 09.02.2021 16:33, Andrew Cooper wrote:
>>> The original limit provided wasn't accurate.  Blobs are in fact rather larger.
>>>
>>> Fixes: fe36a173d1 ("x86/amd: Initial support for Fam19h processors")
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>>> --- a/xen/arch/x86/cpu/microcode/amd.c
>>> +++ b/xen/arch/x86/cpu/microcode/amd.c
>>> @@ -111,7 +111,7 @@ static bool verify_patch_size(uint32_t patch_size)
>>>  #define F15H_MPB_MAX_SIZE 4096
>>>  #define F16H_MPB_MAX_SIZE 3458
>>>  #define F17H_MPB_MAX_SIZE 3200
>>> -#define F19H_MPB_MAX_SIZE 4800
>>> +#define F19H_MPB_MAX_SIZE 5568
>> How certain is it that there's not going to be another increase?
>> And in comparison, how bad would it be if we pulled this upper
>> limit to something that's at least slightly less of an "odd"
>> number, e.g. 0x1800, and thus provide some headroom?
> 5568 does seem really an excessively magic number...

It reads better in hex - 0x15c0.  It is exactly the header,
match/patch-ram, and the digital signature of a fixed algorithm.

Its far simpler than Intel's format which contains multiple embedded
blobs, and support for minor platform variations within the same blob.

There are a lot of problems with how we do patch verification on AMD
right now, but its all a consequence of the header not containing a
length field.

This number wont change now.  Zen3 processors are out in the world.

~Andrew

Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors"):
> On 09/02/2021 17:17, Ian Jackson wrote:
> > Jan Beulich writes ("Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors"):
...
> >> How certain is it that there's not going to be another increase?
> >> And in comparison, how bad would it be if we pulled this upper
> >> limit to something that's at least slightly less of an "odd"
> >> number, e.g. 0x1800, and thus provide some headroom?
> > 5568 does seem really an excessively magic number...
> 
> It reads better in hex - 0x15c0.  It is exactly the header,
> match/patch-ram, and the digital signature of a fixed algorithm.
> 
> Its far simpler than Intel's format which contains multiple embedded
> blobs, and support for minor platform variations within the same blob.
> 
> There are a lot of problems with how we do patch verification on AMD
> right now, but its all a consequence of the header not containing a
> length field.
> 
> This number wont change now.  Zen3 processors are out in the world.

Err.  This Is Fine.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors

[Jan Beulich]

On 09.02.2021 18:39, Andrew Cooper wrote:
> On 09/02/2021 17:17, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors"):
>>> On 09.02.2021 16:33, Andrew Cooper wrote:
>>>> The original limit provided wasn't accurate.  Blobs are in fact rather larger.
>>>>
>>>> Fixes: fe36a173d1 ("x86/amd: Initial support for Fam19h processors")
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>
>>>> --- a/xen/arch/x86/cpu/microcode/amd.c
>>>> +++ b/xen/arch/x86/cpu/microcode/amd.c
>>>> @@ -111,7 +111,7 @@ static bool verify_patch_size(uint32_t patch_size)
>>>>  #define F15H_MPB_MAX_SIZE 4096
>>>>  #define F16H_MPB_MAX_SIZE 3458
>>>>  #define F17H_MPB_MAX_SIZE 3200
>>>> -#define F19H_MPB_MAX_SIZE 4800
>>>> +#define F19H_MPB_MAX_SIZE 5568
>>> How certain is it that there's not going to be another increase?
>>> And in comparison, how bad would it be if we pulled this upper
>>> limit to something that's at least slightly less of an "odd"
>>> number, e.g. 0x1800, and thus provide some headroom?
>> 5568 does seem really an excessively magic number...
> 
> It reads better in hex - 0x15c0.  It is exactly the header,
> match/patch-ram, and the digital signature of a fixed algorithm.

And I realize it's less odd than Fam16's 3458 (0xd82).

> Its far simpler than Intel's format which contains multiple embedded
> blobs, and support for minor platform variations within the same blob.
> 
> There are a lot of problems with how we do patch verification on AMD
> right now, but its all a consequence of the header not containing a
> length field.
> 
> This number wont change now.  Zen3 processors are out in the world.

Given history on earlier families, where in some cases sizes
also vary by model, how can this fact guarantee anything?

Jan

Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors

[Andrew Cooper]

On 10/02/2021 08:37, Jan Beulich wrote:
> On 09.02.2021 18:39, Andrew Cooper wrote:
>> On 09/02/2021 17:17, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH for-4.15] x86/ucode: Fix microcode payload size for Fam19 processors"):
>>>> On 09.02.2021 16:33, Andrew Cooper wrote:
>>>>> The original limit provided wasn't accurate.  Blobs are in fact rather larger.
>>>>>
>>>>> Fixes: fe36a173d1 ("x86/amd: Initial support for Fam19h processors")
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>>> --- a/xen/arch/x86/cpu/microcode/amd.c
>>>>> +++ b/xen/arch/x86/cpu/microcode/amd.c
>>>>> @@ -111,7 +111,7 @@ static bool verify_patch_size(uint32_t patch_size)
>>>>>  #define F15H_MPB_MAX_SIZE 4096
>>>>>  #define F16H_MPB_MAX_SIZE 3458
>>>>>  #define F17H_MPB_MAX_SIZE 3200
>>>>> -#define F19H_MPB_MAX_SIZE 4800
>>>>> +#define F19H_MPB_MAX_SIZE 5568
>>>> How certain is it that there's not going to be another increase?
>>>> And in comparison, how bad would it be if we pulled this upper
>>>> limit to something that's at least slightly less of an "odd"
>>>> number, e.g. 0x1800, and thus provide some headroom?
>>> 5568 does seem really an excessively magic number...
>> It reads better in hex - 0x15c0.  It is exactly the header,
>> match/patch-ram, and the digital signature of a fixed algorithm.
> And I realize it's less odd than Fam16's 3458 (0xd82).

This particular value is under investigation.  Firmware packages for
Fam16's have the blob size at 0xd60.

>> Its far simpler than Intel's format which contains multiple embedded
>> blobs, and support for minor platform variations within the same blob.
>>
>> There are a lot of problems with how we do patch verification on AMD
>> right now, but its all a consequence of the header not containing a
>> length field.
>>
>> This number wont change now.  Zen3 processors are out in the world.
> Given history on earlier families, where in some cases sizes
> also vary by model, how can this fact guarantee anything?

There is one example where it changed, and it got shorter.  Making it
larger would involve someone re-laying out the core so more silicon area
could be used for patch ram space, which is increasingly unlikely with
newer models in the same family.

When 4.16 comes, I do have plans to try and make us more robust to
changes in debug builds, particularly given the lack of suitable
documentation on the matter.

~Andrew