Patchwork pci: Error on PCI capability collisions

login
register
mail settings
Submitter Jan Kiszka
Date Aug. 23, 2011, 5:28 p.m.
Message ID <4E53E328.90601@siemens.com>
Download mbox | patch
Permalink /patch/111149/
State New
Headers show

Comments

Jan Kiszka - Aug. 23, 2011, 5:28 p.m.
From: Alex Williamson <alex.williamson@redhat.com>

Nothing good can happen when we overlap capabilities

[ Jan: rebased over qemu, minor formatting ]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/pci.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Michael S. Tsirkin - Aug. 23, 2011, 6:17 p.m.
On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> From: Alex Williamson <alex.williamson@redhat.com>
> 
> Nothing good can happen when we overlap capabilities
> 
> [ Jan: rebased over qemu, minor formatting ]
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

I'll stick an assert there instead. Normal devices
don't generate overlapping caps unless there's a bug,
and device assignment should do it's own checks.

I really have a mind to rip out the used array too.

> ---
>  hw/pci.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 6124790..ff20631 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>                         uint8_t offset, uint8_t size)
>  {
>      uint8_t *config;
> +    int i;
> +
>      if (!offset) {
>          offset = pci_find_space(pdev, size);
>          if (!offset) {
>              return -ENOSPC;
>          }
> +    } else {
> +        for (i = offset; i < offset + size; i++) {
> +            if (pdev->used[i]) {
> +                fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
> +                        "Attempt to add PCI capability %x at offset "
> +                        "%x overlaps existing capability %x at offset %x\n",
> +                        pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
> +                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> +                        cap_id, offset, pdev->config_map[i], i);
> +                return -EFAULT;
> +            }
> +        }
>      }
>  
>      config = pdev->config + offset;
> -- 
> 1.7.3.4
Alex Williamson - Aug. 23, 2011, 6:21 p.m.
On Tue, 2011-08-23 at 21:17 +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> > From: Alex Williamson <alex.williamson@redhat.com>
> > 
> > Nothing good can happen when we overlap capabilities
> > 
> > [ Jan: rebased over qemu, minor formatting ]
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I'll stick an assert there instead. Normal devices
> don't generate overlapping caps unless there's a bug,
> and device assignment should do it's own checks.
> 
> I really have a mind to rip out the used array too.

So you'd rather kill qemu rather than have a reasonable error return
path... great :(

Alex

> > ---
> >  hw/pci.c |   14 ++++++++++++++
> >  1 files changed, 14 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 6124790..ff20631 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> >                         uint8_t offset, uint8_t size)
> >  {
> >      uint8_t *config;
> > +    int i;
> > +
> >      if (!offset) {
> >          offset = pci_find_space(pdev, size);
> >          if (!offset) {
> >              return -ENOSPC;
> >          }
> > +    } else {
> > +        for (i = offset; i < offset + size; i++) {
> > +            if (pdev->used[i]) {
> > +                fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
> > +                        "Attempt to add PCI capability %x at offset "
> > +                        "%x overlaps existing capability %x at offset %x\n",
> > +                        pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
> > +                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> > +                        cap_id, offset, pdev->config_map[i], i);
> > +                return -EFAULT;
> > +            }
> > +        }
> >      }
> >  
> >      config = pdev->config + offset;
> > -- 
> > 1.7.3.4
Michael S. Tsirkin - Aug. 23, 2011, 6:26 p.m.
On Tue, Aug 23, 2011 at 12:21:47PM -0600, Alex Williamson wrote:
> On Tue, 2011-08-23 at 21:17 +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> > > From: Alex Williamson <alex.williamson@redhat.com>
> > > 
> > > Nothing good can happen when we overlap capabilities
> > > 
> > > [ Jan: rebased over qemu, minor formatting ]
> > > 
> > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > I'll stick an assert there instead. Normal devices
> > don't generate overlapping caps unless there's a bug,
> > and device assignment should do it's own checks.
> > 
> > I really have a mind to rip out the used array too.
> 
> So you'd rather kill qemu rather than have a reasonable error return
> path... great :(
> 
> Alex

Well that will make it possible to make pci_add_capability return void,
less work for callers :) Dev assignment is really the only place where
capability offsets need to be verified.

> > > ---
> > >  hw/pci.c |   14 ++++++++++++++
> > >  1 files changed, 14 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 6124790..ff20631 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> > >                         uint8_t offset, uint8_t size)
> > >  {
> > >      uint8_t *config;
> > > +    int i;
> > > +
> > >      if (!offset) {
> > >          offset = pci_find_space(pdev, size);
> > >          if (!offset) {
> > >              return -ENOSPC;
> > >          }
> > > +    } else {
> > > +        for (i = offset; i < offset + size; i++) {
> > > +            if (pdev->used[i]) {
> > > +                fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
> > > +                        "Attempt to add PCI capability %x at offset "
> > > +                        "%x overlaps existing capability %x at offset %x\n",
> > > +                        pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
> > > +                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> > > +                        cap_id, offset, pdev->config_map[i], i);
> > > +                return -EFAULT;
> > > +            }
> > > +        }
> > >      }
> > >  
> > >      config = pdev->config + offset;
> > > -- 
> > > 1.7.3.4
> 
>
Alex Williamson - Aug. 23, 2011, 7:12 p.m.
On Tue, 2011-08-23 at 21:26 +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 23, 2011 at 12:21:47PM -0600, Alex Williamson wrote:
> > On Tue, 2011-08-23 at 21:17 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > 
> > > > Nothing good can happen when we overlap capabilities
> > > > 
> > > > [ Jan: rebased over qemu, minor formatting ]
> > > > 
> > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > 
> > > I'll stick an assert there instead. Normal devices
> > > don't generate overlapping caps unless there's a bug,
> > > and device assignment should do it's own checks.
> > > 
> > > I really have a mind to rip out the used array too.
> > 
> > So you'd rather kill qemu rather than have a reasonable error return
> > path... great :(
> > 
> > Alex
> 
> Well that will make it possible to make pci_add_capability return void,
> less work for callers :) Dev assignment is really the only place where
> capability offsets need to be verified.

A few issues with that... Since when is error handling so difficult that
we need to pretend that nothing ever fails just to make it easy for the
caller?  Why is device assignment such a special case?  It's actually
rather ironic that we're trying to add error checking to catch bugs that
real hardware is exposing, but assuming that emulated drivers always get
it right.  How will a return void help the emulated driver that has a
coding error?

Alex

> > > > ---
> > > >  hw/pci.c |   14 ++++++++++++++
> > > >  1 files changed, 14 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > index 6124790..ff20631 100644
> > > > --- a/hw/pci.c
> > > > +++ b/hw/pci.c
> > > > @@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> > > >                         uint8_t offset, uint8_t size)
> > > >  {
> > > >      uint8_t *config;
> > > > +    int i;
> > > > +
> > > >      if (!offset) {
> > > >          offset = pci_find_space(pdev, size);
> > > >          if (!offset) {
> > > >              return -ENOSPC;
> > > >          }
> > > > +    } else {
> > > > +        for (i = offset; i < offset + size; i++) {
> > > > +            if (pdev->used[i]) {
> > > > +                fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
> > > > +                        "Attempt to add PCI capability %x at offset "
> > > > +                        "%x overlaps existing capability %x at offset %x\n",
> > > > +                        pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
> > > > +                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> > > > +                        cap_id, offset, pdev->config_map[i], i);
> > > > +                return -EFAULT;
> > > > +            }
> > > > +        }
> > > >      }
> > > >  
> > > >      config = pdev->config + offset;
> > > > -- 
> > > > 1.7.3.4
> > 
> >
Michael S. Tsirkin - Aug. 23, 2011, 7:30 p.m.
On Tue, Aug 23, 2011 at 01:12:19PM -0600, Alex Williamson wrote:
> On Tue, 2011-08-23 at 21:26 +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 23, 2011 at 12:21:47PM -0600, Alex Williamson wrote:
> > > On Tue, 2011-08-23 at 21:17 +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> > > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > > 
> > > > > Nothing good can happen when we overlap capabilities
> > > > > 
> > > > > [ Jan: rebased over qemu, minor formatting ]
> > > > > 
> > > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > 
> > > > I'll stick an assert there instead. Normal devices
> > > > don't generate overlapping caps unless there's a bug,
> > > > and device assignment should do it's own checks.
> > > > 
> > > > I really have a mind to rip out the used array too.
> > > 
> > > So you'd rather kill qemu rather than have a reasonable error return
> > > path... great :(
> > > 
> > > Alex
> > 
> > Well that will make it possible to make pci_add_capability return void,
> > less work for callers :) Dev assignment is really the only place where
> > capability offsets need to be verified.
> 
> A few issues with that... Since when is error handling so difficult that
> we need to pretend that nothing ever fails just to make it easy for the
> caller?

It isn't but no need to introduce error codes just for fun.

>  Why is device assignment such a special case?

Assigned devices are under the guest control so should be assumed
untrusted, and we must verify anything we get from them.

For example, I think it's generally a mistake to read a device
register and use that as an array index, we must check it's in range
first. It's best to do these range checks in the dev assignment code
so that it's easy to verify that all values are used safely.

> It's actually
> rather ironic that we're trying to add error checking to catch bugs that
> real hardware is exposing, but assuming that emulated drivers always get
> it right.  How will a return void help the emulated driver that has a
> coding error?

Drivers use fixed offsets so they will always fail or always work.
If we return an error they might seem to work but behave incrrectly
without the right capability.
Alex Williamson - Aug. 23, 2011, 7:38 p.m.
On Tue, 2011-08-23 at 22:30 +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 23, 2011 at 01:12:19PM -0600, Alex Williamson wrote:
> > On Tue, 2011-08-23 at 21:26 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Aug 23, 2011 at 12:21:47PM -0600, Alex Williamson wrote:
> > > > On Tue, 2011-08-23 at 21:17 +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> > > > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > > > 
> > > > > > Nothing good can happen when we overlap capabilities
> > > > > > 
> > > > > > [ Jan: rebased over qemu, minor formatting ]
> > > > > > 
> > > > > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > 
> > > > > I'll stick an assert there instead. Normal devices
> > > > > don't generate overlapping caps unless there's a bug,
> > > > > and device assignment should do it's own checks.
> > > > > 
> > > > > I really have a mind to rip out the used array too.
> > > > 
> > > > So you'd rather kill qemu rather than have a reasonable error return
> > > > path... great :(
> > > > 
> > > > Alex
> > > 
> > > Well that will make it possible to make pci_add_capability return void,
> > > less work for callers :) Dev assignment is really the only place where
> > > capability offsets need to be verified.
> > 
> > A few issues with that... Since when is error handling so difficult that
> > we need to pretend that nothing ever fails just to make it easy for the
> > caller?
> 
> It isn't but no need to introduce error codes just for fun.

Likewise no need to remove error handling code because you dislike it.

> >  Why is device assignment such a special case?
> 
> Assigned devices are under the guest control so should be assumed
> untrusted, and we must verify anything we get from them.

The device is under guest control, but setting up access to the device
is entirely under qemu control.

> For example, I think it's generally a mistake to read a device
> register and use that as an array index, we must check it's in range
> first. It's best to do these range checks in the dev assignment code
> so that it's easy to verify that all values are used safely.

Except you're assuming that emulated drivers always get it right and
don't need such range checks, which is completely bogus.  We can either
bloat the device assignment code with wrappers that do range checks for
every pci callback or have pci check for everybody and maybe we'll catch
a bug or two...

> > It's actually
> > rather ironic that we're trying to add error checking to catch bugs that
> > real hardware is exposing, but assuming that emulated drivers always get
> > it right.  How will a return void help the emulated driver that has a
> > coding error?
> 
> Drivers use fixed offsets so they will always fail or always work.

Drivers "often" used fixed offsets.  I don't see any requirement for
this.

> If we return an error they might seem to work but behave incrrectly
> without the right capability.

And if we return void and blast capabilities over top of each other,
that's ok?

Alex
Don Dutile - Aug. 23, 2011, 8:59 p.m.
On 08/23/2011 03:30 PM, Michael S. Tsirkin wrote:
> On Tue, Aug 23, 2011 at 01:12:19PM -0600, Alex Williamson wrote:
>> On Tue, 2011-08-23 at 21:26 +0300, Michael S. Tsirkin wrote:
>>> On Tue, Aug 23, 2011 at 12:21:47PM -0600, Alex Williamson wrote:
>>>> On Tue, 2011-08-23 at 21:17 +0300, Michael S. Tsirkin wrote:
>>>>> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
>>>>>> From: Alex Williamson<alex.williamson@redhat.com>
>>>>>>
>>>>>> Nothing good can happen when we overlap capabilities
>>>>>>
>>>>>> [ Jan: rebased over qemu, minor formatting ]
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>>>
>>>>> I'll stick an assert there instead. Normal devices
>>>>> don't generate overlapping caps unless there's a bug,
>>>>> and device assignment should do it's own checks.
>>>>>
>>>>> I really have a mind to rip out the used array too.
>>>>
>>>> So you'd rather kill qemu rather than have a reasonable error return
>>>> path... great :(
>>>>
>>>> Alex
>>>
>>> Well that will make it possible to make pci_add_capability return void,
>>> less work for callers :) Dev assignment is really the only place where
>>> capability offsets need to be verified.
>>
>> A few issues with that... Since when is error handling so difficult that
>> we need to pretend that nothing ever fails just to make it easy for the
>> caller?
>
> It isn't but no need to introduce error codes just for fun.
>
>>   Why is device assignment such a special case?
>
> Assigned devices are under the guest control so should be assumed
> untrusted, and we must verify anything we get from them.
>
> For example, I think it's generally a mistake to read a device
> register and use that as an array index, we must check it's in range
> first. It's best to do these range checks in the dev assignment code
> so that it's easy to verify that all values are used safely.
>
So we want to pollute the dev assignment code with knowledge of this array
for bounds checking, which you're threatening to remove?

The patch is simple, the return error checking is simple, and when
we write error free code, we can remove all error checking.

I found the current array & it's error checking fairly handy when
the array was overflowed and it resulted in oddly succeeding/failing
sequences doing device assignment (yes, due to bad hardware -- shocking! ;-) ).
The error checking quickly pointed out the problem, and made it easy to debug.
I would expect code generators would appreciate keeping the array & it's
related checking, like overlap & bounds checking, a welcomed addition.

Adding such features in each potentially error-ing caller doesn't reduce the code size,
(it'll have to be replicated in several areas), and the return check
is simple & common (and already exists), so removing it will be
more work then augmenting the existing framework.
additionally, that's assuming the coder creates the correct check,
in different variants/locations.

ACK to Jan's patch.

>> It's actually
>> rather ironic that we're trying to add error checking to catch bugs that
>> real hardware is exposing, but assuming that emulated drivers always get
>> it right.  How will a return void help the emulated driver that has a
>> coding error?
>
> Drivers use fixed offsets so they will always fail or always work.
> If we return an error they might seem to work but behave incrrectly
> without the right capability.
>
Michael S. Tsirkin - Aug. 24, 2011, 9:51 a.m.
On Tue, Aug 23, 2011 at 04:59:16PM -0400, Don Dutile wrote:
> So we want to pollute the dev assignment code with knowledge of this array
> for bounds checking, which you're threatening to remove?

OKay, I'll stop being a mule, and apply that patch.
Michael S. Tsirkin - Aug. 24, 2011, 10:04 a.m.
On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> From: Alex Williamson <alex.williamson@redhat.com>
> 
> Nothing good can happen when we overlap capabilities
> 
> [ Jan: rebased over qemu, minor formatting ]
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

This doesn't build for me:

/scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
/scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’

I think that what that includes is the capability including each given
offset, right?  It would be easy to write some code scanning the
capability list to figure this value out.
Something along the lines of (untested):

static
uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
{                                       
    uint8_t next, prev, found = 0;

    if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
        return 0;

    for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
         prev = next + PCI_CAP_LIST_NEXT)
        if (next <= offset && next > found)
            found = next;

    return found;
}



> ---
>  hw/pci.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 6124790..ff20631 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1952,11 +1952,25 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>                         uint8_t offset, uint8_t size)
>  {
>      uint8_t *config;
> +    int i;
> +
>      if (!offset) {
>          offset = pci_find_space(pdev, size);
>          if (!offset) {
>              return -ENOSPC;
>          }
> +    } else {
> +        for (i = offset; i < offset + size; i++) {
> +            if (pdev->used[i]) {
> +                fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
> +                        "Attempt to add PCI capability %x at offset "
> +                        "%x overlaps existing capability %x at offset %x\n",
> +                        pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
> +                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
> +                        cap_id, offset, pdev->config_map[i], i);
> +                return -EFAULT;
> +            }
> +        }
>      }
>  
>      config = pdev->config + offset;
> -- 
> 1.7.3.4
Jan Kiszka - Aug. 24, 2011, 10:10 a.m.
On 2011-08-24 12:04, Michael S. Tsirkin wrote:
> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
>> From: Alex Williamson <alex.williamson@redhat.com>
>>
>> Nothing good can happen when we overlap capabilities
>>
>> [ Jan: rebased over qemu, minor formatting ]
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This doesn't build for me:
> 
> /scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
> /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’

Yeah, sorry, forgot to refresh the commit before posting.

> 
> I think that what that includes is the capability including each given
> offset, right?  It would be easy to write some code scanning the
> capability list to figure this value out.
> Something along the lines of (untested):
> 
> static
> uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
> {                                       
>     uint8_t next, prev, found = 0;
> 
>     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
>         return 0;
> 
>     for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
>          prev = next + PCI_CAP_LIST_NEXT)
>         if (next <= offset && next > found)
>             found = next;
> 
>     return found;
> }

Sounds useful, will enhance the patch.

(Originally, I just wanted to reduce the qemu-kvm delta... :) )

Jan
Michael S. Tsirkin - Aug. 24, 2011, 11:01 a.m.
On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote:
> On 2011-08-24 12:04, Michael S. Tsirkin wrote:
> > On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> >> From: Alex Williamson <alex.williamson@redhat.com>
> >>
> >> Nothing good can happen when we overlap capabilities
> >>
> >> [ Jan: rebased over qemu, minor formatting ]
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > This doesn't build for me:
> > 
> > /scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
> > /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
> 
> Yeah, sorry, forgot to refresh the commit before posting.

Happens to me too, sometimes.

> > 
> > I think that what that includes is the capability including each given
> > offset, right?  It would be easy to write some code scanning the
> > capability list to figure this value out.
> > Something along the lines of (untested):
> > 
> > static
> > uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
> > {                                       
> >     uint8_t next, prev, found = 0;
> > 
> >     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
> >         return 0;
> > 
> >     for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
> >          prev = next + PCI_CAP_LIST_NEXT)
> >         if (next <= offset && next > found)
> >             found = next;
> > 
> >     return found;
> > }
> 
> Sounds useful, will enhance the patch.
> 
> (Originally, I just wanted to reduce the qemu-kvm delta... :) )
> 
> Jan

Oh, if you want to just drop that bit, that's also fine.
Up to you.
Michael S. Tsirkin - Aug. 24, 2011, 11:58 a.m.
On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote:
> On 2011-08-24 12:04, Michael S. Tsirkin wrote:
> > On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> >> From: Alex Williamson <alex.williamson@redhat.com>
> >>
> >> Nothing good can happen when we overlap capabilities
> >>
> >> [ Jan: rebased over qemu, minor formatting ]
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > This doesn't build for me:
> > 
> > /scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
> > /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
> 
> Yeah, sorry, forgot to refresh the commit before posting.
> 
> > 
> > I think that what that includes is the capability including each given
> > offset, right?  It would be easy to write some code scanning the
> > capability list to figure this value out.
> > Something along the lines of (untested):
> > 
> > static
> > uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
> > {                                       
> >     uint8_t next, prev, found = 0;
> > 
> >     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
> >         return 0;
> > 
> >     for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
> >          prev = next + PCI_CAP_LIST_NEXT)
> >         if (next <= offset && next > found)
> >             found = next;
> > 
> >     return found;
> > }
> 
> Sounds useful, will enhance the patch.
> 
> (Originally, I just wanted to reduce the qemu-kvm delta... :) )
> 
> Jan

Also, let's add a comment documenting the
reason for this check: device assignment
depends on this check to verify that the device
is not broken.

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
Jan Kiszka - Aug. 24, 2011, 12:29 p.m.
On 2011-08-24 13:58, Michael S. Tsirkin wrote:
> On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote:
>> On 2011-08-24 12:04, Michael S. Tsirkin wrote:
>>> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
>>>> From: Alex Williamson <alex.williamson@redhat.com>
>>>>
>>>> Nothing good can happen when we overlap capabilities
>>>>
>>>> [ Jan: rebased over qemu, minor formatting ]
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> This doesn't build for me:
>>>
>>> /scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
>>> /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
>>
>> Yeah, sorry, forgot to refresh the commit before posting.
>>
>>>
>>> I think that what that includes is the capability including each given
>>> offset, right?  It would be easy to write some code scanning the
>>> capability list to figure this value out.
>>> Something along the lines of (untested):
>>>
>>> static
>>> uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
>>> {                                       
>>>     uint8_t next, prev, found = 0;
>>>
>>>     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
>>>         return 0;
>>>
>>>     for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
>>>          prev = next + PCI_CAP_LIST_NEXT)
>>>         if (next <= offset && next > found)
>>>             found = next;
>>>
>>>     return found;
>>> }
>>
>> Sounds useful, will enhance the patch.
>>
>> (Originally, I just wanted to reduce the qemu-kvm delta... :) )
>>
>> Jan
> 
> Also, let's add a comment documenting the
> reason for this check: device assignment
> depends on this check to verify that the device
> is not broken.

Based on the previous discussion, I don't think this is accurate as it
will also validate emulated devices.

Jan
Michael S. Tsirkin - Aug. 24, 2011, 12:34 p.m.
On Wed, Aug 24, 2011 at 02:29:36PM +0200, Jan Kiszka wrote:
> On 2011-08-24 13:58, Michael S. Tsirkin wrote:
> > On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote:
> >> On 2011-08-24 12:04, Michael S. Tsirkin wrote:
> >>> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> >>>> From: Alex Williamson <alex.williamson@redhat.com>
> >>>>
> >>>> Nothing good can happen when we overlap capabilities
> >>>>
> >>>> [ Jan: rebased over qemu, minor formatting ]
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>
> >>> This doesn't build for me:
> >>>
> >>> /scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
> >>> /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
> >>
> >> Yeah, sorry, forgot to refresh the commit before posting.
> >>
> >>>
> >>> I think that what that includes is the capability including each given
> >>> offset, right?  It would be easy to write some code scanning the
> >>> capability list to figure this value out.
> >>> Something along the lines of (untested):
> >>>
> >>> static
> >>> uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
> >>> {                                       
> >>>     uint8_t next, prev, found = 0;
> >>>
> >>>     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
> >>>         return 0;
> >>>
> >>>     for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
> >>>          prev = next + PCI_CAP_LIST_NEXT)
> >>>         if (next <= offset && next > found)
> >>>             found = next;
> >>>
> >>>     return found;
> >>> }
> >>
> >> Sounds useful, will enhance the patch.
> >>
> >> (Originally, I just wanted to reduce the qemu-kvm delta... :) )
> >>
> >> Jan
> > 
> > Also, let's add a comment documenting the
> > reason for this check: device assignment
> > depends on this check to verify that the device
> > is not broken.
> 
> Based on the previous discussion, I don't think this is accurate as it
> will also validate emulated devices.
> 
> Jan

Something like the below is accurate, right?

/* Device assignment depends on this check to verify that the device
   is not broken. Should never trigger for emulated devices,
   but it's helpful for debugging these.
 */


> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
Jan Kiszka - Aug. 24, 2011, 12:36 p.m.
On 2011-08-24 14:34, Michael S. Tsirkin wrote:
> On Wed, Aug 24, 2011 at 02:29:36PM +0200, Jan Kiszka wrote:
>> On 2011-08-24 13:58, Michael S. Tsirkin wrote:
>>> On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote:
>>>> On 2011-08-24 12:04, Michael S. Tsirkin wrote:
>>>>> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
>>>>>> From: Alex Williamson <alex.williamson@redhat.com>
>>>>>>
>>>>>> Nothing good can happen when we overlap capabilities
>>>>>>
>>>>>> [ Jan: rebased over qemu, minor formatting ]
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> This doesn't build for me:
>>>>>
>>>>> /scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
>>>>> /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
>>>>
>>>> Yeah, sorry, forgot to refresh the commit before posting.
>>>>
>>>>>
>>>>> I think that what that includes is the capability including each given
>>>>> offset, right?  It would be easy to write some code scanning the
>>>>> capability list to figure this value out.
>>>>> Something along the lines of (untested):
>>>>>
>>>>> static
>>>>> uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
>>>>> {                                       
>>>>>     uint8_t next, prev, found = 0;
>>>>>
>>>>>     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
>>>>>         return 0;
>>>>>
>>>>>     for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
>>>>>          prev = next + PCI_CAP_LIST_NEXT)
>>>>>         if (next <= offset && next > found)
>>>>>             found = next;
>>>>>
>>>>>     return found;
>>>>> }
>>>>
>>>> Sounds useful, will enhance the patch.
>>>>
>>>> (Originally, I just wanted to reduce the qemu-kvm delta... :) )
>>>>
>>>> Jan
>>>
>>> Also, let's add a comment documenting the
>>> reason for this check: device assignment
>>> depends on this check to verify that the device
>>> is not broken.
>>
>> Based on the previous discussion, I don't think this is accurate as it
>> will also validate emulated devices.
>>
>> Jan
> 
> Something like the below is accurate, right?
> 
> /* Device assignment depends on this check to verify that the device
>    is not broken. Should never trigger for emulated devices,
>    but it's helpful for debugging these.
>  */

I've expressed this in the commit message. Unless there is another
reason to do v3, maybe you can merge the comment on commit.

Jan
Michael S. Tsirkin - Aug. 24, 2011, 12:39 p.m.
On Wed, Aug 24, 2011 at 02:36:31PM +0200, Jan Kiszka wrote:
> On 2011-08-24 14:34, Michael S. Tsirkin wrote:
> > On Wed, Aug 24, 2011 at 02:29:36PM +0200, Jan Kiszka wrote:
> >> On 2011-08-24 13:58, Michael S. Tsirkin wrote:
> >>> On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote:
> >>>> On 2011-08-24 12:04, Michael S. Tsirkin wrote:
> >>>>> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
> >>>>>> From: Alex Williamson <alex.williamson@redhat.com>
> >>>>>>
> >>>>>> Nothing good can happen when we overlap capabilities
> >>>>>>
> >>>>>> [ Jan: rebased over qemu, minor formatting ]
> >>>>>>
> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>
> >>>>> This doesn't build for me:
> >>>>>
> >>>>> /scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
> >>>>> /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
> >>>>
> >>>> Yeah, sorry, forgot to refresh the commit before posting.
> >>>>
> >>>>>
> >>>>> I think that what that includes is the capability including each given
> >>>>> offset, right?  It would be easy to write some code scanning the
> >>>>> capability list to figure this value out.
> >>>>> Something along the lines of (untested):
> >>>>>
> >>>>> static
> >>>>> uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
> >>>>> {                                       
> >>>>>     uint8_t next, prev, found = 0;
> >>>>>
> >>>>>     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
> >>>>>         return 0;
> >>>>>
> >>>>>     for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
> >>>>>          prev = next + PCI_CAP_LIST_NEXT)
> >>>>>         if (next <= offset && next > found)
> >>>>>             found = next;
> >>>>>
> >>>>>     return found;
> >>>>> }
> >>>>
> >>>> Sounds useful, will enhance the patch.
> >>>>
> >>>> (Originally, I just wanted to reduce the qemu-kvm delta... :) )
> >>>>
> >>>> Jan
> >>>
> >>> Also, let's add a comment documenting the
> >>> reason for this check: device assignment
> >>> depends on this check to verify that the device
> >>> is not broken.
> >>
> >> Based on the previous discussion, I don't think this is accurate as it
> >> will also validate emulated devices.
> >>
> >> Jan
> > 
> > Something like the below is accurate, right?
> > 
> > /* Device assignment depends on this check to verify that the device
> >    is not broken. Should never trigger for emulated devices,
> >    but it's helpful for debugging these.
> >  */
> 
> I've expressed this in the commit message. Unless there is another
> reason to do v3, maybe you can merge the comment on commit.
> 
> Jan

Sure, I can do that, no need with v3. You are fine with the way
it's formulated?

> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux
Jan Kiszka - Aug. 24, 2011, 12:39 p.m.
On 2011-08-24 14:39, Michael S. Tsirkin wrote:
> On Wed, Aug 24, 2011 at 02:36:31PM +0200, Jan Kiszka wrote:
>> On 2011-08-24 14:34, Michael S. Tsirkin wrote:
>>> On Wed, Aug 24, 2011 at 02:29:36PM +0200, Jan Kiszka wrote:
>>>> On 2011-08-24 13:58, Michael S. Tsirkin wrote:
>>>>> On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote:
>>>>>> On 2011-08-24 12:04, Michael S. Tsirkin wrote:
>>>>>>> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote:
>>>>>>>> From: Alex Williamson <alex.williamson@redhat.com>
>>>>>>>>
>>>>>>>> Nothing good can happen when we overlap capabilities
>>>>>>>>
>>>>>>>> [ Jan: rebased over qemu, minor formatting ]
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>
>>>>>>> This doesn't build for me:
>>>>>>>
>>>>>>> /scm/qemu/hw/pci.c: In function ‘pci_add_capability’:
>>>>>>> /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named ‘config_map’
>>>>>>
>>>>>> Yeah, sorry, forgot to refresh the commit before posting.
>>>>>>
>>>>>>>
>>>>>>> I think that what that includes is the capability including each given
>>>>>>> offset, right?  It would be easy to write some code scanning the
>>>>>>> capability list to figure this value out.
>>>>>>> Something along the lines of (untested):
>>>>>>>
>>>>>>> static
>>>>>>> uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset)
>>>>>>> {                                       
>>>>>>>     uint8_t next, prev, found = 0;
>>>>>>>
>>>>>>>     if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
>>>>>>>         return 0;
>>>>>>>
>>>>>>>     for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
>>>>>>>          prev = next + PCI_CAP_LIST_NEXT)
>>>>>>>         if (next <= offset && next > found)
>>>>>>>             found = next;
>>>>>>>
>>>>>>>     return found;
>>>>>>> }
>>>>>>
>>>>>> Sounds useful, will enhance the patch.
>>>>>>
>>>>>> (Originally, I just wanted to reduce the qemu-kvm delta... :) )
>>>>>>
>>>>>> Jan
>>>>>
>>>>> Also, let's add a comment documenting the
>>>>> reason for this check: device assignment
>>>>> depends on this check to verify that the device
>>>>> is not broken.
>>>>
>>>> Based on the previous discussion, I don't think this is accurate as it
>>>> will also validate emulated devices.
>>>>
>>>> Jan
>>>
>>> Something like the below is accurate, right?
>>>
>>> /* Device assignment depends on this check to verify that the device
>>>    is not broken. Should never trigger for emulated devices,
>>>    but it's helpful for debugging these.
>>>  */
>>
>> I've expressed this in the commit message. Unless there is another
>> reason to do v3, maybe you can merge the comment on commit.
>>
>> Jan
> 
> Sure, I can do that, no need with v3. You are fine with the way
> it's formulated?

Yep.

Jan

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 6124790..ff20631 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1952,11 +1952,25 @@  int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size)
 {
     uint8_t *config;
+    int i;
+
     if (!offset) {
         offset = pci_find_space(pdev, size);
         if (!offset) {
             return -ENOSPC;
         }
+    } else {
+        for (i = offset; i < offset + size; i++) {
+            if (pdev->used[i]) {
+                fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
+                        "Attempt to add PCI capability %x at offset "
+                        "%x overlaps existing capability %x at offset %x\n",
+                        pci_find_domain(pdev->bus), pci_bus_num(pdev->bus),
+                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
+                        cap_id, offset, pdev->config_map[i], i);
+                return -EFAULT;
+            }
+        }
     }
 
     config = pdev->config + offset;