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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

Jan