x86/vpci: two fixes

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

Posted by Roger Pau Monne 5 months, 1 week ago

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

Posted by Stewart Hildebrand 5 months, 1 week ago

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.

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

Posted by Jan Beulich 5 months, 1 week ago

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

Posted by Stewart Hildebrand 5 months, 1 week ago

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

Posted by Roger Pau Monné 5 months, 1 week ago

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

Posted by Stewart Hildebrand 4 months, 4 weeks ago

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

Posted by Roger Pau Monné 4 months, 4 weeks ago

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

Posted by Andrew Cooper 5 months, 1 week ago

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

Posted by Roger Pau Monné 5 months, 1 week ago

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.