From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] x86/PCI: Intercept Dom0 MMCFG from dom0s in HVM containers Date: Wed, 16 Dec 2015 09:04:57 -0500 Message-ID: <56716F89.8080306@oracle.com> References: <1450213365-3490-1-git-send-email-boris.ostrovsky@oracle.com> <5671374602000078000C0079@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5671374602000078000C0079@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: andrew.cooper3@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 12/16/2015 04:04 AM, Jan Beulich wrote: >>>> On 15.12.15 at 22:02, 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. > hvm_emulate_one_mmcfg()? 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). -boris