Re: [PATCH v4] x86/nSVM: Validate the L1 IOPM physical address range

On 17.08.2026 18:59, Abdelkareem Abdelsaamad wrote:
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -282,7 +282,7 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>      return 0;
>  }
>  
> -static int nsvm_vmrun_permissionmap(struct vcpu *v, bool viopm)
> +static int nsvm_vmrun_permissionmap(struct vcpu *v)
>  {
>      struct svm_vcpu *arch_svm = &v->arch.hvm.svm;
>      struct nestedsvm *svm = &vcpu_nestedsvm(v);
> @@ -294,6 +294,17 @@ static int nsvm_vmrun_permissionmap(struct vcpu *v, bool viopm)
>      enum hvm_translation_result ret;
>      unsigned long *ns_viomap;
>      bool ioport_80 = true, ioport_ed = true;
> +    /* IOPM is structured as a linear array of 64K+3 bits. */
> +    const unsigned long nr_iopm_additional_pages = PFN_DOWN((0x10000 + 8) / 8 - 1);

Hm, the expression I did suggest was indeed off by one, yet yours doesn't fit
the comment very well. What's wrong with PFN_DOWN((0x10000 + 3) / 8) or
PFN_DOWN((0xffff + 4) / 8)?

> +    gfn_t ns_iopm_end =
> +        gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa), nr_iopm_additional_pages);

I'm also inclined to suggest to drop the local variable, as it's used just
here. The overall result would be

    /* IOPM is structured as a linear array of 64K+3 bits. */
    gfn_t ns_iopm_end = gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa),
                                PFN_DOWN((0x10000 + 3) / 8));

which imo is a little easier to follow. Preferably with those adjustments
(happy to carry out while committing, but please confirm):
Reviewed-by: Jan Beulich <[email protected]>

Jan