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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2020 18:49, Andrew Cooper wrote:
> 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.

And then
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

>>> --- 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.

The line split is unavoidable if we want the enumeration to be
sensible at all. I have no issue with this, to be honest.

Jan