Re: [PATCH v1 1/3] hvf: use standard CR0 and CR4 register definitions

[Paolo Bonzini]

On 08/04/20 08:09, Cameron Esfahani wrote:
>>>
>>>     if (efer & MSR_EFER_LME) {
>>> -        if (!(old_cr0 & CR0_PG) && (cr0 & CR0_PG)) {
>>> +        if (!(old_cr0 & CR0_PG_MASK) && (cr0 & CR0_PG_MASK)) {
>>>             enter_long_mode(vcpu, cr0, efer);
>>>         }
>>> -        if (/*(old_cr0 & CR0_PG) &&*/ !(cr0 & CR0_PG)) {
>>> +        if (!(cr0 & CR0_PG_MASK)) {
>> IMO the patch should only change CR0_PG to CR0_PG_MASK without removal
>> of the commented condition.
>>
>> In the next patch you're improving how long mode exit is done and
>> replacement of the comment with an implementation fits better there.
>>
> The reason I removed that code was because checkpatch.pl scolded me for a 
> patch with code commented out.
> 
> I assumed that I'd get a similar warning from patchew.org about some 
> erroneous coding styles.
> 
> So I thought the easiest thing would be to remove that code as well.
> 
> But I'll defer to you or Paolo: should I remove that commented code with this 
> patch?

checkpatch errors are not absolutely a no-no, especially if the code is
pre-existing and/or it goes away later in the patch.  In this case,
since you have already written the patch it's okay to keep it as is.

Paolo