> -----Original Message-----
> From: Xen-devel [mailto:[email protected]] On Behalf
> Of Jan Beulich
> Sent: 11 September 2018 15:56
> To: Paul Durrant <[email protected]>
> Cc: Stefano Stabellini <[email protected]>; Wei Liu
> <[email protected]>; Konrad Rzeszutek Wilk <[email protected]>;
> George Dunlap <[email protected]>; Andrew Cooper
> <[email protected]>; Ian Jackson <[email protected]>; Tim
> (Xen.org) <[email protected]>; Julien Grall <[email protected]>; xen-devel
> <[email protected]>
> Subject: Re: [Xen-devel] [PATCH v6 12/14] memory: add get_paged_gfn() as
> a wrapper...
>
> >>> On 23.08.18 at 11:47, <[email protected]> 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
> [email protected]
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
