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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 05, 2020 at 06:32:50PM +0100, Andrew Cooper wrote:
> Rework the vmcbcleanbits_t definitons to use bool, drop 'fields' from the
> namespace, position the comments in an unambiguous position, and include the
> bit position.
> 
> In svm_vmexit_handler(), don't bother conditionally writing ~0 or 0 based on
> hardware support.  The field was entirely unused and ignored on older
> hardware (and we're already setting reserved cleanbits anyway).
> 
> In nsvm_vmcb_prepare4vmrun(), simplify the logic massivly by dropping the
                                                        ^e
> vcleanbit_set() macro using a vmcbcleanbits_t local variable which only gets
> filled in the case that clean bits were valid previously.  Fix up the style on
> impacted lines.
> 
> No practical change in behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 5950e4d52b..aeebeaf873 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -345,7 +345,7 @@ static int svm_vmcb_restore(struct vcpu *v, struct 
> hvm_hw_cpu *c)
>      else
>          vmcb->event_inj.raw = 0;
>  
> -    vmcb->cleanbits.bytes = 0;
> +    vmcb->cleanbits.raw = 0;
>      paging_update_paging_modes(v);
>  
>      return 0;
> @@ -693,12 +693,12 @@ static void svm_set_segment_register(struct vcpu *v, 
> enum x86_segment seg,
>      case x86_seg_ds:
>      case x86_seg_es:
>      case x86_seg_ss: /* cpl */
> -        vmcb->cleanbits.fields.seg = 0;
> +        vmcb->cleanbits.seg = 0;
>          break;
>  
>      case x86_seg_gdtr:
>      case x86_seg_idtr:
> -        vmcb->cleanbits.fields.dt = 0;
> +        vmcb->cleanbits.dt = 0;

Nit: using false here (and above) would be better, since the fields
are now booleans.

Thanks, Roger.