| Message ID | 4E53E328.90601@siemens.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 > >
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 > > > >
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.
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
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. >
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.
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
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
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.
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
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
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
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
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
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
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;