Re: [PATCH v1 1/5] KVM: x86: nVMX: Add vmcs12 field existence bitmap in nested_vmx

[Paolo Bonzini <pbonzini@redhat.com>]

On 17/08/21 11:31, Robert Hoo wrote:
> +#define FIELD_BIT_SET(name, bitmap) set_bit(f_pos(name), bitmap)
> +#define FIELD64_BIT_SET(name, bitmap)	\
> +	do {set_bit(f_pos(name), bitmap);	\
> +	    set_bit(f_pos(name) + (sizeof(u32) / sizeof(u16)), bitmap);\
> +	} while (0)
> +
> +#define FIELD_BIT_CLEAR(name, bitmap) clear_bit(f_pos(name), bitmap)
> +#define FIELD64_BIT_CLEAR(name, bitmap)	\
> +	do {clear_bit(f_pos(name), bitmap);	\
> +	    clear_bit(f_pos(name) + (sizeof(u32) / sizeof(u16)), bitmap);\
> +	} while (0)
> +
> +#define FIELD_BIT_CHANGE(name, bitmap) change_bit(f_pos(name), bitmap)
> +#define FIELD64_BIT_CHANGE(name, bitmap)	\
> +	do {change_bit(f_pos(name), bitmap);	\
> +	    change_bit(f_pos(name) + (sizeof(u32) / sizeof(u16)), bitmap);\
> +	} while (0)
> +
> +/*

Hi Robert,

I'd rather not have FIELD_BIT_CHANGE, and instead have something like

#define FIELD_BIT_ASSIGN(name, bitmap, value) \
	if (value) \
		FIELD_BIT_SET(name, bitmap); \
	else
		FIELD_BIT_CLEAR(name, bitmap);

Also, these set_bit/clear_bit can use the non-atomic variants __set_bit 
and __clear_bit, because the bitmaps are protected by the vCPU mutex.

> +		FIELD64_BIT_CHANGE(posted_intr_desc_addr, bitmap);

Many of the fields you mark as 64-bit are actually natural sized.

> +	if ((old_val ^ new_val) &
> +	    CPU_BASED_ACTIVATE_SECONDARY_CONTROLS) {
> +		FIELD_BIT_CHANGE(secondary_vm_exec_control, bitmap);
> +	}
> +}

If secondary controls are not available, you should treat the 
corresponding MSR as if it was all zeroes.  Likewise if VMFUNC is disabled.

> +	if ((old_val ^ new_val) & SECONDARY_EXEC_PAUSE_LOOP_EXITING) {
> +		FIELD64_BIT_CHANGE(vmread_bitmap, bitmap);
> +		FIELD64_BIT_CHANGE(vmwrite_bitmap, bitmap);

This seems wrong.

Paolo