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

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

On Tue, Mar 28, 2017 at 04:32:05AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 12:18, <roger.pau@xxxxxxxxxx> wrote:
> > Changes since v1:
> >  - Constify some parameters.
> 
> Considering this I'm surprised the first thing I have to ask is ...
> 
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -44,6 +44,54 @@
> >  
> >  static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq);
> >  
> > +static struct hvm_vioapic *addr_vioapic(struct domain *d, unsigned long 
> > addr)
> 
> ... const? The more that you have it like this ...

Shame, I guess I missed this one, sorry.

> > +struct hvm_vioapic *gsi_vioapic(const struct domain *d, unsigned int gsi,
> 
> ... here.
> 
> > +                                unsigned int *base_gsi)
> 
> All callers of the function really want the pin number rather than
> the base GSI - why don't you make the function provide that
> instead?

Done.

> > +static unsigned int pin_gsi(const struct domain *d,
> > +                            const struct hvm_vioapic *vioapic,
> > +                            unsigned int pin)
> > +{
> > +    const struct hvm_vioapic *tmp;
> > +    unsigned int base_gsi = 0;
> > +
> > +    for ( tmp = domain_vioapic(d, 0); tmp != vioapic; tmp++ )
> > +        base_gsi += tmp->nr_pins;
> > +
> > +    return base_gsi + pin;
> > +}
> 
> Didn't we settle on having a function base_gsi(), with callers
> adding in the pin number as needed? The only reason the function
> here takes pin as a parameter is this final addition.
> 
> Also shouldn't this function somehow guard itself against
> overrunning the array?

Done for both.

> > @@ -275,16 +327,17 @@ static inline int pit_channel0_enabled(void)
> >      return pt_active(&current->domain->arch.vpit.pt0);
> >  }
> >  
> > -static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq)
> > +static void vioapic_deliver(struct hvm_vioapic *vioapic, int pin)
> 
> Occasionally we pass around negative IRQ numbers, but if this
> really is a pin number, I think the type wants changing to unsigned
> int - the more that it is being used as an array index below.

Fixed.

> > @@ -454,33 +517,70 @@ HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, 
> > ioapic_load, 1, HVMSR_PER_DOM);
> >  
> >  void vioapic_reset(struct domain *d)
> >  {
> > -    struct hvm_vioapic *vioapic = domain_vioapic(d);
> > -    uint32_t nr_pins = vioapic->nr_pins;
> > -    int i;
> > +    unsigned int i;
> >  
> >      if ( !has_vioapic(d) )
> > +    {
> > +        ASSERT(!d->arch.hvm_domain.nr_vioapics);
> >          return;
> > +    }
> >  
> > -    memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> > -    vioapic->domain = d;
> > -    vioapic->nr_pins = nr_pins;
> > -    for ( i = 0; i < nr_pins; i++ )
> > -        vioapic->redirtbl[i].fields.mask = 1;
> > -    vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
> > +    for ( i = 0; i < d->arch.hvm_domain.nr_vioapics; i++ )
> > +    {
> > +        struct hvm_vioapic *vioapic = domain_vioapic(d, i);
> > +        unsigned int j, nr_pins = vioapic->nr_pins;
> > +
> > +        memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> > +        for ( j = 0; j < nr_pins; j++ )
> > +            vioapic->redirtbl[j].fields.mask = 1;
> > +        vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
> > +                                VIOAPIC_MEM_LENGTH * i;
> > +        vioapic->id = i;
> > +        vioapic->nr_pins = nr_pins;
> > +        vioapic->domain = d;
> > +    }
> > +}
> 
> Is this function validly reachable for Dom0? If not, better leave it
> alone (adding a nr_vioapics == 1 check), as at least the base
> address calculation looks rather suspicious (there shouldn't be
> multiple IO-APICs on a single page). If so, that same memory
> address calculation is actively wrong in the Dom0 case.

Yes, this is called from vioapic_init in order to set the initial vIO APIC
state and mask all the redir entries. For the Dom0 case we use what's found on
the native MADT tables.

For now I could fix that by adding:

BUILD_BUG_ON(VIOAPIC_MEM_LENGTH > PAGE_SIZE);

And then use PAGE_SIZE above instead of VIOAPIC_MEM_LENGTH.

> > +static void vioapic_free(const struct domain *d, unsigned int nr_vioapics)
> > +{
> > +    unsigned int i;
> > +
> > +    for ( i = 0; i < nr_vioapics; i++)
> > +    {
> > +       if ( domain_vioapic(d, i) == NULL )
> > +          break;
> 
> Is this if() really needed?

Not at all, thanks for pointing it out.

> >  int vioapic_init(struct domain *d)
> >  {
> > +    unsigned int i, nr_vioapics = 1;
> > +
> >      if ( !has_vioapic(d) )
> > +    {
> > +        ASSERT(!d->arch.hvm_domain.nr_vioapics);
> >          return 0;
> > +    }
> >  
> >      if ( (d->arch.hvm_domain.vioapic == NULL) &&
> >           ((d->arch.hvm_domain.vioapic =
> > -           xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL) )
> > +           xzalloc_array(struct hvm_vioapic *, nr_vioapics)) == NULL) )
> >          return -ENOMEM;
> >  
> > -    d->arch.hvm_domain.vioapic->domain = d;
> > -    d->arch.hvm_domain.vioapic->nr_pins = VIOAPIC_NUM_PINS;
> > +    for ( i = 0; i < nr_vioapics; i++ )
> > +    {
> > +        if ( (domain_vioapic(d, i) =
> > +              xmalloc_bytes(hvm_vioapic_size(VIOAPIC_NUM_PINS))) == NULL )
> > +        {
> > +            vioapic_free(d, nr_vioapics);
> > +            return -ENOMEM;
> > +        }
> > +        domain_vioapic(d, i)->nr_pins = VIOAPIC_NUM_PINS;
> > +    }
> > +
> > +    d->arch.hvm_domain.nr_vioapics = nr_vioapics;
> >      vioapic_reset(d);
> 
> These adjustments again don't look right for nr_vioapics > 1, so I
> wonder whether again this function wouldn't better be left alone.
> Alternatively, if Dom0 needs to come here, a comment is going to
> be needed to explain the (temporary) wrong setting of nr_pins.

Yes, Dom0 will need to come here. If it's cleaner I could have two different
init functions, one for DomU and one for Dom0, and call them from vioapic_init.

Or add something like:

ASSERT(is_hardware_domain(d) || nr_vioapics == 1);

(the same could be added to vioapic_reset).

> > @@ -91,13 +92,22 @@ 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, &base_gsi);
> > +    if ( !vioapic )
> > +    {
> > +        gdprintk(XENLOG_WARNING, "Invalid GSI (%u) for platform timer\n", 
> > gsi);
> > +        domain_crash(v->domain);
> > +        return -1;
> > +    }
> 
> Now that you have this check, ...
> 
> > @@ -110,9 +120,10 @@ static int pt_irq_masked(struct periodic_time *pt)
> >      isa_irq = pt->irq;
> >      gsi = hvm_isa_irq_to_gsi(isa_irq);
> >      pic_imr = v->domain->arch.hvm_domain.vpic[isa_irq >> 3].imr;
> > +    vioapic = gsi_vioapic(v->domain, gsi, &base_gsi);
> >  
> >      return (((pic_imr & (1 << (isa_irq & 7))) || 
> > !vlapic_accept_pic_intr(v)) &&
> > -            domain_vioapic(v->domain)->redirtbl[gsi].fields.mask);
> > +            vioapic->redirtbl[gsi - base_gsi].fields.mask);
> >  }
> 
> ... why is there still none here?

Fixed.

Thanks, Roger.

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