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

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

On Fri, Mar 03, 2017 at 06:02:58AM -0700, Jan Beulich wrote:
> >>> 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).

In that same file hvm_msr struct is using a variable size array defined as
follows:

#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
    } msr[];
#elif defined(__GNUC__)
    } msr[0];
#else
    } msr[1 /* variable size */];
#endif

I guess using the same should be fine.

Note that I'm also adding a new field to the struct here (nr_pins) so this is
going to look a little bit weird IMHO:

union vioapic_redir_entry
{
    uint64_t bits;
    struct {
        uint8_t vector;
        uint8_t delivery_mode:3;
        uint8_t dest_mode:1;
        uint8_t delivery_status:1;
        uint8_t polarity:1;
        uint8_t remote_irr:1;
        uint8_t trig_mode:1;
        uint8_t mask:1;
        uint8_t reserve:7;
        uint8_t reserved[4];
        uint8_t dest_id;
    } fields;
};

struct hvm_hw_vioapic {
    uint64_t base_address;
    uint32_t ioregsel;
    uint32_t id;
    uint32_t nr_pins;

#ifndef __XEN__
    union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS];
#else
    union {
        union vioapic_redir_entry gredirtbl[VIOAPIC_NUM_PINS];
        union vioapic_redir_entry
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
        hredirtbl[];
#elif defined(__GNUC__)
        hredirtbl[0];
#else
        hredirtbl[1 /* variable size */];
#endif
#endif
};

Wouldn't it be better to keep hvm_hw_vioapic as-is, and introduce a new
hvm_vioapic struct in vioapic.h, that's used only internally by Xen?

vioapic_{save/load} would need to perform the translation to/from
hvm_hw_ioapic, but I wouldn't need to add any compatibility stuff in save.h.

And I could just declare the hvm_vioapic struct as:

struct hvm_hw_vioapic {
    uint64_t base_address;
    uint32_t ioregsel;
    uint32_t id;
    uint32_t nr_pins;
    union vioapic_redir_entry
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
    hredirtbl[];
#elif defined(__GNUC__)
    hredirtbl[0];
#else
    hredirtbl[1 /* variable size */];
#endif
};

In vioapic.h

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

Right, I would rather prefer an array of pointers to hvm_hw_vioapic, so
domain_vioapic doesn't need to traverse the list. That way I can add the
variable size array without issues.

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

Right, thanks!

Roger.

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