| Message ID | 20190903161428.7159-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ioreq: add support for internal servers | expand |
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 03 September 2019 17:14 > To: xen-devel@lists.xenproject.org > Cc: Roger Pau Monne <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich > <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu <wl@xen.org> > Subject: [PATCH v2 01/11] ioreq: fix hvm_all_ioreq_servers_add_vcpu fail path cleanup > > The loop in FOR_EACH_IOREQ_SERVER is backwards hence the cleanup on > failure needs to be done forwards. > > Fixes: 97a5a3e30161 ('x86/hvm/ioreq: maintain an array of ioreq servers rather than a list') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Good spot! Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > --- > Changes since v1: > - New in this version. > --- > xen/arch/x86/hvm/ioreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c > index a79cabb680..692b710b02 100644 > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -1195,7 +1195,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v) > return 0; > > fail: > - while ( id-- != 0 ) > + while ( id++ != MAX_NR_IOREQ_SERVERS ) > { > s = GET_IOREQ_SERVER(d, id); > > -- > 2.22.0
On 03.09.2019 18:14, Roger Pau Monne wrote: > --- a/xen/arch/x86/hvm/ioreq.c > +++ b/xen/arch/x86/hvm/ioreq.c > @@ -1195,7 +1195,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v) > return 0; > > fail: > - while ( id-- != 0 ) > + while ( id++ != MAX_NR_IOREQ_SERVERS ) > { > s = GET_IOREQ_SERVER(d, id); With Paul's R-b I was about to commit this, but doesn't this need to be ++id? (If so, I'll be happy to fix while committing.) Jan
On Tue, Sep 10, 2019 at 03:28:57PM +0200, Jan Beulich wrote: > On 03.09.2019 18:14, Roger Pau Monne wrote: > > --- a/xen/arch/x86/hvm/ioreq.c > > +++ b/xen/arch/x86/hvm/ioreq.c > > @@ -1195,7 +1195,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v) > > return 0; > > > > fail: > > - while ( id-- != 0 ) > > + while ( id++ != MAX_NR_IOREQ_SERVERS ) > > { > > s = GET_IOREQ_SERVER(d, id); > > With Paul's R-b I was about to commit this, but doesn't this > need to be ++id? (If so, I'll be happy to fix while committing.) The increment is already done in the loop condition. Thanks, Roger.
On 10.09.2019 15:33, Roger Pau Monné wrote: > On Tue, Sep 10, 2019 at 03:28:57PM +0200, Jan Beulich wrote: >> On 03.09.2019 18:14, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/hvm/ioreq.c >>> +++ b/xen/arch/x86/hvm/ioreq.c >>> @@ -1195,7 +1195,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v) >>> return 0; >>> >>> fail: >>> - while ( id-- != 0 ) >>> + while ( id++ != MAX_NR_IOREQ_SERVERS ) >>> { >>> s = GET_IOREQ_SERVER(d, id); >> >> With Paul's R-b I was about to commit this, but doesn't this >> need to be ++id? (If so, I'll be happy to fix while committing.) > > The increment is already done in the loop condition. That's the increment I mean. I'm sorry for the ambiguity; I didn't want to cut too much of the context. Jan
On Tue, Sep 10, 2019 at 03:35:06PM +0200, Jan Beulich wrote: > On 10.09.2019 15:33, Roger Pau Monné wrote: > > On Tue, Sep 10, 2019 at 03:28:57PM +0200, Jan Beulich wrote: > >> On 03.09.2019 18:14, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/hvm/ioreq.c > >>> +++ b/xen/arch/x86/hvm/ioreq.c > >>> @@ -1195,7 +1195,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v) > >>> return 0; > >>> > >>> fail: > >>> - while ( id-- != 0 ) > >>> + while ( id++ != MAX_NR_IOREQ_SERVERS ) > >>> { > >>> s = GET_IOREQ_SERVER(d, id); > >> > >> With Paul's R-b I was about to commit this, but doesn't this > >> need to be ++id? (If so, I'll be happy to fix while committing.) > > > > The increment is already done in the loop condition. > > That's the increment I mean. I'm sorry for the ambiguity; I > didn't want to cut too much of the context. Oh sorry, yes I think you are correct, or else we would overrun the array by one. Thanks, Roger.
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 10 September 2019 14:42 > To: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; xen- > devel@lists.xenproject.org; Wei Liu <wl@xen.org> > Subject: Re: [PATCH v2 01/11] ioreq: fix hvm_all_ioreq_servers_add_vcpu fail path cleanup > > On Tue, Sep 10, 2019 at 03:35:06PM +0200, Jan Beulich wrote: > > On 10.09.2019 15:33, Roger Pau Monné wrote: > > > On Tue, Sep 10, 2019 at 03:28:57PM +0200, Jan Beulich wrote: > > >> On 03.09.2019 18:14, Roger Pau Monne wrote: > > >>> --- a/xen/arch/x86/hvm/ioreq.c > > >>> +++ b/xen/arch/x86/hvm/ioreq.c > > >>> @@ -1195,7 +1195,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v) > > >>> return 0; > > >>> > > >>> fail: > > >>> - while ( id-- != 0 ) > > >>> + while ( id++ != MAX_NR_IOREQ_SERVERS ) > > >>> { > > >>> s = GET_IOREQ_SERVER(d, id); > > >> > > >> With Paul's R-b I was about to commit this, but doesn't this > > >> need to be ++id? (If so, I'll be happy to fix while committing.) > > > > > > The increment is already done in the loop condition. > > > > That's the increment I mean. I'm sorry for the ambiguity; I > > didn't want to cut too much of the context. > > Oh sorry, yes I think you are correct, or else we would overrun the > array by one. Indeed. I should have spotted that. Paul > > Thanks, Roger.
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index a79cabb680..692b710b02 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -1195,7 +1195,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v) return 0; fail: - while ( id-- != 0 ) + while ( id++ != MAX_NR_IOREQ_SERVERS ) { s = GET_IOREQ_SERVER(d, id);
The loop in FOR_EACH_IOREQ_SERVER is backwards hence the cleanup on failure needs to be done forwards. Fixes: 97a5a3e30161 ('x86/hvm/ioreq: maintain an array of ioreq servers rather than a list') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/hvm/ioreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)