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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.12.15 at 15:04, <boris.ostrovsky@oracle.com> wrote:
> On 12/16/2015 04:04 AM, Jan Beulich wrote:
>>>>> On 15.12.15 at 22:02,<boris.ostrovsky@oracle.com>  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?

We might. I named it that way just to match its neighbors.

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

I simply hinted at this also likely needing s/mmio/mmcfg/.

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

It discards writes to the config space of r/o devices.

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

Oh, I wasn't questioning that part - you certainly need to use that
function. I was questioning the zero-initialization of all the fields
other than .data prior to calling this function.

Jan