Re: [PATCH v6 2/5] xen/vpci: move xen_domctl_createdomain vPCI flag to common

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 12/15/23 04:36, Rahul Singh wrote:
> Hi Stewart,
> 
>> On 29 Nov 2023, at 9:25 pm, Stewart Hildebrand <Stewart.Hildebrand@xxxxxxx> 
>> wrote:
>>
>> On 11/14/23 04:11, Jan Beulich wrote:
>>> On 13.11.2023 23:21, Stewart Hildebrand wrote:
>>>> @@ -709,10 +710,17 @@ int arch_sanitise_domain_config(struct 
>>>> xen_domctl_createdomain *config)
>>>>         return -EINVAL;
>>>>     }
>>>>
>>>> +    if ( vpci && !hvm )
>>>> +    {
>>>> +        dprintk(XENLOG_INFO, "vPCI requested for non-HVM guest\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>>     return 0;
>>>> }
>>>
>>> As said on the v5 thread, I think my comment was misguided (I'm sorry)
>>> and this wants keeping in common code as you had it.
>>
>> I'll move it back to xen/common/domain.c. No worries.
> 
> I tested this patch and observed build failure when compiling the "x86_64” 
> arch with
> "CONFIG_HVM=n“ option.
> 
> x86_64-linux-gnu-ld    -melf_x86_64  -T arch/x86/xen.lds -N prelink.o 
> --build-id=sha1 \
>     ./common/symbols-dummy.o -o ./.xen-syms.0 
> x86_64-linux-gnu-ld: prelink.o: in function `arch_iommu_hwdom_init’:
> (.init.text+0x2192b): undefined reference to `vpci_is_mmcfg_address’
> (.init.text+0x2192b): relocation truncated to fit: R_X86_64_PLT32 against 
> undefined symbol `vpci_is_mmcfg_address'
> x86_64-linux-gnu-ld: (.init.text+0x21947): undefined reference to 
> `vpci_is_mmcfg_address'
> (.init.text+0x21947): relocation truncated to fit: R_X86_64_PLT32 against 
> undefined symbol `vpci_is_mmcfg_address'
> x86_64-linux-gnu-ld: prelink.o: in function `do_physdev_op’: 
> (.text.do_physdev_op+0x6db): undefined reference to 
> `register_vpci_mmcfg_handler'
> (.text.do_physdev_op+0x6db): relocation truncated to fit: R_X86_64_PLT32 
> against undefined symbol `register_vpci_mmcfg_handler'
> x86_64-linux-gnu-ld: ./.xen-syms.0: hidden symbol `vpci_is_mmcfg_address' 
> isn't defined
> x86_64-linux-gnu-ld: final link failed: bad value                

Ah, good catch. Before moving it, the flag was defined in 
xen/arch/x86/include/asm/domain.h:

#ifdef CONFIG_HVM
#define X86_EMU_VPCI     XEN_X86_EMU_VPCI
#else
#define X86_EMU_VPCI     0
#endif

#define has_vpci(d)        (!!((d)->arch.emulation_flags & X86_EMU_VPCI))


With CONFIG_HVM not set, in xen/drivers/passthrough/x86/iommu.c, the compiler 
optimized out the call to vpci_is_mmcfg_address():

    if ( has_vpci(d) && vpci_is_mmcfg_address(d, pfn_to_paddr(pfn)) )

After moving the flag, there are a couple of options to make the issue go away. 
I don't think #defining XEN_DOMCTL_CDF_vpci 0 in the !HVM case would be a good 
idea since that's a flag shared with the toolstack. We could change the 
definition of has_vpci():

#define has_vpci(d) (((d)->options & XEN_DOMCTL_CDF_vpci) && \
                     IS_ENABLED(CONFIG_HVM))

Or we could provide empty static inline definitions of vpci_is_mmcfg_address() 
and register_vpci_mmcfg_handler():

#ifdef CONFIG_HVM
bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr);
#else
static inline bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr)
{
    return false;
}
#endif

I don't have a strong preference for which way, but changing has_vpci() has a 
smaller diffstat, so I'll go with that for now. This is assuming that we still 
want to go with this approach. Given recent related discussions [1], it's 
possible we may not need a vPCI flag in struct xen_domctl_createdomain, but 
instead a flag internal to the hypervisor somewhere in struct domain.

[1] https://lists.xenproject.org/archives/html/xen-devel/2023-12/msg00212.html