[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] x86/PCI: Intercept Dom0 MMCFG from dom0s in HVM containers

On 12/16/2015 04:04 AM, Jan Beulich wrote:
On 15.12.15 at 22:02,<boris.ostrovsky@xxxxxxxxxx>  wrote:
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1778,6 +1778,28 @@ int hvm_emulate_one_no_write(
      return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops_no_write);
+static const struct x86_emulate_ops hvm_emulate_ops_mmio = {
+    .read       = mmio_ro_emulated_read,
+    .insn_fetch = hvmemul_insn_fetch,
+    .write      = mmio_intercept_write,
Blank line missing here, but as a probably better alternative this could
move into the function below.

+int hvm_emulate_one_mmio(unsigned seg, unsigned bdf, unsigned long gla)
Name and function parameters don't match up (also for e.g. the
changed struct mmio_ro_emulate_ctxt). DYM e.g.

Yes. I in fact first named it *_mmcfg() and then renamed it to _*mmio(). But then by the same logic wouldn't we want to rename mmio_intercept_write() as well, not necessarily because of arguments but because of what it actually does?

I am not sure what you mean by your statement in parentheses about mmio_ro_emulate_ctxt.

(The function naming in x86/mm.c
is actually using mmio because its serving wider purpose than
just MMCFG write interception. Which makes me wonder whether
we don't actually need that other part for PVH too, in which case
the naming would again be fine.)

I don't think I understand what the other part is used for in PV so I don't know whether it is useful for PVH.

+    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = {
+        .cr2 = gla,
+        .seg = seg,
+        .bdf = bdf
+    };
+    struct hvm_emulate_ctxt ctxt = { .ctxt.data = &mmio_ro_ctxt };
hvm_mem_access_emulate_one() so far is the only example pre-
initializing ctxt before calling hvm_emulate_prepare(), and I don't
think it actually needs this. I think the proper place to set .data is
after the call to hvm_emulate_prepare().

I used hvm_emulate_prepare() as a convenient way to initialize the context to a reasonable state, with definition of "reasonable" possibly changing at some point later. Other emulating routines have hvm_emulate_ctxt passed in so it may already have certain fields set.

(Yes, .data needs to be set afterwards).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.