Re: [Xen-devel] [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@xxxxxxxxxxxxxxx>]

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?

[snip]

Yu

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