Re: [PATCH v4] x86: properly handle MSI-X unmask operation from guests

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 23/11/13 01:40, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Saturday, November 23, 2013 12:19 AM
>> To: Wu, Feng
>> Cc: Nakajima, Jun; Auld, Will; Zhang, Xiantao; xen-devel
>> Subject: Re: [PATCH v4] x86: properly handle MSI-X unmask operation from
>> guests
>>
>>>>> On 22.11.13 at 02:08, "Wu, Feng" <feng.wu@intel.com> wrote:
>>> v1: Initial patch to handle this issue involving changing the hypercall interface
>>> v2:Totally handled inside hypervisor.
>>> v3:Change some logics of handling msi-x pending unmask operations.
>>> v4:Some changes related to coding style according to Andrew Cooper's
>> comments
>>
>> So this is _much_ less intrusive than what you did before - good!
>>
>>> --- a/xen/arch/x86/hvm/io.c
>>> +++ b/xen/arch/x86/hvm/io.c
>>> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
>>>          break;
>>>      }
>>>
>>> +    if ( msix_post_handler(curr) )
>>> +        gdprintk(XENLOG_ERR, "msix_post_handler error\n");
>>> +
>>>      if ( p->state == STATE_IOREQ_NONE )
>>>          vcpu_end_shutdown_deferral(curr);
>> I think the addition should be moved into the body of this if(),
>> so that it gets executed only upon completion of I/O, not when
>> it e.g. need retrying.
>>
>> Also, XENLOG_ERR seems to heavy a message. XENLOG_WARN
>> would be the highest I'd accept.
>>
>>> +int msix_post_handler(struct vcpu *v)
>>> +{
>>> +    int rc;
>>> +
>>> +    if ( v->arch.pending_msix_unmask.valid == 0 )
>> Iff you keep this (see below), then boolean checks are
>> conventionally done with ! rather than == 0.
>>
>>> +        return 0;
>>> +
>>> +    v->arch.pending_msix_unmask.valid = 0;
>>> +
>>> +    rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0);
>>> +    return rc != X86EMUL_OKAY ? -1 : 0;
>> Make the function return bool_t, and then simply
>>
>>    return msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0) ==
>> X86EMUL_OKAY;
>>
>>> +struct pending_msix_unmask_info
>>> +{
>>> +    unsigned long ctrl_address;
>>> +    bool_t valid;
>>> +};
>>> +
>>>  struct arch_vcpu
>>>  {
>>>      /*
>>> @@ -439,6 +445,8 @@ struct arch_vcpu
>>>
>>>      /* A secondary copy of the vcpu time info. */
>>>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>>> +
>>> +    struct pending_msix_unmask_info pending_msix_unmask;
>> I don't think you need a separate boolean here - for one I don't
>> think the address can reasonably be zero, and even if you have
>> the bottom two bits available (as it being 4-byte aligned gets
>> checked before you consume it).
>
> The boolean variant "valid", which is set in msixtbl_write(), means whether there is a
> msix pending unmask, if there is, just clean this flag and unmask the msix in hardware,
> otherwise we just do nothing. If removing "valid", how can I know whether there is a
> msix pending unmask operation ? Thanks you!

Address can't reasonably be 0, meaning that an address of 0 indicates
that an unmask is not pending.

~Andrew