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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.03.17 at 13:40, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Mar 28, 2017 at 04:32:05AM -0600, Jan Beulich wrote:
>> >>> On 27.03.17 at 12:18, <roger.pau@xxxxxxxxxx> wrote:
>> > @@ -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.

Hmm - see my reply to patch 7.

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

Which is basically in line with what I've said in reply to patch 7.

Jan

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