Re: [PATCH] target/i386: add missing bits to CR4_RESERVED_MASK

[Paolo Bonzini <pbonzini@redhat.com>]

On 31/08/21 19:57, Richard W.M. Jones wrote:
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index 6c50d3ab4f..ce85f1a29d 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -257,6 +257,7 @@ typedef enum X86Seg {
>>                   | CR4_DE_MASK | CR4_PSE_MASK | CR4_PAE_MASK \
>>                   | CR4_MCE_MASK | CR4_PGE_MASK | CR4_PCE_MASK \
>>                   | CR4_OSFXSR_MASK | CR4_OSXMMEXCPT_MASK |CR4_UMIP_MASK \
>> +                | CR4_LA57_MASK | CR4_VMXE_MASK | CR4_SMXE_MASK \
>>                   | CR4_FSGSBASE_MASK | CR4_PCIDE_MASK | CR4_OSXSAVE_MASK \
>>                   | CR4_SMEP_MASK | CR4_SMAP_MASK | CR4_PKE_MASK | CR4_PKS_MASK))
> First thing to say is I tested this locally and it fixes the
> problem seen inhttps://bugzilla.redhat.com/show_bug.cgi?id=1999700.
> I will also add this patch to Fedora soon.  So:
> 
> Tested-by: Richard W.M. Jones<rjones@redhat.com>
> 
> But my question is, does this mean that every time a new CPU feature
> appears we must remember to update this code?

This is used only by TCG, which explains why VMXE/SMXE were not there; 
however, LA57 is missing indeed.

New features must be included both here and in cr4_reserved_bits, but 
only if TCG supports them, otherwise they can be left out.  Since 
VMXE/SMXE are not supported by TCG, they should be either added both 
here and in cr4_reserved_bits (keyed on env->features[FEAT_1_ECX] & 
CPUID_EXT_{VMX,SMX} respectively), or they should not be added to 
CR4_RESERVED_MASK either.  On the other hand LA57 is already handled by 
cr4_reserved_bits, so it is okay to just add it here.

Thanks,

Paolo