Re: [Xen-devel] [PATCH for-4.9 v3] x86/vioapic: allow holes in the GSI range for PVH Dom0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.04.17 at 13:55, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Apr 18, 2017 at 05:52:47AM -0600, Jan Beulich wrote:
>> >>> On 18.04.17 at 13:42, <roger.pau@xxxxxxxxxx> wrote:
>> > @@ -538,7 +523,8 @@ void vioapic_reset(struct domain *d)
>> >      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;
>> > +        unsigned int nr_pins = vioapic->nr_pins, base_gsi = 
>> > vioapic->base_gsi;
>> > +        unsigned int pin;
>> >  
>> >          memset(vioapic, 0, hvm_vioapic_size(nr_pins));
>> >          for ( pin = 0; pin < nr_pins; pin++ )
>> > @@ -546,7 +532,7 @@ void vioapic_reset(struct domain *d)
>> >  
>> >          if ( !is_hardware_domain(d) )
>> >          {
>> > -            ASSERT(!i);
>> > +            ASSERT(!i && base_gsi == 0);
>> 
>> If you really want to check this here, then please don't mix styles:
>> Either ! in both cases or, less preferred, == 0.
> 
> Please fix that while committing.
> 
>> > @@ -554,6 +540,7 @@ void vioapic_reset(struct domain *d)
>> >          {
>> >              vioapic->base_address = mp_ioapics[i].mpc_apicaddr;
>> >              vioapic->id = mp_ioapics[i].mpc_apicid;
>> > +            vioapic->base_gsi = base_gsi;
>> 
>> Why did you leave this in place? There's no need to write back what
>> you've just read.
> 
> vioapic_reset zeroes the whole hvm_vioapic structure, so this is needed in
> order to set the base_gsi to it's value.

Oh, indeed. But that would have been far better noticeable if you
had placed the assignment next to the nr_pins one (which it also
clearly pairs with). I guess I'll do that if I end up committing this.

Jan


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