| Message ID | 26fe963007e0a43b8fefd915027e90ddace1be73.1660746990.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/pci: implement is_memory_hole for ARM | expand |
On 17.08.2022 16:45, Rahul Singh wrote: > @@ -363,6 +373,42 @@ 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) s/u64/uint64_t/g please. > +{ > + 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_UP(addr)) && (e <= PFN_UP(addr + len - 1)) ) Doesn't this need to be s >= PFN_DOWN(addr)? Or else why is e checked against PFN_UP()? > + 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); Just as a side note - I find such (the first instance here) uses of the unary & operator odd. The same effect will be had without it. So all it does (in my opinion) is make things harder to read (just very slightly, of course). > + if ( ret < 0 ) > + return false; > + > + if ( !bar_data.is_valid ) > + return false; > + > + return true; Simply "return bar_data.is_valid;"? Jan
Hello Jan, > On 17 Aug 2022, at 4:18 pm, Jan Beulich <jbeulich@suse.com> wrote: > > On 17.08.2022 16:45, Rahul Singh wrote: >> @@ -363,6 +373,42 @@ 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) > > s/u64/uint64_t/g please. Ack. > >> +{ >> + 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_UP(addr)) && (e <= PFN_UP(addr + len - 1)) ) > > Doesn't this need to be s >= PFN_DOWN(addr)? Or else why is e checked > against PFN_UP()? Ack. I will modify as 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); > > Just as a side note - I find such (the first instance here) uses of the > unary & operator odd. The same effect will be had without it. So all it > does (in my opinion) is make things harder to read (just very slightly, > of course). I follow the same calling method as used in map_device_children() function in file arch/arm/domain_build.c > >> + if ( ret < 0 ) >> + return false; >> + >> + if ( !bar_data.is_valid ) >> + return false; >> + >> + return true; > > Simply "return bar_data.is_valid;"? Ack. Regards, Rahul
On 18.08.2022 16:58, Rahul Singh wrote: >> On 17 Aug 2022, at 4:18 pm, Jan Beulich <jbeulich@suse.com> wrote: >> On 17.08.2022 16:45, Rahul Singh wrote: >>> @@ -363,6 +373,42 @@ 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) >> >> s/u64/uint64_t/g please. > > Ack. >> >>> +{ >>> + struct pdev_bar *bar_data = data; const? >>> + unsigned long s = mfn_x(bar_data->start); >>> + unsigned long e = mfn_x(bar_data->end); >>> + >>> + if ( (s < e) && (s >= PFN_UP(addr)) && (e <= PFN_UP(addr + len - 1)) ) >> >> Doesn't this need to be s >= PFN_DOWN(addr)? Or else why is e checked >> against PFN_UP()? > > Ack. I will modify as if ( (s < e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) Hmm, doesn't it further need to be s <= e, seeing that the range passed to pci_check_bar() is an inclusive one? Jan
Hi Jan, > On 19 Aug 2022, at 7:07 am, Jan Beulich <jbeulich@suse.com> wrote: > > On 18.08.2022 16:58, Rahul Singh wrote: >>> On 17 Aug 2022, at 4:18 pm, Jan Beulich <jbeulich@suse.com> wrote: >>> On 17.08.2022 16:45, Rahul Singh wrote: >>>> @@ -363,6 +373,42 @@ 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) >>> >>> s/u64/uint64_t/g please. >> >> Ack. >>> >>>> +{ >>>> + struct pdev_bar *bar_data = data; > > const? Ack. > >>>> + unsigned long s = mfn_x(bar_data->start); >>>> + unsigned long e = mfn_x(bar_data->end); >>>> + >>>> + if ( (s < e) && (s >= PFN_UP(addr)) && (e <= PFN_UP(addr + len - 1)) ) >>> >>> Doesn't this need to be s >= PFN_DOWN(addr)? Or else why is e checked >>> against PFN_UP()? >> >> Ack. I will modify as if ( (s < e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) > > Hmm, doesn't it further need to be s <= e, seeing that the range passed > to pci_check_bar() is an inclusive one? Agree, I will do the modification in next version. Regards, Rahul
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..04f9d8580b 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,42 @@ 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_UP(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; + + 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 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));
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 v2: - fixed minor comment - modify pci_find_host_bridge_node argument to const pdev --- xen/arch/arm/include/asm/pci.h | 2 ++ xen/arch/arm/pci/pci-host-common.c | 46 ++++++++++++++++++++++++++++++ xen/arch/x86/include/asm/pci.h | 10 +++++++ xen/drivers/passthrough/pci.c | 8 +++--- 4 files changed, 62 insertions(+), 4 deletions(-)