* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-01 9:29 ` [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar Rahul Singh
@ 2022-09-01 17:08 ` Oleksandr
2022-09-03 0:24 ` Stefano Stabellini
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Oleksandr @ 2022-09-01 17:08 UTC (permalink / raw)
To: Rahul Singh, xen-devel
Cc: bertrand.marquis, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant
On 01.09.22 12:29, Rahul Singh wrote:
Hello Rahul
> 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@arm.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Thanks!
> ---
> Changes in v3:
> - fix minor comments
> ---
> xen/arch/arm/include/asm/pci.h | 2 ++
> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++
> xen/arch/x86/include/asm/pci.h | 10 +++++++
> xen/drivers/passthrough/pci.c | 8 +++---
> 4 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 80a2431804..8cb46f6b71 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -126,6 +126,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 89ef30028e..0eb121666d 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -24,6 +24,16 @@
>
> #include <asm/setup.h>
>
> +/*
> + * struct to hold pci device bar.
> + */
> +struct pdev_bar
> +{
> + mfn_t start;
> + mfn_t end;
> + bool is_valid;
> +};
> +
> /*
> * List for all the pci host bridges.
> */
> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
> return 0;
> }
>
> +static int is_bar_valid(const struct dt_device_node *dev,
> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
> + bar_data->is_valid = true;
> +
> + return 0;
> +}
> +
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> +{
> + int ret;
> + const struct dt_device_node *dt_node;
> + struct pdev_bar bar_data = {
> + .start = start,
> + .end = end,
> + .is_valid = false
> + };
> +
> + dt_node = pci_find_host_bridge_node(pdev);
> + if ( !dt_node )
> + return false;
> +
> + ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
> + if ( ret < 0 )
> + return false;
> +
> + return bar_data.is_valid;
> +}
> /*
> * 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);
> +}
> +
> #endif /* __X86_PCI_H__ */
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index cdaf5c247f..149f68bb6e 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -304,8 +304,8 @@ static void check_pdev(const struct pci_dev *pdev)
> if ( rc < 0 )
> /* Unable to size, better leave memory decoding disabled. */
> return;
> - if ( size && !is_memory_hole(maddr_to_mfn(addr),
> - maddr_to_mfn(addr + size - 1)) )
> + if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
> + maddr_to_mfn(addr + size - 1)) )
> {
> /*
> * Return without enabling memory decoding if BAR position is not
> @@ -331,8 +331,8 @@ static void check_pdev(const struct pci_dev *pdev)
>
> if ( rc < 0 )
> return;
> - if ( size && !is_memory_hole(maddr_to_mfn(addr),
> - maddr_to_mfn(addr + size - 1)) )
> + if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
> + maddr_to_mfn(addr + size - 1)) )
> {
> printk(warn, &pdev->sbdf, "ROM ", PFN_DOWN(addr),
> PFN_DOWN(addr + size - 1));
--
Regards,
Oleksandr Tyshchenko
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-01 9:29 ` [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar Rahul Singh
2022-09-01 17:08 ` Oleksandr
@ 2022-09-03 0:24 ` Stefano Stabellini
2022-09-06 8:53 ` Jan Beulich
2022-09-03 7:18 ` Julien Grall
2022-09-06 8:54 ` Jan Beulich
3 siblings, 1 reply; 16+ messages in thread
From: Stefano Stabellini @ 2022-09-03 0:24 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, bertrand.marquis, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant
On Thu, 1 Sep 2022, Rahul Singh wrote:
> 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@arm.com>
> ---
> Changes in v3:
> - fix minor comments
> ---
> xen/arch/arm/include/asm/pci.h | 2 ++
> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++
> xen/arch/x86/include/asm/pci.h | 10 +++++++
> xen/drivers/passthrough/pci.c | 8 +++---
> 4 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 80a2431804..8cb46f6b71 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -126,6 +126,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 89ef30028e..0eb121666d 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -24,6 +24,16 @@
>
> #include <asm/setup.h>
>
> +/*
> + * struct to hold pci device bar.
> + */
> +struct pdev_bar
> +{
> + mfn_t start;
> + mfn_t end;
> + bool is_valid;
> +};
> +
> /*
> * List for all the pci host bridges.
> */
> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
> return 0;
> }
>
> +static int is_bar_valid(const struct dt_device_node *dev,
> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
> + bar_data->is_valid = true;
This patch looks good and you addressed all Jan's comment well. Before I
ack it, one question.
I know that you made this change to address Jan's comment but using
PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
PFN_UP(addr + len - 1)) check means that we are relaxing the
requirements, aren't we?
I know that this discussion is a bit pointless because addr and len should
always be page aligned, and if they weren't it would be a mistake. But
assuming that they are not page aligned, wouldn't we want this check to
be a strict as possible?
Wouldn't we want to ensure that the [s,e] range is a strict subset of
[addr,addr+len-1] ? If so we would need to do the following instead:
if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) )
bar_data->is_valid = true;
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-03 0:24 ` Stefano Stabellini
@ 2022-09-06 8:53 ` Jan Beulich
2022-09-07 8:58 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2022-09-06 8:53 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, bertrand.marquis, Julien Grall, Volodymyr Babchuk,
Andrew Cooper, Roger Pau Monné, Wei Liu, Paul Durrant,
Rahul Singh
On 03.09.2022 02:24, Stefano Stabellini wrote:
> On Thu, 1 Sep 2022, Rahul Singh wrote:
>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>> return 0;
>> }
>>
>> +static int is_bar_valid(const struct dt_device_node *dev,
>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>> + bar_data->is_valid = true;
>
>
> This patch looks good and you addressed all Jan's comment well. Before I
> ack it, one question.
>
> I know that you made this change to address Jan's comment but using
> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
> PFN_UP(addr + len - 1)) check means that we are relaxing the
> requirements, aren't we?
>
> I know that this discussion is a bit pointless because addr and len should
> always be page aligned, and if they weren't it would be a mistake. But
> assuming that they are not page aligned, wouldn't we want this check to
> be a strict as possible?
>
> Wouldn't we want to ensure that the [s,e] range is a strict subset of
> [addr,addr+len-1] ? If so we would need to do the following instead:
>
> if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) )
> bar_data->is_valid = true;
But that might mean (in theory at least) a partial overlap, which has
to be avoided. The only alternative that I see to Rahul's original
code is to omit use of PFN_DOWN() and PFN_UP() in this construct
altogether. Assuming that's correct for the passed in (addr,len)
tuple.
Jan
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-06 8:53 ` Jan Beulich
@ 2022-09-07 8:58 ` Julien Grall
2022-09-07 9:07 ` Jan Beulich
0 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2022-09-07 8:58 UTC (permalink / raw)
To: Jan Beulich, Stefano Stabellini
Cc: xen-devel, bertrand.marquis, Volodymyr Babchuk, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant, Rahul Singh
Hi Jan & Stefano,
On 06/09/2022 09:53, Jan Beulich wrote:
> On 03.09.2022 02:24, Stefano Stabellini wrote:
>> On Thu, 1 Sep 2022, Rahul Singh wrote:
>>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>> return 0;
>>> }
>>>
>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>>> + bar_data->is_valid = true;
>>
>>
>> This patch looks good and you addressed all Jan's comment well. Before I
>> ack it, one question.
>>
>> I know that you made this change to address Jan's comment but using
>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
>> PFN_UP(addr + len - 1)) check means that we are relaxing the
>> requirements, aren't we?
>>
>> I know that this discussion is a bit pointless because addr and len should
>> always be page aligned, and if they weren't it would be a mistake.
Hmmm.... Is that requirement written down somewhere? The reason I am
asking is "page-aligned" will vary depending on the software. In the
past we had a couple of cases where the region would be 4KB-aligned but
not necessarily 64KB-aligned.
If the answer is no to my question then...
> But
>> assuming that they are not page aligned, wouldn't we want this check to
>> be a strict as possible?
>>
>> Wouldn't we want to ensure that the [s,e] range is a strict subset of
>> [addr,addr+len-1] ? If so we would need to do the following instead:
>>
>> if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) )
>> bar_data->is_valid = true;
>
> But that might mean (in theory at least) a partial overlap, which has
> to be avoided. The only alternative that I see to Rahul's original
> code is to omit use of PFN_DOWN() and PFN_UP() in this construct
> altogether. Assuming that's correct for the passed in (addr,len)
> tuple.
... I think we would want to drop PFN_DOWN/PFN_UP here.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-07 8:58 ` Julien Grall
@ 2022-09-07 9:07 ` Jan Beulich
2022-09-07 10:00 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2022-09-07 9:07 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, bertrand.marquis, Volodymyr Babchuk, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant, Rahul Singh,
Stefano Stabellini
On 07.09.2022 10:58, Julien Grall wrote:
> Hi Jan & Stefano,
>
> On 06/09/2022 09:53, Jan Beulich wrote:
>> On 03.09.2022 02:24, Stefano Stabellini wrote:
>>> On Thu, 1 Sep 2022, Rahul Singh wrote:
>>>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>>> return 0;
>>>> }
>>>>
>>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>>>> + bar_data->is_valid = true;
>>>
>>>
>>> This patch looks good and you addressed all Jan's comment well. Before I
>>> ack it, one question.
>>>
>>> I know that you made this change to address Jan's comment but using
>>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
>>> PFN_UP(addr + len - 1)) check means that we are relaxing the
>>> requirements, aren't we?
>>>
>>> I know that this discussion is a bit pointless because addr and len should
>>> always be page aligned, and if they weren't it would be a mistake.
>
> Hmmm.... Is that requirement written down somewhere?
What do you mean here? Isn't it quite obvious that every byte in the
address space may only be used for a single purpose? I.e. if a byte
is covered by a BAR, it cannot also be covered by a RAM region or
yet something else (e.g. MMIO beyond BARs of PCI devices). What
happens if BAR and RAM indeed overlap depends on fabric and chipset,
but it'll either result in chaos if two parties respond to a single
request on the bus, or it'll be (hopefully) deterministic (for any
individual system) which of the two takes "precedence".
I think we've had a similar discussion a little while ago already in
the context of vPCI with guest address space in mind. The same (imo
obvious) "rule" spelled out above applies there and here.
Jan
> The reason I am
> asking is "page-aligned" will vary depending on the software. In the
> past we had a couple of cases where the region would be 4KB-aligned but
> not necessarily 64KB-aligned.
>
> If the answer is no to my question then...
>
>> But
>>> assuming that they are not page aligned, wouldn't we want this check to
>>> be a strict as possible?
>>>
>>> Wouldn't we want to ensure that the [s,e] range is a strict subset of
>>> [addr,addr+len-1] ? If so we would need to do the following instead:
>>>
>>> if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) )
>>> bar_data->is_valid = true;
>>
>> But that might mean (in theory at least) a partial overlap, which has
>> to be avoided. The only alternative that I see to Rahul's original
>> code is to omit use of PFN_DOWN() and PFN_UP() in this construct
>> altogether. Assuming that's correct for the passed in (addr,len)
>> tuple.
>
> ... I think we would want to drop PFN_DOWN/PFN_UP here.
>
> Cheers,
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-07 9:07 ` Jan Beulich
@ 2022-09-07 10:00 ` Julien Grall
2022-09-07 12:01 ` Jan Beulich
0 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2022-09-07 10:00 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, bertrand.marquis, Volodymyr Babchuk, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant, Rahul Singh,
Stefano Stabellini
Hi Jan,
On 07/09/2022 10:07, Jan Beulich wrote:
> On 07.09.2022 10:58, Julien Grall wrote:
>> Hi Jan & Stefano,
>>
>> On 06/09/2022 09:53, Jan Beulich wrote:
>>> On 03.09.2022 02:24, Stefano Stabellini wrote:
>>>> On Thu, 1 Sep 2022, Rahul Singh wrote:
>>>>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>>>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>>>>> + bar_data->is_valid = true;
>>>>
>>>>
>>>> This patch looks good and you addressed all Jan's comment well. Before I
>>>> ack it, one question.
>>>>
>>>> I know that you made this change to address Jan's comment but using
>>>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
>>>> PFN_UP(addr + len - 1)) check means that we are relaxing the
>>>> requirements, aren't we?
>>>>
>>>> I know that this discussion is a bit pointless because addr and len should
>>>> always be page aligned, and if they weren't it would be a mistake.
>>
>> Hmmm.... Is that requirement written down somewhere?
>
> What do you mean here? Isn't it quite obvious that every byte in the
> address space may only be used for a single purpose? I.e. if a byte
> is covered by a BAR, it cannot also be covered by a RAM region or
> yet something else (e.g. MMIO beyond BARs of PCI devices). What
> happens if BAR and RAM indeed overlap depends on fabric and chipset,
> but it'll either result in chaos if two parties respond to a single
> request on the bus, or it'll be (hopefully) deterministic (for any
> individual system) which of the two takes "precedence".
I am well aware about that and I am not sure how you implied this is
what I was referring to from what I wrote (in particular if you read the
next sentence).
Stefano wrote that it would be a mistake if the address/length is not
page-aligned. However, I am not aware from such requirement written
down. It seems to be more an expected common sense that was IIRC not
always respected on HW supporting multiple page-granularity.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-07 10:00 ` Julien Grall
@ 2022-09-07 12:01 ` Jan Beulich
2022-09-07 12:47 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2022-09-07 12:01 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, bertrand.marquis, Volodymyr Babchuk, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant, Rahul Singh,
Stefano Stabellini
On 07.09.2022 12:00, Julien Grall wrote:
> On 07/09/2022 10:07, Jan Beulich wrote:
>> On 07.09.2022 10:58, Julien Grall wrote:
>>> On 06/09/2022 09:53, Jan Beulich wrote:
>>>> On 03.09.2022 02:24, Stefano Stabellini wrote:
>>>>> On Thu, 1 Sep 2022, Rahul Singh wrote:
>>>>>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>>>>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>>>>>> + bar_data->is_valid = true;
>>>>>
>>>>>
>>>>> This patch looks good and you addressed all Jan's comment well. Before I
>>>>> ack it, one question.
>>>>>
>>>>> I know that you made this change to address Jan's comment but using
>>>>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
>>>>> PFN_UP(addr + len - 1)) check means that we are relaxing the
>>>>> requirements, aren't we?
>>>>>
>>>>> I know that this discussion is a bit pointless because addr and len should
>>>>> always be page aligned, and if they weren't it would be a mistake.
>>>
>>> Hmmm.... Is that requirement written down somewhere?
>>
>> What do you mean here? Isn't it quite obvious that every byte in the
>> address space may only be used for a single purpose? I.e. if a byte
>> is covered by a BAR, it cannot also be covered by a RAM region or
>> yet something else (e.g. MMIO beyond BARs of PCI devices). What
>> happens if BAR and RAM indeed overlap depends on fabric and chipset,
>> but it'll either result in chaos if two parties respond to a single
>> request on the bus, or it'll be (hopefully) deterministic (for any
>> individual system) which of the two takes "precedence".
>
> I am well aware about that and I am not sure how you implied this is
> what I was referring to from what I wrote (in particular if you read the
> next sentence).
>
> Stefano wrote that it would be a mistake if the address/length is not
> page-aligned. However, I am not aware from such requirement written
> down. It seems to be more an expected common sense that was IIRC not
> always respected on HW supporting multiple page-granularity.
I guess the question was then solely directed at Stefano? I have no
idea, after all, what guarantees there are for addr and len, which
presumably originate from DT somewhere.
Yet if there aren't any guarantees, then aligning the incoming range
may be necessary: BARs can be sub-page size (even more so when
considering page sizes above 4k), and so a not-really-overlap (at
byte granularity) could still be an issue if there's an overlap at
4k or system page size granularity.
Jan
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-07 12:01 ` Jan Beulich
@ 2022-09-07 12:47 ` Julien Grall
0 siblings, 0 replies; 16+ messages in thread
From: Julien Grall @ 2022-09-07 12:47 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, bertrand.marquis, Volodymyr Babchuk, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant, Rahul Singh,
Stefano Stabellini
On 07/09/2022 13:01, Jan Beulich wrote:
> On 07.09.2022 12:00, Julien Grall wrote:
>> On 07/09/2022 10:07, Jan Beulich wrote:
>>> On 07.09.2022 10:58, Julien Grall wrote:
>>>> On 06/09/2022 09:53, Jan Beulich wrote:
>>>>> On 03.09.2022 02:24, Stefano Stabellini wrote:
>>>>>> On Thu, 1 Sep 2022, Rahul Singh wrote:
>>>>>>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>>>>>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>>>>>>> + bar_data->is_valid = true;
>>>>>>
>>>>>>
>>>>>> This patch looks good and you addressed all Jan's comment well. Before I
>>>>>> ack it, one question.
>>>>>>
>>>>>> I know that you made this change to address Jan's comment but using
>>>>>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
>>>>>> PFN_UP(addr + len - 1)) check means that we are relaxing the
>>>>>> requirements, aren't we?
>>>>>>
>>>>>> I know that this discussion is a bit pointless because addr and len should
>>>>>> always be page aligned, and if they weren't it would be a mistake.
>>>>
>>>> Hmmm.... Is that requirement written down somewhere?
>>>
>>> What do you mean here? Isn't it quite obvious that every byte in the
>>> address space may only be used for a single purpose? I.e. if a byte
>>> is covered by a BAR, it cannot also be covered by a RAM region or
>>> yet something else (e.g. MMIO beyond BARs of PCI devices). What
>>> happens if BAR and RAM indeed overlap depends on fabric and chipset,
>>> but it'll either result in chaos if two parties respond to a single
>>> request on the bus, or it'll be (hopefully) deterministic (for any
>>> individual system) which of the two takes "precedence".
>>
>> I am well aware about that and I am not sure how you implied this is
>> what I was referring to from what I wrote (in particular if you read the
>> next sentence).
>>
>> Stefano wrote that it would be a mistake if the address/length is not
>> page-aligned. However, I am not aware from such requirement written
>> down. It seems to be more an expected common sense that was IIRC not
>> always respected on HW supporting multiple page-granularity.
>
> I guess the question was then solely directed at Stefano?
This question yes. The rest was a reply to your suggestion. I will wait
for Stefano to answer.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-01 9:29 ` [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar Rahul Singh
2022-09-01 17:08 ` Oleksandr
2022-09-03 0:24 ` Stefano Stabellini
@ 2022-09-03 7:18 ` Julien Grall
2022-09-06 9:39 ` Rahul Singh
2022-09-06 8:54 ` Jan Beulich
3 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2022-09-03 7:18 UTC (permalink / raw)
To: Rahul Singh, xen-devel
Cc: bertrand.marquis, Stefano Stabellini, Volodymyr Babchuk,
Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu,
Paul Durrant
Hi Rahul,
On 01/09/2022 10:29, Rahul Singh wrote:
> 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@arm.com>
> ---
> Changes in v3:
> - fix minor comments
> ---
> xen/arch/arm/include/asm/pci.h | 2 ++
> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++
> xen/arch/x86/include/asm/pci.h | 10 +++++++
> xen/drivers/passthrough/pci.c | 8 +++---
> 4 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 80a2431804..8cb46f6b71 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -126,6 +126,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 89ef30028e..0eb121666d 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -24,6 +24,16 @@
>
> #include <asm/setup.h>
>
> +/*
> + * struct to hold pci device bar.
> + */
I find this comment a bit misleading. What you are storing is a
candidate region. IOW, this may or may not be a PCI device bar.
Given the current use below, I would rename the structure to something
more specific like: pdev_bar_check.
> +struct pdev_bar
> +{
> + mfn_t start;
> + mfn_t end;
> + bool is_valid;
> +};
> +
> /*
> * List for all the pci host bridges.
> */
> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
> return 0;
> }
>
> +static int is_bar_valid(const struct dt_device_node *dev,
> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
AFAICT 's' and 'e' are provided by pci_check_bar() and will never
change. So can we move the check 's <= e' outside of the callback?
> + bar_data->is_valid = true;
> +
> + return 0;
> +}
> +
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> +{
Other than the current calls in check_pdev(), do you have plan to use it
in more places? The reason I am asking it is this function is
non-trivial on Arm (dt_for_each_range() is quite complex).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-03 7:18 ` Julien Grall
@ 2022-09-06 9:39 ` Rahul Singh
2022-09-06 9:53 ` Julien Grall
0 siblings, 1 reply; 16+ messages in thread
From: Rahul Singh @ 2022-09-06 9:39 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Bertrand Marquis, Stefano Stabellini,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant
Hi Julien,
> On 3 Sep 2022, at 8:18 am, Julien Grall <julien@xen.org> wrote:
>
> Hi Rahul,
>
> On 01/09/2022 10:29, Rahul Singh wrote:
>> 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@arm.com>
>> ---
>> Changes in v3:
>> - fix minor comments
>> ---
>> xen/arch/arm/include/asm/pci.h | 2 ++
>> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++
>> xen/arch/x86/include/asm/pci.h | 10 +++++++
>> xen/drivers/passthrough/pci.c | 8 +++---
>> 4 files changed, 59 insertions(+), 4 deletions(-)
>> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
>> index 80a2431804..8cb46f6b71 100644
>> --- a/xen/arch/arm/include/asm/pci.h
>> +++ b/xen/arch/arm/include/asm/pci.h
>> @@ -126,6 +126,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 89ef30028e..0eb121666d 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -24,6 +24,16 @@
>> #include <asm/setup.h>
>> +/*
>> + * struct to hold pci device bar.
>> + */
>
> I find this comment a bit misleading. What you are storing is a
> candidate region. IOW, this may or may not be a PCI device bar.
>
> Given the current use below, I would rename the structure to something more specific like: pdev_bar_check.
Ack.
>
>> +struct pdev_bar
>> +{
>> + mfn_t start;
>> + mfn_t end;
>> + bool is_valid;
>> +};
>> +
>> /*
>> * List for all the pci host bridges.
>> */
>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>> return 0;
>> }
>> +static int is_bar_valid(const struct dt_device_node *dev,
>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>
> AFAICT 's' and 'e' are provided by pci_check_bar() and will never change. So can we move the check 's <= e' outside of the callback?
Yes, We can move the check outside the callback but I feel that if we check here then it is more
readable that we are checking for all possible values in one statement. Let me know your view on this.
>
>> + bar_data->is_valid = true;
>> +
>> + return 0;
>> +}
>> +
>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>> +{
>
> Other than the current calls in check_pdev(), do you have plan to use it in more places? The reason I am asking it is this function is non-trivial on Arm (dt_for_each_range() is quite complex).
I don’t see any use of this function in more places. As this function will be called during dom0 boot when the PCI devices are
added I don’t see any performance issues. We may need to revisit this function when we add ACPI PCI passthrough support.
I will add TODO that we need to revisit this function for ACPI PCI passthrough support.
Regards,
Rahul
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-06 9:39 ` Rahul Singh
@ 2022-09-06 9:53 ` Julien Grall
2022-09-07 8:15 ` Rahul Singh
0 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2022-09-06 9:53 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, Bertrand Marquis, Stefano Stabellini,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant
On 06/09/2022 10:39, Rahul Singh wrote:
> Hi Julien,
>
>> On 3 Sep 2022, at 8:18 am, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Rahul,
>>
>> On 01/09/2022 10:29, Rahul Singh wrote:
>>> 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@arm.com>
>>> ---
>>> Changes in v3:
>>> - fix minor comments
>>> ---
>>> xen/arch/arm/include/asm/pci.h | 2 ++
>>> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++
>>> xen/arch/x86/include/asm/pci.h | 10 +++++++
>>> xen/drivers/passthrough/pci.c | 8 +++---
>>> 4 files changed, 59 insertions(+), 4 deletions(-)
>>> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
>>> index 80a2431804..8cb46f6b71 100644
>>> --- a/xen/arch/arm/include/asm/pci.h
>>> +++ b/xen/arch/arm/include/asm/pci.h
>>> @@ -126,6 +126,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 89ef30028e..0eb121666d 100644
>>> --- a/xen/arch/arm/pci/pci-host-common.c
>>> +++ b/xen/arch/arm/pci/pci-host-common.c
>>> @@ -24,6 +24,16 @@
>>> #include <asm/setup.h>
>>> +/*
>>> + * struct to hold pci device bar.
>>> + */
>>
>> I find this comment a bit misleading. What you are storing is a
>> candidate region. IOW, this may or may not be a PCI device bar.
>>
>> Given the current use below, I would rename the structure to something more specific like: pdev_bar_check.
>
> Ack.
>>
>>> +struct pdev_bar
>>> +{
>>> + mfn_t start;
>>> + mfn_t end;
>>> + bool is_valid;
>>> +};
>>> +
>>> /*
>>> * List for all the pci host bridges.
>>> */
>>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>> return 0;
>>> }
>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>>
>> AFAICT 's' and 'e' are provided by pci_check_bar() and will never change. So can we move the check 's <= e' outside of the callback?
>
> Yes, We can move the check outside the callback but I feel that if we check here then it is more
> readable that we are checking for all possible values in one statement. Let me know your view on this.
The readability is really a matter of taste here. But my point is more
on the number of time a check is done.
It seems pointless to do the same check N times when you know the values
are not going to change. Admittedly, the operation is fast (this is a
comparison) and N should be small (?).
However, I think it raises the question on where do you draw the line?
Personally, I think all invariant should be checked outside of
callbacks. So the line is very clear.
>
>>
>>> + bar_data->is_valid = true;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>>> +{
>>
>> Other than the current calls in check_pdev(), do you have plan to use it in more places? The reason I am asking it is this function is non-trivial on Arm (dt_for_each_range() is quite complex).
>
> I don’t see any use of this function in more places. As this function will be called during dom0 boot when the PCI devices are
> added I don’t see any performance issues. We may need to revisit this function when we add ACPI PCI passthrough support.
> I will add TODO that we need to revisit this function for ACPI PCI passthrough support.
Thanks.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-06 9:53 ` Julien Grall
@ 2022-09-07 8:15 ` Rahul Singh
0 siblings, 0 replies; 16+ messages in thread
From: Rahul Singh @ 2022-09-07 8:15 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Bertrand Marquis, Stefano Stabellini,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu, Paul Durrant
Hi Julien,
> On 6 Sep 2022, at 10:53 am, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 06/09/2022 10:39, Rahul Singh wrote:
>> Hi Julien,
>>> On 3 Sep 2022, at 8:18 am, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Rahul,
>>>
>>> On 01/09/2022 10:29, Rahul Singh wrote:
>>>> 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@arm.com>
>>>> ---
>>>> Changes in v3:
>>>> - fix minor comments
>>>> ---
>>>> xen/arch/arm/include/asm/pci.h | 2 ++
>>>> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++
>>>> xen/arch/x86/include/asm/pci.h | 10 +++++++
>>>> xen/drivers/passthrough/pci.c | 8 +++---
>>>> 4 files changed, 59 insertions(+), 4 deletions(-)
>>>> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
>>>> index 80a2431804..8cb46f6b71 100644
>>>> --- a/xen/arch/arm/include/asm/pci.h
>>>> +++ b/xen/arch/arm/include/asm/pci.h
>>>> @@ -126,6 +126,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 89ef30028e..0eb121666d 100644
>>>> --- a/xen/arch/arm/pci/pci-host-common.c
>>>> +++ b/xen/arch/arm/pci/pci-host-common.c
>>>> @@ -24,6 +24,16 @@
>>>> #include <asm/setup.h>
>>>> +/*
>>>> + * struct to hold pci device bar.
>>>> + */
>>>
>>> I find this comment a bit misleading. What you are storing is a
>>> candidate region. IOW, this may or may not be a PCI device bar.
>>>
>>> Given the current use below, I would rename the structure to something more specific like: pdev_bar_check.
>> Ack.
>>>
>>>> +struct pdev_bar
>>>> +{
>>>> + mfn_t start;
>>>> + mfn_t end;
>>>> + bool is_valid;
>>>> +};
>>>> +
>>>> /*
>>>> * List for all the pci host bridges.
>>>> */
>>>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>>> return 0;
>>>> }
>>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>>> + uint64_t addr, uint64_t 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_UP(addr + len - 1)) )
>>>
>>> AFAICT 's' and 'e' are provided by pci_check_bar() and will never change. So can we move the check 's <= e' outside of the callback?
>> Yes, We can move the check outside the callback but I feel that if we check here then it is more
>> readable that we are checking for all possible values in one statement. Let me know your view on this.
> The readability is really a matter of taste here. But my point is more on the number of time a check is done.
>
> It seems pointless to do the same check N times when you know the values are not going to change. Admittedly, the operation is fast (this is a comparison) and N should be small (?).
>
> However, I think it raises the question on where do you draw the line?
>
> Personally, I think all invariant should be checked outside of callbacks. So the line is very clear.
>
I will move the check for "s <=e” outside the callback and will send it for review.
Regards,
Rahul
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar
2022-09-01 9:29 ` [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar Rahul Singh
` (2 preceding siblings ...)
2022-09-03 7:18 ` Julien Grall
@ 2022-09-06 8:54 ` Jan Beulich
3 siblings, 0 replies; 16+ messages in thread
From: Jan Beulich @ 2022-09-06 8:54 UTC (permalink / raw)
To: Rahul Singh
Cc: bertrand.marquis, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, Roger Pau Monné, Wei Liu,
Paul Durrant, xen-devel
On 01.09.2022 11:29, Rahul Singh wrote:
> 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@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 16+ messages in thread