> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: 06 February 2015 14:45
> To: Paul Durrant
> Cc: Andrew Cooper; Ian Campbell; Wei Liu; [email protected]; Keir
> (Xen.org)
> Subject: Re: [PATCH] x86/hvm: wait for at least one ioreq server to be
> enabled
>
> >>> On 06.02.15 at 13:27, <[email protected]> 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?
>
Yes, and that was deemed too invasive for backport to stable; plus xl/libxl
have not yet been taught about multiple emulators.
> > --- 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.
Ok, I'll stick a comment in.
>
> > @@ -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.
>
Ok.
> > --- 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.
True, I'll re-order.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
