Re: [PATCH 1/3] target/i386: Added consistency checks for VMRUN intercept and ASID

[Paolo Bonzini <pbonzini@redhat.com>]

On 14/06/21 12:09, Lara Lazier wrote:
> +#define SVM_VMRUN_INTERCEPT (1ULL << 32)
> +
>   struct QEMU_PACKED vmcb_control_area {
>   	uint16_t intercept_cr_read;
>   	uint16_t intercept_cr_write;

...

> +    if (!(env->intercept & SVM_VMRUN_INTERCEPT)) {
> +        cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC());
> +    }

Hi Lara,

as discussed in our weekly meeting, the only issue with these patch is a 
matter of aesthetics and maintainability more than functionality; 
namely, the duplication between SVM_VMRUN_INTERCEPT and SVM_EXIT_VMRUN, 
and likewise in patch 3 between INTERCEPT_SELECTIVE_CR0 and 
SVM_EXIT_CR0_SEL_WRITE.  Showing them side by side also makes it 
apparent that the names are not consistent, but it's even better to 
avoid the duplication altogether if possible.

In particular, one way to do so is to extract the intercept checks to a 
function that you can call like

     cpu_svm_has_intercept(env, SVM_EXIT_VMRUN)

so that the function computes the right bit of the bitmap based on the 
second argument.  Most of the code to do this is already in 
svm_helper.c's cpu_svm_check_intercept_param, which you're already 
familiar with.  cpu_svm_check_intercept_param can also be modified to 
call the new cpu_svm_has_intercept.

When your second version of the patches are ready, you can add the "-v2" 
argument to git format-patch and it will automatically start the 
subjects with "[PATCH v2 ...]" instead of just "[PATCH ...]".

Paolo