Re: [Xen-devel] [PATCH v2 REPOST 09/12] x86/hvm/ioreq: simplify code and use consistent naming

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 24 August 2017 18:03
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v2 REPOST 09/12] x86/hvm/ioreq: simplify
> code and use consistent naming
> 
> On Tue, Aug 22, 2017 at 03:51:03PM +0100, Paul Durrant wrote:
> > This patch re-works much of the IOREQ server initialization and teardown
> > code:
> >
> > - The hvm_map/unmap_ioreq_gfn() functions are expanded to call
> through
> >   to hvm_alloc/free_ioreq_gfn() rather than expecting them to be called
> >   separately by outer functions.
> > - Several functions now test the validity of the hvm_ioreq_page gfn value
> >   to determine whether they need to act. This means can be safely called
> >   for the bufioreq page even when it is not used.
> > - hvm_add/remove_ioreq_gfn() simply return in the case of the default
> >   IOREQ server so callers no longer need to test before calling.
> > - hvm_ioreq_server_setup_pages() is renamed to
> hvm_ioreq_server_map_pages()
> >   to mirror the existing hvm_ioreq_server_unmap_pages().
> >
> > All of this significantly shortens the code.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/ioreq.c | 181 ++++++++++++++++++----------------------
> -------
> >  1 file changed, 69 insertions(+), 112 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> > index 5737082238..edfb394c59 100644
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -181,63 +181,76 @@ bool handle_hvm_io_completion(struct vcpu *v)
> >      return true;
> >  }
> >
> > -static int hvm_alloc_ioreq_gfn(struct domain *d, unsigned long *gfn)
> > +static unsigned long hvm_alloc_ioreq_gfn(struct hvm_ioreq_server *s)
> 
> gfn_t as the return type instead? I see that you are moving it, so I
> won't insist (I assume there's also some other refactoring involved in
> making this return gfn_t). I see there are also further uses of
> unsigned long to store gfns, I'm not going to point those out.
> 
> >  {
> > +    struct domain *d = s->domain;
> >      unsigned int i;
> > -    int rc;
> >
> > -    rc = -ENOMEM;
> > +    ASSERT(!s->is_default);
> > +
> >      for ( i = 0; i < sizeof(d->arch.hvm_domain.ioreq_gfn.mask) * 8; i++ )
> >      {
> >          if ( test_and_clear_bit(i, &d->arch.hvm_domain.ioreq_gfn.mask) )
> >          {
> 
> The braces are not strictly needed anymore, but that's a question of
> taste.
> 
> > -            *gfn = d->arch.hvm_domain.ioreq_gfn.base + i;
> > -            rc = 0;
> > -            break;
> > +            return d->arch.hvm_domain.ioreq_gfn.base + i;
> >          }
> >      }
> >
> > -    return rc;
> > +    return gfn_x(INVALID_GFN);
> >  }
> >
> > -static void hvm_free_ioreq_gfn(struct domain *d, unsigned long gfn)
> > +static void hvm_free_ioreq_gfn(struct hvm_ioreq_server *s,
> > +                               unsigned long gfn)
> >  {
> > +    struct domain *d = s->domain;
> >      unsigned int i = gfn - d->arch.hvm_domain.ioreq_gfn.base;
> >
> > -    if ( gfn != gfn_x(INVALID_GFN) )
> > -        set_bit(i, &d->arch.hvm_domain.ioreq_gfn.mask);
> > +    ASSERT(!s->is_default);
> 
> I would maybe add a gfn != gfn_x(INVALID_GFN) in the ASSERT.
> 
> > +
> > +    set_bit(i, &d->arch.hvm_domain.ioreq_gfn.mask);
> >  }
> >
> > -static void hvm_unmap_ioreq_page(struct hvm_ioreq_server *s, bool
> buf)
> > +static void hvm_unmap_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
> 
> I'm not sure if you need the buf parameter, it seems in all cases you
> want to unmap everything when calling hvm_unmap_ioreq_gfn? (same
> applies to hvm_remove_ioreq_gfn and hvm_add_ioreq_gfn)

It's really just so map/unmap and add/remove mirror each other.

> 
> >  static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s,
> > -                                      unsigned long ioreq_gfn,
> > -                                      unsigned long bufioreq_gfn)
> > -{
> > -    int rc;
> > -
> > -    rc = hvm_map_ioreq_page(s, false, ioreq_gfn);
> > -    if ( rc )
> > -        return rc;
> > -
> > -    if ( bufioreq_gfn != gfn_x(INVALID_GFN) )
> > -        rc = hvm_map_ioreq_page(s, true, bufioreq_gfn);
> > -
> > -    if ( rc )
> > -        hvm_unmap_ioreq_page(s, false);
> > -
> > -    return rc;
> > -}
> > -
> > -static int hvm_ioreq_server_setup_pages(struct hvm_ioreq_server *s,
> > -                                        bool handle_bufioreq)
> > +                                      bool handle_bufioreq)
> >  {
> > -    struct domain *d = s->domain;
> > -    unsigned long ioreq_gfn = gfn_x(INVALID_GFN);
> > -    unsigned long bufioreq_gfn = gfn_x(INVALID_GFN);
> > -    int rc;
> > -
> > -    if ( s->is_default )
> > -    {
> > -        /*
> > -         * The default ioreq server must handle buffered ioreqs, for
> > -         * backwards compatibility.
> > -         */
> > -        ASSERT(handle_bufioreq);
> > -        return hvm_ioreq_server_map_pages(s,
> > -                   d->arch.hvm_domain.params[HVM_PARAM_IOREQ_PFN],
> > -                   d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN]);
> > -    }
> > +    int rc = -ENOMEM;
> 
> No need to set rc, you are just overwriting it in the next line.
> 

Indeed.

Thanks,

  Paul

> Roger.

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