[PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions
@ 2022-04-29 13:05 Jan Beulich
  2022-05-05 15:00 ` Roger Pau Monné
  2022-05-05 19:10 ` Andrew Cooper
  0 siblings, 2 replies; 7+ messages in thread
From: Jan Beulich @ 2022-04-29 13:05 UTC (permalink / raw)
  To: xen-devel@lists.xenproject.org; +Cc: Paul Durrant, Andrew Cooper

IOMMU code mapping / unmapping devices and interrupts will misbehave if
a wrong command line option declared a function "phantom" when there's a
real device at that position. Warn about this and adjust the specified
stride (in the worst case ignoring the option altogether).

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -451,7 +451,24 @@ static struct pci_dev *alloc_pdev(struct
                          phantom_devs[i].slot == PCI_SLOT(devfn) &&
                          phantom_devs[i].stride > PCI_FUNC(devfn) )
                     {
-                        pdev->phantom_stride = phantom_devs[i].stride;
+                        pci_sbdf_t sbdf = pdev->sbdf;
+                        unsigned int stride = phantom_devs[i].stride;
+
+                        while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )
+                        {
+                            if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) == 0xffff &&
+                                 pci_conf_read16(sbdf, PCI_DEVICE_ID) == 0xffff )
+                                continue;
+                            stride <<= 1;
+                            printk(XENLOG_WARNING
+                                   "%pp looks to be a real device; bumping %04x:%02x:%02x stride to %u\n",
+                                   &sbdf, phantom_devs[i].seg,
+                                   phantom_devs[i].bus, phantom_devs[i].slot,
+                                   stride);
+                            sbdf = pdev->sbdf;
+                        }
+                        if ( PCI_FUNC(stride) )
+                           pdev->phantom_stride = stride;
                         break;
                     }
             }



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions
  2022-04-29 13:05 [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions Jan Beulich
@ 2022-05-05 15:00 ` Roger Pau Monné
  2022-05-05 15:14   ` Jan Beulich
  2022-05-05 19:10 ` Andrew Cooper
  1 sibling, 1 reply; 7+ messages in thread
From: Roger Pau Monné @ 2022-05-05 15:00 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel@lists.xenproject.org, Paul Durrant, Andrew Cooper

On Fri, Apr 29, 2022 at 03:05:32PM +0200, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> IOMMU code mapping / unmapping devices and interrupts will misbehave if
> a wrong command line option declared a function "phantom" when there's a
> real device at that position. Warn about this and adjust the specified
> stride (in the worst case ignoring the option altogether).
> 
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

FWIW, I would be fine with just discarding the stride option if one of
the phantom devices happen to report vendor/device IDs on the config
space.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -451,7 +451,24 @@ static struct pci_dev *alloc_pdev(struct
>                           phantom_devs[i].slot == PCI_SLOT(devfn) &&
>                           phantom_devs[i].stride > PCI_FUNC(devfn) )
>                      {
> -                        pdev->phantom_stride = phantom_devs[i].stride;
> +                        pci_sbdf_t sbdf = pdev->sbdf;
> +                        unsigned int stride = phantom_devs[i].stride;
> +
> +                        while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )
> +                        {
> +                            if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) == 0xffff &&
> +                                 pci_conf_read16(sbdf, PCI_DEVICE_ID) == 0xffff )
> +                                continue;
> +                            stride <<= 1;
> +                            printk(XENLOG_WARNING
> +                                   "%pp looks to be a real device; bumping %04x:%02x:%02x stride to %u\n",
> +                                   &sbdf, phantom_devs[i].seg,
> +                                   phantom_devs[i].bus, phantom_devs[i].slot,

Can't you use pdev->sbdf here?

Thanks, Roger.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions
  2022-05-05 15:00 ` Roger Pau Monné
@ 2022-05-05 15:14   ` Jan Beulich
  2022-05-05 15:36     ` Roger Pau Monné
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2022-05-05 15:14 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: xen-devel@lists.xenproject.org, Paul Durrant, Andrew Cooper

On 05.05.2022 17:00, Roger Pau Monné wrote:
> On Fri, Apr 29, 2022 at 03:05:32PM +0200, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>
>> IOMMU code mapping / unmapping devices and interrupts will misbehave if
>> a wrong command line option declared a function "phantom" when there's a
>> real device at that position. Warn about this and adjust the specified
>> stride (in the worst case ignoring the option altogether).
>>
>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> FWIW, I would be fine with just discarding the stride option if one of
> the phantom devices happen to report vendor/device IDs on the config
> space.

Well, I thought I'd try a best-effort adjustment rather than simply
ignoring an option.

>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -451,7 +451,24 @@ static struct pci_dev *alloc_pdev(struct
>>                           phantom_devs[i].slot == PCI_SLOT(devfn) &&
>>                           phantom_devs[i].stride > PCI_FUNC(devfn) )
>>                      {
>> -                        pdev->phantom_stride = phantom_devs[i].stride;
>> +                        pci_sbdf_t sbdf = pdev->sbdf;
>> +                        unsigned int stride = phantom_devs[i].stride;
>> +
>> +                        while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )
>> +                        {
>> +                            if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) == 0xffff &&
>> +                                 pci_conf_read16(sbdf, PCI_DEVICE_ID) == 0xffff )
>> +                                continue;
>> +                            stride <<= 1;
>> +                            printk(XENLOG_WARNING
>> +                                   "%pp looks to be a real device; bumping %04x:%02x:%02x stride to %u\n",
>> +                                   &sbdf, phantom_devs[i].seg,
>> +                                   phantom_devs[i].bus, phantom_devs[i].slot,
> 
> Can't you use pdev->sbdf here?

No - sbdf was altered from pdev->sbdf (and is also shorter to use),
and for the 2nd item I'm intentionally omitting the function part
(to match the command line option).

Jan



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions
  2022-05-05 15:14   ` Jan Beulich
@ 2022-05-05 15:36     ` Roger Pau Monné
  0 siblings, 0 replies; 7+ messages in thread
From: Roger Pau Monné @ 2022-05-05 15:36 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel@lists.xenproject.org, Paul Durrant, Andrew Cooper

On Thu, May 05, 2022 at 05:14:14PM +0200, Jan Beulich wrote:
> On 05.05.2022 17:00, Roger Pau Monné wrote:
> > On Fri, Apr 29, 2022 at 03:05:32PM +0200, Jan Beulich wrote:
> >> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> >>
> >> IOMMU code mapping / unmapping devices and interrupts will misbehave if
> >> a wrong command line option declared a function "phantom" when there's a
> >> real device at that position. Warn about this and adjust the specified
> >> stride (in the worst case ignoring the option altogether).
> >>
> >> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> > FWIW, I would be fine with just discarding the stride option if one of
> > the phantom devices happen to report vendor/device IDs on the config
> > space.
> 
> Well, I thought I'd try a best-effort adjustment rather than simply
> ignoring an option.
> 
> >> --- a/xen/drivers/passthrough/pci.c
> >> +++ b/xen/drivers/passthrough/pci.c
> >> @@ -451,7 +451,24 @@ static struct pci_dev *alloc_pdev(struct
> >>                           phantom_devs[i].slot == PCI_SLOT(devfn) &&
> >>                           phantom_devs[i].stride > PCI_FUNC(devfn) )
> >>                      {
> >> -                        pdev->phantom_stride = phantom_devs[i].stride;
> >> +                        pci_sbdf_t sbdf = pdev->sbdf;
> >> +                        unsigned int stride = phantom_devs[i].stride;
> >> +
> >> +                        while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )
> >> +                        {
> >> +                            if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) == 0xffff &&
> >> +                                 pci_conf_read16(sbdf, PCI_DEVICE_ID) == 0xffff )
> >> +                                continue;
> >> +                            stride <<= 1;
> >> +                            printk(XENLOG_WARNING
> >> +                                   "%pp looks to be a real device; bumping %04x:%02x:%02x stride to %u\n",
> >> +                                   &sbdf, phantom_devs[i].seg,
> >> +                                   phantom_devs[i].bus, phantom_devs[i].slot,
> > 
> > Can't you use pdev->sbdf here?
> 
> No - sbdf was altered from pdev->sbdf (and is also shorter to use),
> and for the 2nd item I'm intentionally omitting the function part
> (to match the command line option).

Sorry, should have been clearer. My question was to use pdev->sbdf for
the second instance.  I see now that you don't print the function, so
that's fine.

Thanks, Roger.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions
  2022-04-29 13:05 [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions Jan Beulich
  2022-05-05 15:00 ` Roger Pau Monné
@ 2022-05-05 19:10 ` Andrew Cooper
  2022-05-06  6:21   ` Jan Beulich
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2022-05-05 19:10 UTC (permalink / raw)
  To: Jan Beulich, xen-devel@lists.xenproject.org; +Cc: Paul Durrant

On 29/04/2022 14:05, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>
> IOMMU code mapping / unmapping devices and interrupts will misbehave if
> a wrong command line option declared a function "phantom" when there's a
> real device at that position. Warn about this and adjust the specified
> stride (in the worst case ignoring the option altogether).
>
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -451,7 +451,24 @@ static struct pci_dev *alloc_pdev(struct
>                           phantom_devs[i].slot == PCI_SLOT(devfn) &&
>                           phantom_devs[i].stride > PCI_FUNC(devfn) )
>                      {
> -                        pdev->phantom_stride = phantom_devs[i].stride;
> +                        pci_sbdf_t sbdf = pdev->sbdf;
> +                        unsigned int stride = phantom_devs[i].stride;
> +
> +                        while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )

I'm fairly sure this doesn't do what you want it to.

.fn is a 3 bit bitfield, meaning the += will be truncated before the
compare.

> +                        {
> +                            if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) == 0xffff &&
> +                                 pci_conf_read16(sbdf, PCI_DEVICE_ID) == 0xffff )
> +                                continue;
> +                            stride <<= 1;
> +                            printk(XENLOG_WARNING
> +                                   "%pp looks to be a real device; bumping %04x:%02x:%02x stride to %u\n",
> +                                   &sbdf, phantom_devs[i].seg,
> +                                   phantom_devs[i].bus, phantom_devs[i].slot,
> +                                   stride);
> +                            sbdf = pdev->sbdf;
> +                        }
> +                        if ( PCI_FUNC(stride) )

This is an obfuscated way of writing stride < 8.

Given the printk(), if we actually find an 8-function device, what gets
printed (AFAICT) will be "bumping to 8" when in fact we mean "totally
ignoring the option".  I think this really wants an else clause.

~Andrew

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions
  2022-05-05 19:10 ` Andrew Cooper
@ 2022-05-06  6:21   ` Jan Beulich
  2022-05-19 12:22     ` Jan Beulich
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2022-05-06  6:21 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Paul Durrant, xen-devel@lists.xenproject.org

On 05.05.2022 21:10, Andrew Cooper wrote:
> On 29/04/2022 14:05, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>
>> IOMMU code mapping / unmapping devices and interrupts will misbehave if
>> a wrong command line option declared a function "phantom" when there's a
>> real device at that position. Warn about this and adjust the specified
>> stride (in the worst case ignoring the option altogether).
>>
>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -451,7 +451,24 @@ static struct pci_dev *alloc_pdev(struct
>>                           phantom_devs[i].slot == PCI_SLOT(devfn) &&
>>                           phantom_devs[i].stride > PCI_FUNC(devfn) )
>>                      {
>> -                        pdev->phantom_stride = phantom_devs[i].stride;
>> +                        pci_sbdf_t sbdf = pdev->sbdf;
>> +                        unsigned int stride = phantom_devs[i].stride;
>> +
>> +                        while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )
> 
> I'm fairly sure this doesn't do what you want it to.
> 
> .fn is a 3 bit bitfield, meaning the += will be truncated before the
> compare.

And this is precisely what I'm after: I want to stop once the value
has wrapped.

>> +                        {
>> +                            if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) == 0xffff &&
>> +                                 pci_conf_read16(sbdf, PCI_DEVICE_ID) == 0xffff )
>> +                                continue;
>> +                            stride <<= 1;
>> +                            printk(XENLOG_WARNING
>> +                                   "%pp looks to be a real device; bumping %04x:%02x:%02x stride to %u\n",
>> +                                   &sbdf, phantom_devs[i].seg,
>> +                                   phantom_devs[i].bus, phantom_devs[i].slot,
>> +                                   stride);
>> +                            sbdf = pdev->sbdf;
>> +                        }
>> +                        if ( PCI_FUNC(stride) )
> 
> This is an obfuscated way of writing stride < 8.

And intentionally so, matching a few other similar instances elsewhere.
An open-coded 8 here doesn't really make clear where that 8 would be
coming from. The use of PCI_FUNC(), otoh, documents what's meant.

> Given the printk(), if we actually find an 8-function device, what gets
> printed (AFAICT) will be "bumping to 8" when in fact we mean "totally
> ignoring the option".  I think this really wants an else clause.

Yes, "bumping to 8" is what is being printed in that case. I did
realize the slight anomaly when writing the code and I observed
(verified) it also in testing. But I didn't see a good reason for an
"else" here - 8 being mentioned in the log message is clear enough
for anyone vaguely understanding phantom functions. But if you strongly
think we need to make the code yet larger and indentation yet
unhelpfully deeper, then I will (begrudgingly) do what you ask for. But
please explicitly confirm.

Jan



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions
  2022-05-06  6:21   ` Jan Beulich
@ 2022-05-19 12:22     ` Jan Beulich
  0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2022-05-19 12:22 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Paul Durrant, xen-devel@lists.xenproject.org

On 06.05.2022 08:21, Jan Beulich wrote:
> On 05.05.2022 21:10, Andrew Cooper wrote:
>> On 29/04/2022 14:05, Jan Beulich wrote:
>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>
>>> IOMMU code mapping / unmapping devices and interrupts will misbehave if
>>> a wrong command line option declared a function "phantom" when there's a
>>> real device at that position. Warn about this and adjust the specified
>>> stride (in the worst case ignoring the option altogether).
>>>
>>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -451,7 +451,24 @@ static struct pci_dev *alloc_pdev(struct
>>>                           phantom_devs[i].slot == PCI_SLOT(devfn) &&
>>>                           phantom_devs[i].stride > PCI_FUNC(devfn) )
>>>                      {
>>> -                        pdev->phantom_stride = phantom_devs[i].stride;
>>> +                        pci_sbdf_t sbdf = pdev->sbdf;
>>> +                        unsigned int stride = phantom_devs[i].stride;
>>> +
>>> +                        while ( (sbdf.fn += stride) > PCI_FUNC(devfn) )
>>
>> I'm fairly sure this doesn't do what you want it to.
>>
>> .fn is a 3 bit bitfield, meaning the += will be truncated before the
>> compare.
> 
> And this is precisely what I'm after: I want to stop once the value
> has wrapped.
> 
>>> +                        {
>>> +                            if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) == 0xffff &&
>>> +                                 pci_conf_read16(sbdf, PCI_DEVICE_ID) == 0xffff )
>>> +                                continue;
>>> +                            stride <<= 1;
>>> +                            printk(XENLOG_WARNING
>>> +                                   "%pp looks to be a real device; bumping %04x:%02x:%02x stride to %u\n",
>>> +                                   &sbdf, phantom_devs[i].seg,
>>> +                                   phantom_devs[i].bus, phantom_devs[i].slot,
>>> +                                   stride);
>>> +                            sbdf = pdev->sbdf;
>>> +                        }
>>> +                        if ( PCI_FUNC(stride) )
>>
>> This is an obfuscated way of writing stride < 8.
> 
> And intentionally so, matching a few other similar instances elsewhere.
> An open-coded 8 here doesn't really make clear where that 8 would be
> coming from. The use of PCI_FUNC(), otoh, documents what's meant.
> 
>> Given the printk(), if we actually find an 8-function device, what gets
>> printed (AFAICT) will be "bumping to 8" when in fact we mean "totally
>> ignoring the option".  I think this really wants an else clause.
> 
> Yes, "bumping to 8" is what is being printed in that case. I did
> realize the slight anomaly when writing the code and I observed
> (verified) it also in testing. But I didn't see a good reason for an
> "else" here - 8 being mentioned in the log message is clear enough
> for anyone vaguely understanding phantom functions. But if you strongly
> think we need to make the code yet larger and indentation yet
> unhelpfully deeper, then I will (begrudgingly) do what you ask for. But
> please explicitly confirm.

Like for the first few patches of the IOMMU large page series, I'm
going to put this in (with Roger's R-b) by the end of the week on
the assumption that my reply (above) did address your concerns.

Jan



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-05-19 12:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-29 13:05 [PATCH] PCI: don't allow "pci-phantom=" to mark real devices as phantom functions Jan Beulich
2022-05-05 15:00 ` Roger Pau Monné
2022-05-05 15:14   ` Jan Beulich
2022-05-05 15:36     ` Roger Pau Monné
2022-05-05 19:10 ` Andrew Cooper
2022-05-06  6:21   ` Jan Beulich
2022-05-19 12:22     ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.