Re: [PATCH] x86/nestedhvm: process the MSR-load and MSR-store areas of L1's VMCS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.11.2026 07:33, Mahmoud Abumandour wrote:
> Nothing in vvmx.c reads the VM-entry MSR-load, VM-exit MSR-store or VM-exit
> MSR-load area of L1's VMCS, so L2 runs with the listed MSRs as the previous
> L2 or L1 left them, and L1 gets neither L2's values stored nor its own
> loaded back. Process each area where the SDM does, with the SDM's checks,
> an entry as a WRMSR or RDMSR at CPL 0 that L1 does not intercept. A bad
> VM-entry entry fails the VM entry with exit reason 34 and leaves the VMCS
> unlaunched, a bad VM-exit entry is a VMX abort, which crashes the domain,
> and IA32_VMX_MISC now recommends the 512 entries an area may hold.
> 
> Fixes: 9390642b7fa5 ("Nested VMX: Switch shadow/virtual VMCS between n1/n2 
> guests.")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Mahmoud Abumandour <ma.mandourr@xxxxxxxxx>

Just a couple of mechanical remarks here for the moment:

> @@ -1212,11 +1214,67 @@ static void nvmx_clear_vmcs_pointer(struct vcpu *v, 
> struct vmcs_struct *vvmcs)
>      __vmwrite(VMWRITE_BITMAP, 0);
>  }
>  
> -static void virtual_vmentry(struct cpu_user_regs *regs)
> +/* The longest MSR list that IA32_VMX_MISC recommends to L1. */
> +#define NVMX_MAX_MSRS 512
> +
> +/* A nonempty MSR area must be 16-byte aligned and within MAXPHYADDR. */
> +static bool msr_area_valid(const struct vcpu *v, unsigned int addr_field,
> +                           unsigned int count_field)
> +{
> +    uint64_t addr = get_vvmcs(v, addr_field);
> +    uint64_t count = get_vvmcs(v, count_field);

Please see ./CODING_STYLE for the use of fixed-width types. "addr" likely
wants to be paddr_t, and for "count" unsigned int looks to suffice. Once
changed that way, ...

> +    unsigned int width = v->domain->arch.cpuid->extd.maxphysaddr;
> +
> +    return !count ||
> +           (!(addr & 0xf) && !(addr >> width) &&
> +            !((addr + count * sizeof(struct vmx_msr_entry) - 1) >> width));

... it'll also become clear that the multiplication here cannot overflow
(if that was possible, your check here would be insufficient).

> +/*
> + * Load or store an MSR area of L1's VMCS, each entry as an unintercepted
> + * WRMSR or RDMSR.  Returns 0, or the 1-based index of the failing entry.
> + */
> +static unsigned int process_msr_area(struct vcpu *v, unsigned int addr_field,
> +                                     unsigned int count_field)

"Load or store ..." in the comment is a little odd; the function name
using "process" better describes the purpose of the function. Maybe
"Process an MSR load or store area ..."?

> +{
> +    paddr_t addr = get_vvmcs(v, addr_field);
> +    unsigned int i, count = get_vvmcs(v, count_field);

Interestingly here you already use unsigned int.

Jan