Re: [PATCH] xen/pci: replace call to is_memory_hole to pci_check_bar

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Oleksandr,


> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@xxxxxxxxx> wrote:
> 
> 
> On 05.08.22 18:43, Rahul Singh wrote:
> 
> 
> Hello Rahul
> 
> 
> Thank you very much for that patch!
> 
> 
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> I am not 100% sure regarding that. This is *completely* different patch from 
> what Oleksandr initially made here:
> 
> https://lore.kernel.org/xen-devel/20220719174253.541965-2-olekstysh@xxxxxxxxx/
> 
> Copy below for the convenience:
> 
> 
> +bool is_memory_hole(mfn_t start, mfn_t end)
> +{
> +    /* TODO: this needs to be properly implemented. */
> +    return true;
> +}
> 
> 
> 
> 
> Patch looks good, just a couple of minor comments/nits.

Ok. I will remove “From: … “ in next version.
> 
>> 
>> is_memory_hole was implemented for x86 and not for ARM when introduced.
>> Replace is_memory_hole call to pci_check_bar as function should check
>> if device BAR is in defined memory range. Also, add an implementation
>> for ARM which is required for PCI passthrough.
>> 
>> On x86, pci_check_bar will call is_memory_hole which will check if BAR
>> is not overlapping with any memory region defined in the memory map.
>> 
>> On ARM, pci_check_bar will go through the host bridge ranges and check
>> if the BAR is in the range of defined ranges.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> ---
>>  xen/arch/arm/include/asm/pci.h     | 12 ++++++++++
>>  xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
>>  xen/arch/x86/include/asm/pci.h     | 10 +++++++++
>>  xen/drivers/passthrough/pci.c      |  8 +++----
>>  4 files changed, 61 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
>> index 7c7449d64f..5c4ab2c4dc 100644
>> --- a/xen/arch/arm/include/asm/pci.h
>> +++ b/xen/arch/arm/include/asm/pci.h
>> @@ -91,6 +91,16 @@ struct pci_ecam_ops {
>>      int (*init)(struct pci_config_window *);
>>  };
>>  +/*
>> + * struct to hold pci device bar.
>> + */
>> +struct pdev_bar
>> +{
>> +    mfn_t start;
>> +    mfn_t end;
>> +    bool is_valid;
>> +};
> 
> 
> Nit: This is only used by pci-host-common.c, so I think it could be declared 
> there.

Ack.
> 
> 
> 
>> +
>>  /* Default ECAM ops */
>>  extern const struct pci_ecam_ops pci_generic_ecam_ops;
>>  @@ -125,6 +135,8 @@ int pci_host_iterate_bridges_and_count(struct domain *d,
>>    int pci_host_bridge_mappings(struct domain *d);
>>  +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
>> +
>>  #else   /*!CONFIG_HAS_PCI*/
>>    struct arch_pci_dev { };
>> diff --git a/xen/arch/arm/pci/pci-host-common.c 
>> b/xen/arch/arm/pci/pci-host-common.c
>> index fd8c0f837a..8ea1aaeece 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -363,6 +363,41 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>      return 0;
>>  }
>>  +static int is_bar_valid(const struct dt_device_node *dev,
>> +                        u64 addr, u64 len, void *data)
>> +{
>> +    struct pdev_bar *bar_data = data;
>> +    unsigned long s = mfn_x(bar_data->start);
>> +    unsigned long e = mfn_x(bar_data->end);
>> +
>> +    if ( (s < e) && (s >= PFN_DOWN(addr)) && (e<= PFN_DOWN(addr + len - 1)) 
>> )
> 
> 
> Nit: white space after 'e' is missed in the last part of the check

Ack.

> 
> 
>> +        bar_data->is_valid =  true;
>> +
>> +    return 0;
>> +}
>> +
>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>> +{
>> +    int ret;
>> +    struct dt_device_node *dt_node;
>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
> 
> 
> The cast is present here because of the const?

Yes you are right, cast is because of the const.
> 
> I would consider passing "const struct pci_dev *pdev" instead of "struct 
> device *dev" to pci_find_host_bridge_node() and dropping conversion 
> (pci<->dev) in both functions.

Yes make sense. I will do that in next version.
> 
> 
> Something like below (not tested):
> 
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 5c4ab2c4dc..a17ef32252 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -116,7 +116,7 @@ bool pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
>                                       struct pci_host_bridge *bridge,
>                                       uint64_t addr);
>  struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
> -struct dt_device_node *pci_find_host_bridge_node(struct device *dev);
> +struct dt_device_node *pci_find_host_bridge_node(const struct pci_dev *pdev);
>  int pci_get_host_bridge_segment(const struct dt_device_node *node,
>                                  uint16_t *segment);
> 
> diff --git a/xen/arch/arm/pci/pci-host-common.c 
> b/xen/arch/arm/pci/pci-host-common.c
> index 8ea1aaeece..3a64a7350f 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -243,10 +243,9 @@ err_exit:
>  /*
>   * Get host bridge node given a device attached to it.
>   */
> -struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
> +struct dt_device_node *pci_find_host_bridge_node(const struct pci_dev *pdev)
>  {
>      struct pci_host_bridge *bridge;
> -    struct pci_dev *pdev = dev_to_pci(dev);
> 
>      bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
>      if ( unlikely(!bridge) )
> @@ -380,14 +379,13 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t 
> start, mfn_t end)
>  {
>      int ret;
>      struct dt_device_node *dt_node;
> -    struct device *dev = (struct device *)pci_to_dev(pdev);
>      struct pdev_bar bar_data =  {
>          .start = start,
>          .end = end,
>          .is_valid = false
>      };
> 
> -    dt_node = pci_find_host_bridge_node(dev);
> +    dt_node = pci_find_host_bridge_node(pdev);
> 
>      ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
>      if ( ret < 0 )
> 
> 
>> +    struct pdev_bar bar_data =  {
>> +        .start = start,
>> +        .end = end,
>> +        .is_valid = false
>> +    };
>> +
>> +    dt_node = pci_find_host_bridge_node(dev);
> 
>     if ( !dt_node )
>         return false;

Ack. 
> 
> 
>> +
>> +    ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
>> +    if ( ret < 0 )
>> +        return ret;
> 
> s/return ret;/return false;

Ack. 
> 
> 
>> +
>> +    if ( !bar_data.is_valid )
>> +        return false;
>> +
>> +    return true;
>> +}
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
>> index c8e1a9ecdb..f4a58c8acf 100644
>> --- a/xen/arch/x86/include/asm/pci.h
>> +++ b/xen/arch/x86/include/asm/pci.h
>> @@ -57,4 +57,14 @@ static always_inline bool is_pci_passthrough_enabled(void)
>>    void arch_pci_init_pdev(struct pci_dev *pdev);
>>  +static inline bool pci_check_bar(const struct pci_dev *pdev,
>> +                                 mfn_t start, mfn_t end)
>> +{
>> +    /*
>> +     * Check if BAR is not overlapping with any memory region defined
>> +     * in the memory map.
>> +     */
>> +    return is_memory_hole(start, end);
>> +}
> 
> 
> Nit: I would use simple #define instead of static inline here
> 
> But I am not 100% sure that x86 maintainers would be happy.
> 

Jan replied to this and I will check what is suggested by Jan.

Regards,
Rahul