Re: [Xen-devel] [PATCH v6 12/14] memory: add get_paged_gfn() as a wrapper...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

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