On 09/19/2018 02:01 PM, Razvan Cojocaru wrote: > On 9/19/18 3:15 PM, George Dunlap wrote: >> Hey Razvan, thanks for doing this, and sorry it's taken so long to respond. > > No problem, thanks for the review! > >>> We should discuss if just copying over >>> logdirty_ranges (which is a pointer) is sufficient, or if >>> this code requires more synchronization). >> >> It's clearly not sufficient. :-) The logdirty_ranges struct is >> protected by the lock of the p2m structure that contains it; if you >> point to it from a different p2m structure, then you'll have >> inconsistent logging, and you'll have problems if one vcpu is reading >> the structure while another is modifying it. >> >> Another issue is that it doesn't look like you're propagating updates to >> this shared state either -- if someone enables or disables >> global_logdirty, or changes default_access, the altp2ms will still have >> the old value. >> >> I wonder if we should collect the various bits that need to be kept in >> sync between hostp2m/altp2ms, put them all in a 'sync' sub-struct within >> the p2m, and enforce using a function / macro to modify the values inside. > > Right, I'll get on with the sync structure then. > >>> Another aspect is that, while new modifications should work with >>> these changes, _old_ modifications (written to the host2pm >>> _before_ we've created the new altp2m) will, if I understand the >>> code correctly be lost. That is to say, misconfigurations >>> performed before p2m_init_altp2m_ept() in the hostp2m will >>> presumably not trigger the necessary faults after switching to >>> the new altp2m. >> >> You're worried about the following sequence? >> >> 1. Misconfigure hostp2m >> 2. Enable altp2m >> 3. Switch to altp2m 1 >> 4. Fault on a previously-misconfigured p2m entry > > No, I'm worried that at step 4 the fault will no longer happen, because > the EPTP index corresponding to altp2m 1 points to an EPT where the > entries misconfigured in the hostp2m are _not_ misconfigured. > > But actually the sequence I'm worried about is: > > 1. Misconfigure hostp2m > 2. Create altp2m > 3. Enable altp2m > 4. Switch to altp2m 1 > 5. A would-be-fault in the hostp2m no longer occurs
But how would a fault not occur? The altp2m at this point won't have *any* entries; any p2m access at all should fault, yes? At which point the altp2m code will say, "Oh, look, here's an entry I didn't have; I'll copy it from the host p2m". That will call hostp2m->get_entry(), which will resolve the misconfig. Or do I need to go back and review the altp2m code again so I have a clue how it works? -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
