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(-)
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
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>
---
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;
+};
+
/* 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)) )
+ 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);
+ struct pdev_bar bar_data = {
+ .start = start,
+ .end = end,
+ .is_valid = false
+ };
+
+ dt_node = pci_find_host_bridge_node(dev);
+
+ ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
+ if ( ret < 0 )
+ return ret;
+
+ 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);
+}
+
#endif /* __X86_PCI_H__ */
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 29356d59a7..52453a05fe 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));
--
2.25.1
On 05.08.22 18:43, Rahul Singh wrote: Hello Rahul Thank you very much for that patch! > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> 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@gmail.com/ 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. > > 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> > --- > 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. > + > /* 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 > + 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? 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. 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; > + > + ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data); > + if ( ret < 0 ) > + return ret; s/return ret;/return false; > + > + 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. > + > #endif /* __X86_PCI_H__ */ > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 29356d59a7..52453a05fe 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
Hi Oleksandr, > On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote: > > > On 05.08.22 18:43, Rahul Singh wrote: > > > Hello Rahul > > > Thank you very much for that patch! > > >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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@gmail.com/ > > 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@arm.com> >> --- >> 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
Hi Rahul, This patch seems to have been sent in-reply-to the SMMUv1 patch. Was it intended? On 09/08/2022 16:22, Rahul Singh wrote: >> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote: >>> +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. It looks like this function was added without any callers. The commit message claim there will be some. Can you (or Oleksandr) confirm this is not going to be problem for future patches? That said, I agree that the conversion pci -> dev -> pci is pointless. So I would say if there are use case where we only have a 'dev' in hand, then we could ask the caller to do the conversation or we provide an helper if there are too many cases. > > Yes make sense. I will do that in next version. While you are modifying the prototype for pci_find_host_bridge_node() can you consider to also constify the return (it should not be modified)? In any case, the change suggested by Oleksandr should preferably be separate to this patch and added before. Cheers, -- Julien Grall
Hi Julien, > On 9 Aug 2022, at 4:46 pm, Julien Grall <julien@xen.org> wrote: > > Hi Rahul, > > This patch seems to have been sent in-reply-to the SMMUv1 patch. Was it intended? That was by mistake I want to send all the patches independently but somehow I send it from single git "send-email” command because of that I think this patch comes in-reply-to SMMUv1 patch. > > On 09/08/2022 16:22, Rahul Singh wrote: >>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote: >>>> +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. > > It looks like this function was added without any callers. The commit message claim there will be some. Can you (or Oleksandr) confirm this is not going to be problem for future patches? I checked the whole PCI passthrough feature branch this function will be used when we add iommu support for PCI device. > > That said, I agree that the conversion pci -> dev -> pci is pointless. So I would say if there are use case where we only have a 'dev' in hand, then we could ask the caller to do the conversation or we provide an helper if there are too many cases. > >> Yes make sense. I will do that in next version. > > While you are modifying the prototype for pci_find_host_bridge_node() can you consider to also constify the return (it should not be modified)? Agree, I will constify the retrun also. > > In any case, the change suggested by Oleksandr should preferably be separate to this patch and added before. Ack. Regards, Rahul
On Tue, Aug 9, 2022 at 7:26 PM Rahul Singh <Rahul.Singh@arm.com> wrote: > Hi Julien, > Hello Julien, Rahul [sorry for possible format issues] [snip] > > > > > > On 09/08/2022 16:22, Rahul Singh wrote: > >>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote: > >>>> +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. > > > > It looks like this function was added without any callers. The commit > message claim there will be some. Can you (or Oleksandr) confirm this is > not going to be problem for future patches? > > I checked the whole PCI passthrough feature branch this function will be > used when > we add iommu support for PCI device. Can confirm that, it will be called by the iommu code, as I understand there won't be an issue, the more, the exact place where the pci_find_host_bridge_node() will be called will have "pdev" in hand. > > > > > > That said, I agree that the conversion pci -> dev -> pci is pointless. > So I would say if there are use case where we only have a 'dev' in hand, > then we could ask the caller to do the conversation or we provide an helper > if there are too many cases. > > > >> Yes make sense. I will do that in next version. > > > > While you are modifying the prototype for pci_find_host_bridge_node() > can you consider to also constify the return (it should not be modified)? > > Agree, I will constify the retrun also. > > > > > In any case, the change suggested by Oleksandr should preferably be > separate to this patch and added before. > > Ack. > > Regards, > Rahul > > -- Regards, Oleksandr Tyshchenko
On 08.08.2022 17:30, Oleksandr wrote: > On 05.08.22 18:43, Rahul Singh wrote: >> --- 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) Nit: No new uses of u64 please use uint64_t instead. >> --- 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. Quite the other way around - when possible we prefer inline functions. And note that the two functions are strictly aliases of one another (in which case a simplified #define pci_check_bar is_memory_hole might indeed have been worth a consideration, as there's no type safety to be lost in such cases). Jan
© 2016 - 2024 Red Hat, Inc.