Re: [Xen-devel] [PATCH v8 5/7] memory: add check_get_page_from_gfn() as a wrapper...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 13 September 2018 14:23
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH v8 5/7] memory: add check_get_page_from_gfn() as a
> wrapper...
> 
> >>> On 13.09.18 at 12:31, <paul.durrant@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -353,33 +353,20 @@ 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) )
> > +    switch ( check_get_page_from_gfn(current->domain, _gfn(gmfn),
> false,
> > +                                     NULL, page) )
> >      {
> > -        put_page(*page);
> > -        p2m_mem_paging_populate(curr_d, gmfn);
> > -        return X86EMUL_RETRY;
> > -    }
> > +    case 0:
> > +        break;
> >
> > -    if ( p2m_is_shared(p2mt) )
> > -    {
> > -        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) )
> 
> This gets replaced by
> 
>     if ( !p2m_is_ram(p2mt) || (!readonly && p2m_is_readonly(p2mt)) )
>
> in the function, which I'm afraid does not fulfill the claim of
> "the desired semantic is the same",

Yes, but the test I replace does not match the comment anyway so I still think 
the new semantic is what was desired.

> since there are (or at the
> very least could be, yet you also don't want to introduce
> latent bugs) types which fall into neither category. Would it
> perhaps make sense to defer p2mt checks beyond
> paging/shared to the caller when passing a respective
> non-NULL pointer into the function? This is the more that, as
> is looks, this "only almost the same" applies to all of the
> instances you replace, which makes me even wonder whether
> the p2mt pointer might need to be mandatory to be passed in.
> 

Ok. I'll p2mt_p mandatory and defer checks beyond p2m_is_shared() to the caller.

  Paul

> Jan
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel