Re: [PATCH] x86/svm: Clean up vmcbcleanbits_t handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/05/2020 16:10, Jan Beulich wrote:
> On 05.05.2020 19:32, Andrew Cooper wrote:
>> @@ -435,17 +435,13 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, 
>> struct cpu_user_regs *regs)
>>      ASSERT(n2vmcb != NULL);
>>  
>>      /* Check if virtual VMCB cleanbits are valid */
>> -    vcleanbits_valid = 1;
>> -    if ( svm->ns_ovvmcb_pa == INVALID_PADDR )
>> -        vcleanbits_valid = 0;
>> -    if (svm->ns_ovvmcb_pa != nv->nv_vvmcxaddr)
>> -        vcleanbits_valid = 0;
>> -
>> -#define vcleanbit_set(_name)        \
>> -    (vcleanbits_valid && ns_vmcb->cleanbits.fields._name)
>> +    if ( svm->ns_ovvmcb_pa != INVALID_PADDR &&
>> +         svm->ns_ovvmcb_pa != nv->nv_vvmcxaddr )
>> +        clean = ns_vmcb->cleanbits;
> It looks to me as if the proper inversion of the original condition
> would mean == on the right side of &&, not != .

Oops, yes.  Fixed.

>
>> --- a/xen/include/asm-x86/hvm/svm/vmcb.h
>> +++ b/xen/include/asm-x86/hvm/svm/vmcb.h
>> @@ -384,34 +384,21 @@ typedef union
>>  
>>  typedef union
>>  {
>> -    uint32_t bytes;
>> -    struct
>> -    {
>> -        /* cr_intercepts, dr_intercepts, exception_intercepts,
>> -         * general{1,2}_intercepts, pause_filter_count, tsc_offset */
>> -        uint32_t intercepts: 1;
>> -        /* iopm_base_pa, msrpm_base_pa */
>> -        uint32_t iopm: 1;
>> -        /* guest_asid */
>> -        uint32_t asid: 1;
>> -        /* vintr */
>> -        uint32_t tpr: 1;
>> -        /* np_enable, h_cr3, g_pat */
>> -        uint32_t np: 1;
>> -        /* cr0, cr3, cr4, efer */
>> -        uint32_t cr: 1;
>> -        /* dr6, dr7 */
>> -        uint32_t dr: 1;
>> -        /* gdtr, idtr */
>> -        uint32_t dt: 1;
>> -        /* cs, ds, es, ss, cpl */
>> -        uint32_t seg: 1;
>> -        /* cr2 */
>> -        uint32_t cr2: 1;
>> -        /* debugctlmsr, last{branch,int}{to,from}ip */
>> -        uint32_t lbr: 1;
>> -        uint32_t resv: 21;
>> -    } fields;
>> +    struct {
>> +        bool intercepts:1; /* 0:  cr/dr/exception/general1/2_intercepts,
>> +                            *     pause_filter_count, tsc_offset */
> Could I talk you into omitting the 1/2 part, as there's going to
> be a 3 for at least MCOMMIT? Just "general" ought to be clear
> enough, I would think.

Can do.  I'm not overly happy about this spilling onto two lines, but I
can't think of how to usefully shrink the comment further without losing
information.

~Andrew