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

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

On Fri, Mar 03, 2017 at 10:06:03AM -0700, Jan Beulich wrote:
> >>> On 23.02.17 at 12:52, <roger.pau@xxxxxxxxxx> wrote:
> > +struct hvm_hw_vioapic *gsi_vioapic(struct domain *d, unsigned int gsi,
> 
> const struct domain *d ?
> 
> > +static unsigned int pin_gsi(struct domain *d, struct hvm_hw_vioapic 
> > *vioapic,
> 
> const for both?
>
> > +                            unsigned int pin)
> > +{
> > +    struct hvm_hw_vioapic *tmp;
> 
> And here.

Done for all the above.

> Also wouldn't this function more naturally (i.e. more generally useful)
> simply return the base GSI, leaving it to the caller to add the pin
> number?

I don't have a strong opinion. Most (if not all callers) seem to want the pin,
but you can obtain that by just doing gsi - base_gsi, so I changed it to return
base_gsi (which also simplifies the code).

> > @@ -157,21 +210,22 @@ static void vioapic_write_redirent(
> >          pent->fields.remote_irr = 0;
> >      else if ( !ent.fields.mask &&
> >                !ent.fields.remote_irr &&
> > -              hvm_irq->gsi_assert_count[idx] )
> > +              hvm_irq->gsi_assert_count[gsi] )
> 
> Neither this nor any earlier patch modified the size of this array, afaics.

Hm, right. This is still hardcoded to VIOAPIC_NUM_PINS. I should change it in
the earlier patch also.

> > -static void vioapic_write_indirect(struct domain *d, uint32_t val)
> > +static void vioapic_write_indirect(struct domain *d, unsigned long addr,
> > +                                   uint32_t val)
> >  {
> > -    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
> > +    struct hvm_hw_vioapic *vioapic = addr_vioapic(d, addr);
> 
> I think it would be better for the caller to pass the vioapic it
> already established (and ASSERT()ed).

Done. Now that hvm_vioapic has a reference to struct domain I no longer need to
pass the domain around.

> > @@ -430,11 +493,14 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
> >  
> >  static int vioapic_save(struct domain *d, hvm_domain_context_t *h)
> >  {
> > -    struct hvm_hw_vioapic *vioapic = domain_vioapic(d);
> > +    struct hvm_hw_vioapic *vioapic = domain_vioapic(d, 0);
> >  
> >      if ( !has_vioapic(d) )
> >          return 0;
> >  
> > +    if ( d->arch.hvm_domain.nr_vioapics != 1 )
> > +        return -ENOSYS;
> > +
> >      if ( vioapic->nr_pins != VIOAPIC_NUM_PINS )
> >          return -ENOSYS;
> 
> Perhaps merge the two if()s? Otherwise -EOPNOTSUPP again.

OK.

> >  int vioapic_init(struct domain *d)
> >  {
> >      if ( !has_vioapic(d) )
> > +    {
> > +        d->arch.hvm_domain.nr_vioapics = 0;
> 
> I don't think this is needed - if anything you may want to again
> ASSERT() it being zero.

Done.

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

> > --- a/xen/arch/x86/hvm/vpt.c
> > +++ b/xen/arch/x86/hvm/vpt.c
> > @@ -78,7 +78,8 @@ void hvm_set_guest_time(struct vcpu *v, u64 guest_time)
> >  static int pt_irq_vector(struct periodic_time *pt, enum hvm_intsrc src)
> >  {
> >      struct vcpu *v = pt->vcpu;
> > -    unsigned int gsi, isa_irq;
> > +    struct hvm_hw_vioapic *vioapic;
> > +    unsigned int gsi, isa_irq, pin;
> >  
> >      if ( pt->source == PTSRC_lapic )
> >          return pt->irq;
> > @@ -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.

Roger.

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