[PATCH 0/2] x86/vpci: two fixes

[PATCH 0/2] x86/vpci: two fixes

[Roger Pau Monne]

Hello,

Patch 1 fixes a regression reported by the Qubes ADL runner, so it's
required to unblock the testing.  Patch 2 is possibly more controversial,
it's not strictly required to unblock the testing, but might be good to
consider.

Thanks, Roger.

Roger Pau Monne (2):
  x86/vpci: fix off-by-one
  x86/vpci: refuse to map BARs at position 0

 xen/arch/x86/pci.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

-- 
2.49.0

[PATCH 1/2] x86/vpci: fix off-by-one

[Roger Pau Monne]

rangeset_remove_range() uses inclusive ranges, and hence the end of the
range should be calculated using PFN_DOWN(), not PFN_UP().

Fixes: 4acab25a9300 ('x86/vpci: fix handling of BAR overlaps with non-hole regions')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/pci.c b/xen/arch/x86/pci.c
index afaf9fe1c053..26bb7f6a3c3a 100644
--- a/xen/arch/x86/pci.c
+++ b/xen/arch/x86/pci.c
@@ -131,7 +131,7 @@ int pci_sanitize_bar_memory(struct rangeset *r)
             continue;
 
         rc = rangeset_remove_range(r, PFN_DOWN(entry->addr),
-                                   PFN_UP(entry->addr + entry->size - 1));
+                                   PFN_DOWN(entry->addr + entry->size - 1));
         if ( rc )
             return rc;
     }
-- 
2.49.0

Re: [PATCH 1/2] x86/vpci: fix off-by-one

[Andrew Cooper]

On 22/05/2025 3:03 pm, Roger Pau Monne wrote:
> rangeset_remove_range() uses inclusive ranges, and hence the end of the
> range should be calculated using PFN_DOWN(), not PFN_UP().
>
> Fixes: 4acab25a9300 ('x86/vpci: fix handling of BAR overlaps with non-hole regions')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Roger Pau Monne]

A BAR at position 0 is not initialized (not positioned).  While Xen could
attempt to map it into the p2m, marking it as mapped will prevent dom0 to
change the position of the BAR, as the vPCI code has a shortcomming of not
allowing to write to BAR registers while the BAR is mapped on the p2m.

Workaround this limitation by returning false from pci_check_bar() if the
BAR address is 0, thus causing the bar->enabled field to also be set to
false and allowing bar_write() to change the BAR position.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

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;
+
     /*
      * Check if BAR is not overlapping with any memory region defined
      * in the memory map.
-- 
2.49.0

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

[Andrew Cooper]

On 22/05/2025 3:03 pm, Roger Pau Monne wrote:
> A BAR at position 0 is not initialized (not positioned).  While Xen could
> attempt to map it into the p2m, marking it as mapped will prevent dom0 to
> change the position of the BAR, as the vPCI code has a shortcomming of not

Minor grammar point.  "prevent dom0 from changing".

> allowing to write to BAR registers while the BAR is mapped on the p2m.
>
> Workaround this limitation by returning false from pci_check_bar() if the
> BAR address is 0, thus causing the bar->enabled field to also be set to
> false and allowing bar_write() to change the BAR position.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/pci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> 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

"0, as they are not"

> +     * be required by Linux, that can reposition BARs with memory decoding

"Linux, which may reposition".

Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +     * 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;
> +
>      /*
>       * Check if BAR is not overlapping with any memory region defined
>       * in the memory map.

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

[Roger Pau Monné]

On Thu, May 22, 2025 at 03:22:53PM +0100, Andrew Cooper wrote:
> On 22/05/2025 3:03 pm, Roger Pau Monne wrote:
> > A BAR at position 0 is not initialized (not positioned).  While Xen could
> > attempt to map it into the p2m, marking it as mapped will prevent dom0 to
> > change the position of the BAR, as the vPCI code has a shortcomming of not
> 
> Minor grammar point.  "prevent dom0 from changing".
> 
> > allowing to write to BAR registers while the BAR is mapped on the p2m.
> >
> > Workaround this limitation by returning false from pci_check_bar() if the
> > BAR address is 0, thus causing the bar->enabled field to also be set to
> > false and allowing bar_write() to change the BAR position.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/pci.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > 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
> 
> "0, as they are not"
> 
> > +     * be required by Linux, that can reposition BARs with memory decoding
> 
> "Linux, which may reposition".
> 
> Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, since this is not blocking the CI right now I will probably
wait a bit to gather more feedback.

Roger.

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

[Jan Beulich]

On 22.05.2025 16:03, Roger Pau Monne wrote:
> A BAR at position 0 is not initialized (not positioned).  While Xen could
> attempt to map it into the p2m, marking it as mapped will prevent dom0 to
> change the position of the BAR,

With memory decoding enabled, that is?

> as the vPCI code has a shortcomming of not
> allowing to write to BAR registers while the BAR is mapped on the p2m.

Again only under that extra condition, aiui.

> Workaround this limitation by returning false from pci_check_bar() if the
> BAR address is 0, thus causing the bar->enabled field to also be set to
> false and allowing bar_write() to change the BAR position.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/pci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> 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?

Jan

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

[Stewart Hildebrand]

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.

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

[Roger Pau Monné]

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.

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

[Stewart Hildebrand]

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.

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

[Roger Pau Monné]

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.

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

[Stewart Hildebrand]

On 5/22/25 10:03, Roger Pau Monne wrote:
> A BAR at position 0 is not initialized (not positioned).  While Xen could
> attempt to map it into the p2m, marking it as mapped will prevent dom0 to
> change the position of the BAR, as the vPCI code has a shortcomming of not
> allowing to write to BAR registers while the BAR is mapped on the p2m.
> 
> Workaround this limitation by returning false from pci_check_bar() if the
> BAR address is 0, thus causing the bar->enabled field to also be set to
> false and allowing bar_write() to change the BAR position.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/pci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> 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.
> +     */

Technically speaking, the particular corner case is plausible.

However, if I understand it correctly, when Linux finds an uninitialized
BAR, it checks if the BAR (resource) has been allocated, and won't
enable memory decoding if unallocated. See Linux
drivers/pci/setup-res.c:pci_enable_resources().

So I would consider dropping the "This might be required by Linux"
part from the comment.

> +    if ( mfn_eq(start, _mfn(0)) )
> +        return false;
> +
>      /*
>       * Check if BAR is not overlapping with any memory region defined
>       * in the memory map.