> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf
> Of Jan Beulich
> Sent: 11 September 2018 15:56
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Stefano Stabellini <sstabell...@kernel.org>; Wei Liu
> <wei.l...@citrix.com>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>;
> George Dunlap <george.dun...@citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
> (Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; xen-devel
> <xen-devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH v6 12/14] memory: add get_paged_gfn() as
> a wrapper...
>
> >>> On 23.08.18 at 11:47, <paul.durr...@citrix.com> wrote:
> > ...for some uses of get_page_from_gfn().
> >
> > There are many occurences of the following pattern in the code:
> >
> > q = <readonly look-up> ? P2M_ALLOC : P2M_UNSHARE;
>
> Especially with this UNSHARE in mind - is "paged" in the helper
> function's name really suitable? Since we (I think) already have
> get_gfn(), how about try_get_gfn()?
That name may be a little misleading since it suggests a close functional
relationship with get_gfn() whereas it does more than that. How about
try_get_page_from_gfn()?
>
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -350,34 +350,16 @@ static int hvmemul_do_io_buffer(
> >
> > static int hvmemul_acquire_page(unsigned long gmfn, struct page_info
> **page)
> > {
> > - struct domain *curr_d = current->domain;
> > - p2m_type_t p2mt;
> > -
> > - *page = get_page_from_gfn(curr_d, gmfn, &p2mt, P2M_UNSHARE);
> > -
> > - if ( *page == NULL )
> > - return X86EMUL_UNHANDLEABLE;
> > -
> > - if ( p2m_is_paging(p2mt) )
> > - {
> > - put_page(*page);
> > - p2m_mem_paging_populate(curr_d, gmfn);
> > - return X86EMUL_RETRY;
> > - }
> > -
> > - if ( p2m_is_shared(p2mt) )
> > + switch ( get_paged_gfn(current->domain, _gfn(gmfn), false, NULL,
> page) )
> > {
> > - put_page(*page);
> > + case -EAGAIN:
> > return X86EMUL_RETRY;
> > - }
> > -
> > - /* This code should not be reached if the gmfn is not RAM */
> > - if ( p2m_is_mmio(p2mt) )
> > - {
> > - domain_crash(curr_d);
> > -
> > - put_page(*page);
> > + case -EINVAL:
> > return X86EMUL_UNHANDLEABLE;
> > + default:
> > + ASSERT_UNREACHABLE();
> > + case 0:
>
> I think you'd better have "default:" fall through to "case -EINVAL".
> Similarly elsewhere.
Ok. I'll keep the ASSERT_UNREACHABLE() though.
Paul
>
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -2557,24 +2557,12 @@ static void *_hvm_map_guest_frame(unsigned
> long gfn, bool_t permanent,
> > bool_t *writable)
> > {
> > void *map;
> > - p2m_type_t p2mt;
> > struct page_info *page;
> > struct domain *d = current->domain;
> > + p2m_type_t p2mt;
>
> ???
>
> Jan
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
