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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 10 August 2016 11:33
> To: Paul Durrant; Yu Zhang
> Cc: Andrew Cooper; George Dunlap; Jun Nakajima; Kevin Tian;
> zhiyuan.lv@xxxxxxxxx; xen-devel@xxxxxxxxxxxxx; 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@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.)
>

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?

  Paul
 
> Jan


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