| Message ID | 20170327101823.99368-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote: > The reason to expand the hvm_vioapic structure instead of the hvm_hw_vioapic > one is that the variable number of pins functionality is only going to be used > by the hardware domain, so no modifications are needed to the save format. As you say here you expand an existing structure, so the title isn't really correct. > @@ -426,38 +426,43 @@ void vioapic_update_EOI(struct domain *d, u8 vector) > > static int ioapic_save(struct domain *d, hvm_domain_context_t *h) > { > - struct hvm_hw_vioapic *s = domain_vioapic(d); > + struct hvm_vioapic *s = domain_vioapic(d); > > if ( !has_vioapic(d) ) > return 0; > > - return hvm_save_entry(IOAPIC, 0, h, s); > + BUILD_BUG_ON(sizeof(struct hvm_hw_vioapic) != > + sizeof(struct hvm_vioapic) - > + offsetof(struct hvm_vioapic, base_address)); This is too weak a check for my taste, and ... > + return hvm_save_entry(IOAPIC, 0, h, &s->base_address); ... this too fragile a use. See also below. > --- a/xen/include/asm-x86/hvm/vioapic.h > +++ b/xen/include/asm-x86/hvm/vioapic.h > @@ -48,13 +48,16 @@ > #define VIOAPIC_REG_RTE0 0x10 > > struct hvm_vioapic { > - struct hvm_hw_vioapic hvm_hw_vioapic; > struct domain *domain; > + /* Layout below must match hvm_hw_vioapic. */ > + uint64_t base_address; > + uint32_t ioregsel; > + uint32_t id; > + union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS]; > }; It is mere luck that last old and first new fields aren't both 32-bit ones, or else this approach would not have worked at all. I think we need some better approach here, but the absolute minimum would be to also add a comment on the other side. One possible approach would be to move the entire set of field declarations of struct hvm_hw_vioapic into a macro, using it both there and here. Which would then leave making sure there are no alignment effects because of fields added ahead of that macro's use (perhaps via BUILD_BUG_ON(), or maybe even better by making this an unnamed structure member inside struct hvm_ioapic). The macro should then be #undef-ed in the header, except in the __XEN__ case. Perhaps the macro would also want to be given a parameter right away for the invoking site to specify the number of pins. Jan
On Mon, Mar 27, 2017 at 09:38:49AM -0600, Jan Beulich wrote: > >>> On 27.03.17 at 12:18, <roger.pau@citrix.com> wrote: > > The reason to expand the hvm_vioapic structure instead of the hvm_hw_vioapic > > one is that the variable number of pins functionality is only going to be used > > by the hardware domain, so no modifications are needed to the save format. > > As you say here you expand an existing structure, so the title > isn't really correct. > > > @@ -426,38 +426,43 @@ void vioapic_update_EOI(struct domain *d, u8 vector) > > > > static int ioapic_save(struct domain *d, hvm_domain_context_t *h) > > { > > - struct hvm_hw_vioapic *s = domain_vioapic(d); > > + struct hvm_vioapic *s = domain_vioapic(d); > > > > if ( !has_vioapic(d) ) > > return 0; > > > > - return hvm_save_entry(IOAPIC, 0, h, s); > > + BUILD_BUG_ON(sizeof(struct hvm_hw_vioapic) != > > + sizeof(struct hvm_vioapic) - > > + offsetof(struct hvm_vioapic, base_address)); > > This is too weak a check for my taste, and ... I've removed this BUILD_BUG_ON. > > + return hvm_save_entry(IOAPIC, 0, h, &s->base_address); > > ... this too fragile a use. See also below. > > > --- a/xen/include/asm-x86/hvm/vioapic.h > > +++ b/xen/include/asm-x86/hvm/vioapic.h > > @@ -48,13 +48,16 @@ > > #define VIOAPIC_REG_RTE0 0x10 > > > > struct hvm_vioapic { > > - struct hvm_hw_vioapic hvm_hw_vioapic; > > struct domain *domain; > > + /* Layout below must match hvm_hw_vioapic. */ > > + uint64_t base_address; > > + uint32_t ioregsel; > > + uint32_t id; > > + union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS]; > > }; > > It is mere luck that last old and first new fields aren't both 32-bit > ones, or else this approach would not have worked at all. I think > we need some better approach here, but the absolute minimum > would be to also add a comment on the other side. > > One possible approach would be to move the entire set of field > declarations of struct hvm_hw_vioapic into a macro, using it both > there and here. Which would then leave making sure there are no > alignment effects because of fields added ahead of that macro's > use (perhaps via BUILD_BUG_ON(), or maybe even better by > making this an unnamed structure member inside struct > hvm_ioapic). > > The macro should then be #undef-ed in the header, except in the > __XEN__ case. Perhaps the macro would also want to be given a > parameter right away for the invoking site to specify the number > of pins. Yes, I think the unnamed structure is way better, here's what I've done: save.h: 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; }; #define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */ #define DECLARE_VIOAPIC(name, cnt) \ struct name { \ uint64_t base_address; \ uint32_t ioregsel; \ uint32_t id; \ union vioapic_redir_entry redirtbl[cnt]; \ } DECLARE_VIOAPIC(hvm_hw_vioapic, VIOAPIC_NUM_PINS); #ifndef __XEN__ #undef DECLARE_VIOAPIC #endif vioapic.h: struct hvm_vioapic { struct domain *domain; DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS); }; This seems to work fine, and now the BUILD_BUG_ON is just pointless. Thanks for the tip, Roger.
>>> On 27.03.17 at 18:49, <roger.pau@citrix.com> wrote: > Yes, I think the unnamed structure is way better, here's what I've done: > > save.h: > > 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; > }; > > #define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */ > > #define DECLARE_VIOAPIC(name, cnt) \ > struct name { \ > uint64_t base_address; \ > uint32_t ioregsel; \ > uint32_t id; \ > union vioapic_redir_entry redirtbl[cnt]; \ > } > > DECLARE_VIOAPIC(hvm_hw_vioapic, VIOAPIC_NUM_PINS); > > #ifndef __XEN__ > #undef DECLARE_VIOAPIC > #endif > > vioapic.h: > > struct hvm_vioapic { > struct domain *domain; > DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS); > }; > > This seems to work fine, and now the BUILD_BUG_ON is just pointless. Well, no, not entirely. As said you still want to exclude alignment effects prior structure members of hvm_vioapic may have (please continue to not make assumptions on the alignment of the first field of the structure here). Furthermore, despite the #undef the macro name should start with XEN_, perhaps even with XEN_HVM_. Whether the DECLARE part is really needed/useful I'm not sure. Jan
On Tue, Mar 28, 2017 at 01:17:27AM -0600, Jan Beulich wrote: > >>> On 27.03.17 at 18:49, <roger.pau@citrix.com> wrote: > > Yes, I think the unnamed structure is way better, here's what I've done: > > > > save.h: > > > > 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; > > }; > > > > #define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */ > > > > #define DECLARE_VIOAPIC(name, cnt) \ > > struct name { \ > > uint64_t base_address; \ > > uint32_t ioregsel; \ > > uint32_t id; \ > > union vioapic_redir_entry redirtbl[cnt]; \ > > } > > > > DECLARE_VIOAPIC(hvm_hw_vioapic, VIOAPIC_NUM_PINS); > > > > #ifndef __XEN__ > > #undef DECLARE_VIOAPIC > > #endif > > > > vioapic.h: > > > > struct hvm_vioapic { > > struct domain *domain; > > DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS); > > }; > > > > This seems to work fine, and now the BUILD_BUG_ON is just pointless. > > Well, no, not entirely. As said you still want to exclude alignment > effects prior structure members of hvm_vioapic may have (please > continue to not make assumptions on the alignment of the first > field of the structure here). But doesn't the usage of an unnamed structure get rid of the alignment issue? For example I have the following code: struct bar { uint8_t b; uint8_t c; uint32_t d; uint32_t e; }; struct foo { uint8_t a; struct { uint8_t b; uint8_t c; uint32_t d; uint32_t e; }; }; struct foobar { uint8_t a; uint8_t b; uint8_t c; uint32_t d; uint32_t e; }; This according to pahole has the following layout: struct bar { uint8_t b; /* 0 1 */ uint8_t c; /* 1 1 */ /* XXX 2 bytes hole, try to pack */ uint32_t d; /* 4 4 */ uint32_t e; /* 8 4 */ /* size: 12, cachelines: 1, members: 4 */ /* sum members: 10, holes: 1, sum holes: 2 */ /* last cacheline: 12 bytes */ }; struct foo { uint8_t a; /* 0 1 */ /* XXX 3 bytes hole, try to pack */ struct { uint8_t b; /* 4 1 */ uint8_t c; /* 5 1 */ uint32_t d; /* 8 4 */ uint32_t e; /* 12 4 */ }; /* 4 12 */ /* size: 16, cachelines: 1, members: 2 */ /* sum members: 13, holes: 1, sum holes: 3 */ /* last cacheline: 16 bytes */ }; struct foobar { uint8_t a; /* 0 1 */ uint8_t b; /* 1 1 */ uint8_t c; /* 2 1 */ /* XXX 1 byte hole, try to pack */ uint32_t d; /* 4 4 */ uint32_t e; /* 8 4 */ /* size: 12, cachelines: 1, members: 5 */ /* sum members: 11, holes: 1, sum holes: 1 */ /* last cacheline: 12 bytes */ }; The unnamed struct (clone of bar) inside of foo is already aligned (so it ends up with the same layout as bar), as opposed to foobar, which indeed ends up with a different layout. > Furthermore, despite the #undef the macro name should start > with XEN_, perhaps even with XEN_HVM_. Whether the > DECLARE part is really needed/useful I'm not sure. I've renamed it to XEN_HVM_VIOAPIC. Thanks, Roger.
>>> On 28.03.17 at 10:15, <roger.pau@citrix.com> wrote: > On Tue, Mar 28, 2017 at 01:17:27AM -0600, Jan Beulich wrote: >> >>> On 27.03.17 at 18:49, <roger.pau@citrix.com> wrote: >> > Yes, I think the unnamed structure is way better, here's what I've done: >> > >> > save.h: >> > >> > 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; >> > }; >> > >> > #define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */ >> > >> > #define DECLARE_VIOAPIC(name, cnt) \ >> > struct name { \ >> > uint64_t base_address; \ >> > uint32_t ioregsel; \ >> > uint32_t id; \ >> > union vioapic_redir_entry redirtbl[cnt]; \ >> > } >> > >> > DECLARE_VIOAPIC(hvm_hw_vioapic, VIOAPIC_NUM_PINS); >> > >> > #ifndef __XEN__ >> > #undef DECLARE_VIOAPIC >> > #endif >> > >> > vioapic.h: >> > >> > struct hvm_vioapic { >> > struct domain *domain; >> > DECLARE_VIOAPIC(, VIOAPIC_NUM_PINS); >> > }; >> > >> > This seems to work fine, and now the BUILD_BUG_ON is just pointless. >> >> Well, no, not entirely. As said you still want to exclude alignment >> effects prior structure members of hvm_vioapic may have (please >> continue to not make assumptions on the alignment of the first >> field of the structure here). > > But doesn't the usage of an unnamed structure get rid of the alignment issue? Oh, of course it does - provided there are no differing packing options in effect at the time the two structures get actually declared. But yes, perhaps we can live with this being implied. And too big of an alignment is no problem in the first place. Jan
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index fdbb21f097..84139a3b7d 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -42,9 +42,9 @@ /* HACK: Route IRQ0 only to VCPU0 to prevent time jumps. */ #define IRQ0_SPECIAL_ROUTING 1 -static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq); +static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq); -static uint32_t vioapic_read_indirect(const struct hvm_hw_vioapic *vioapic) +static uint32_t vioapic_read_indirect(const struct hvm_vioapic *vioapic) { uint32_t result = 0; @@ -94,7 +94,7 @@ static int vioapic_read( struct vcpu *v, unsigned long addr, unsigned int length, unsigned long *pval) { - const struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain); + const struct hvm_vioapic *vioapic = domain_vioapic(v->domain); uint32_t result; HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "addr %lx", addr); @@ -119,7 +119,7 @@ static int vioapic_read( } static void vioapic_write_redirent( - struct hvm_hw_vioapic *vioapic, unsigned int idx, + struct hvm_vioapic *vioapic, unsigned int idx, int top_word, uint32_t val) { struct domain *d = vioapic_domain(vioapic); @@ -170,7 +170,7 @@ static void vioapic_write_redirent( } static void vioapic_write_indirect( - struct hvm_hw_vioapic *vioapic, uint32_t val) + struct hvm_vioapic *vioapic, uint32_t val) { switch ( vioapic->ioregsel ) { @@ -215,7 +215,7 @@ static int vioapic_write( struct vcpu *v, unsigned long addr, unsigned int length, unsigned long val) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain); + struct hvm_vioapic *vioapic = domain_vioapic(v->domain); switch ( addr & 0xff ) { @@ -242,7 +242,7 @@ static int vioapic_write( static int vioapic_range(struct vcpu *v, unsigned long addr) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain); + struct hvm_vioapic *vioapic = domain_vioapic(v->domain); return ((addr >= vioapic->base_address && (addr < vioapic->base_address + VIOAPIC_MEM_LENGTH))); @@ -255,7 +255,7 @@ static const struct hvm_mmio_ops vioapic_mmio_ops = { }; static void ioapic_inj_irq( - struct hvm_hw_vioapic *vioapic, + struct hvm_vioapic *vioapic, struct vlapic *target, uint8_t vector, uint8_t trig_mode, @@ -275,7 +275,7 @@ static inline int pit_channel0_enabled(void) return pt_active(¤t->domain->arch.vpit.pt0); } -static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq) +static void vioapic_deliver(struct hvm_vioapic *vioapic, int irq) { uint16_t dest = vioapic->redirtbl[irq].fields.dest_id; uint8_t dest_mode = vioapic->redirtbl[irq].fields.dest_mode; @@ -361,7 +361,7 @@ static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq) void vioapic_irq_positive_edge(struct domain *d, unsigned int irq) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); + struct hvm_vioapic *vioapic = domain_vioapic(d); union vioapic_redir_entry *ent; ASSERT(has_vioapic(d)); @@ -388,7 +388,7 @@ void vioapic_irq_positive_edge(struct domain *d, unsigned int irq) void vioapic_update_EOI(struct domain *d, u8 vector) { - struct hvm_hw_vioapic *vioapic = domain_vioapic(d); + struct hvm_vioapic *vioapic = domain_vioapic(d); struct hvm_irq *hvm_irq = &d->arch.hvm_domain.irq; union vioapic_redir_entry *ent; int gsi; @@ -426,38 +426,43 @@ void vioapic_update_EOI(struct domain *d, u8 vector) static int ioapic_save(struct domain *d, hvm_domain_context_t *h) { - struct hvm_hw_vioapic *s = domain_vioapic(d); + struct hvm_vioapic *s = domain_vioapic(d); if ( !has_vioapic(d) ) return 0; - return hvm_save_entry(IOAPIC, 0, h, s); + BUILD_BUG_ON(sizeof(struct hvm_hw_vioapic) != + sizeof(struct hvm_vioapic) - + offsetof(struct hvm_vioapic, base_address)); + + return hvm_save_entry(IOAPIC, 0, h, &s->base_address); } static int ioapic_load(struct domain *d, hvm_domain_context_t *h) { - struct hvm_hw_vioapic *s = domain_vioapic(d); + struct hvm_vioapic *s = domain_vioapic(d); if ( !has_vioapic(d) ) return -ENODEV; - return hvm_load_entry(IOAPIC, h, s); + return hvm_load_entry(IOAPIC, h, &s->base_address); } HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_load, 1, HVMSR_PER_DOM); void vioapic_reset(struct domain *d) { - struct hvm_vioapic *vioapic = d->arch.hvm_domain.vioapic; + struct hvm_vioapic *vioapic = domain_vioapic(d); int i; if ( !has_vioapic(d) ) return; - memset(&vioapic->hvm_hw_vioapic, 0, sizeof(vioapic->hvm_hw_vioapic)); + memset(vioapic, 0, sizeof(*vioapic)); + vioapic->domain = d; for ( i = 0; i < VIOAPIC_NUM_PINS; i++ ) - vioapic->hvm_hw_vioapic.redirtbl[i].fields.mask = 1; - vioapic->hvm_hw_vioapic.base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; + vioapic->redirtbl[i].fields.mask = 1; + vioapic->base_address = VIOAPIC_DEFAULT_BASE_ADDRESS; } int vioapic_init(struct domain *d) diff --git a/xen/include/asm-x86/hvm/vioapic.h b/xen/include/asm-x86/hvm/vioapic.h index 745c09ab5c..e8ec0be7b6 100644 --- a/xen/include/asm-x86/hvm/vioapic.h +++ b/xen/include/asm-x86/hvm/vioapic.h @@ -48,13 +48,16 @@ #define VIOAPIC_REG_RTE0 0x10 struct hvm_vioapic { - struct hvm_hw_vioapic hvm_hw_vioapic; struct domain *domain; + /* Layout below must match hvm_hw_vioapic. */ + uint64_t base_address; + uint32_t ioregsel; + uint32_t id; + union vioapic_redir_entry redirtbl[VIOAPIC_NUM_PINS]; }; -#define domain_vioapic(d) (&(d)->arch.hvm_domain.vioapic->hvm_hw_vioapic) -#define vioapic_domain(v) (container_of((v), struct hvm_vioapic, \ - hvm_hw_vioapic)->domain) +#define domain_vioapic(d) ((d)->arch.hvm_domain.vioapic) +#define vioapic_domain(v) ((v)->domain) int vioapic_init(struct domain *d); void vioapic_deinit(struct domain *d);
This is required in order to have a variable number of vIO APIC pins, instead of the current fixed value (48). Note that this patch only expands the fields of the hvm_vioapic struct, without actually introducing any new fields or functionality. The reason to expand the hvm_vioapic structure instead of the hvm_hw_vioapic one is that the variable number of pins functionality is only going to be used by the hardware domain, so no modifications are needed to the save format. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/hvm/vioapic.c | 43 ++++++++++++++++++++++----------------- xen/include/asm-x86/hvm/vioapic.h | 11 ++++++---- 2 files changed, 31 insertions(+), 23 deletions(-)