Re: [Xen-devel] [PATCH v4] x86/mm: Add mem access rights to NPT

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/26/2018 09:17 AM, Isaila Alexandru wrote:
> On Wed, 2018-07-25 at 04:37 -0600, Jan Beulich wrote:
>>>>> On 25.07.18 at 11:25, <george.dunlap@xxxxxxxxxx> wrote:
>>>
>>> On 07/24/2018 01:02 PM, Jan Beulich wrote:
>>>>>>> On 24.07.18 at 13:26, <george.dunlap@xxxxxxxxxx> wrote:
>>>>>
>>>>> On 07/24/2018 09:55 AM, Jan Beulich wrote:
>>>>>>>>> On 23.07.18 at 15:48, <aisaila@xxxxxxxxxxxxxxx> wrote:
>>>>>>>
>>>>>>> +        {
>>>>>>> +            xfree(d->arch.monitor.msr_bitmap);
>>>>>>> +            return -ENOMEM;
>>>>>>> +        }
>>>>>>> +        radix_tree_init(p2m->mem_access_settings);
>>>>>>> +    }
>>>>>>
>>>>>> What's the SVM connection here? Please don't forget that p2m-
>>>>>> pt.c
>>>>>> also serves the shadow case. Perhaps struct p2m_domain should
>>>>>> contain a boolean indicator whether this auxiliary data
>>>>>> structure is
>>>>>> needed?
>>>>>
>>>>> It's basically just "hap_enabled()" isn't it?
>>>>
>>>> Only if we can't make it there when EPT is active.
>>>
>>> It can make it here when VMX is active and shadow is enabled, but
>>> it
>>> shouldn't be able to get here when EPT is active.  We could add an
>>> ASSERT() to that effect; it should be safe in production, as the
>>> only
>>> side effect should be that we do a small pointless allocation.
>>
>> So I've looked a little more closely: This is being added to
>> arch_monitor_init_domain(), called from vm_event_domctl(). I can't
>> see why this wouldn't be reachable with EPT enabled.
>>
> Hi George, 
> 
> Any input on this?

Sorry, you're still waiting for me to weigh in on whether you can get to
arch_monitor_init_domain() with EPT enabled?

The obvious answer is 'yes'; I hope you don't need me to tell you that. :-)

Going back through this conversation, I'm not 100% clear what I meant
with my "hap_enabled()" comment -- my best guess is that I was
suggesting the check be `( cpu_has_svm || !hap_enabled() )`.

But in any case, on the whole patch, I think that:

1) The feature is not abstracted well enough. mem_access.c should not
have to know whether additional storage is required or not; that should
be all taken care of within p2m-pt.c (or p2m-ept.c, should that ever
become necessary).

2) We've been talking for a long time about having a place to store
additional per-pfn information; it would be good if this mechanism were
made general enough to be used for other types of storage.

Let me play around with what you have and see if I can get a mock-up of
something like what I'm looking for.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel