Re: [Xen-devel] [PATCH v5 3/4] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.08.16 at 10:09, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:

> 
> On 8/8/2016 11:40 PM, Jan Beulich wrote:
>>>>> On 12.07.16 at 11:02, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>> @@ -178,8 +179,34 @@ static int hvmemul_do_io(
>>>           break;
>>>       case X86EMUL_UNHANDLEABLE:
>>>       {
>>> -        struct hvm_ioreq_server *s =
>>> -            hvm_select_ioreq_server(curr->domain, &p);
>>> +        struct hvm_ioreq_server *s;
>>> +
>>> +        if ( is_mmio )
>>> +        {
>>> +            unsigned long gmfn = paddr_to_pfn(addr);
>>> +            p2m_type_t p2mt;
>>> +
>>> +            (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
>>> +
>>> +            if ( p2mt == p2m_ioreq_server )
>>> +            {
>>> +                unsigned int flags;
>>> +
>>> +                if ( dir != IOREQ_WRITE )
>>> +                    s = NULL;
>>> +                else
>>> +                {
>>> +                    s = p2m_get_ioreq_server(currd, &flags);
>>> +
>>> +                    if ( !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS) )
>>> +                        s = NULL;
>>> +                }
>>> +            }
>>> +            else
>>> +                s = hvm_select_ioreq_server(currd, &p);
>>> +        }
>>> +        else
>>> +            s = hvm_select_ioreq_server(currd, &p);
>> Wouldn't it both be more natural and make the logic even easier
>> to follow if s got set to NULL up front, all the "else"-s dropped,
>> and a simple
>>
>>          if ( !s )
>>              s = hvm_select_ioreq_server(currd, &p);
>>
>> be done in the end?
>>
> 
> Sorry, Jan. I tried to simplify above code, but found the new code is 
> still not very
> clean,  because in some cases the s is supposed to return NULL instead 
> of to be
> set from the hvm_select_ioreq_server().
> To keep the same logic, the simplified code looks like this:
> 
>       case X86EMUL_UNHANDLEABLE:
>       {
> -        struct hvm_ioreq_server *s =
> -            hvm_select_ioreq_server(curr->domain, &p);
> +        struct hvm_ioreq_server *s = NULL;
> +        p2m_type_t p2mt = p2m_invalid;
> +
> +        if ( is_mmio && dir == IOREQ_WRITE )
> +        {
> +            unsigned long gmfn = paddr_to_pfn(addr);
> +
> +            (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
> +
> +            if ( p2mt == p2m_ioreq_server )
> +            {
> +                unsigned int flags;
> +
> +                s = p2m_get_ioreq_server(currd, &flags);
> +                if ( !(flags & XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE) )
> +                    s = NULL;
> +            }
> +        }
> +
> +        if ( !s && p2mt != p2m_ioreq_server )
> +            s = hvm_select_ioreq_server(currd, &p);
> 
>           /* If there is no suitable backing DM, just ignore accesses */
>           if ( !s )
> 
> As you can see, definition of p2mt is moved outside the if ( is_mmio ) 
> judgement,
> and is checked against p2m_ioreq_server before we search the ioreq 
> server's rangeset
> in hvm_select_ioreq_server(). So I am not quite satisfied with this 
> simplification.
> Any suggestions?

I think it's better than the code was before, but an implicit part of
my suggestion was that I'm not really convinced the
" && p2mt != p2m_ioreq_server" part of your new conditional is
really needed: Would it indeed be wrong to hand such a request
to the "normal" ioreq server, instead of terminating it right away?
(I guess that's a question to you as much as to Paul.)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel