[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/1] xen/pci: Install vpci handlers on x86 and fix exit path


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 20 Oct 2021 10:34:51 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=TZ8+6Pozb3p16OqmS6EHXaSK9qcum+AzoOeS59642s8=; b=mg/Qv4HEEZ9odQYC6WRJkA3YvMooax3riTNlihETq8Cc2lX2Tk1ANDcrlw49RCYlfzOB7VrwQjW+/6o4fc6lhZf/HEgn7nSSlWuYNMDowBflYLnMVMNADSDEBVzrcFTx1erqDm0u0HQoDkOA+ig5yQy9tddOS/vpQOdio9M3GX2MjVCxpQ5IH9BrYq7fQvjr+CbKEkNZdt291oHdBeZIuxWA6t549avRa08m+KYCYvvNqc4K/P47nCq1bl/kIOxPlc++Eyil0+ENBnHyQuXU4v2Tommk3+8I5vIb5l+tIq1bDMNPGv2WRHyoqNhqe8BOfiG3lNNhlBamOsBKU7Zw2w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eo832QnQna0cWQxF7C3s5CUFbI0rz7z4hFtCRYlIojreEgT4ju03FeXXSmghh0MAd7ioAuRUEgUyNEDfQsQHz9cnijc/6B5nYGhdie7mzDPw3TSkk9TgRyjy6DTQOcTsvg96WRDqncCzm4Ulr+b3iOEV9s9Od22gto6eMoEQGJ5XrHykWNnhcDX7IIELVwI9G2q/EEJEGOJpZmysA3K17zDTjZpaf2pazMBi9MDFDziPHPcOSaHGA8v8Bs2UcCOqhHrJMmKNOq7XgclsYotgWWxL+z8+9yEsozv0IfkpbDKLWiHTFjEK603WNdY/nVOQXvzpgajniXOXL/4a4n2lfw==
  • Authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=suse.com;
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, iwj@xxxxxxxxxxxxxx, Oleksandr_Andrushchenko@xxxxxxxx, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Wed, 20 Oct 2021 08:35:07 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 20.10.2021 10:27, Roger Pau Monné wrote:
> On Tue, Oct 19, 2021 at 05:08:28PM +0100, Bertrand Marquis wrote:
>> Xen might not be able to discover at boot time all devices or some devices
>> might appear after specific actions from dom0.
>> In this case dom0 can use the PHYSDEVOP_pci_device_add to signal some
>> PCI devices to Xen.
>> As those devices where not known from Xen before, the vpci handlers must
>> be properly installed during pci_device_add for x86 PVH Dom0, in the
>> same way as what is done currently on arm (where Xen does not detect PCI
>> devices but relies on Dom0 to declare them all the time).
>>
>> So this patch is removing the ifdef protecting the call to
>> vpci_add_handlers and the comment which was arm specific.
>>
>> vpci_add_handlers is called on during pci_device_add which can be called
>> at runtime through hypercall physdev_op.
>> Remove __hwdom_init as the call is not limited anymore to hardware
>> domain init and fix linker script to only keep vpci_array in rodata
>> section.
>>
>> Add missing vpci handlers cleanup during pci_device_remove and in case
>> of error with iommu during pci_device_add.
>>
>> Add empty static inline for vpci_remove_device when CONFIG_VPCI is not
>> defined.
>>
>> Fixes: d59168dc05 ("xen/arm: Enable the existing x86 virtual PCI support
>> for ARM")
>> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> ---
>> Changes in v2
>> - add comment suggested by Jan on top of vpci_add_handlers call
>> - merge the 3 patches of the serie in one patch and renamed it
>> - fix x86 and arm linker script to only keep vpci_array in rodata and
>> only when CONFIG_VPCI is set.
>> ---
>>  xen/arch/arm/xen.lds.S        | 9 +--------
>>  xen/arch/x86/xen.lds.S        | 9 +--------
>>  xen/drivers/passthrough/pci.c | 8 ++++----
>>  xen/drivers/vpci/vpci.c       | 2 +-
>>  xen/include/xen/vpci.h        | 2 ++
>>  5 files changed, 9 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index b773f91f1c..08016948ab 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -60,7 +60,7 @@ SECTIONS
>>         *(.proc.info)
>>         __proc_info_end = .;
>>  
>> -#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
>> +#ifdef CONFIG_HAS_VPCI
>>         . = ALIGN(POINTER_ALIGN);
>>         __start_vpci_array = .;
>>         *(SORT(.data.vpci.*))
>> @@ -189,13 +189,6 @@ SECTIONS
>>         *(.init_array)
>>         *(SORT(.init_array.*))
>>         __ctors_end = .;
>> -
>> -#if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
>> -       . = ALIGN(POINTER_ALIGN);
>> -       __start_vpci_array = .;
>> -       *(SORT(.data.vpci.*))
>> -       __end_vpci_array = .;
>> -#endif
>>    } :text
>>    __init_end_efi = .;
>>    . = ALIGN(STACK_SIZE);
>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>> index 11b1da2154..87e344d4dd 100644
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -134,7 +134,7 @@ SECTIONS
>>         *(.ex_table.pre)
>>         __stop___pre_ex_table = .;
>>  
>> -#if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
>> +#ifdef CONFIG_HAS_VPCI
>>         . = ALIGN(POINTER_ALIGN);
>>         __start_vpci_array = .;
>>         *(SORT(.data.vpci.*))
>> @@ -247,13 +247,6 @@ SECTIONS
>>         *(.init_array)
>>         *(SORT(.init_array.*))
>>         __ctors_end = .;
>> -
>> -#if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
>> -       . = ALIGN(POINTER_ALIGN);
>> -       __start_vpci_array = .;
>> -       *(SORT(.data.vpci.*))
>> -       __end_vpci_array = .;
>> -#endif
>>    } PHDR(text)
>>  
>>    . = ALIGN(SECTION_ALIGN);
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 35e0190796..8928a1c07d 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -756,10 +756,9 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>      if ( !pdev->domain )
>>      {
>>          pdev->domain = hardware_domain;
>> -#ifdef CONFIG_ARM
>>          /*
>> -         * On ARM PCI devices discovery will be done by Dom0. Add vpci 
>> handler
>> -         * when Dom0 inform XEN to add the PCI devices in XEN.
>> +         * For devices not discovered by Xen during boot, add vPCI handlers
>> +         * when Dom0 first informs Xen about such devices.
>>           */
>>          ret = vpci_add_handlers(pdev);
>>          if ( ret )
> 
> Sorry to be a pain, but I think this placement of the call to
> vpci_add_handlers is bogus and should instead be done strictly after
> the device has been added to the hardware_domain->pdev_list list.
> 
> Otherwise the loop over domain->pdev_list (for_each_pdev) in
> modify_bars won't be able to find the device and hit the assert below
> it. That can happen in vpci_add_handlers as it will call init_bars
> which in turn will call into modify_bars if memory decoding is enabled
> for the device.

Oh, good point. And I should have thought of this myself, given that
I did hit that ASSERT() recently with a hidden device. FTAOD I think
this means that the list addition will need to move up (and then
would need undoing on the error path(s)).

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.