Re: [Xen-devel] [PATCH 1/2] x86/hvm: Fix HVMOP_get_param when skipping creating the default ioreq server

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: 12 December 2016 18:30
> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich
> <JBeulich@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Boris
> Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Subject: [PATCH 1/2] x86/hvm: Fix HVMOP_get_param when skipping
> creating the default ioreq server
> 
> c/s e7dabe5 "x86/hvm: don't unconditionally create a default ioreq server"
> added a break statement, but the logic previously depended on falling
> through
> into the default case to fill in the value the caller asked for.
>

Yes, I mistakenly thought I was breaking out of an inner switch. Thanks for 
fixing this up.

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
 
> This causes the sending migration code to put a junk PARAM into the stream,
> and the receiving side to fail to zero the IOREQ pages, causing QEMU to
> object
> when it finds stale requests while starting up.
> 
> Reorder the code so it more clearly falls through into the default case.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 2b3977a..61f5029 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5275,9 +5275,6 @@ static int hvmop_get_param(
>      case HVM_PARAM_IOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_EVTCHN:
> -    {
> -        domid_t domid;
> -
>          /*
>           * It may be necessary to create a default ioreq server here,
>           * because legacy versions of QEMU are not aware of the new API for
> @@ -5285,15 +5282,16 @@ static int hvmop_get_param(
>           * under construction then it will not be QEMU querying the
>           * parameters and thus the query should not have that side-effect.
>           */
> -        if ( d->creation_finished )
> -            break;
> +        if ( !d->creation_finished )
> +        {
> +            domid_t domid = d-
> >arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> +
> +            rc = hvm_create_ioreq_server(d, domid, 1,
> +                                         HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
> +            if ( rc != 0 && rc != -EEXIST )
> +                goto out;
> +        }
> 
> -        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> -        rc = hvm_create_ioreq_server(d, domid, 1,
> -                                     HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
> -        if ( rc != 0 && rc != -EEXIST )
> -            goto out;
> -    }
>      /*FALLTHRU*/
>      default:
>          a.value = d->arch.hvm_domain.params[a.index];
> --
> 2.1.4


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