Re: [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM

["Jan Beulich" <JBeulich@suse.com>]

>>> On 02.02.15 at 16:22, <dslutz@verizon.com> wrote:
>   Jan Beulich:
>     The change on what to do when hvm_send_assist_req() fails is bad.
>       That is correct.  Made hvm_has_dm() do full checking so
>       that the extra VMEXIT and VMENTRY can be skipped.
>     hvm_complete_assist_req() be a separate function yet having only
>     a single caller ...
>       Folded hvm_complete_assist_req() in to hvm_has_dm() which
>       is the only place it is called.

This makes pretty little sense to me - previously the only caller was
hvm_send_assist_req(), folding into which would make sense. But
moving this code into hvm_has_dm() doesn't look right both from
an abstract perspective (completely contrary to the function's name)
and from what operations get carried out when:

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -218,8 +218,11 @@ static int hvmemul_do_io(
>              vio->io_state = HVMIO_handle_mmio_awaiting_completion;
>          break;
>      case X86EMUL_UNHANDLEABLE:
> +    {
> +        struct hvm_ioreq_server *s = hvm_has_dm(curr->domain, &p);
> +
>          /* If there is no backing DM, just ignore accesses */
> -        if ( !hvm_has_dm(curr->domain) )
> +        if ( !s )
>          {
>              rc = X86EMUL_OKAY;
>              vio->io_state = HVMIO_none;

You alter the flow here, but leave the (now stale) comment
untouched - !hvm_has_dm() no longer has the original meaning.

> @@ -227,11 +230,12 @@ static int hvmemul_do_io(
>          else
>          {
>              rc = X86EMUL_RETRY;
> -            if ( !hvm_send_assist_req(&p) )
> +            if ( !hvm_send_assist_req_to_ioreq_server(s, &p) )
>                  vio->io_state = HVMIO_none;
>              else if ( p_data == NULL )
>                  rc = X86EMUL_OKAY;
>          }

In particular, the returning of X86EMUL_RETRY vs X86EMUL_OKAY
now change for the case where !s but also
!list_empty(&d->arch.hvm_domain.ioreq_server.list). While this
_may_ be intended, the description of the patch still doesn't make
clear why this is so (again keeping in mind why the behavior prior to
this patch was done in this particular way).

So in the end, considering that Paul approved of what you do, maybe
all is well and it's just one step too many folded together for me to
grok. In which case - please take the time to describe your changes
suitably; this isn't the first time I have to ask you to do so.

> +    }
>          break;

For indentation to be meaningful, the closing brace should follow
the break statement.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -228,7 +228,9 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v);
>  void hvm_vcpu_cacheattr_destroy(struct vcpu *v);
>  void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip);
>  
> -bool_t hvm_send_assist_req(ioreq_t *p);
> +struct hvm_ioreq_server;

I think you could avoid this by simply moving up here the
hvm_has_dm() declaration.

> +bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
> +                                           ioreq_t *p);

If, with the comments above in mind, you still intend to remove
hvm_send_assist_req(), I'd much favor you renaming
hvm_send_assist_req_to_ioreq_server() to hvm_send_assist_req().

Jan