[for-4.14,3/8] x86/hvm: fix ISA IRQ 0 handling when set as lowest priority mode in IO APIC
diff mbox series

Message ID 20200612155640.4101-4-roger.pau@citrix.com
State New
Headers show
Series
  • x86/vpt: fixes for vpt and enable vPIT for PVH dom0
Related show

Commit Message

Roger Pau Monné June 12, 2020, 3:56 p.m. UTC
Lowest priority destination mode does allow the vIO APIC code to
select a vCPU to inject the interrupt to, but the selected vCPU must
be part of the possible destinations configured for such IO APIC pin.

Fix the code in order to only force vCPU 0 if it's part of the
listed destinations.

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

Comments

Jan Beulich June 18, 2020, 2:26 p.m. UTC | #1
On 12.06.2020 17:56, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -422,12 +422,13 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>      case dest_LowestPrio:
>      {
>  #ifdef IRQ0_SPECIAL_ROUTING
> -        /* Force round-robin to pick VCPU 0 */
> -        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
> -        {
> -            v = d->vcpu ? d->vcpu[0] : NULL;
> -            target = v ? vcpu_vlapic(v) : NULL;
> -        }
> +        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
> +
> +        /* Force to pick vCPU 0 if part of the destination list */
> +        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
> +             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
> +             vlapic_enabled(lapic0) )

The vlapic_enabled() part needs justification in the commit message
(if it is to stay), the more that the other path that patch 2 touched
doesn't have / gain it. I'm unconvinced this is a helpful check here
(or anywhere when it's not current's LAPIC that gets probed), as its
result may be stale right after probing.

Having thought about this (including patch 2) some more, I also wonder
whether, if no destination match was found, the IRQ0_SPECIAL_ROUTING
hack should become to nevertheless deliver to CPU0.

Jan
Roger Pau Monné June 18, 2020, 2:55 p.m. UTC | #2
On Thu, Jun 18, 2020 at 04:26:08PM +0200, Jan Beulich wrote:
> On 12.06.2020 17:56, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -422,12 +422,13 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
> >      case dest_LowestPrio:
> >      {
> >  #ifdef IRQ0_SPECIAL_ROUTING
> > -        /* Force round-robin to pick VCPU 0 */
> > -        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
> > -        {
> > -            v = d->vcpu ? d->vcpu[0] : NULL;
> > -            target = v ? vcpu_vlapic(v) : NULL;
> > -        }
> > +        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
> > +
> > +        /* Force to pick vCPU 0 if part of the destination list */
> > +        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
> > +             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
> > +             vlapic_enabled(lapic0) )
> 
> The vlapic_enabled() part needs justification in the commit message
> (if it is to stay), the more that the other path that patch 2 touched
> doesn't have / gain it. I'm unconvinced this is a helpful check here
> (or anywhere when it's not current's LAPIC that gets probed), as its
> result may be stale right after probing.

This is modeled after what vlapic_lowest_prio does, which includes the
vlapic_enabled check. I assumed this was done to prevent injecting to
disabled lapics if possible.

I agree it's stale by the point it gets acted upon, but anyone playing
with enabling/disabling a lapic part of a destination list shouldn't
expect anything sensible to happen IMO.

> Having thought about this (including patch 2) some more, I also wonder
> whether, if no destination match was found, the IRQ0_SPECIAL_ROUTING
> hack should become to nevertheless deliver to CPU0.

Hm, that wouldn't match what real hardware would do, but would indeed
match what old Xen would do for IRQ 0. TBH I would be more comfortable
with attempting to remove this behaviour, and hence don't inject to
any vCPU if none match the list.

Thanks, Roger.
Jan Beulich June 18, 2020, 3:20 p.m. UTC | #3
On 18.06.2020 16:55, Roger Pau Monné wrote:
> On Thu, Jun 18, 2020 at 04:26:08PM +0200, Jan Beulich wrote:
>> On 12.06.2020 17:56, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/vioapic.c
>>> +++ b/xen/arch/x86/hvm/vioapic.c
>>> @@ -422,12 +422,13 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>>>      case dest_LowestPrio:
>>>      {
>>>  #ifdef IRQ0_SPECIAL_ROUTING
>>> -        /* Force round-robin to pick VCPU 0 */
>>> -        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
>>> -        {
>>> -            v = d->vcpu ? d->vcpu[0] : NULL;
>>> -            target = v ? vcpu_vlapic(v) : NULL;
>>> -        }
>>> +        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
>>> +
>>> +        /* Force to pick vCPU 0 if part of the destination list */
>>> +        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
>>> +             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
>>> +             vlapic_enabled(lapic0) )
>>
>> The vlapic_enabled() part needs justification in the commit message
>> (if it is to stay), the more that the other path that patch 2 touched
>> doesn't have / gain it. I'm unconvinced this is a helpful check here
>> (or anywhere when it's not current's LAPIC that gets probed), as its
>> result may be stale right after probing.
> 
> This is modeled after what vlapic_lowest_prio does, which includes the
> vlapic_enabled check. I assumed this was done to prevent injecting to
> disabled lapics if possible.

All understood, but it wants justifying like that in the description,
and the discrepancy to the fixed dest mode wants taking care of
(either again verbally, or by a code change).

> I agree it's stale by the point it gets acted upon, but anyone playing
> with enabling/disabling a lapic part of a destination list shouldn't
> expect anything sensible to happen IMO.

Well, yes, agreed.

>> Having thought about this (including patch 2) some more, I also wonder
>> whether, if no destination match was found, the IRQ0_SPECIAL_ROUTING
>> hack should become to nevertheless deliver to CPU0.
> 
> Hm, that wouldn't match what real hardware would do, but would indeed
> match what old Xen would do for IRQ 0. TBH I would be more comfortable
> with attempting to remove this behaviour, and hence don't inject to
> any vCPU if none match the list.

I agree from an abstract perspective. But at the same time I fear
hard to understand / debug regressions. I'd be curious to know what
others think ...

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 67472e5934..e1417cc6a7 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -422,12 +422,13 @@  static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
     case dest_LowestPrio:
     {
 #ifdef IRQ0_SPECIAL_ROUTING
-        /* Force round-robin to pick VCPU 0 */
-        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
-        {
-            v = d->vcpu ? d->vcpu[0] : NULL;
-            target = v ? vcpu_vlapic(v) : NULL;
-        }
+        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
+
+        /* Force to pick vCPU 0 if part of the destination list */
+        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
+             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
+             vlapic_enabled(lapic0) )
+            target = lapic0;
         else
 #endif
             target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);