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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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)

>  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.

Roger.

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