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:33 PM, Jan Beulich wrote:
>>>> 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.)
>

Thanks for your reply, Jan.
For " && p2mt != p2m_ioreq_server" condition, it is just to guarantee 
that if a write
operation is trapped, and at the same period, device model changed the 
status of
ioreq server, it should be discarded.

A second thought is, I am now more worried about the " && dir == 
IOREQ_WRITE"
condition, which we used previously to set s to NULL if it is not a 
write operation.
However, if HVM uses a read-modify-write instruction to operate on a 
write-protected
address, it will be treated as both read and write accesses in 
ept_handle_violation(). In
such situation, we need to emulate the read access first(by just 
returning the value being
fetched either in hypervisor or in device model), instead of discarding 
the read access.

Thanks
Yu


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