Re: [Xen-devel] [PATCH 2/5] x86/vioapic: allow the vIO APIC to have a variable number of pins

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.03.17 at 13:53, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Mar 03, 2017 at 04:56:20AM -0700, Jan Beulich wrote:
>> >>> On 23.02.17 at 12:52, <roger.pau@xxxxxxxxxx> wrote:
>> > @@ -424,40 +424,141 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>> >      spin_unlock(&d->arch.hvm_domain.irq_lock);
>> >  }
>> >  
>> > -static int ioapic_save(struct domain *d, hvm_domain_context_t *h)
>> > +#define VIOAPIC_SAVE_CONST offsetof(struct hvm_hw_vioapic, redirtbl)
>> 
>> What is this redirtbl field, which oddly enough is a pointer (not allowed
>> in the public interface) good for? I can't seem to find any use of it
>> other than as marker (for use with offsetof()). With all the
>> complications here and below plus the fixed number of entries
>> remaining to be fixed for actual migration purposes I wonder if you
>> wouldn't be better off by keeping the structure mostly as is, simply
>> converting the redirtbl[] field to a union (guarded by a __XEN__
>> conditional) containing both a fixed size array and a variable size
>> one (or to be precise, a zero size one, as a variable size one is not
>> allowed there).
>> 
>> Another alternative would be to introduce a Xen internal sibling
>> struct with a variable size array, and with all fields properly build-
>> time-asserted to be identical between both.
>> 
>> I'll skip the rest of the patch assuming much of it could be dropped
>> this way.
> 
> That was indeed my first approach, but later patches introduce an array of
> hvm_hw_vioapic structs (in order to support multiple IO APICs per domain), and
> this doesn't work if the struct itself contains a variable size array.

By what are you indexing that array? How about using a linked list
instead (or see below)? There won't normally be many entries, so
traversal is not an issue.

> I can
> encapsulate this inside of a hvm_vioapic struct, like:
> 
> struct hvm_vioapic {
>     struct hvm_hw_ioapic *vioapic;
> }
> 
> And make and array of hvm_vioapics instead, but that seemed more cumbersome.

Well, you don't need a structure here at all. Just have an array
of pointers. In any event I think the respective public interface
change should be avoided here if at all possible.

Jan


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