1. Patchew
  2. Xen
  3. View series

[PATCH] xen/arm: pci: fix check in pci_check_bar()

Stewart Hildebrand posted 1 patch 9 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230711154648.729567-1-stewart.hildebrand@amd.com
There is a newer version of this series
xen/arch/arm/pci/pci-host-common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] xen/arm: pci: fix check in pci_check_bar()
Posted by Stewart Hildebrand 9 months, 3 weeks ago 
When mapping BARs for vPCI, it's valid for a BAR start address to equal the BAR
end address (i.e. s == e). However, pci_check_bar() currently returns false in
this case, which results in Xen not mapping the BAR. In this example boot log,
Linux has mapped the BARs, but since Xen did not map them, Linux encounters a
data abort and panics:

[    2.593300] pci 0000:00:00.0: BAR 0: assigned [mem 0x50008000-0x50008fff]
[    2.593682] pci 0000:00:00.0: BAR 2: assigned [mem 0x50009000-0x50009fff]
[    2.594066] pci 0000:00:00.0: BAR 4: assigned [mem 0x5000a000-0x5000afff]
...
[    2.810502] virtio-pci 0000:00:00.0: enabling device (0000 -> 0002)
(XEN) 0000:00:00.0: not mapping BAR [50008, 50008] invalid position
(XEN) 0000:00:00.0: not mapping BAR [50009, 50009] invalid position
(XEN) 0000:00:00.0: not mapping BAR [5000a, 5000a] invalid position
[    2.817502] virtio-pci 0000:00:00.0: virtio_pci: leaving for legacy driver
[    2.817853] virtio-pci 0000:00:00.0: enabling bus mastering
(XEN) arch/arm/traps.c:1992:d0v0 HSR=0x00000093010045 pc=0xffff8000089507d4 gva=0xffff80000c46d012 gpa=0x00000050008012
[    2.818397] Unable to handle kernel ttbr address size fault at virtual address ffff80000c46d012
...

Fix this by changing the condition in pci_check_bar().

Fixes: cc80e2bab0d0 ("xen/pci: replace call to is_memory_hole to pci_check_bar")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
 xen/arch/arm/pci/pci-host-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index 7cdfc89e5211..e0ec526f9776 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -406,7 +406,7 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
         .is_valid = false
     };
 
-    if ( s >= e )
+    if ( s > e )
         return false;
 
     dt_node = pci_find_host_bridge_node(pdev);

base-commit: b831326ee2f9ed94523b3d8b0fb2da2a82113e9e
-- 
2.41.0
Re: [PATCH] xen/arm: pci: fix check in pci_check_bar()
Posted by Julien Grall 9 months, 3 weeks ago 
Hi,

On 11/07/2023 16:46, Stewart Hildebrand wrote:
> When mapping BARs for vPCI, it's valid for a BAR start address to equal the BAR
> end address (i.e. s == e). However, pci_check_bar() currently returns false in
> this case, which results in Xen not mapping the BAR. In this example boot log,
> Linux has mapped the BARs, but since Xen did not map them, Linux encounters a
> data abort and panics:
> 
> [    2.593300] pci 0000:00:00.0: BAR 0: assigned [mem 0x50008000-0x50008fff]
> [    2.593682] pci 0000:00:00.0: BAR 2: assigned [mem 0x50009000-0x50009fff]
> [    2.594066] pci 0000:00:00.0: BAR 4: assigned [mem 0x5000a000-0x5000afff]
> ...
> [    2.810502] virtio-pci 0000:00:00.0: enabling device (0000 -> 0002)
> (XEN) 0000:00:00.0: not mapping BAR [50008, 50008] invalid position
> (XEN) 0000:00:00.0: not mapping BAR [50009, 50009] invalid position
> (XEN) 0000:00:00.0: not mapping BAR [5000a, 5000a] invalid position
> [    2.817502] virtio-pci 0000:00:00.0: virtio_pci: leaving for legacy driver
> [    2.817853] virtio-pci 0000:00:00.0: enabling bus mastering
> (XEN) arch/arm/traps.c:1992:d0v0 HSR=0x00000093010045 pc=0xffff8000089507d4 gva=0xffff80000c46d012 gpa=0x00000050008012
> [    2.818397] Unable to handle kernel ttbr address size fault at virtual address ffff80000c46d012
> ...
> 
> Fix this by changing the condition in pci_check_bar().
> 
> Fixes: cc80e2bab0d0 ("xen/pci: replace call to is_memory_hole to pci_check_bar")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
>   xen/arch/arm/pci/pci-host-common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 7cdfc89e5211..e0ec526f9776 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -406,7 +406,7 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>           .is_valid = false
>       };
>   
> -    if ( s >= e )
> +    if ( s > e )
>           return false;

This is yet another example why using start/end in parameters are a bad 
idea :). I am OK if you want to keep the same interface, but can we at 
least document on top of the function whether we are expecting 'end' to 
be excluded or included?

Cheers,

-- 
Julien Grall
Re: [PATCH] xen/arm: pci: fix check in pci_check_bar()
Posted by Stewart Hildebrand 9 months, 3 weeks ago 
On 7/11/23 12:10, Julien Grall wrote:
> Hi,
> 
> On 11/07/2023 16:46, Stewart Hildebrand wrote:
>> When mapping BARs for vPCI, it's valid for a BAR start address to equal the BAR
>> end address (i.e. s == e). However, pci_check_bar() currently returns false in
>> this case, which results in Xen not mapping the BAR. In this example boot log,
>> Linux has mapped the BARs, but since Xen did not map them, Linux encounters a
>> data abort and panics:
>>
>> [    2.593300] pci 0000:00:00.0: BAR 0: assigned [mem 0x50008000-0x50008fff]
>> [    2.593682] pci 0000:00:00.0: BAR 2: assigned [mem 0x50009000-0x50009fff]
>> [    2.594066] pci 0000:00:00.0: BAR 4: assigned [mem 0x5000a000-0x5000afff]
>> ...
>> [    2.810502] virtio-pci 0000:00:00.0: enabling device (0000 -> 0002)
>> (XEN) 0000:00:00.0: not mapping BAR [50008, 50008] invalid position
>> (XEN) 0000:00:00.0: not mapping BAR [50009, 50009] invalid position
>> (XEN) 0000:00:00.0: not mapping BAR [5000a, 5000a] invalid position
>> [    2.817502] virtio-pci 0000:00:00.0: virtio_pci: leaving for legacy driver
>> [    2.817853] virtio-pci 0000:00:00.0: enabling bus mastering
>> (XEN) arch/arm/traps.c:1992:d0v0 HSR=0x00000093010045 pc=0xffff8000089507d4 gva=0xffff80000c46d012 gpa=0x00000050008012
>> [    2.818397] Unable to handle kernel ttbr address size fault at virtual address ffff80000c46d012
>> ...
>>
>> Fix this by changing the condition in pci_check_bar().
>>
>> Fixes: cc80e2bab0d0 ("xen/pci: replace call to is_memory_hole to pci_check_bar")
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>>   xen/arch/arm/pci/pci-host-common.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> index 7cdfc89e5211..e0ec526f9776 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -406,7 +406,7 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>>           .is_valid = false
>>       };
>>
>> -    if ( s >= e )
>> +    if ( s > e )
>>           return false;
> 
> This is yet another example why using start/end in parameters are a bad
> idea :). I am OK if you want to keep the same interface, but can we at
> least document on top of the function whether we are expecting 'end' to
> be excluded or included?

Yes, will do. I will send a v2 that also addresses Roger's comments. For clarity's sake, e is inclusive.
Re: [PATCH] xen/arm: pci: fix check in pci_check_bar()
Posted by Roger Pau Monné 9 months, 3 weeks ago 
On Tue, Jul 11, 2023 at 11:46:47AM -0400, Stewart Hildebrand wrote:
> When mapping BARs for vPCI, it's valid for a BAR start address to equal the BAR
> end address (i.e. s == e). However, pci_check_bar() currently returns false in
> this case, which results in Xen not mapping the BAR. In this example boot log,
> Linux has mapped the BARs, but since Xen did not map them, Linux encounters a
> data abort and panics:
> 
> [    2.593300] pci 0000:00:00.0: BAR 0: assigned [mem 0x50008000-0x50008fff]
> [    2.593682] pci 0000:00:00.0: BAR 2: assigned [mem 0x50009000-0x50009fff]
> [    2.594066] pci 0000:00:00.0: BAR 4: assigned [mem 0x5000a000-0x5000afff]
> ...
> [    2.810502] virtio-pci 0000:00:00.0: enabling device (0000 -> 0002)
> (XEN) 0000:00:00.0: not mapping BAR [50008, 50008] invalid position
> (XEN) 0000:00:00.0: not mapping BAR [50009, 50009] invalid position
> (XEN) 0000:00:00.0: not mapping BAR [5000a, 5000a] invalid position
> [    2.817502] virtio-pci 0000:00:00.0: virtio_pci: leaving for legacy driver
> [    2.817853] virtio-pci 0000:00:00.0: enabling bus mastering
> (XEN) arch/arm/traps.c:1992:d0v0 HSR=0x00000093010045 pc=0xffff8000089507d4 gva=0xffff80000c46d012 gpa=0x00000050008012
> [    2.818397] Unable to handle kernel ttbr address size fault at virtual address ffff80000c46d012
> ...
> 
> Fix this by changing the condition in pci_check_bar().
> 
> Fixes: cc80e2bab0d0 ("xen/pci: replace call to is_memory_hole to pci_check_bar")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
>  xen/arch/arm/pci/pci-host-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 7cdfc89e5211..e0ec526f9776 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -406,7 +406,7 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>          .is_valid = false
>      };
>  
> -    if ( s >= e )
> +    if ( s > e )

I think you want to adjust e to include the full page, ie:

paddr_t e = mfn_to_maddr(end + 1) - 1;

As passing start == end should be assumed to cover a full page, and s
== e won't be possible anymore.  Adjusting the check to drop the equal
is still good IMO.

Thanks, Roger.
Re: [PATCH] xen/arm: pci: fix check in pci_check_bar()
Posted by Stewart Hildebrand 9 months, 3 weeks ago 
On 7/11/23 12:04, Roger Pau Monné wrote:
> On Tue, Jul 11, 2023 at 11:46:47AM -0400, Stewart Hildebrand wrote:
>> When mapping BARs for vPCI, it's valid for a BAR start address to equal the BAR
>> end address (i.e. s == e). However, pci_check_bar() currently returns false in
>> this case, which results in Xen not mapping the BAR. In this example boot log,
>> Linux has mapped the BARs, but since Xen did not map them, Linux encounters a
>> data abort and panics:
>>
>> [    2.593300] pci 0000:00:00.0: BAR 0: assigned [mem 0x50008000-0x50008fff]
>> [    2.593682] pci 0000:00:00.0: BAR 2: assigned [mem 0x50009000-0x50009fff]
>> [    2.594066] pci 0000:00:00.0: BAR 4: assigned [mem 0x5000a000-0x5000afff]
>> ...
>> [    2.810502] virtio-pci 0000:00:00.0: enabling device (0000 -> 0002)
>> (XEN) 0000:00:00.0: not mapping BAR [50008, 50008] invalid position
>> (XEN) 0000:00:00.0: not mapping BAR [50009, 50009] invalid position
>> (XEN) 0000:00:00.0: not mapping BAR [5000a, 5000a] invalid position
>> [    2.817502] virtio-pci 0000:00:00.0: virtio_pci: leaving for legacy driver
>> [    2.817853] virtio-pci 0000:00:00.0: enabling bus mastering
>> (XEN) arch/arm/traps.c:1992:d0v0 HSR=0x00000093010045 pc=0xffff8000089507d4 gva=0xffff80000c46d012 gpa=0x00000050008012
>> [    2.818397] Unable to handle kernel ttbr address size fault at virtual address ffff80000c46d012
>> ...
>>
>> Fix this by changing the condition in pci_check_bar().
>>
>> Fixes: cc80e2bab0d0 ("xen/pci: replace call to is_memory_hole to pci_check_bar")
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>>  xen/arch/arm/pci/pci-host-common.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> index 7cdfc89e5211..e0ec526f9776 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -406,7 +406,7 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>>          .is_valid = false
>>      };
>>
>> -    if ( s >= e )
>> +    if ( s > e )
> 
> I think you want to adjust e to include the full page, ie:
> 
> paddr_t e = mfn_to_maddr(end + 1) - 1;
> 
> As passing start == end should be assumed to cover a full page, and s
> == e won't be possible anymore.

Yes, I will do this. This will also increase the accuracy of the subsequent is_bar_valid check. Since can't add directly to a mfn_t, it will actually look like this:

paddr_t e = mfn_to_maddr(mfn_add(end, 1)) - 1;

> Adjusting the check to drop the equal is still good IMO.

Agreed, since e is still inclusive after applying your suggested math.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.