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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.17 at 12:18, <roger.pau@xxxxxxxxxx> 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


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