Re: [PATCH v3 for 4.5] ioreq-server: handle the lack of a default emulator properly

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

On 30/09/14 10:43, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 30 September 2014 10:29
>> To: Paul Durrant; xen-devel@lists.xen.org
>> Cc: Keir (Xen.org); Jan Beulich
>> Subject: Re: [Xen-devel] [PATCH v3 for 4.5] ioreq-server: handle the lack of a
>> default emulator properly
>>
>> On 30/09/14 10:18, Paul Durrant wrote:
>>> I started porting QEMU over to use the new ioreq server API and hit a
>>> problem with PCI bus enumeration. Because, with my patches, QEMU only
>>> registers to handle config space accesses for the PCI device it implements
>>> all other attempts by the guest to access 0xcfc go nowhere and this was
>>> causing the vcpu to wedge up because nothing was completing the I/O.
>>>
>>> This patch introduces an I/O completion handler into the hypervisor for the
>>> case where no ioreq server matches a particular request. Read requests
>> are
>>> completed with 0xf's in the data buffer, writes and all other I/O req types
>>> are ignored.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>> Cc: Keir Fraser <keir@xen.org>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>> One bug, couple of nits.
>>
>> It is probably worth having a sentence in the commit message concerning
>> the removal of list_is_singular().
>>
>>> ---
>>> v3: - Fix for backwards string instruction emulation
>>>
>>> v2: - First non-RFC submission
>>>     - Removed warning on unemulated MMIO accesses
>>>
>>>  xen/arch/x86/hvm/hvm.c |   35 ++++++++++++++++++++++++++++++++-
>> --
>>>  1 file changed, 32 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 5c7e0a4..e6611ed 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -2386,8 +2386,7 @@ static struct hvm_ioreq_server
>> *hvm_select_ioreq_server(struct domain *d,
>>>      if ( list_empty(&d->arch.hvm_domain.ioreq_server.list) )
>>>          return NULL;
>>>
>>> -    if ( list_is_singular(&d->arch.hvm_domain.ioreq_server.list) ||
>>> -         (p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO) )
>>> +    if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
>>>          return d->arch.hvm_domain.default_ioreq_server;
>>>
>>>      cf8 = d->arch.hvm_domain.pci_cf8;
>>> @@ -2618,12 +2617,42 @@ bool_t
>> hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>>>      return 0;
>>>  }
>>>
>>> +static bool_t hvm_complete_assist_req(ioreq_t *p)
>>> +{
>>> +    switch (p->type)
>> Style: ( p-> type )
>>
> <Sigh> Yes, missed that one.
>
>>> +    {
>>> +    case IOREQ_TYPE_COPY:
>>> +    case IOREQ_TYPE_PIO:
>>> +        if ( p->dir == IOREQ_READ )
>>> +        {
>>> +            if ( !p->data_is_ptr )
>>> +                p->data = ~0ul;
>>> +            else
>>> +            {
>>> +                int i, step = p->df ? -p->size : p->size;
>> 'i' must be unsigned or larger, given p->count being uint32_t.
>>
> Theoretically true I guess, but I can't see an I/O ever having that many reps!
>
>   Paul

I would certainly hope not as well, but in the case that some bug
somewhere else manages to set the top bit of p->count, a bounded loop is
better than an unbounded loop.

~Andrew