Re: [PATCH 13/27] nVMX: Add VMCS fields to the vmcs12

[Avi Kivity <avi@redhat.com>]

  On 10/17/2010 12:10 PM, Nadav Har'El wrote:
> In this patch we add to vmcs12 (the VMCS that L1 keeps for L2) all the
> standard VMCS fields. These fields are encapsulated in a struct vmcs_fields.
>
> Later patches will enable L1 to read and write these fields using VMREAD/
> VMWRITE, and they will be used during a VMLAUNCH/VMRESUME in preparing vmcs02,
> a hardware VMCS for running L2.
>
> Signed-off-by: Nadav Har'El<nyh@il.ibm.com>
> ---
>   arch/x86/kvm/vmx.c |  295 +++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 295 insertions(+)
>
> --- .before/arch/x86/kvm/vmx.c	2010-10-17 11:52:01.000000000 +0200
> +++ .after/arch/x86/kvm/vmx.c	2010-10-17 11:52:01.000000000 +0200
> @@ -128,6 +128,137 @@ struct shared_msr_entry {
>   };
>
>   /*
> + * vmcs_fields is a structure used in nested VMX for holding a copy of all
> + * standard VMCS fields. It is used for emulating a VMCS for L1 (see struct
> + * vmcs12), and also for easier access to VMCS data (see vmcs01_fields).
> + */
> +struct __packed vmcs_fields {

...

> +	unsigned long cr0_guest_host_mask;
> +	unsigned long cr4_guest_host_mask;

Those ulongs won't survive live migrations.  ABIs always want explicitly 
sized types.

Better move them above the u32 so we don't have to check whether there's 
an even number of them.
> +
> +/*
>    * struct vmcs12 describes the state that our guest hypervisor (L1) keeps for a
>    * single nested guest (L2), hence the name vmcs12. Any VMX implementation has
>    * a VMCS structure, and vmcs12 is our emulated VMX's VMCS. This structure is
> @@ -147,6 +278,8 @@ struct __packed vmcs12 {
>   	u32 revision_id;
>   	u32 abort;
>

Reserve some space here.

> +	struct vmcs_fields fields;
> +
>   	bool launch_state; /* set to 0 by VMCLEAR, to 1 by VMLAUNCH */

And move this above fields, so we can expand it later.

>   };
>
> @@ -241,6 +374,168 @@ static inline struct vcpu_vmx *to_vmx(st
>   	return container_of(vcpu, struct vcpu_vmx, vcpu);
>   }
>
> +#define OFFSET(x) offsetof(struct vmcs_fields, x)
> +
> +static unsigned short vmcs_field_to_offset_table[HOST_RIP+1] = {
>
> +	[IO_BITMAP_A] = OFFSET(io_bitmap_a),
> +	[IO_BITMAP_A_HIGH] = OFFSET(io_bitmap_a)+4,

Might have a FIELD(name, field) to define ordinary fields and 
FIELD64(name, field) macros to define both sub-fields of a 64-bit field 
at one.  Can defer until later.

> +};
> +
> +static inline short vmcs_field_to_offset(unsigned long field)
> +{
> +
> +	if (field>  HOST_RIP || vmcs_field_to_offset_table[field] == 0) {
> +		printk(KERN_ERR "invalid vmcs field 0x%lx\n", field);

Guest exploitable printk() - remove.

> +		return -1;
> +	}
> +	return vmcs_field_to_offset_table[field];
> +}
> +

-- 
error compiling committee.c: too many arguments to function