Re: [PATCH 2/2] PV framebuffer

[Markus Armbruster <armbru@redhat.com>]

Steven Smith <sos22@cam.ac.uk> writes:

[...]
>> >> +	if (xenfb_hotplug(xsh, vfb_backdir) < 0)
>> >> +		goto error;
>> >> +	if (xenfb_hotplug(xsh, vkbd_backdir) < 0)
>> >> +		goto error;
>> >> +
>> >> +	if (xenfb_xs_printf(xsh, vkbd_backdir, "feature-abs-pointer", "1"))
>> >> +		goto error;
>> >> +	if (xenfb_xs_printf(xsh, vfb_backdir, "state", "%d",
>> >> +			    XenbusStateInitWait))
>> >> +		goto error;
>> >> +	if (xenfb_xs_printf(xsh, vkbd_backdir, "state", "%d",
>> >> +			    XenbusStateInitWait))
>> >> +		goto error;
>> >> +
>> > I'd probably reorder this a little to look more like this:
>> >
>> > (1) Set feature-abs-pointer
>> > (2) Set state to InitWait
>> > (3) Set hotplug status
>> >
>> > The only actual *required* constraint is (1) before (2), so that the
>> > frontend doesn't initialise before we've set the feature and
>> > potentially miss it.
>> >
>> > (2) before (3) is kind of nice, in that it makes sure that the backend
>> > is ready before xend unpauses the domain, and so the frontend'll be
>> > able to connect the first time it tries, but that's a lot less
>> > important here than for e.g. block devices.
>> Based on our previous discussions, I designed the startup protocol
>> this way:
>> 
>>     backend                          frontend
>>     ------------------------------------------------------------------
>>     hotplug_status = connected
>>     [makes xend populate xenstore, set fe and be state = Initialising]
>>     wait for be state = Initialising
>>       [i.e. wait for xend]
>>     write xs: feature-abs-pointer    write xs: feature-update
>>     be state = InitWait              fe state = Initialised
>>     ------------------------------ sync ------------------------------
>>     wait for fe state = Initialised  wait for be state = InitWait
>>     ------------------------------ sync ------------------------------
>>     read xs: feature-update          read xs: feature-abs-pointer
>>     write xs: request-update         write xs: request-abs-pointer
>>     be state = Connected             fe state = Connected
>>     ------------------------------ sync ------------------------------
>>     wait for fe state = Connected    wait for be state = Connected
>>     ------------------------------ sync ------------------------------
>>     read xs: request-abs-pointer     read xs: request-update
>> 
>> The symmetry made sense to me.
> Ah, sorry, I wasn't clear enough.  You've got everything right after
> the first sync line, but the bit before that isn't quite right.  Xend
> creates xenstore area when the domain is created, and then waits until
> hotplug-status is set before starting the domain running.  The idea is
> that the backend driver should be watching its area of xenbus (so
> /local/domain/0/backend/vif, say), so that it notices when the area is
> created and creates the appropriate backend devices.  Creating the
> backend devices triggers the hotplug scripts via some udev magic which
> I've never quite understood, and they then e.g. connect vifs up to the
> bridge.  Once they've finished, the backends are all ready, and so
> it's safe to start the guest.  If you start the guest before the
> backends are ready, you potentially have issues like your root
> fileystem not becoming available until after the guest has booted,
> which tends to make Linux unhappy.
>
> xend				backend driver		hotplug scripts
> Creates a new domain
> 	paused
> Creates backend area
> 				Notices new backend
> 					area, creates
> 					backend device
> 				Does some basic setup
> 					on backend device
> 				Go to state InitWait
> 				Kicks udev
> 							Does a bit more setup
> 								on backend
> 								device
> 							Sets hotplug-status
> Notices hotplug status,
> 	unpauses domain
>
> Now, the obvious mapping of this protocol onto the PVFB would have
> xend create the xenstore area when the guest is created and spawn the
> backend itself.  The backend could then set hotplug-status to indicate
> that it's ready, which would unpause the guest and things would then
> proceed in the usual way.

I coded this, and it works.

> This would work, and I'd be quite happy with it, but it does have the
> slight disadvantage that you can't start a domain with a framebuffer
> configured and no backend attached.  If this worries you, you could
> have xend not wait for the hotplug status but instead start the domain
> immediately (there's a waitForBackend method on class DevController
> which you'd have to override.  I don't remember the details, but I
> don't think it's very hard).  If you go this path, you need to
> consider the possibility that the backend area isn't set up in time
> when the _probe function is run in the frontend, but I think that
> works already.

I like this, but I'm saving it for another day.

Thanks again for your help!

[...]