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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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", 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.

Jan



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