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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>> > @@ -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?

Jan


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