On Wed, Apr 14, 2021 at 09:08:53AM +0200, Jan Beulich wrote:
> On 13.04.2021 19:12, Julien Grall wrote:
> > On 12/04/2021 11:49, Roger Pau Monné wrote:
> >> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
> >>> --- a/xen/include/xen/vpci.h
> >>> +++ b/xen/include/xen/vpci.h
> >>> @@ -91,6 +91,7 @@ struct vpci {
> >>> /* FIXME: currently there's no support for SR-IOV. */
> >>> } header;
> >>>
> >>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
> >>> /* MSI data. */
> >>> struct vpci_msi {
> >>> /* Address. */
> >>> @@ -136,6 +137,7 @@ struct vpci {
> >>> struct vpci_arch_msix_entry arch;
> >>> } entries[];
> >>> } *msix;
> >>> +#endif /* CONFIG_PCI_MSI_INTERCEPT */
> >>
> >> Note that here you just remove two pointers from the struct, not that
> >> I'm opposed to it, but it's not that much space that's saved anyway.
> >> Ie: it might also be fine to just leave them as NULL unconditionally
> >> on Arm.
> >
> > Can the two pointers be NULL on x86? If not, then I would prefer if they
> > disappear on Arm so there is less chance to make any mistake (such as
> > unconditionally accessing the pointer in common code).
>
> Alternative proposal: How about making it effectively impossible to
> de-reference the pointer on Arm by leaving the field there, but having
> the struct definition available on non-Arm only?
We could place the struct definitions somewhere else protected by
CONFIG_PCI_MSI_INTERCEPT, but I'm not sure that would be much
different than the current proposal, and overall I think I prefer this
approach then, as we keep the definition and the usage closer
together.
Maybe we could slightly modify the current layout so that
the field is always present, but the struct definition is made
conditional to CONFIG_PCI_MSI_INTERCEPT?
Thanks, Roger.
