Re: [PATCH 1/2] x86/hvm: remove unreachable MMCFG write emulation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 11:31, Roger Pau Monne wrote:
> The current implementation of PVH dom0 relies on vPCI to trap and handle
> accesses to the MMCFG area.  Previous implementation of PVH dom0 (v1)
> didn't have vPCI, and as a classic PV dom0, relied on the MMCFG range being
> RO.  As such hvm_emulate_one_mmio() had to special case write accesses to
> the MMCFG area.
> 
> With PVH dom0 using vPCI, and the MMCFG accesses being fully handled there,
> hvm_emulate_one_mmio() should never handle accesses to MMCFG, making the
> code effectively unreachable.
> 
> Remove it and leave an ASSERT to make sure MMCFG accesses never get into
> hvm_emulate_one_mmio().  As a result of the removal of one of the users of
> mmcfg_intercept_write(), the function can now be moved into the same
> translation unit where it's solely used, allowing it to be made static and
> effectively built only when PV support is enabled.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one cosmetic suggestion:

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2858,12 +2858,6 @@ int hvm_emulate_one(
>  
>  int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>  {
> -    static const struct x86_emulate_ops hvm_intercept_ops_mmcfg = {
> -        .read       = x86emul_unhandleable_rw,
> -        .insn_fetch = hvmemul_insn_fetch,
> -        .write      = mmcfg_intercept_write,
> -        .validate   = hvmemul_validate,
> -    };
>      static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = {
>          .read       = x86emul_unhandleable_rw,
>          .insn_fetch = hvmemul_insn_fetch,
> @@ -2872,28 +2866,28 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned 
> long gla)
>      };
>      struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla, .mfn = 
> _mfn(mfn) };
>      struct hvm_emulate_ctxt ctxt;
> -    const struct x86_emulate_ops *ops;
>      unsigned int seg, bdf;
>      int rc;
>  
>      if ( pci_ro_mmcfg_decode(mfn, &seg, &bdf) )
>      {
> -        mmio_ro_ctxt.seg = seg;
> -        mmio_ro_ctxt.bdf = bdf;
> -        ops = &hvm_intercept_ops_mmcfg;
> +        /* Should be always handled by vPCI for PVH dom0. */
> +        gdprintk(XENLOG_ERR, "unhandled MMCFG access for %pp\n",
> +                 &PCI_SBDF(seg, bdf));
> +        ASSERT_UNREACHABLE();
> +        return X86EMUL_UNHANDLEABLE;
>      }
> -    else
> -        ops = &hvm_ro_emulate_ops_mmio;
>  
>      hvm_emulate_init_once(&ctxt, x86_insn_is_mem_write,
>                            guest_cpu_user_regs());
>      ctxt.ctxt.data = &mmio_ro_ctxt;
>  
> -    switch ( rc = _hvm_emulate_one(&ctxt, ops, VIO_no_completion) )
> +    switch ( rc = _hvm_emulate_one(&ctxt, &hvm_ro_emulate_ops_mmio,
> +                                   VIO_no_completion) )
>      {
>      case X86EMUL_UNHANDLEABLE:
>      case X86EMUL_UNIMPLEMENTED:
> -        hvm_dump_emulation_state(XENLOG_G_WARNING, "MMCFG", &ctxt, rc);
> +        hvm_dump_emulation_state(XENLOG_G_WARNING, "RO MMIO", &ctxt, rc);

The string doesn't need to be all capitals (see other uses of the function).
How about "r/o MMIO"?

Jan