Re: [Xen-devel] [PATCH 3/5] x86/vioapic: introduce support for multiple vIO APICS

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Tue, Mar 21, 2017 at 01:56:51AM -0600, Jan Beulich wrote:
> >>> On 20.03.17 at 19:27, <roger.pau@xxxxxxxxxx> wrote:
> > On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote:
> >> >>> On 23.02.17 at 12:52, <roger.pau@xxxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/hvm/vlapic.c
> >> > +++ b/xen/arch/x86/hvm/vlapic.c
> >> > @@ -1120,7 +1120,7 @@ static int __vlapic_accept_pic_intr(struct vcpu *v)
> >> >      if ( !has_vioapic(d) )
> >> >          return 0;
> >> >  
> >> > -    redir0 = domain_vioapic(d)->redirtbl[0];
> >> > +    redir0 = domain_vioapic(d, 0)->redirtbl[0];
> >> 
> >> What if the first IO-APIC has less than 16 pins?
> > 
> > I'm not sure I understand what this piece of code is trying to do. Why is 
> > PIC
> > relying on the value of the first redirection entry of the IO APIC? Aren't 
> > the
> > PIC and the IO APIC modes mutually exclusive?
> > 
> > I would appreciate if you could provide some reference here.
> 
> Well, we're both in the same position here: All there is as explanation
> is the code plus its history in git. Looking at the code I see that it
> uses RTE 0 for ExtINT handling, which is reasonable. After all that's
> the main connection between PIC and IO-APIC (so called Virtual Wire
> Mode B iirc). See also our own code dealing with this (just look for
> ExtINT in io_apic.c).

Oh, so that's hardcoded to pin 0 of the first IO APIC (that's where the
8259A-master output pin is hooked up AFAICT), in which case the code above is
right, and doesn't depend on whether IO APIC #0 has less than 16 pins.

> >> > @@ -91,13 +92,16 @@ static int pt_irq_vector(struct periodic_time *pt, 
> >> > enum hvm_intsrc src)
> >> >                  + (isa_irq & 7));
> >> >  
> >> >      ASSERT(src == hvm_intsrc_lapic);
> >> > -    return domain_vioapic(v->domain)->redirtbl[gsi].fields.vector;
> >> > +    vioapic = gsi_vioapic(v->domain, gsi, &pin);
> >> > +
> >> > +    return vioapic->redirtbl[pin].fields.vector;
> >> 
> >> Please don't chance de-referencing NULL here and below.
> > 
> > Done, I've added an ASSERT.
> 
> How about release builds then?

OK, I can add a BUG_ON, but maybe it would be better to add a domain_crash and
suitable printk in case this triggers. AFAICT if Xen happens to trigger this it
would mean that the gsi of the platform timer is higher than the maximum gsi,
which at the moment it's impossible.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel