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

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 8/10/2016 6:43 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 10 August 2016 11:33
>> To: Paul Durrant; Yu Zhang
>> Cc: Andrew Cooper; George Dunlap; Jun Nakajima; Kevin Tian;
>> zhiyuan.lv@intel.com; xen-devel@lists.xen.org; Tim (Xen.org)
>> Subject: Re: [PATCH v5 3/4] x86/ioreq server: Add HVMOP to map guest ram
>> with p2m_ioreq_server to an ioreq server.
>>
>>>>> On 10.08.16 at 10:09, <yu.c.zhang@linux.intel.com> wrote:
>>> On 8/8/2016 11:40 PM, Jan Beulich wrote:
>>>>>>> On 12.07.16 at 11:02, <yu.c.zhang@linux.intel.com> 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.)
>>
> Well, I thought the correct semantics for p2m_ioreq_server without any interception are the same as p2m_ram_rw. In which case if we find an ioreq server, but it does not want to emulate writes, then we should complete the I/O by writing to guest RAM. But, how are we ever going to hit this code-path without some form of race with EPT configuration?
>

Thanks Paul. I think the p2m type change race condition can be avoided in
hvm_hap_nested_page_fault(), by delaying the call of put_gfn() after the
ioreq is delivered.  :)

Otherwise we will need to keep the previous mem_handler, and use 
get_gfn_type()
instead of get_gfn_query_unlocked() in hvmemul_do_io() - because, 
without a lock,
another race condition can happen when
1> a p2m_ioreq_server gfn is changed to p2m_ram_rw;
2> we got this p2mt with value p2m_ram_rw by get_gfn_query_unlocked();
3> p2m type of this gfn is changed back to p2m_ioreq_server;
4> mem_handler is called instead of deliver the ioreq to device model;

However, the mem_handler may really be helpful for the read-modify-write 
case
I mentioned in another mail. So hypervisor do not need to forward the 
read ioreq
to the device model.

B.R.
Yu

Yu



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