Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2021 10:28, Roger Pau Monné wrote:
> 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?

You mean like this

    /* MSI data. */
    struct vpci_msi {
#ifdef CONFIG_PCI_MSI_INTERCEPT
        /* Address. */
...
            struct vpci_arch_msix_entry arch;
        } entries[];
#endif /* CONFIG_PCI_MSI_INTERCEPT */
    } *msix;

? I could live with it, but this would have the compiler not
refuse e.g. sizeof(struct vpci_msi) or instantiation of the
struct as a (local) variable, unlike my proposal.

Jan