Re: [Xen-devel] [PATCH v2 1/7] x86/vioapic: introduce a internal vIO APIC structure

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

On Tue, Mar 28, 2017 at 01:17:27AM -0600, Jan Beulich wrote:
> >>> On 27.03.17 at 18:49, <roger.pau@xxxxxxxxxx> 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.

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