| Message ID | 20200310174917.1514-6-paul@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remove one more shared xenheap page: shared_info | expand |
On 10.03.2020 18:49, paul@xen.org wrote: > From: Paul Durrant <pdurrant@amazon.com> > > Currently shared_info is a shared xenheap page but shared xenheap pages > complicate future plans for live-update of Xen so it is desirable to, > where possible, not use them [1]. This patch therefore converts shared_info > into a PGC_extra domheap page. This does entail freeing shared_info during > domain_relinquish_resources() rather than domain_destroy() so care is > needed to avoid de-referencing a NULL shared_info pointer hence some > extra checks of 'is_dying' are needed. If there's going to be agreement to follow this route, the implementation, with a really minor cosmetic adjustment - see below -, looks okay to me. Nevertheless I continue to dislike the implication from the extra care that's now needed. As I think I have said before, I'd like to have at least one other REST maintainer's opinion here. > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -260,9 +260,12 @@ void dump_pageframe_info(struct domain *d) > > page_list_for_each ( page, &d->extra_page_list ) > { > - printk(" ExtraPage %p: caf=%08lx, taf=%" PRtype_info "\n", > + const char *tag = mfn_eq(page_to_mfn(page), d->shared_info.mfn) ? > + "[SHARED INFO]" : ""; Please can this be " [SHARED INFO]" with ... > + printk(" ExtraPage %p: caf=%08lx, taf=%" PRtype_info " %s\n", ... the blank before the final %s dropped here, such that we won't have a trailing blank in the output? Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 17 March 2020 13:14 > To: paul@xen.org > Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Stefano Stabellini > <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap > <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Wei Liu <wl@xen.org> > Subject: RE: [EXTERNAL] [PATCH v6 5/5] domain: use PGC_extra domheap page for shared_info > > CAUTION: This email originated from outside of the organization. Do not click links or open > attachments unless you can confirm the sender and know the content is safe. > > > > On 10.03.2020 18:49, paul@xen.org wrote: > > From: Paul Durrant <pdurrant@amazon.com> > > > > Currently shared_info is a shared xenheap page but shared xenheap pages > > complicate future plans for live-update of Xen so it is desirable to, > > where possible, not use them [1]. This patch therefore converts shared_info > > into a PGC_extra domheap page. This does entail freeing shared_info during > > domain_relinquish_resources() rather than domain_destroy() so care is > > needed to avoid de-referencing a NULL shared_info pointer hence some > > extra checks of 'is_dying' are needed. > > If there's going to be agreement to follow this route, the implementation, > with a really minor cosmetic adjustment - see below -, looks okay to me. > Nevertheless I continue to dislike the implication from the extra care > that's now needed. As I think I have said before, I'd like to have at > least one other REST maintainer's opinion here. > Ok, fair enough. > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -260,9 +260,12 @@ void dump_pageframe_info(struct domain *d) > > > > page_list_for_each ( page, &d->extra_page_list ) > > { > > - printk(" ExtraPage %p: caf=%08lx, taf=%" PRtype_info "\n", > > + const char *tag = mfn_eq(page_to_mfn(page), d->shared_info.mfn) ? > > + "[SHARED INFO]" : ""; > > Please can this be " [SHARED INFO]" with ... > > > + printk(" ExtraPage %p: caf=%08lx, taf=%" PRtype_info " %s\n", > > ... the blank before the final %s dropped here, such that we won't > have a trailing blank in the output? Sure. Paul > > Jan
On 10.03.2020 18:49, paul@xen.org wrote: > From: Paul Durrant <pdurrant@amazon.com> > > Currently shared_info is a shared xenheap page but shared xenheap pages > complicate future plans for live-update of Xen so it is desirable to, > where possible, not use them [1]. This patch therefore converts shared_info > into a PGC_extra domheap page. This does entail freeing shared_info during > domain_relinquish_resources() rather than domain_destroy() so care is > needed to avoid de-referencing a NULL shared_info pointer hence some > extra checks of 'is_dying' are needed. > > NOTE: For Arm, the call to free_shared_info() in arch_domain_destroy() is > left in place since it is idempotent and called in the error path for > arch_domain_create(). > > [1] See https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg02018.html > > Signed-off-by: Paul Durrant <paul@xen.org> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien@xen.org> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Wei Liu <wl@xen.org> > > v6: > - Drop dump_shared_info() but tag the shared info in the 'ExtraPage' > dump > > v5: > - Incorporate new dump_shared_info() function > > v2: > - Addressed comments from Julien > - Expanded the commit comment to explain why this patch is wanted > --- > xen/arch/arm/domain.c | 2 ++ Julien, Stefano? (I'd prefer to commit the entire series in one go, rather than leaving out just this last patch.) Jan
On 24.03.2020 10:26, Jan Beulich wrote: > On 10.03.2020 18:49, paul@xen.org wrote: >> From: Paul Durrant <pdurrant@amazon.com> >> >> Currently shared_info is a shared xenheap page but shared xenheap pages >> complicate future plans for live-update of Xen so it is desirable to, >> where possible, not use them [1]. This patch therefore converts shared_info >> into a PGC_extra domheap page. This does entail freeing shared_info during >> domain_relinquish_resources() rather than domain_destroy() so care is >> needed to avoid de-referencing a NULL shared_info pointer hence some >> extra checks of 'is_dying' are needed. >> >> NOTE: For Arm, the call to free_shared_info() in arch_domain_destroy() is >> left in place since it is idempotent and called in the error path for >> arch_domain_create(). >> >> [1] See https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg02018.html >> >> Signed-off-by: Paul Durrant <paul@xen.org> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien@xen.org> >> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: George Dunlap <george.dunlap@citrix.com> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: Wei Liu <wl@xen.org> >> >> v6: >> - Drop dump_shared_info() but tag the shared info in the 'ExtraPage' >> dump >> >> v5: >> - Incorporate new dump_shared_info() function >> >> v2: >> - Addressed comments from Julien >> - Expanded the commit comment to explain why this patch is wanted >> --- >> xen/arch/arm/domain.c | 2 ++ > > Julien, Stefano? (I'd prefer to commit the entire series in one go, > rather than leaving out just this last patch.) Actually - never mind, I've just realized that there are still some pending items on the last two patches of this series. Jan
Hi Paul, On 10/03/2020 17:49, paul@xen.org wrote: > From: Paul Durrant <pdurrant@amazon.com> > > Currently shared_info is a shared xenheap page but shared xenheap pages > complicate future plans for live-update of Xen so it is desirable to, > where possible, not use them [1]. This patch therefore converts shared_info > into a PGC_extra domheap page. This does entail freeing shared_info during > domain_relinquish_resources() rather than domain_destroy() so care is > needed to avoid de-referencing a NULL shared_info pointer hence some > extra checks of 'is_dying' are needed. > > NOTE: For Arm, the call to free_shared_info() in arch_domain_destroy() is > left in place since it is idempotent and called in the error path for > arch_domain_create(). The approach looks good to me. I have one comment below. [...] > diff --git a/xen/common/domain.c b/xen/common/domain.c > index 4ef0d3b21e..4f3266454f 100644 > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -1651,24 +1651,44 @@ int continue_hypercall_on_cpu( > > int alloc_shared_info(struct domain *d, unsigned int memflags) > { > - if ( (d->shared_info.virt = alloc_xenheap_pages(0, memflags)) == NULL ) > + struct page_info *pg; > + > + pg = alloc_domheap_page(d, MEMF_no_refcount | memflags); > + if ( !pg ) > return -ENOMEM; > > - d->shared_info.mfn = virt_to_mfn(d->shared_info.virt); > + if ( !get_page_and_type(pg, d, PGT_writable_page) ) > + { > + /* > + * The domain should not be running at this point so there is > + * no way we should reach this error path. > + */ > + ASSERT_UNREACHABLE(); > + return -ENODATA; > + } > + > + d->shared_info.mfn = page_to_mfn(pg); > + d->shared_info.virt = __map_domain_page_global(pg); __map_domain_page_global() is not guaranteed to succeed. For instance, on Arm32 this will be a call to vmap(). So you want to check the return. Cheers,
> -----Original Message----- [snip] > > Currently shared_info is a shared xenheap page but shared xenheap pages > > complicate future plans for live-update of Xen so it is desirable to, > > where possible, not use them [1]. This patch therefore converts shared_info > > into a PGC_extra domheap page. This does entail freeing shared_info during > > domain_relinquish_resources() rather than domain_destroy() so care is > > needed to avoid de-referencing a NULL shared_info pointer hence some > > extra checks of 'is_dying' are needed. > > > > NOTE: For Arm, the call to free_shared_info() in arch_domain_destroy() is > > left in place since it is idempotent and called in the error path for > > arch_domain_create(). > > The approach looks good to me. I have one comment below. > Thanks. > [...] > > > diff --git a/xen/common/domain.c b/xen/common/domain.c > > index 4ef0d3b21e..4f3266454f 100644 > > --- a/xen/common/domain.c > > +++ b/xen/common/domain.c > > @@ -1651,24 +1651,44 @@ int continue_hypercall_on_cpu( > > > > int alloc_shared_info(struct domain *d, unsigned int memflags) > > { > > - if ( (d->shared_info.virt = alloc_xenheap_pages(0, memflags)) == NULL ) > > + struct page_info *pg; > > + > > + pg = alloc_domheap_page(d, MEMF_no_refcount | memflags); > > + if ( !pg ) > > return -ENOMEM; > > > > - d->shared_info.mfn = virt_to_mfn(d->shared_info.virt); > > + if ( !get_page_and_type(pg, d, PGT_writable_page) ) > > + { > > + /* > > + * The domain should not be running at this point so there is > > + * no way we should reach this error path. > > + */ > > + ASSERT_UNREACHABLE(); > > + return -ENODATA; > > + } > > + > > + d->shared_info.mfn = page_to_mfn(pg); > > + d->shared_info.virt = __map_domain_page_global(pg); > > __map_domain_page_global() is not guaranteed to succeed. For instance, > on Arm32 this will be a call to vmap(). > > So you want to check the return. > Ok, I'll add a check. As Jan discovered, I messed up the version numbering so there is already a v7 series where I dropped this patch (until I can group it with conversion of other shared xenheap pages). Paul > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 5298d80bd2..741f6dd444 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -1005,6 +1005,8 @@ int domain_relinquish_resources(struct domain *d) BUG(); } + free_shared_info(d); + return 0; } diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 2dda2dbca1..539b5d9fe0 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -260,9 +260,12 @@ void dump_pageframe_info(struct domain *d) page_list_for_each ( page, &d->extra_page_list ) { - printk(" ExtraPage %p: caf=%08lx, taf=%" PRtype_info "\n", + const char *tag = mfn_eq(page_to_mfn(page), d->shared_info.mfn) ? + "[SHARED INFO]" : ""; + + printk(" ExtraPage %p: caf=%08lx, taf=%" PRtype_info " %s\n", _p(mfn_x(page_to_mfn(page))), - page->count_info, page->u.inuse.type_info); + page->count_info, page->u.inuse.type_info, tag); } spin_unlock(&d->page_alloc_lock); } @@ -697,7 +700,6 @@ void arch_domain_destroy(struct domain *d) pv_domain_destroy(d); free_perdomain_mappings(d); - free_shared_info(d); cleanup_domain_irq_mapping(d); psr_domain_free(d); @@ -2252,6 +2254,8 @@ int domain_relinquish_resources(struct domain *d) if ( is_hvm_domain(d) ) hvm_domain_relinquish_resources(d); + free_shared_info(d); + return 0; } diff --git a/xen/common/domain.c b/xen/common/domain.c index 4ef0d3b21e..4f3266454f 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1651,24 +1651,44 @@ int continue_hypercall_on_cpu( int alloc_shared_info(struct domain *d, unsigned int memflags) { - if ( (d->shared_info.virt = alloc_xenheap_pages(0, memflags)) == NULL ) + struct page_info *pg; + + pg = alloc_domheap_page(d, MEMF_no_refcount | memflags); + if ( !pg ) return -ENOMEM; - d->shared_info.mfn = virt_to_mfn(d->shared_info.virt); + if ( !get_page_and_type(pg, d, PGT_writable_page) ) + { + /* + * The domain should not be running at this point so there is + * no way we should reach this error path. + */ + ASSERT_UNREACHABLE(); + return -ENODATA; + } + + d->shared_info.mfn = page_to_mfn(pg); + d->shared_info.virt = __map_domain_page_global(pg); clear_page(d->shared_info.virt); - share_xen_page_with_guest(mfn_to_page(d->shared_info.mfn), d, SHARE_rw); return 0; } void free_shared_info(struct domain *d) { + struct page_info *pg; + if ( !d->shared_info.virt ) return; - free_xenheap_page(d->shared_info.virt); + unmap_domain_page_global(d->shared_info.virt); d->shared_info.virt = NULL; + + pg = mfn_to_page(d->shared_info.mfn); + + put_page_alloc_ref(pg); + put_page_and_type(pg); } /* diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index e86e2bfab0..a17422284d 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -1325,6 +1325,9 @@ void evtchn_destroy(struct domain *d) { unsigned int i; + /* This must be done before shared_info is freed */ + BUG_ON(!d->shared_info.virt); + /* After this barrier no new event-channel allocations can occur. */ BUG_ON(!d->is_dying); spin_barrier(&d->event_lock); diff --git a/xen/common/time.c b/xen/common/time.c index 58fa9abc40..ada02faf07 100644 --- a/xen/common/time.c +++ b/xen/common/time.c @@ -99,6 +99,18 @@ void update_domain_wallclock_time(struct domain *d) uint32_t *wc_version; uint64_t sec; + if ( d != current->domain ) + { + /* + * We need to check is_dying here as, if it is set, the + * shared_info may have been freed. To do this safely we need + * hold the domain lock. + */ + domain_lock(d); + if ( d->is_dying ) + goto unlock; + } + spin_lock(&wc_lock); wc_version = &shared_info(d, wc_version); @@ -121,6 +133,9 @@ void update_domain_wallclock_time(struct domain *d) *wc_version = version_update_end(*wc_version); spin_unlock(&wc_lock); + unlock: + if ( d != current->domain ) + domain_unlock(d); } /* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */