Re: [RFC v2] misc/xenmicrocode: Upload /lib/firmware/<some blob> to the hypervisor

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 01/30/2015 03:05 PM, Luis R. Rodriguez wrote:
> On Fri, Jan 30, 2015 at 09:44:56AM -0500, Boris Ostrovsky wrote:
>> On 01/29/2015 08:14 PM, Luis R. Rodriguez wrote:
>>> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>>> +/*
>>> + * Do not pause already paused domains, and allow us to
>>> + * unpause only quiesced domains.
>>> + */
>>> +static int quiesce_all_domains(xc_interface *xch,
>>> +		               struct xc_quiesce_request *quiesce_request)
>>> +{
>>> +    xc_domaininfo_t info[1024];
>>> +    int i, nb_domain;
>>> +
>>> +    nb_domain = xc_domain_getinfolist(xch, 0, 1024, info);
>>> +    if ( nb_domain < 0 )
>>> +    {
>>> +	return -1;
>>> +    }
>>> +
>>> +    for ( i = 0; i < nb_domain; i++ )
>>> +    {
>>> +        if ( info[i].domain == 0 )
>>> +		continue;
>>> +	if ( info[i].flags & XEN_DOMINF_paused )
>>> +		continue;
>>> +
>>> +        xc_domain_pause(xch, info[i].domain);
>>
>> You are not handling errors returned by xc_domain_pause().
> Thanks for the review, shall we just bail if that cannot happen?

I think so (after unpausing the already-paused domains, obviously).

But then I thought that Andrew is advocating having the hypervisor doing 
the pause.

>
>> So then you will
>> try to unpause a domain that may not have been paused and (I think more
>> importantly) may proceed with microcode update while not all domains are
>> paused.
> Yeah this would be bad. Perhaps bail and tell the user the domain that
> we could not pause / quiesce (depending on what we decide to do).
>
>>> +
>>> +	quiesce_request->domids[quiesce_request->num_quiesced] = info[i].domain;
>>> +	quiesce_request->num_quiesced++;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void unquiesce_all_domains(xc_interface *xch,
>>> +                                  struct xc_quiesce_request *quiesce_request)
>>> +{
>>> +    int i;
>>> +
>>> +    for ( i = 0; i < quiesce_request->num_quiesced; i++ )
>>> +    {
>>> +	xc_domain_unpause(xch, quiesce_request->domids[i]);
>>
>> Same here --- you may fail unpausing a domain and noone will know about it.
> True, that seems like a rather informational thing we can spit out, do we want
> to return an error for that though too?

I'd print the error and continue unpausing.


-boris