On 27/07/23 17:41, Jan Beulich wrote:
On 27.07.2023 12:48, Nicola Vetrini wrote:--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1483,7 +1483,7 @@ x86_emulate( { enum x86_segment seg; struct segment_register cs, sreg; - struct cpuid_leaf cpuid_leaf; + struct cpuid_leaf res;This is too generic a name for a variable with a scope of several thousand lines. Perhaps just "leaf"?
It can also be defined inside the switch clause, since it has no other purpose than store a result.
@@ -8408,8 +8408,6 @@ x86_emulate( generate_exception(X86_EXC_MF); if ( stub_exn.info.fields.trapnr == X86_EXC_XM ) { - unsigned long cr4; - if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY ) cr4 = X86_CR4_OSXMMEXCPT; generate_exception(cr4 & X86_CR4_OSXMMEXCPT ? X86_EXC_XM : X86_EXC_UD);This change looks okay to me, but I'd like to strongly encourage you to split both changes. They're of different nature, and for the latter it may even be worthwhile pointing out when exactly this duplication of variables was introduced (it clearly would better have been avoided).
I did it this way because they are the only violations of R5.3 left in this file (among those not subject to deviation). By splitting you mean two patches in this series or a separate patch just for this change?
-- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
