[PATCH 0/2] x86/vpic: minor fixes

[PATCH 0/2] x86/vpic: minor fixes

[Roger Pau Monne]

Hello,

This series contains one non-functional change and one small fix for
pci-passthrough when using the 8259A PIC. I very much doubt anyone has
done pci-passthrough on guests using the legacy PIC, but nonetheless
let's aim for it to be correct.

Thanks, Roger.

Roger Pau Monne (2):
  x86/vpic: rename irq to pin in vpic_ioport_write
  x86/vpic: also execute dpci callback for non-specific EOI

 xen/arch/x86/hvm/vpic.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

-- 
2.28.0

[PATCH 1/2] x86/vpic: rename irq to pin in vpic_ioport_write

[Roger Pau Monne]

The irq variable is wrongly named, as it's used to store the pin on
the 8259 chip, but not the global irq value. While renaming reduce
it's scope and make it unsigned.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vpic.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/vpic.c b/xen/arch/x86/hvm/vpic.c
index 936c7b27c6..feb1db2ee3 100644
--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -184,7 +184,7 @@ static int vpic_intack(struct hvm_hw_vpic *vpic)
 static void vpic_ioport_write(
     struct hvm_hw_vpic *vpic, uint32_t addr, uint32_t val)
 {
-    int priority, cmd, irq;
+    int priority, cmd;
     uint8_t mask, unmasked = 0;
 
     vpic_lock(vpic);
@@ -230,6 +230,8 @@ static void vpic_ioport_write(
         }
         else
         {
+            unsigned int pin;
+
             /* OCW2 */
             cmd = val >> 5;
             switch ( cmd )
@@ -246,22 +248,22 @@ static void vpic_ioport_write(
                 priority = vpic_get_priority(vpic, mask);
                 if ( priority == VPIC_PRIO_NONE )
                     break;
-                irq = (priority + vpic->priority_add) & 7;
-                vpic->isr &= ~(1 << irq);
+                pin = (priority + vpic->priority_add) & 7;
+                vpic->isr &= ~(1 << pin);
                 if ( cmd == 5 )
-                    vpic->priority_add = (irq + 1) & 7;
+                    vpic->priority_add = (pin + 1) & 7;
                 break;
             case 3: /* Specific EOI                */
             case 7: /* Specific EOI & Rotate       */
-                irq = val & 7;
-                vpic->isr &= ~(1 << irq);
+                pin = val & 7;
+                vpic->isr &= ~(1 << pin);
                 if ( cmd == 7 )
-                    vpic->priority_add = (irq + 1) & 7;
+                    vpic->priority_add = (pin + 1) & 7;
                 /* Release lock and EOI the physical interrupt (if any). */
                 vpic_update_int_output(vpic);
                 vpic_unlock(vpic);
                 hvm_dpci_eoi(current->domain,
-                             hvm_isa_irq_to_gsi((addr >> 7) ? (irq|8) : irq),
+                             hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : pin),
                              NULL);
                 return; /* bail immediately */
             case 6: /* Set Priority                */
-- 
2.28.0

[PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Roger Pau Monne]

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;
+
+            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);
-- 
2.28.0

Re: [PATCH 1/2] x86/vpic: rename irq to pin in vpic_ioport_write

[Andrew Cooper]

On 20/08/2020 16:34, Roger Pau Monne wrote:
> The irq variable is wrongly named, as it's used to store the pin on
> the 8259 chip, but not the global irq value. While renaming reduce
> it's scope and make it unsigned.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Andrew Cooper]

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

Re: [PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Roger Pau Monné]

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.

Re: [PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Jan Beulich]

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

Ping: [PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Jan Beulich]

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

Re: Ping: [PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Roger Pau Monné]

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.

Re: Ping: [PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Jan Beulich]

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

Re: Ping: [PATCH 2/2] x86/vpic: also execute dpci callback for non-specific EOI

[Roger Pau Monné]

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.