On 12/15/23 04:36, Rahul Singh wrote:
> Hi Stewart,
>
>> On 29 Nov 2023, at 9:25 pm, Stewart Hildebrand <stewart.hildebr...@amd.com>
>> 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
