Re: [PATCH v2 10/17] xen/arm: Add cmdline boot option "pci-passthrough = <boolean>"

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

> On 23 Sep 2021, at 1:14 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Wed, 22 Sep 2021, Rahul Singh wrote:
>> Add cmdline boot option "pci-passthrough = = <boolean>" to enable
>> disable the PCI passthrough support on ARM.
> ^ or disable
Ack.
> 
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>> Change in v2:
>> - Add option in xen-command-line.pandoc
>> - Change pci option to pci-passthrough
>> - modify option from custom_param to boolean param
>> ---
>> docs/misc/xen-command-line.pandoc |  7 +++++++
>> xen/arch/arm/pci/pci.c            | 14 ++++++++++++++
>> xen/common/physdev.c              |  6 ++++++
>> xen/include/asm-arm/pci.h         | 13 +++++++++++++
>> xen/include/asm-x86/pci.h         |  8 ++++++++
>> 5 files changed, 48 insertions(+)
>> 
>> diff --git a/docs/misc/xen-command-line.pandoc 
>> b/docs/misc/xen-command-line.pandoc
>> index b175645fde..c867f1cf58 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1783,6 +1783,13 @@ All numbers specified must be hexadecimal ones.
>> 
>> This option can be specified more than once (up to 8 times at present).
>> 
>> +### pci-passthrough (arm)
>> +> `= <boolean>`
>> +
>> +> Default: `false`
>> +
>> +Flag to enable or disable support for PCI passthrough
>> +
>> ### pcid (x86)
>>> `= <boolean> | xpti=<bool>`
>> 
>> diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
>> index 71fa532842..fe96a9b135 100644
>> --- a/xen/arch/arm/pci/pci.c
>> +++ b/xen/arch/arm/pci/pci.c
>> @@ -16,6 +16,7 @@
>> #include <xen/device_tree.h>
>> #include <xen/errno.h>
>> #include <xen/init.h>
>> +#include <xen/param.h>
>> #include <xen/pci.h>
>> 
>> /*
>> @@ -65,8 +66,21 @@ static inline int __init acpi_pci_init(void)
>> }
>> #endif
>> 
>> +/*
>> + * By default pci passthrough is disabled
>> + */
>> +bool_t __read_mostly pci_passthrough_enabled = 0;
> 
> I think we are using bool rather than bool_t nowadays. Also could do =
> false.

Yes I missed that I will use bool.
> 
> 
>> +boolean_param("pci-passthrough", pci_passthrough_enabled);
>> +
>> static int __init pci_init(void)
>> {
>> +    /*
>> +     * Enable PCI passthrough when has been enabled explicitly
>> +     * (pci-passthrough=on)
>> +     */
>> +    if ( !pci_passthrough_enabled)
> 
> missing space

Ack. 
> 
> 
>> +        return 0;
>> +
>>     pci_segments_init();
>> 
>>     if ( acpi_disabled )
>> diff --git a/xen/common/physdev.c b/xen/common/physdev.c
>> index 8d44b20db8..7390d5d584 100644
>> --- a/xen/common/physdev.c
>> +++ b/xen/common/physdev.c
>> @@ -19,6 +19,9 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
>> arg)
>>         struct pci_dev_info pdev_info;
>>         nodeid_t node;
>> 
>> +        if ( !is_pci_passthrough_enabled() )
>> +            return -ENOSYS;
>> +
>>         ret = -EFAULT;
>>         if ( copy_from_guest(&add, arg, 1) != 0 )
>>             break;
>> @@ -54,6 +57,9 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
>> arg)
>>     case PHYSDEVOP_pci_device_remove: {
>>         struct physdev_pci_device dev;
>> 
>> +        if ( !is_pci_passthrough_enabled() )
>> +            return -ENOSYS;
>> +
>>         ret = -EFAULT;
>>         if ( copy_from_guest(&dev, arg, 1) != 0 )
>>             break;
>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index 7dd9eb3dba..f2f86be9bc 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -19,14 +19,27 @@
>> 
>> #define pci_to_dev(pcidev) (&(pcidev)->arch.dev)
>> 
>> +extern bool_t pci_passthrough_enabled;
>> +
>> /* Arch pci dev struct */
>> struct arch_pci_dev {
>>     struct device dev;
>> };
>> 
>> +static always_inline bool is_pci_passthrough_enabled(void)
>> +{
>> +    return pci_passthrough_enabled;
>> +}
>> #else   /*!CONFIG_HAS_PCI*/
>> 
>> +#define pci_passthrough_enabled (false)
> 
> We don't need to define it, do we?

No need to define I will remove.
> 
>> struct arch_pci_dev { };
>> 
>> +static always_inline bool is_pci_passthrough_enabled(void)
>> +{
>> +    return false;
>> +}
>> +
>> #endif  /*!CONFIG_HAS_PCI*/
>> #endif /* __ARM_PCI_H__ */
>> diff --git a/xen/include/asm-x86/pci.h b/xen/include/asm-x86/pci.h
>> index cc05045e9c..0e160c6d01 100644
>> --- a/xen/include/asm-x86/pci.h
>> +++ b/xen/include/asm-x86/pci.h
>> @@ -32,4 +32,12 @@ bool_t pci_ro_mmcfg_decode(unsigned long mfn, unsigned 
>> int *seg,
>> extern int pci_mmcfg_config_num;
>> extern struct acpi_mcfg_allocation *pci_mmcfg_config;
>> 
>> +/*
>> + * Unlike ARM, PCI passthrough always enabled for x86.
>                                 ^ is

Ack.

Regards,
Rahul
> 
> 
>> + */
>> +static always_inline bool is_pci_passthrough_enabled(void)
>> +{
>> +    return true;
>> +}
>> +
>> #endif /* __X86_PCI_H__ */
>> -- 
>> 2.17.1