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/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?

[snip]

Yu

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