Re: [Xen-devel] [PATCH v2 4/4] SVM: clean up svm_vmcb_isvalid()

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

>  
> -bool_t
> -svm_vmcb_isvalid(const char *from, struct vmcb_struct *vmcb,
> -                 bool_t verbose)
> +bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
> +                      const struct vcpu *v, bool verbose)
>  {
> -    bool_t ret = 0; /* ok */
> +    bool ret = false; /* ok */

I think stashing control register and EFER (and maybe more) into local
variables will make code much more readable.


> +    if ( (vmcb_get_cr0(vmcb) & X86_CR0_PG) &&
> +         ((vmcb_get_cr3(vmcb) & 0x7) ||
> +          ((!(vmcb_get_cr4(vmcb) & X86_CR4_PAE) ||
> +            (vmcb_get_efer(vmcb) & EFER_LMA)) &&
> +           (vmcb_get_cr3(vmcb) & 0xfe0)) ||
> +          ((vmcb_get_efer(vmcb) & EFER_LMA) &&
> +           (vmcb_get_cr3(vmcb) >> v->domain->arch.cpuid->extd.maxphysaddr))) 
> )
> +        PRINTF("CR3: MBZ bits are set (%#"PRIx64")\n", vmcb_get_cr3(vmcb));

For example, this fragment took me a while to mentally unwrap. Shorter
names should allow you to group them better per line.

(also might generate less code, depending on compiler)

-boris



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel