Re: [PATCH 2/2] x86/vpci: refuse to map BARs at position 0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jun 03, 2025 at 04:47:55PM -0400, Stewart Hildebrand wrote:
> On 5/22/25 12:24, Roger Pau Monné wrote:
> > On Thu, May 22, 2025 at 11:44:24AM -0400, Stewart Hildebrand wrote:
> >> On 5/22/25 10:59, Jan Beulich wrote:
> >>> On 22.05.2025 16:03, Roger Pau Monne wrote:
> >>>> diff --git a/xen/arch/x86/pci.c b/xen/arch/x86/pci.c
> >>>> index 26bb7f6a3c3a..39fd5a16a4aa 100644
> >>>> --- a/xen/arch/x86/pci.c
> >>>> +++ b/xen/arch/x86/pci.c
> >>>> @@ -101,6 +101,15 @@ int pci_conf_write_intercept(unsigned int seg, 
> >>>> unsigned int bdf,
> >>>>  
> >>>>  bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> >>>>  {
> >>>> +    /*
> >>>> +     * Refuse to map BARs at position 0, those are not initialized.  
> >>>> This might
> >>>> +     * be required by Linux, that can reposition BARs with memory 
> >>>> decoding
> >>>> +     * enabled.  By returning false here bar->enabled will be set to 
> >>>> false, and
> >>>> +     * bar_write() will work as expected.
> >>>> +     */
> >>>> +    if ( mfn_eq(start, _mfn(0)) )
> >>>> +        return false;
> >>>
> >>> Is this really x86-specific?
> >>
> >> No, I think Arm would benefit from this check too. I'm in favor of
> >> moving the check to common.
> > 
> > I think on ARM pci_check_bar() is more strict, and doesn't really need
> > this check since it explicitly checks whether the BAR falls inside of
> > a bridge window.
> > 
> > So unless you have a bridge window at mfn 0 this won't make a
> > difference.  And if you have a bridge window at mfn 0 you really want
> > to be able to position BARs at address 0.
> > 
> > Thanks, Roger.
> 
> True, but I was thinking more generally: if a BAR is not initialized,
> don't map it. On Arm, it seems to be hit or miss whether BARs have been
> initialized or not. I guess the difficulty lies in whether comparing to
> zero is a reliable test to determine if the BAR is uninitialized.

Indeed.  I think on ARM it is better to check whether the BAR position
matches the bridge window, if it does not match then the BAR is not
initialized, which is what the current check already does?

On x86 this is more complex, since Xen doesn't track bridge windows,
hence the sub-optimal solution of checking against 0.  Also on x86
while not impossible I think it's extremely unlikely to have a bridge
window starting at 0, given all the legacy stuff that resides in the
low 1MB, and the fact that the AP trampoline must be in the low 1MB.

Thanks, Roger.