Re: [PATCH 2/5] x86: Refactor microcode_update() hypercall with flags field

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2024 16:53, Fouad Hilly wrote:
> On Mon, Apr 8, 2024 at 10:16 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 05.04.2024 14:11, Fouad Hilly wrote:
>>> @@ -708,11 +712,13 @@ static long cf_check microcode_update_helper(void 
>>> *data)
>>>      return ret;
>>>  }
>>>
>>> -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
>>> +int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len, 
>>> unsigned int flags)
>>>  {
>>>      int ret;
>>>      struct ucode_buf *buffer;
>>>
>>> +    ucode_force_flag = (flags == XENPF_UCODE_FLAG_FORCE_SET)? 1: 0;
>>
>> No need for ?: when the lhs has type bool.
>>
>> But - do we really need to resort to parameter passing via static variables
>> here? If it's unavoidable, its setting needs to move inside a locked region
>> (with that region covering everything up to all consumption of the value).
> There are many function calls and checks of the firmware between
> microcode_update() and the actual update, which makes static variable
> the viable option.
> In V2 I broke it down between the actual update_flags (static) and
> force_flag (local to firmware update function), I understand that
> might not be enough, I will look into further improvement for
> microcode_update flags in V3.
>>
>> Further, to avoid the same issue again when another flag wants adding, you
>> want to check that all other bits in the flags field are clear.
> The above check is checking all bits in the flags field. Are you
> referring to flag per bit where multiple flags can be set
> simultaneously?

No. What you do is treat a flags value of, say, 2 the same as a flags
value of 0. That's wrong when considering that the value 2 may gain a
meaning going forward. At this point you want to refuse flags values
other than 0 or 1.

Jan