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

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

[Paul Durrant]

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>
---
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)
+    {
+    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;
+                uint32_t data = ~0;
+
+                for ( i = 0; i < p->count; i++ )
+                    hvm_copy_to_guest_phys(p->data + step * i, &data,
+                                           p->size);
+            }
+        }
+        /* FALLTHRU */
+    default:
+        p->state = STATE_IORESP_READY;
+        hvm_io_assist(p);
+        break;
+    }
+
+    return 1;
+}
+
 bool_t hvm_send_assist_req(ioreq_t *p)
 {
     struct hvm_ioreq_server *s = hvm_select_ioreq_server(current->domain, p);
 
     if ( !s )
-        return 0;
+        return hvm_complete_assist_req(p);
 
     return hvm_send_assist_req_to_ioreq_server(s, p);
 }
-- 
1.7.10.4

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

[Andrew Cooper]

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 )

> +    {
> +    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.

~Andrew

> +                uint32_t data = ~0;
> +
> +                for ( i = 0; i < p->count; i++ )
> +                    hvm_copy_to_guest_phys(p->data + step * i, &data,
> +                                           p->size);
> +            }
> +        }
> +        /* FALLTHRU */
> +    default:
> +        p->state = STATE_IORESP_READY;
> +        hvm_io_assist(p);
> +        break;
> +    }
> +
> +    return 1;
> +}
> +
>  bool_t hvm_send_assist_req(ioreq_t *p)
>  {
>      struct hvm_ioreq_server *s = hvm_select_ioreq_server(current->domain, p);
>  
>      if ( !s )
> -        return 0;
> +        return hvm_complete_assist_req(p);
>  
>      return hvm_send_assist_req_to_ioreq_server(s, p);
>  }

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

[Andrew Cooper]

On 30/09/14 10:29, Andrew Cooper wrote:
> 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 )

Hmm - I am not sure where the space following -> appeared from, but I
meant "( p->type )".  Apologies.

~Andrew

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

[Paul Durrant]

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

> ~Andrew
> 
> > +                uint32_t data = ~0;
> > +
> > +                for ( i = 0; i < p->count; i++ )
> > +                    hvm_copy_to_guest_phys(p->data + step * i, &data,
> > +                                           p->size);
> > +            }
> > +        }
> > +        /* FALLTHRU */
> > +    default:
> > +        p->state = STATE_IORESP_READY;
> > +        hvm_io_assist(p);
> > +        break;
> > +    }
> > +
> > +    return 1;
> > +}
> > +
> >  bool_t hvm_send_assist_req(ioreq_t *p)
> >  {
> >      struct hvm_ioreq_server *s = hvm_select_ioreq_server(current-
> >domain, p);
> >
> >      if ( !s )
> > -        return 0;
> > +        return hvm_complete_assist_req(p);
> >
> >      return hvm_send_assist_req_to_ioreq_server(s, p);
> >  }
> 

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

[Andrew Cooper]

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

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

[Jan Beulich]

>>> On 30.09.14 at 11:43, <Paul.Durrant@citrix.com> 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.

And I can fix that while committing...

Jan

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 30 September 2014 10:49
> To: Andrew Cooper; Paul Durrant
> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: RE: [Xen-devel] [PATCH v3 for 4.5] ioreq-server: handle the lack of a
> default emulator properly
> 
> >>> On 30.09.14 at 11:43, <Paul.Durrant@citrix.com> 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.
> 
> And I can fix that while committing...
> 

Ok. Thanks,

  Paul

> Jan

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 30 September 2014 10:49
> To: Andrew Cooper; Paul Durrant
> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: RE: [Xen-devel] [PATCH v3 for 4.5] ioreq-server: handle the lack of a
> default emulator properly
> 
> >>> On 30.09.14 at 11:43, <Paul.Durrant@citrix.com> 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.
> 
> And I can fix that while committing...
> 

BTW, can I take that as an ack?

  Paul

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

[Jan Beulich]

>>> On 02.10.14 at 11:54, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 30 September 2014 10:49
>> To: Andrew Cooper; Paul Durrant
>> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
>> Subject: RE: [Xen-devel] [PATCH v3 for 4.5] ioreq-server: handle the lack of a
>> default emulator properly
>> 
>> >>> On 30.09.14 at 11:43, <Paul.Durrant@citrix.com> 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.
>> 
>> And I can fix that while committing...
>> 
> 
> BTW, can I take that as an ack?

Sure, but is that still relevant considering that the patch got committed
already?

Jan

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 02 October 2014 11:28
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: RE: [Xen-devel] [PATCH v3 for 4.5] ioreq-server: handle the lack of a
> default emulator properly
> 
> >>> On 02.10.14 at 11:54, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 30 September 2014 10:49
> >> To: Andrew Cooper; Paul Durrant
> >> Cc: xen-devel@lists.xen.org; Keir (Xen.org)
> >> Subject: RE: [Xen-devel] [PATCH v3 for 4.5] ioreq-server: handle the lack
> of a
> >> default emulator properly
> >>
> >> >>> On 30.09.14 at 11:43, <Paul.Durrant@citrix.com> 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.
> >>
> >> And I can fix that while committing...
> >>
> >
> > BTW, can I take that as an ack?
> 
> Sure, but is that still relevant considering that the patch got committed
> already?
> 

Ok, that's fine then :-) I just pulled staging and didn't see it. I'll check again.

  Paul

> Jan

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

[Jan Beulich]

>>> On 30.09.14 at 11:29, <andrew.cooper3@citrix.com> wrote:
> 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 )
> 
>> +    {
>> +    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.

No (or else similar changes would be needed elsewhere) - the field
being uint32_t doesn't imply the full value range to be used. This is
an ioreq_t, which we fill ourselves. Remember the code I pointed
you to yesterday? The correctness of the above follows from
similar implications afaict.

Jan

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

[Andrew Cooper]

On 30/09/14 10:48, Jan Beulich wrote:
>>>> On 30.09.14 at 11:29, <andrew.cooper3@citrix.com> wrote:
>> 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 )
>>
>>> +    {
>>> +    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.
> No (or else similar changes would be needed elsewhere) - the field
> being uint32_t doesn't imply the full value range to be used. This is
> an ioreq_t, which we fill ourselves. Remember the code I pointed
> you to yesterday? The correctness of the above follows from
> similar implications afaict.
>
> Jan
>

It is a matter of defensive coding.  Just because we do not expect
p->size * p->count to be greater than a page doesn't mean that some bug
wont cause it to happen.

At this point, the different between a signed and unsigned i is a
bounded or unbounded loop.

~Andrew

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

[Jan Beulich]

>>> On 30.09.14 at 11:52, <andrew.cooper3@citrix.com> wrote:
> On 30/09/14 10:48, Jan Beulich wrote:
>>>>> On 30.09.14 at 11:29, <andrew.cooper3@citrix.com> wrote:
>>> 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 )
>>>
>>>> +    {
>>>> +    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.
>> No (or else similar changes would be needed elsewhere) - the field
>> being uint32_t doesn't imply the full value range to be used. This is
>> an ioreq_t, which we fill ourselves. Remember the code I pointed
>> you to yesterday? The correctness of the above follows from
>> similar implications afaict.
> 
> It is a matter of defensive coding.  Just because we do not expect
> p->size * p->count to be greater than a page doesn't mean that some bug
> wont cause it to happen.
> 
> At this point, the different between a signed and unsigned i is a
> bounded or unbounded loop.

Again - if you strongly feel about this, submit a patch to fix it
everywhere. When I fixed the backward string ops here, I did
consider what you refer to above but in the end didn't think it
was worth forcing the compiled code to grow (due to added REX
prefixes) for no real reason.

Jan