| Message ID | fcf622c778b440f4ef2a0cbe707e018216a3aaab.1580148227.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 27.01.2020 19:06, Tamas K Lengyel wrote: > Use __get_gfn_type_access instead of p2m->get_entry to trigger page-forking > when the mem_access permission is being set on a page that has not yet been > copied over from the parent. You talking of page-forking here, don't you mean ... > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -303,11 +303,10 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m, > ASSERT(!ap2m); > #endif > { > - mfn_t mfn; > p2m_access_t _a; > p2m_type_t t; > - > - mfn = p2m->get_entry(p2m, gfn, &t, &_a, 0, NULL, NULL); > + mfn_t mfn = __get_gfn_type_access(p2m, gfn_x(gfn), &t, &_a, > + P2M_ALLOC, NULL, false); ... P2M_UNSHARE here? Also shouldn't you have Cc-ed Petre and Alexandru on this patch (for their R: entries) and at least George (perhaps also Andrew and me) to get an ack, seeing that you're the only maintainer of the file? Jan
On Wed, Jan 29, 2020 at 6:10 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 27.01.2020 19:06, Tamas K Lengyel wrote: > > Use __get_gfn_type_access instead of p2m->get_entry to trigger page-forking > > when the mem_access permission is being set on a page that has not yet been > > copied over from the parent. > > You talking of page-forking here, don't you mean ... > > > --- a/xen/arch/x86/mm/mem_access.c > > +++ b/xen/arch/x86/mm/mem_access.c > > @@ -303,11 +303,10 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m, > > ASSERT(!ap2m); > > #endif > > { > > - mfn_t mfn; > > p2m_access_t _a; > > p2m_type_t t; > > - > > - mfn = p2m->get_entry(p2m, gfn, &t, &_a, 0, NULL, NULL); > > + mfn_t mfn = __get_gfn_type_access(p2m, gfn_x(gfn), &t, &_a, > > + P2M_ALLOC, NULL, false); > > ... P2M_UNSHARE here? No, P2M_UNSHARE is only required if you are doing a memory write. Setting memory access permissions is not a memory write, so it's sufficient to just allocate the p2m entry. P2M_ALLOCATE also encompasses forking the entry if there is a parent VM. > > Also shouldn't you have Cc-ed Petre and Alexandru on this patch > (for their R: entries) and at least George (perhaps also Andrew > and me) to get an ack, seeing that you're the only maintainer > of the file? I've ran ./add_maintainers.pl on the patches, not sure why noone else got CC-d. Tamas
On 29.01.2020 14:53, Tamas K Lengyel wrote: > On Wed, Jan 29, 2020 at 6:10 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 27.01.2020 19:06, Tamas K Lengyel wrote: >>> Use __get_gfn_type_access instead of p2m->get_entry to trigger page-forking >>> when the mem_access permission is being set on a page that has not yet been >>> copied over from the parent. >> >> You talking of page-forking here, don't you mean ... >> >>> --- a/xen/arch/x86/mm/mem_access.c >>> +++ b/xen/arch/x86/mm/mem_access.c >>> @@ -303,11 +303,10 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m, >>> ASSERT(!ap2m); >>> #endif >>> { >>> - mfn_t mfn; >>> p2m_access_t _a; >>> p2m_type_t t; >>> - >>> - mfn = p2m->get_entry(p2m, gfn, &t, &_a, 0, NULL, NULL); >>> + mfn_t mfn = __get_gfn_type_access(p2m, gfn_x(gfn), &t, &_a, >>> + P2M_ALLOC, NULL, false); >> >> ... P2M_UNSHARE here? > > No, P2M_UNSHARE is only required if you are doing a memory write. > Setting memory access permissions is not a memory write, so it's > sufficient to just allocate the p2m entry. P2M_ALLOCATE also > encompasses forking the entry if there is a parent VM. Ah, I see. And hence Reviewed-by: Jan Beulich <jbeulich@suse.com> >> Also shouldn't you have Cc-ed Petre and Alexandru on this patch >> (for their R: entries) and at least George (perhaps also Andrew >> and me) to get an ack, seeing that you're the only maintainer >> of the file? > > I've ran ./add_maintainers.pl on the patches, not sure why noone else got CC-d. It not picking up R: entries would seem like a bug to me. It not knowing of nesting of maintainership is entirely expected (or else it would also need to know that it's - in this case - you who is invoking it). Jan
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index d16540a9aa..ede774fb50 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -303,11 +303,10 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m, ASSERT(!ap2m); #endif { - mfn_t mfn; p2m_access_t _a; p2m_type_t t; - - mfn = p2m->get_entry(p2m, gfn, &t, &_a, 0, NULL, NULL); + mfn_t mfn = __get_gfn_type_access(p2m, gfn_x(gfn), &t, &_a, + P2M_ALLOC, NULL, false); rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a, -1); }
Use __get_gfn_type_access instead of p2m->get_entry to trigger page-forking when the mem_access permission is being set on a page that has not yet been copied over from the parent. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- xen/arch/x86/mm/mem_access.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)