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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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@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 )
> 
> 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.