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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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.
> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel