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....@citrix.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
