Re: [Xen-devel] [PATCH] x86/hvm: wait for at least one ioreq server to be enabled

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.02.15 at 13:27, <paul.durrant@xxxxxxxxxx> wrote:
> In the case where a stub domain is providing emulation for an HVM
> guest, there is no interlock in the toolstack to make sure that
> the stub domain is up and running before the guest is unpaused.
> 
> Prior to the introduction of ioreq servers this was not a problem,
> since there was only ever one emulator so ioreqs were simply
> created anyway and the vcpu remained blocked until the stub domain
> started and picked up the ioreq.
> 
> Since ioreq servers allow for multiple emulators for a single guest
> it's not possible to know a priori which emulator will handle a
> particular ioreq, so emulators must attach to a guest before the
> guest runs.
> 
> This patch works around the lack of interlock in the toolstack for
> stub domains by keeping the domain paused until at least one ioreq
> server is created and enabled, which in practice means the stub
> domain is indeed up and running.

Do I understand correctly that this only deals correctly with the
single server case, and the multi server case becoming functional
depends on the tool stack change to be done?

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -885,6 +885,12 @@ static void hvm_ioreq_server_enable(struct 
> hvm_ioreq_server *s,
>  
>    done:
>      spin_unlock(&s->lock);
> +
> +    if ( d->arch.hvm_domain.ioreq_server.waiting )
> +    {
> +        d->arch.hvm_domain.ioreq_server.waiting = 0;
> +        domain_unpause(d);
> +    }
>  }

So it takes looking up all callers to understand that this doesn't need
to be atomic. This would have been avoided by mentioning this in
the description or in a comment.

> @@ -1435,6 +1441,19 @@ int hvm_domain_initialise(struct domain *d)
>  
>      spin_lock_init(&d->arch.hvm_domain.ioreq_server.lock);
>      INIT_LIST_HEAD(&d->arch.hvm_domain.ioreq_server.list);
> +    
> +    /*
> +     * In the case where a stub domain is providing emulation for
> +     * the guest, there is no interlock in the toolstack to prevent
> +     * the guest from running before the stub domain is ready.
> +     * Hence the domain must remain paused until at least one ioreq
> +     * server is created and enabled.
> +     */
> +    if ( !is_pvh_domain(d) ) {

Coding style.

> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -84,6 +84,7 @@ struct hvm_domain {
>          spinlock_t       lock;
>          ioservid_t       id;
>          struct list_head list;
> +        bool_t           waiting;

At least non normal non-debug builds you've got a slot of 32 bits
between id and list - please fill such gaps when adding new
members rather than adding further slack space.

Jan


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