On 23.01.2023 15:32, Sergey Dyasli wrote: > On Mon, Jan 16, 2023 at 2:47 PM Jan Beulich <[email protected]> wrote: >> On 11.01.2023 15:23, Sergey Dyasli wrote: >>> --- a/xen/arch/x86/cpu/microcode/amd.c >>> +++ b/xen/arch/x86/cpu/microcode/amd.c >>> @@ -176,8 +176,13 @@ static enum microcode_match_result compare_revisions( >>> if ( new_rev > old_rev ) >>> return NEW_UCODE; >>> >>> - if ( opt_ucode_allow_same && new_rev == old_rev ) >>> - return NEW_UCODE; >>> + if ( new_rev == old_rev ) >>> + { >>> + if ( opt_ucode_allow_same ) >>> + return NEW_UCODE; >>> + else >>> + return SAME_UCODE; >>> + } >> >> I find this misleading: "same" should not depend on the command line >> option. > > The alternative diff I was considering is this: > > --- a/xen/arch/x86/cpu/microcode/amd.c > +++ b/xen/arch/x86/cpu/microcode/amd.c > @@ -179,6 +179,9 @@ static enum microcode_match_result compare_revisions( > if ( opt_ucode_allow_same && new_rev == old_rev ) > return NEW_UCODE; > > + if ( new_rev == old_rev ) > + return SAME_UCODE; > + > return OLD_UCODE; > } > > Do you think the logic is clearer this way? Or should I simply remove > "else" from the first diff above?
Neither addresses my comment. I think the command line option check needs to move out of this function, into ... >> In fact the command line option should affect only the cases >> where ucode is actually to be loaded; it should not affect cases where >> the check is done merely to know whether the cache needs updating. ... some (but not all) of the callers. Jan
