[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 5/9] x86/mem_sharing: use default_access in add_to_physmap
On Wed, Jan 29, 2020 at 6:27 AM Jan Beulich <jbeulich@xxxxxxxx> wrote: > > On 28.01.2020 18:02, Tamas K Lengyel wrote: > > On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@xxxxxxxx> wrote: > >> > >> On 27.01.2020 19:06, Tamas K Lengyel wrote: > >>> When plugging a hole in the target physmap don't use the access permission > >>> returned by __get_gfn_type_access as it can be non-sensical, leading to > >>> spurious vm_events being sent out for access violations at unexpected > >>> locations. Make use of p2m->default_access instead. > >> > >> As before, to me "can be non-sensical" is insufficient as a reason > >> here. If it can also be a "good" value, it still remains unclear > >> why in that case p2m->default_access is nevertheless the right > >> value to use. > > > > I have already explained in the previous version of the patch why I > > said "can be". Forgot to change the commit message from "can be" to > > "is". > > Changing just the commit message would be easy while committing. > But even with the change I would ask why this is. Looking at > ept_get_entry() (and assuming p2m_pt_get_entry() will work > similarly, minus the p2m_access_t which can't come out of the > PTE just yet), I see > > if ( is_epte_valid(ept_entry) ) > { > *t = p2m_recalc_type(recalc || ept_entry->recalc, > ept_entry->sa_p2mt, p2m, gfn); > *a = ept_entry->access; > > near its end. Which means even a hole can have its access field > set. So it's still not clear to me from the description why > p2m->default_access is uniformly the value to use. Wouldn't you > rather want to override the original value only if it's > p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not > paged-out pages)? At this point I would just rather state that add_to_physmap only works on actual holes, not with paged-out pages. In fact, I would like to see mem_paging being dropped from the codebase entirely since it's been abandoned for years and noone expressing any interest in keeping it. In the interim I would rather not spend unnecessary cycles on speculating about potential corner-cases of mem_paging when noone actually uses it. > Of course then the question is whether there > wouldn't be an ambiguity with p2m_access_n having got set > explicitly on the page. But maybe this is impossible for > p2m_invalid / p2m_mmio_dm? As far as mem_access permissions go, I don't know of any usecase that would set mem_access permission on a hole even if by looks of it it is technically possible. At this point I would rather just put this corner-case's description in a comment. Tamas _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |