| Message ID | 20200820153442.28305-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vpic: minor fixes | expand |
On 20/08/2020 16:34, Roger Pau Monne wrote: > Currently the dpci EOI callback is only executed for specific EOIs. > This is wrong as non-specific EOIs will also clear the ISR bit and > thus end the interrupt. Re-arrange the code a bit so that the common > EOI handling path can be shared between all EOI modes. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/hvm/vpic.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c > index feb1db2ee3..3cf12581e9 100644 > --- a/xen/arch/x86/hvm/vpic.c > +++ b/xen/arch/x86/hvm/vpic.c > @@ -249,15 +249,15 @@ static void vpic_ioport_write( > if ( priority == VPIC_PRIO_NONE ) > break; > pin = (priority + vpic->priority_add) & 7; > - vpic->isr &= ~(1 << pin); > - if ( cmd == 5 ) > - vpic->priority_add = (pin + 1) & 7; > - break; > + goto common_eoi; > + > case 3: /* Specific EOI */ > case 7: /* Specific EOI & Rotate */ > pin = val & 7; You'll need a /* Fallthrough */ here to keep various things happy. Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Can fix on commit if you're happy. > + > + common_eoi: > vpic->isr &= ~(1 << pin); > - if ( cmd == 7 ) > + if ( cmd == 7 || cmd == 5 ) > vpic->priority_add = (pin + 1) & 7; > /* Release lock and EOI the physical interrupt (if any). */ > vpic_update_int_output(vpic);
On Thu, Aug 20, 2020 at 05:28:21PM +0100, Andrew Cooper wrote: > On 20/08/2020 16:34, Roger Pau Monne wrote: > > Currently the dpci EOI callback is only executed for specific EOIs. > > This is wrong as non-specific EOIs will also clear the ISR bit and > > thus end the interrupt. Re-arrange the code a bit so that the common > > EOI handling path can be shared between all EOI modes. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > xen/arch/x86/hvm/vpic.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c > > index feb1db2ee3..3cf12581e9 100644 > > --- a/xen/arch/x86/hvm/vpic.c > > +++ b/xen/arch/x86/hvm/vpic.c > > @@ -249,15 +249,15 @@ static void vpic_ioport_write( > > if ( priority == VPIC_PRIO_NONE ) > > break; > > pin = (priority + vpic->priority_add) & 7; > > - vpic->isr &= ~(1 << pin); > > - if ( cmd == 5 ) > > - vpic->priority_add = (pin + 1) & 7; > > - break; > > + goto common_eoi; > > + > > case 3: /* Specific EOI */ > > case 7: /* Specific EOI & Rotate */ > > pin = val & 7; > > You'll need a /* Fallthrough */ here to keep various things happy. > > Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Can fix on commit if you're happy. Sure, I was borderline to add it but somehow assumed that /* Fallthrough */ was required for cases but not labels. Thanks, Roger.
On 20.08.2020 18:28, Andrew Cooper wrote: > On 20/08/2020 16:34, Roger Pau Monne wrote: >> Currently the dpci EOI callback is only executed for specific EOIs. >> This is wrong as non-specific EOIs will also clear the ISR bit and >> thus end the interrupt. Re-arrange the code a bit so that the common >> EOI handling path can be shared between all EOI modes. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> xen/arch/x86/hvm/vpic.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c >> index feb1db2ee3..3cf12581e9 100644 >> --- a/xen/arch/x86/hvm/vpic.c >> +++ b/xen/arch/x86/hvm/vpic.c >> @@ -249,15 +249,15 @@ static void vpic_ioport_write( >> if ( priority == VPIC_PRIO_NONE ) >> break; >> pin = (priority + vpic->priority_add) & 7; >> - vpic->isr &= ~(1 << pin); >> - if ( cmd == 5 ) >> - vpic->priority_add = (pin + 1) & 7; >> - break; >> + goto common_eoi; >> + >> case 3: /* Specific EOI */ >> case 7: /* Specific EOI & Rotate */ >> pin = val & 7; > > You'll need a /* Fallthrough */ here to keep various things happy. Are you sure? There's ... > Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Can fix on commit if you're happy. > >> + >> + common_eoi: ... an ordinary label here, not a case one. Jan
On 21.08.2020 09:45, Jan Beulich wrote: > On 20.08.2020 18:28, Andrew Cooper wrote: >> On 20/08/2020 16:34, Roger Pau Monne wrote: >>> Currently the dpci EOI callback is only executed for specific EOIs. >>> This is wrong as non-specific EOIs will also clear the ISR bit and >>> thus end the interrupt. Re-arrange the code a bit so that the common >>> EOI handling path can be shared between all EOI modes. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> xen/arch/x86/hvm/vpic.c | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c >>> index feb1db2ee3..3cf12581e9 100644 >>> --- a/xen/arch/x86/hvm/vpic.c >>> +++ b/xen/arch/x86/hvm/vpic.c >>> @@ -249,15 +249,15 @@ static void vpic_ioport_write( >>> if ( priority == VPIC_PRIO_NONE ) >>> break; >>> pin = (priority + vpic->priority_add) & 7; >>> - vpic->isr &= ~(1 << pin); >>> - if ( cmd == 5 ) >>> - vpic->priority_add = (pin + 1) & 7; >>> - break; >>> + goto common_eoi; >>> + >>> case 3: /* Specific EOI */ >>> case 7: /* Specific EOI & Rotate */ >>> pin = val & 7; >> >> You'll need a /* Fallthrough */ here to keep various things happy. > > Are you sure? There's ... > >> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> Can fix on commit if you're happy. >> >>> + >>> + common_eoi: > > ... an ordinary label here, not a case one. I would have wanted to commit this, but it's still not clear to me whether the adjustment you ask for is really needed. Thanks for following up, Jan
On Mon, Sep 21, 2020 at 12:05:51PM +0200, Jan Beulich wrote: > On 21.08.2020 09:45, Jan Beulich wrote: > > On 20.08.2020 18:28, Andrew Cooper wrote: > >> On 20/08/2020 16:34, Roger Pau Monne wrote: > >>> Currently the dpci EOI callback is only executed for specific EOIs. > >>> This is wrong as non-specific EOIs will also clear the ISR bit and > >>> thus end the interrupt. Re-arrange the code a bit so that the common > >>> EOI handling path can be shared between all EOI modes. > >>> > >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >>> --- > >>> xen/arch/x86/hvm/vpic.c | 10 +++++----- > >>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c > >>> index feb1db2ee3..3cf12581e9 100644 > >>> --- a/xen/arch/x86/hvm/vpic.c > >>> +++ b/xen/arch/x86/hvm/vpic.c > >>> @@ -249,15 +249,15 @@ static void vpic_ioport_write( > >>> if ( priority == VPIC_PRIO_NONE ) > >>> break; > >>> pin = (priority + vpic->priority_add) & 7; > >>> - vpic->isr &= ~(1 << pin); > >>> - if ( cmd == 5 ) > >>> - vpic->priority_add = (pin + 1) & 7; > >>> - break; > >>> + goto common_eoi; > >>> + > >>> case 3: /* Specific EOI */ > >>> case 7: /* Specific EOI & Rotate */ > >>> pin = val & 7; > >> > >> You'll need a /* Fallthrough */ here to keep various things happy. > > > > Are you sure? There's ... > > > >> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> > >> Can fix on commit if you're happy. > >> > >>> + > >>> + common_eoi: > > > > ... an ordinary label here, not a case one. > > I would have wanted to commit this, but it's still not clear to me > whether the adjustment you ask for is really needed. Hello, Was about to send a further series I have on top of this and saw this is still on my patch queue. I'm happy with either way, but I would like to get this committed if possible (as I think from a technical PoV we all agree it's correct). Thanks, Roger.
On 29.09.2020 12:27, Roger Pau Monné wrote: > On Mon, Sep 21, 2020 at 12:05:51PM +0200, Jan Beulich wrote: >> On 21.08.2020 09:45, Jan Beulich wrote: >>> On 20.08.2020 18:28, Andrew Cooper wrote: >>>> On 20/08/2020 16:34, Roger Pau Monne wrote: >>>>> Currently the dpci EOI callback is only executed for specific EOIs. >>>>> This is wrong as non-specific EOIs will also clear the ISR bit and >>>>> thus end the interrupt. Re-arrange the code a bit so that the common >>>>> EOI handling path can be shared between all EOI modes. >>>>> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>>> --- >>>>> xen/arch/x86/hvm/vpic.c | 10 +++++----- >>>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c >>>>> index feb1db2ee3..3cf12581e9 100644 >>>>> --- a/xen/arch/x86/hvm/vpic.c >>>>> +++ b/xen/arch/x86/hvm/vpic.c >>>>> @@ -249,15 +249,15 @@ static void vpic_ioport_write( >>>>> if ( priority == VPIC_PRIO_NONE ) >>>>> break; >>>>> pin = (priority + vpic->priority_add) & 7; >>>>> - vpic->isr &= ~(1 << pin); >>>>> - if ( cmd == 5 ) >>>>> - vpic->priority_add = (pin + 1) & 7; >>>>> - break; >>>>> + goto common_eoi; >>>>> + >>>>> case 3: /* Specific EOI */ >>>>> case 7: /* Specific EOI & Rotate */ >>>>> pin = val & 7; >>>> >>>> You'll need a /* Fallthrough */ here to keep various things happy. >>> >>> Are you sure? There's ... >>> >>>> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> >>>> Can fix on commit if you're happy. >>>> >>>>> + >>>>> + common_eoi: >>> >>> ... an ordinary label here, not a case one. >> >> I would have wanted to commit this, but it's still not clear to me >> whether the adjustment you ask for is really needed. > > Was about to send a further series I have on top of this and saw this > is still on my patch queue. I'm happy with either way, but I would > like to get this committed if possible (as I think from a technical > PoV we all agree it's correct). Hmm, did you mean to send this _to_ Andrew, with me on _cc_? There's nothing I can do without his further input. Jan
On Tue, Sep 29, 2020 at 12:58:20PM +0200, Jan Beulich wrote: > On 29.09.2020 12:27, Roger Pau Monné wrote: > > On Mon, Sep 21, 2020 at 12:05:51PM +0200, Jan Beulich wrote: > >> On 21.08.2020 09:45, Jan Beulich wrote: > >>> On 20.08.2020 18:28, Andrew Cooper wrote: > >>>> On 20/08/2020 16:34, Roger Pau Monne wrote: > >>>>> Currently the dpci EOI callback is only executed for specific EOIs. > >>>>> This is wrong as non-specific EOIs will also clear the ISR bit and > >>>>> thus end the interrupt. Re-arrange the code a bit so that the common > >>>>> EOI handling path can be shared between all EOI modes. > >>>>> > >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >>>>> --- > >>>>> xen/arch/x86/hvm/vpic.c | 10 +++++----- > >>>>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>>>> > >>>>> diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c > >>>>> index feb1db2ee3..3cf12581e9 100644 > >>>>> --- a/xen/arch/x86/hvm/vpic.c > >>>>> +++ b/xen/arch/x86/hvm/vpic.c > >>>>> @@ -249,15 +249,15 @@ static void vpic_ioport_write( > >>>>> if ( priority == VPIC_PRIO_NONE ) > >>>>> break; > >>>>> pin = (priority + vpic->priority_add) & 7; > >>>>> - vpic->isr &= ~(1 << pin); > >>>>> - if ( cmd == 5 ) > >>>>> - vpic->priority_add = (pin + 1) & 7; > >>>>> - break; > >>>>> + goto common_eoi; > >>>>> + > >>>>> case 3: /* Specific EOI */ > >>>>> case 7: /* Specific EOI & Rotate */ > >>>>> pin = val & 7; > >>>> > >>>> You'll need a /* Fallthrough */ here to keep various things happy. > >>> > >>> Are you sure? There's ... > >>> > >>>> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > >>>> > >>>> Can fix on commit if you're happy. > >>>> > >>>>> + > >>>>> + common_eoi: > >>> > >>> ... an ordinary label here, not a case one. > >> > >> I would have wanted to commit this, but it's still not clear to me > >> whether the adjustment you ask for is really needed. > > > > Was about to send a further series I have on top of this and saw this > > is still on my patch queue. I'm happy with either way, but I would > > like to get this committed if possible (as I think from a technical > > PoV we all agree it's correct). > > Hmm, did you mean to send this _to_ Andrew, with me on _cc_? There's > nothing I can do without his further input. Yes, it's fixed now. Please see above Andrew. Roger.
diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c index feb1db2ee3..3cf12581e9 100644 --- a/xen/arch/x86/hvm/vpic.c +++ b/xen/arch/x86/hvm/vpic.c @@ -249,15 +249,15 @@ static void vpic_ioport_write( if ( priority == VPIC_PRIO_NONE ) break; pin = (priority + vpic->priority_add) & 7; - vpic->isr &= ~(1 << pin); - if ( cmd == 5 ) - vpic->priority_add = (pin + 1) & 7; - break; + goto common_eoi; + case 3: /* Specific EOI */ case 7: /* Specific EOI & Rotate */ pin = val & 7; + + common_eoi: vpic->isr &= ~(1 << pin); - if ( cmd == 7 ) + if ( cmd == 7 || cmd == 5 ) vpic->priority_add = (pin + 1) & 7; /* Release lock and EOI the physical interrupt (if any). */ vpic_update_int_output(vpic);
Currently the dpci EOI callback is only executed for specific EOIs. This is wrong as non-specific EOIs will also clear the ISR bit and thus end the interrupt. Re-arrange the code a bit so that the common EOI handling path can be shared between all EOI modes. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/vpic.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)