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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.18 at 11:10, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
>> Of Jan Beulich
>> Sent: 11 September 2018 15:56
>> 
>> >>> 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()?

Fine with me; George?

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

That's what I was implying by saying "fall through" - otherwise
"case -EINVAL:" could as well have gone away.

Jan



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