Re: [Xen-devel] [PATCH for-4.9 v4 1/2] x86/vioapic: introduce support for multiple vIO APICS

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

On Wed, Apr 05, 2017 at 08:10:35AM -0600, Jan Beulich wrote:
> >>> On 05.04.17 at 11:23, <roger.pau@xxxxxxxxxx> wrote:
> > +static unsigned int base_gsi(const struct domain *d,
> > +                             const struct hvm_vioapic *vioapic)
> > +{
> > +    unsigned int nr_vioapics = d->arch.hvm_domain.nr_vioapics;
> > +    unsigned int base_gsi = 0, i = 0;
> > +    const struct hvm_vioapic *tmp;
> > +
> > +    while ( --nr_vioapics && (tmp = domain_vioapic(d, i++)) != vioapic )
> > +        base_gsi += tmp->nr_pins;
> 
> While for valid input it should be benign, I think the first part of the
> condition would better use post-decrement (or "i < nr_vioapics").

i < nr_vioapic seems like the best option to me.

> That way you'll return an out of range GSI for a not found vioapic
> input.

Right, with the current code it would return a valid looking base_gsi.

> > @@ -454,32 +523,68 @@ 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 pin, nr_pins = vioapic->nr_pins;
> > +
> > +        memset(vioapic, 0, hvm_vioapic_size(nr_pins));
> > +        for ( pin = 0; pin < nr_pins; pin++ )
> > +            vioapic->redirtbl[pin].fields.mask = 1;
> > +        ASSERT(!i);
> > +        vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS +
> > +                                VIOAPIC_MEM_LENGTH * 0;
> 
> Why not simply
> 
>         vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS;
> 
> ?

Sight, yes please.

> With these taken care of (which can be done while committing, if you
> agree)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks, Roger.

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