xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly
@ 2014-09-29 10:21 Paul Durrant
  2014-09-29 10:59 ` Andrew Cooper
  2014-09-30  9:03 ` Jan Beulich
  0 siblings, 2 replies; 8+ messages in thread
From: Paul Durrant @ 2014-09-29 10:21 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant, Keir Fraser, Jan Beulich

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>
---
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..822ac37 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, sign = p->df ? -1 : 1;
+                uint32_t data = ~0;
+
+                for ( i = 0; i < p->count; i++ )
+                    hvm_copy_to_guest_phys(p->data + sign * i * p->size, &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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly
  2014-09-29 10:21 [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly Paul Durrant
@ 2014-09-29 10:59 ` Andrew Cooper
  2014-09-29 12:07   ` Paul Durrant
  2014-09-29 12:20   ` Jan Beulich
  2014-09-30  9:03 ` Jan Beulich
  1 sibling, 2 replies; 8+ messages in thread
From: Andrew Cooper @ 2014-09-29 10:59 UTC (permalink / raw)
  To: Paul Durrant, xen-devel; +Cc: Keir Fraser, Jan Beulich

On 29/09/14 11:21, 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>
> ---
> 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..822ac37 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, sign = p->df ? -1 : 1;
> +                uint32_t data = ~0;
> +
> +                for ( i = 0; i < p->count; i++ )
> +                    hvm_copy_to_guest_phys(p->data + sign * i * p->size, &data,
> +                                           p->size);

This is surely bogus for an `ins` which crosses a page boundary?

~Andrew

> +            }
> +        }
> +        /* 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);
>  }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly
  2014-09-29 10:59 ` Andrew Cooper
@ 2014-09-29 12:07   ` Paul Durrant
  2014-09-29 12:20   ` Jan Beulich
  1 sibling, 0 replies; 8+ messages in thread
From: Paul Durrant @ 2014-09-29 12:07 UTC (permalink / raw)
  To: Andrew Cooper, xen-devel@lists.xen.org; +Cc: Keir (Xen.org), Jan Beulich

> -----Original Message-----
> From: Andrew Cooper
> Sent: 29 September 2014 11:59
> To: Paul Durrant; xen-devel@lists.xen.org
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [Xen-devel] [PATCH v2 for 4.5] ioreq-server: handle the lack of a
> default emulator properly
> 
> On 29/09/14 11:21, 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>
> > ---
> > 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..822ac37 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, sign = p->df ? -1 : 1;
> > +                uint32_t data = ~0;
> > +
> > +                for ( i = 0; i < p->count; i++ )
> > +                    hvm_copy_to_guest_phys(p->data + sign * i * p->size, &data,
> > +                                           p->size);
> 
> This is surely bogus for an `ins` which crosses a page boundary?
> 

hvmemul_do_io() is only given a guest physical address and a size, so handling of page boundary conditions must be done at a higher level.

  Paul

> ~Andrew
> 
> > +            }
> > +        }
> > +        /* 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);
> >  }
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly
  2014-09-29 10:59 ` Andrew Cooper
  2014-09-29 12:07   ` Paul Durrant
@ 2014-09-29 12:20   ` Jan Beulich
  2014-09-29 12:51     ` Andrew Cooper
  1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2014-09-29 12:20 UTC (permalink / raw)
  To: Andrew Cooper, Paul Durrant, xen-devel; +Cc: Keir Fraser

>>> On 29.09.14 at 12:59, <andrew.cooper3@citrix.com> wrote:
> On 29/09/14 11:21, 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>
>> ---
>> 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..822ac37 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, sign = p->df ? -1 : 1;
>> +                uint32_t data = ~0;
>> +
>> +                for ( i = 0; i < p->count; i++ )
>> +                    hvm_copy_to_guest_phys(p->data + sign * i * p->size, &data,
>> +                                           p->size);
> 
> This is surely bogus for an `ins` which crosses a page boundary?

Crossing page boundaries gets dealt with up the call stack in
hvmemul_linear_to_phys(), namely the path exiting with
X86EMUL_UNHANDLEABLE when done == 0.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly
  2014-09-29 12:20   ` Jan Beulich
@ 2014-09-29 12:51     ` Andrew Cooper
  2014-09-29 13:03       ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2014-09-29 12:51 UTC (permalink / raw)
  To: Jan Beulich, Paul Durrant, xen-devel; +Cc: Keir Fraser

On 29/09/14 13:20, Jan Beulich wrote:
>>>> On 29.09.14 at 12:59, <andrew.cooper3@citrix.com> wrote:
>> On 29/09/14 11:21, 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>
>>> ---
>>> 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..822ac37 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, sign = p->df ? -1 : 1;
>>> +                uint32_t data = ~0;
>>> +
>>> +                for ( i = 0; i < p->count; i++ )
>>> +                    hvm_copy_to_guest_phys(p->data + sign * i * p->size, &data,
>>> +                                           p->size);
>> This is surely bogus for an `ins` which crosses a page boundary?
> Crossing page boundaries gets dealt with up the call stack in
> hvmemul_linear_to_phys(), namely the path exiting with
> X86EMUL_UNHANDLEABLE when done == 0.
>
> Jan
>

Paul also pointed this out in person, which indicates that
hvm_copy_to_guest_phys() is indeed correct in this case.

Therefore it is fine, but only because the caller guarentees that
"p->data + sign * i * p->size" does not cross a page boundary.


However, what I cant spot is any logic which copes with addr not being
aligned with bytes_per_rep.  This appears to be valid in x86, and would
constitute an individual repetition accessing two pages.

~Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly
  2014-09-29 12:51     ` Andrew Cooper
@ 2014-09-29 13:03       ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2014-09-29 13:03 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Paul Durrant, Keir Fraser, xen-devel

>>> On 29.09.14 at 14:51, <andrew.cooper3@citrix.com> wrote:
> On 29/09/14 13:20, Jan Beulich wrote:
>>>>> On 29.09.14 at 12:59, <andrew.cooper3@citrix.com> wrote:
>>> On 29/09/14 11:21, 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>
>>>> ---
>>>> 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..822ac37 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, sign = p->df ? -1 : 1;
>>>> +                uint32_t data = ~0;
>>>> +
>>>> +                for ( i = 0; i < p->count; i++ )
>>>> +                    hvm_copy_to_guest_phys(p->data + sign * i * p->size, &data,
>>>> +                                           p->size);
>>> This is surely bogus for an `ins` which crosses a page boundary?
>> Crossing page boundaries gets dealt with up the call stack in
>> hvmemul_linear_to_phys(), namely the path exiting with
>> X86EMUL_UNHANDLEABLE when done == 0.
> 
> Paul also pointed this out in person, which indicates that
> hvm_copy_to_guest_phys() is indeed correct in this case.
> 
> Therefore it is fine, but only because the caller guarentees that
> "p->data + sign * i * p->size" does not cross a page boundary.
> 
> 
> However, what I cant spot is any logic which copes with addr not being
> aligned with bytes_per_rep.  This appears to be valid in x86, and would
> constitute an individual repetition accessing two pages.

Just go to the place in the code I pointed you to above - that case
is being taken care of afaict.

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly
  2014-09-29 10:21 [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly Paul Durrant
  2014-09-29 10:59 ` Andrew Cooper
@ 2014-09-30  9:03 ` Jan Beulich
  2014-09-30  9:07   ` Paul Durrant
  1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2014-09-30  9:03 UTC (permalink / raw)
  To: Paul Durrant; +Cc: Keir Fraser, xen-devel

>>> On 29.09.14 at 12:21, <paul.durrant@citrix.com> wrote:
> +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, sign = p->df ? -1 : 1;
> +                uint32_t data = ~0;
> +
> +                for ( i = 0; i < p->count; i++ )
> +                    hvm_copy_to_guest_phys(p->data + sign * i * p->size, &data,
> +                                           p->size);

It's not clear which code you cloned this from - these constructs did
all get replaced by commit f21399e148 ("x86/HVM: properly handle
backward string instruction emulation "), and hence this new addition
has to follow suit.

Jan

> +            }
> +        }
> +        /* FALLTHRU */
> +    default:
> +        p->state = STATE_IORESP_READY;
> +        hvm_io_assist(p);
> +        break;
> +    }
> +
> +    return 1;
> +}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly
  2014-09-30  9:03 ` Jan Beulich
@ 2014-09-30  9:07   ` Paul Durrant
  0 siblings, 0 replies; 8+ messages in thread
From: Paul Durrant @ 2014-09-30  9:07 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Keir (Xen.org), xen-devel@lists.xen.org

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Jan Beulich
> Sent: 30 September 2014 10:04
> To: Paul Durrant
> Cc: Keir (Xen.org); xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v2 for 4.5] ioreq-server: handle the lack of a
> default emulator properly
> 
> >>> On 29.09.14 at 12:21, <paul.durrant@citrix.com> wrote:
> > +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, sign = p->df ? -1 : 1;
> > +                uint32_t data = ~0;
> > +
> > +                for ( i = 0; i < p->count; i++ )
> > +                    hvm_copy_to_guest_phys(p->data + sign * i * p->size, &data,
> > +                                           p->size);
> 
> It's not clear which code you cloned this from - these constructs did
> all get replaced by commit f21399e148 ("x86/HVM: properly handle
> backward string instruction emulation "), and hence this new addition
> has to follow suit.
> 

Ok. It was some old code I had lying around. I'll have a look at that commit.

  Paul

> Jan
> 
> > +            }
> > +        }
> > +        /* FALLTHRU */
> > +    default:
> > +        p->state = STATE_IORESP_READY;
> > +        hvm_io_assist(p);
> > +        break;
> > +    }
> > +
> > +    return 1;
> > +}
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-09-30  9:07 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-29 10:21 [PATCH v2 for 4.5] ioreq-server: handle the lack of a default emulator properly Paul Durrant
2014-09-29 10:59 ` Andrew Cooper
2014-09-29 12:07   ` Paul Durrant
2014-09-29 12:20   ` Jan Beulich
2014-09-29 12:51     ` Andrew Cooper
2014-09-29 13:03       ` Jan Beulich
2014-09-30  9:03 ` Jan Beulich
2014-09-30  9:07   ` Paul Durrant

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).