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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
> ---
>   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