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

["Luis R. Rodriguez" <mcgrof@xxxxxxxx>]

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@xxxxxxxx>
>> +/*
>> + * 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?

> 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?

>> +    }
>> +}
>> +
>> +int xc_microcode_update(xc_interface *xch, uint8_t *fbuf, size_t len)
>> +{
>> +    int ret = 0;
>> +    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc);
>> +    DECLARE_HYPERCALL;
>> +    struct xen_platform_op op_s;
>> +    struct xen_platform_op *op = &op_s;
>> +    DECLARE_HYPERCALL_BOUNCE(op, sizeof(*op), 
>> XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
>> +    struct xc_quiesce_request quiesce_request;
>> +
>> +    memset(&quiesce_request, 0, sizeof(struct xc_quiesce_request));
>> +
>> +    if ( !xch )
>> +    {
>> +        return -1;
>> +    }
>
>
> Not sure what tools coding convention is but you may not need {} here (and 
> a few more places)

I was not sure so went with that. I'm happy to remove that stuff from
one liners.

>> +    /* microcode file as present on /lib/firmware/ */
>
> On Linux but not necessarily on other OSs. You code doesn't require it to 
> be there so you probably want to avoid referring to this path in comments 
> and commit subject/body.

Amended.

>> +    printf("%s: %ld\n", filename, buf.st_size);
>
>
> Do you need this? (probably leftover from debugging?)

Killed.

  Luis

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