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, <[email protected]> wrote: >>> >>> On 07/24/2018 01:02 PM, Jan Beulich wrote: >>>>>>> On 24.07.18 at 13:26, <[email protected]> wrote: >>>>> >>>>> On 07/24/2018 09:55 AM, Jan Beulich wrote: >>>>>>>>> On 23.07.18 at 15:48, <[email protected]> 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 [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
