* [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups
@ 2011-04-29 9:05 Jan Kiszka
2011-04-29 9:05 ` [PATCH v2 1/6] pci-assign: Clean up assigned_dev_pci_read/write_config Jan Kiszka
` (7 more replies)
0 siblings, 8 replies; 12+ messages in thread
From: Jan Kiszka @ 2011-04-29 9:05 UTC (permalink / raw)
To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Alex Williamson
This round addresses the review comments and also add another fix to
ensure that non-virtualized config areas always hit the real device.
Jan Kiszka (6):
pci-assign: Clean up assigned_dev_pci_read/write_config
pci-assign: Move merge_bits
pci-assign: Fix dword read at PCI_COMMAND
pci-assign: Properly handle more overlapping accesses
pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config
pci-assign: Convert need_emulate_cmd into a bitmask
hw/device-assignment.c | 125 +++++++++++++++++++++++++++++++----------------
hw/device-assignment.h | 2 +-
2 files changed, 83 insertions(+), 44 deletions(-)
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v2 1/6] pci-assign: Clean up assigned_dev_pci_read/write_config 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka @ 2011-04-29 9:05 ` Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 2/6] pci-assign: Move merge_bits Jan Kiszka ` (6 subsequent siblings) 7 siblings, 0 replies; 12+ messages in thread From: Jan Kiszka @ 2011-04-29 9:05 UTC (permalink / raw) To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Alex Williamson Use ranges_overlap and proper constants to match the access range against regions that need special handling. This also fixes yet uncaught high-byte write access to the command register. Moreover, use more constants instead of magic numbers. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/device-assignment.c | 39 +++++++++++++++++++++++++++++---------- 1 files changed, 29 insertions(+), 10 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index 606d725..0bf93b1 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -404,13 +404,20 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, return assigned_device_pci_cap_write_config(d, address, val, len); } - if (address == 0x4) { + if (ranges_overlap(address, len, PCI_COMMAND, 2)) { pci_default_write_config(d, address, val, len); /* Continue to program the card */ } - if ((address >= 0x10 && address <= 0x24) || address == 0x30 || - address == 0x34 || address == 0x3c || address == 0x3d) { + /* + * Catch access to + * - base address registers + * - ROM base address & capability pointer + * - interrupt line & pin + */ + if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || + ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || + ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { /* used for update-mappings (BAR emulation) */ pci_default_write_config(d, address, val, len); return; @@ -450,9 +457,20 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, return val; } - if (address < 0x4 || (pci_dev->need_emulate_cmd && address == 0x4) || - (address >= 0x10 && address <= 0x24) || address == 0x30 || - address == 0x34 || address == 0x3c || address == 0x3d) { + /* + * Catch access to + * - vendor & device ID + * - command register (if emulation needed) + * - base address registers + * - ROM base address & capability pointer + * - interrupt line & pin + */ + if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) || + (pci_dev->need_emulate_cmd && + ranges_overlap(address, len, PCI_COMMAND, 2)) || + ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || + ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || + ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { val = pci_default_read_config(d, address, len); DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); @@ -483,10 +501,11 @@ do_log: if (!pci_dev->cap.available) { /* kill the special capabilities */ - if (address == 4 && len == 4) - val &= ~0x100000; - else if (address == 6) - val &= ~0x10; + if (address == PCI_COMMAND && len == 4) { + val &= ~(PCI_STATUS_CAP_LIST << 16); + } else if (address == PCI_STATUS) { + val &= ~PCI_STATUS_CAP_LIST; + } } return val; -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/6] pci-assign: Move merge_bits 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 1/6] pci-assign: Clean up assigned_dev_pci_read/write_config Jan Kiszka @ 2011-04-29 9:05 ` Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 3/6] pci-assign: Fix dword read at PCI_COMMAND Jan Kiszka ` (5 subsequent siblings) 7 siblings, 0 replies; 12+ messages in thread From: Jan Kiszka @ 2011-04-29 9:05 UTC (permalink / raw) To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Alex Williamson We will need it earlier in the file, so move it unmodified to the top. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/device-assignment.c | 44 ++++++++++++++++++++++---------------------- 1 files changed, 22 insertions(+), 22 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index 0bf93b1..ea1d7f1 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -71,6 +71,28 @@ static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev, static uint32_t assigned_device_pci_cap_read_config(PCIDevice *pci_dev, uint32_t address, int len); +/* Merge the bits set in mask from mval into val. Both val and mval are + * at the same addr offset, pos is the starting offset of the mask. */ +static uint32_t merge_bits(uint32_t val, uint32_t mval, uint8_t addr, + int len, uint8_t pos, uint32_t mask) +{ + if (!ranges_overlap(addr, len, pos, 4)) { + return val; + } + + if (addr >= pos) { + mask >>= (addr - pos) * 8; + } else { + mask <<= (pos - addr) * 8; + } + mask &= 0xffffffffU >> (4 - len) * 8; + + val &= ~mask; + val |= (mval & mask); + + return val; +} + static uint32_t assigned_dev_ioport_rw(AssignedDevRegion *dev_region, uint32_t addr, int len, uint32_t *val) { @@ -1278,28 +1300,6 @@ static uint8_t find_vndr_start(PCIDevice *pci_dev, uint32_t address) return cap; } -/* Merge the bits set in mask from mval into val. Both val and mval are - * at the same addr offset, pos is the starting offset of the mask. */ -static uint32_t merge_bits(uint32_t val, uint32_t mval, uint8_t addr, - int len, uint8_t pos, uint32_t mask) -{ - if (!ranges_overlap(addr, len, pos, 4)) { - return val; - } - - if (addr >= pos) { - mask >>= (addr - pos) * 8; - } else { - mask <<= (pos - addr) * 8; - } - mask &= 0xffffffffU >> (4 - len) * 8; - - val &= ~mask; - val |= (mval & mask); - - return val; -} - static uint32_t assigned_device_pci_cap_read_config(PCIDevice *pci_dev, uint32_t address, int len) { -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/6] pci-assign: Fix dword read at PCI_COMMAND 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 1/6] pci-assign: Clean up assigned_dev_pci_read/write_config Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 2/6] pci-assign: Move merge_bits Jan Kiszka @ 2011-04-29 9:05 ` Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses Jan Kiszka ` (4 subsequent siblings) 7 siblings, 0 replies; 12+ messages in thread From: Jan Kiszka @ 2011-04-29 9:05 UTC (permalink / raw) To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Alex Williamson If we emulate the command register, we must only read its content from the shadow config space. For dword read of both PCI_COMMAND and PCI_STATUS, at least the latter must be read from the device. For simplicity reasons and as the code path is not considered performance critical for the affected SRIOV devices, the fix performes device access to the command word unconditionally, even if emulation is enabled and only that word is read. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/device-assignment.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index ea1d7f1..cea072e 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -482,14 +482,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, /* * Catch access to * - vendor & device ID - * - command register (if emulation needed) * - base address registers * - ROM base address & capability pointer * - interrupt line & pin */ if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) || - (pci_dev->need_emulate_cmd && - ranges_overlap(address, len, PCI_COMMAND, 2)) || ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { @@ -521,6 +518,11 @@ do_log: DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); + if (pci_dev->need_emulate_cmd) { + val = merge_bits(val, pci_default_read_config(d, address, len), + address, len, PCI_COMMAND, 0xffff); + } + if (!pci_dev->cap.available) { /* kill the special capabilities */ if (address == PCI_COMMAND && len == 4) { -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka ` (2 preceding siblings ...) 2011-04-29 9:05 ` [PATCH v2 3/6] pci-assign: Fix dword read at PCI_COMMAND Jan Kiszka @ 2011-04-29 9:05 ` Jan Kiszka 2011-04-29 20:41 ` Alex Williamson 2011-04-29 9:05 ` [PATCH v2 5/6] pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config Jan Kiszka ` (3 subsequent siblings) 7 siblings, 1 reply; 12+ messages in thread From: Jan Kiszka @ 2011-04-29 9:05 UTC (permalink / raw) To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Alex Williamson Ensure that accesses exceeding PCI_CAPABILITY_LIST and PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not virtualize. Again, we do not optimize these checks and accesses a lot, they are considered to be slow paths. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/device-assignment.c | 34 +++++++++++++++++++++++++++++----- 1 files changed, 29 insertions(+), 5 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index cea072e..37c77e3 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { /* used for update-mappings (BAR emulation) */ pci_default_write_config(d, address, val, len); - return; + + /* Ensure that writes to overlapping areas we don't virtualize still + * hit the device. */ + switch (address) { + case PCI_CAPABILITY_LIST: + if (len > 1) { + len -= 1; + address += 1; + val >>= 8; + break; /* continue writing to the device */ + } + return; + case PCI_INTERRUPT_LINE: + if (len > 2) { + len -= 2; + address += 2; + val >>= 16; + break; /* continue writing to the device */ + } + return; + default: + return; + } } DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n", @@ -467,7 +489,7 @@ again: static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, int len) { - uint32_t val = 0; + uint32_t val = 0, virt_val; int fd; ssize_t ret; AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d); @@ -484,12 +506,10 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, * - vendor & device ID * - base address registers * - ROM base address & capability pointer - * - interrupt line & pin */ if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) || ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || - ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || - ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { + ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { val = pci_default_read_config(d, address, len); DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); @@ -523,6 +543,10 @@ do_log: address, len, PCI_COMMAND, 0xffff); } + virt_val = pci_default_read_config(d, address, len); + val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff); + val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff); + if (!pci_dev->cap.available) { /* kill the special capabilities */ if (address == PCI_COMMAND && len == 4) { -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses 2011-04-29 9:05 ` [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses Jan Kiszka @ 2011-04-29 20:41 ` Alex Williamson 2011-05-02 10:29 ` [PATCH v3 " Jan Kiszka 0 siblings, 1 reply; 12+ messages in thread From: Alex Williamson @ 2011-04-29 20:41 UTC (permalink / raw) To: Jan Kiszka; +Cc: Avi Kivity, Marcelo Tosatti, kvm On Fri, 2011-04-29 at 11:05 +0200, Jan Kiszka wrote: > Ensure that accesses exceeding PCI_CAPABILITY_LIST and > PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not > virtualize. Again, we do not optimize these checks and accesses a lot, > they are considered to be slow paths. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > hw/device-assignment.c | 34 +++++++++++++++++++++++++++++----- > 1 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c > index cea072e..37c77e3 100644 > --- a/hw/device-assignment.c > +++ b/hw/device-assignment.c > @@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, > ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { > /* used for update-mappings (BAR emulation) */ > pci_default_write_config(d, address, val, len); > - return; > + > + /* Ensure that writes to overlapping areas we don't virtualize still > + * hit the device. */ > + switch (address) { > + case PCI_CAPABILITY_LIST: > + if (len > 1) { > + len -= 1; > + address += 1; > + val >>= 8; > + break; /* continue writing to the device */ > + } > + return; > + case PCI_INTERRUPT_LINE: > + if (len > 2) { > + len -= 2; > + address += 2; > + val >>= 16; > + break; /* continue writing to the device */ > + } > + return; > + default: > + return; > + } > } It seems like we could be more symmetric with the below read. Maybe something like: if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { pci_default_write_config(d, address, val, len); return; } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) || ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { uint32_t real_val = assigned_dev_pci_read(d, address, len); pci_default_write_config(d, address, val, len); val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff); val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff); } We might write out to real hardware when we could avoid it, but I don't think it matters. Thanks, Alex > DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n", > @@ -467,7 +489,7 @@ again: > static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, > int len) > { > - uint32_t val = 0; > + uint32_t val = 0, virt_val; > int fd; > ssize_t ret; > AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d); > @@ -484,12 +506,10 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, > * - vendor & device ID > * - base address registers > * - ROM base address & capability pointer > - * - interrupt line & pin > */ > if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) || > ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || > - ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || > - ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { > + ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { > val = pci_default_read_config(d, address, len); > DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", > (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); > @@ -523,6 +543,10 @@ do_log: > address, len, PCI_COMMAND, 0xffff); > } > > + virt_val = pci_default_read_config(d, address, len); > + val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff); > + val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff); > + > if (!pci_dev->cap.available) { > /* kill the special capabilities */ > if (address == PCI_COMMAND && len == 4) { ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 4/6] pci-assign: Properly handle more overlapping accesses 2011-04-29 20:41 ` Alex Williamson @ 2011-05-02 10:29 ` Jan Kiszka 2011-05-02 14:09 ` Alex Williamson 0 siblings, 1 reply; 12+ messages in thread From: Jan Kiszka @ 2011-05-02 10:29 UTC (permalink / raw) To: Alex Williamson; +Cc: Avi Kivity, Marcelo Tosatti, kvm@vger.kernel.org On 2011-04-29 22:41, Alex Williamson wrote: > On Fri, 2011-04-29 at 11:05 +0200, Jan Kiszka wrote: >> Ensure that accesses exceeding PCI_CAPABILITY_LIST and >> PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not >> virtualize. Again, we do not optimize these checks and accesses a lot, >> they are considered to be slow paths. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> hw/device-assignment.c | 34 +++++++++++++++++++++++++++++----- >> 1 files changed, 29 insertions(+), 5 deletions(-) >> >> diff --git a/hw/device-assignment.c b/hw/device-assignment.c >> index cea072e..37c77e3 100644 >> --- a/hw/device-assignment.c >> +++ b/hw/device-assignment.c >> @@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, >> ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { >> /* used for update-mappings (BAR emulation) */ >> pci_default_write_config(d, address, val, len); >> - return; >> + >> + /* Ensure that writes to overlapping areas we don't virtualize still >> + * hit the device. */ >> + switch (address) { >> + case PCI_CAPABILITY_LIST: >> + if (len > 1) { >> + len -= 1; >> + address += 1; >> + val >>= 8; >> + break; /* continue writing to the device */ >> + } >> + return; >> + case PCI_INTERRUPT_LINE: >> + if (len > 2) { >> + len -= 2; >> + address += 2; >> + val >>= 16; >> + break; /* continue writing to the device */ >> + } >> + return; >> + default: >> + return; >> + } >> } > > It seems like we could be more symmetric with the below read. Maybe > something like: > > if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || > ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { > pci_default_write_config(d, address, val, len); > return; > } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) || > ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { > uint32_t real_val = assigned_dev_pci_read(d, address, len); > pci_default_write_config(d, address, val, len); > val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff); > val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff); > } > > We might write out to real hardware when we could avoid it, but I don't > think it matters. Thanks, > Yep, makes sense. Find such a version below. Thanks, Jan ------8<------- Ensure that accesses exceeding PCI_CAPABILITY_LIST and PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not virtualize. Again, we do not optimize these checks and accesses a lot, they are considered to be slow paths. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/device-assignment.c | 34 ++++++++++++++++++++++++++-------- 1 files changed, 26 insertions(+), 8 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index cea072e..8e95730 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -438,11 +438,22 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, * - interrupt line & pin */ if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || - ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || - ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { - /* used for update-mappings (BAR emulation) */ + ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { pci_default_write_config(d, address, val, len); return; + } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) || + ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { + uint32_t real_val; + + pci_default_write_config(d, address, val, len); + + /* Ensure that writes to overlapping areas we don't virtualize still + * hit the device. */ + real_val = assigned_dev_pci_read(d, address, len); + val = merge_bits(val, real_val, address, len, + PCI_CAPABILITY_LIST, 0xff); + val = merge_bits(val, real_val, address, len, + PCI_INTERRUPT_LINE, 0xffff); } DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n", @@ -467,7 +478,7 @@ again: static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, int len) { - uint32_t val = 0; + uint32_t val = 0, virt_val; int fd; ssize_t ret; AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d); @@ -483,13 +494,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, * Catch access to * - vendor & device ID * - base address registers - * - ROM base address & capability pointer - * - interrupt line & pin + * - ROM base address */ if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) || ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || - ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || - ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { + ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { val = pci_default_read_config(d, address, len); DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); @@ -523,6 +532,15 @@ do_log: address, len, PCI_COMMAND, 0xffff); } + /* + * Merge bits from virtualized + * - capability pointer + * - interrupt line & pin + */ + virt_val = pci_default_read_config(d, address, len); + val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff); + val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff); + if (!pci_dev->cap.available) { /* kill the special capabilities */ if (address == PCI_COMMAND && len == 4) { -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 4/6] pci-assign: Properly handle more overlapping accesses 2011-05-02 10:29 ` [PATCH v3 " Jan Kiszka @ 2011-05-02 14:09 ` Alex Williamson 0 siblings, 0 replies; 12+ messages in thread From: Alex Williamson @ 2011-05-02 14:09 UTC (permalink / raw) To: Jan Kiszka; +Cc: Avi Kivity, Marcelo Tosatti, kvm@vger.kernel.org On Mon, 2011-05-02 at 12:29 +0200, Jan Kiszka wrote: > On 2011-04-29 22:41, Alex Williamson wrote: > > On Fri, 2011-04-29 at 11:05 +0200, Jan Kiszka wrote: > >> Ensure that accesses exceeding PCI_CAPABILITY_LIST and > >> PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not > >> virtualize. Again, we do not optimize these checks and accesses a lot, > >> they are considered to be slow paths. > >> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >> --- > >> hw/device-assignment.c | 34 +++++++++++++++++++++++++++++----- > >> 1 files changed, 29 insertions(+), 5 deletions(-) > >> > >> diff --git a/hw/device-assignment.c b/hw/device-assignment.c > >> index cea072e..37c77e3 100644 > >> --- a/hw/device-assignment.c > >> +++ b/hw/device-assignment.c > >> @@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, > >> ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { > >> /* used for update-mappings (BAR emulation) */ > >> pci_default_write_config(d, address, val, len); > >> - return; > >> + > >> + /* Ensure that writes to overlapping areas we don't virtualize still > >> + * hit the device. */ > >> + switch (address) { > >> + case PCI_CAPABILITY_LIST: > >> + if (len > 1) { > >> + len -= 1; > >> + address += 1; > >> + val >>= 8; > >> + break; /* continue writing to the device */ > >> + } > >> + return; > >> + case PCI_INTERRUPT_LINE: > >> + if (len > 2) { > >> + len -= 2; > >> + address += 2; > >> + val >>= 16; > >> + break; /* continue writing to the device */ > >> + } > >> + return; > >> + default: > >> + return; > >> + } > >> } > > > > It seems like we could be more symmetric with the below read. Maybe > > something like: > > > > if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || > > ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { > > pci_default_write_config(d, address, val, len); > > return; > > } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) || > > ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { > > uint32_t real_val = assigned_dev_pci_read(d, address, len); > > pci_default_write_config(d, address, val, len); > > val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff); > > val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff); > > } > > > > We might write out to real hardware when we could avoid it, but I don't > > think it matters. Thanks, > > > > Yep, makes sense. Find such a version below. > > Thanks, > Jan > > > ------8<------- > > Ensure that accesses exceeding PCI_CAPABILITY_LIST and > PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not > virtualize. Again, we do not optimize these checks and accesses a lot, > they are considered to be slow paths. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Acked-by: Alex Williamson <alex.williamson@redhat.com> > --- > hw/device-assignment.c | 34 ++++++++++++++++++++++++++-------- > 1 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c > index cea072e..8e95730 100644 > --- a/hw/device-assignment.c > +++ b/hw/device-assignment.c > @@ -438,11 +438,22 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address, > * - interrupt line & pin > */ > if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || > - ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || > - ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { > - /* used for update-mappings (BAR emulation) */ > + ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { > pci_default_write_config(d, address, val, len); > return; > + } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) || > + ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { > + uint32_t real_val; > + > + pci_default_write_config(d, address, val, len); > + > + /* Ensure that writes to overlapping areas we don't virtualize still > + * hit the device. */ > + real_val = assigned_dev_pci_read(d, address, len); > + val = merge_bits(val, real_val, address, len, > + PCI_CAPABILITY_LIST, 0xff); > + val = merge_bits(val, real_val, address, len, > + PCI_INTERRUPT_LINE, 0xffff); > } > > DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n", > @@ -467,7 +478,7 @@ again: > static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, > int len) > { > - uint32_t val = 0; > + uint32_t val = 0, virt_val; > int fd; > ssize_t ret; > AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d); > @@ -483,13 +494,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, > * Catch access to > * - vendor & device ID > * - base address registers > - * - ROM base address & capability pointer > - * - interrupt line & pin > + * - ROM base address > */ > if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) || > ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) || > - ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) || > - ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) { > + ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) { > val = pci_default_read_config(d, address, len); > DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", > (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); > @@ -523,6 +532,15 @@ do_log: > address, len, PCI_COMMAND, 0xffff); > } > > + /* > + * Merge bits from virtualized > + * - capability pointer > + * - interrupt line & pin > + */ > + virt_val = pci_default_read_config(d, address, len); > + val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff); > + val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff); > + > if (!pci_dev->cap.available) { > /* kill the special capabilities */ > if (address == PCI_COMMAND && len == 4) { ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 5/6] pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka ` (3 preceding siblings ...) 2011-04-29 9:05 ` [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses Jan Kiszka @ 2011-04-29 9:05 ` Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 6/6] pci-assign: Convert need_emulate_cmd into a bitmask Jan Kiszka ` (2 subsequent siblings) 7 siblings, 0 replies; 12+ messages in thread From: Jan Kiszka @ 2011-04-29 9:05 UTC (permalink / raw) To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Alex Williamson No one can remember where this came from, and it looks very hacky anyway (we return 0 for config space address 0xfc of _every_ assigned device, not only vga as the comment claims). So better remove it and wait for the underlying issue to reappear. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/device-assignment.c | 5 ----- 1 files changed, 0 insertions(+), 5 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index 37c77e3..7db34c4 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -516,10 +516,6 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address, return val; } - /* vga specific, remove later */ - if (address == 0xFC) - goto do_log; - fd = pci_dev->real_device.config_fd; again: @@ -534,7 +530,6 @@ again: exit(1); } -do_log: DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 6/6] pci-assign: Convert need_emulate_cmd into a bitmask 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka ` (4 preceding siblings ...) 2011-04-29 9:05 ` [PATCH v2 5/6] pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config Jan Kiszka @ 2011-04-29 9:05 ` Jan Kiszka 2011-05-02 14:11 ` [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Alex Williamson 2011-05-03 19:17 ` Marcelo Tosatti 7 siblings, 0 replies; 12+ messages in thread From: Jan Kiszka @ 2011-04-29 9:05 UTC (permalink / raw) To: Avi Kivity, Marcelo Tosatti; +Cc: kvm, Alex Williamson Define a mask of PCI command register bits that need to be emulated, i.e. read back from their shadow state. We will need this for selectively emulating the INTx mask bit. Note: No initialization of emulate_cmd_mask to zero needed, the device state is already zero-initialized. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/device-assignment.c | 11 +++++------ hw/device-assignment.h | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/hw/device-assignment.c b/hw/device-assignment.c index 7db34c4..4625527 100644 --- a/hw/device-assignment.c +++ b/hw/device-assignment.c @@ -533,9 +533,9 @@ again: DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); - if (pci_dev->need_emulate_cmd) { + if (pci_dev->emulate_cmd_mask) { val = merge_bits(val, pci_default_read_config(d, address, len), - address, len, PCI_COMMAND, 0xffff); + address, len, PCI_COMMAND, pci_dev->emulate_cmd_mask); } virt_val = pci_default_read_config(d, address, len); @@ -802,10 +802,9 @@ again: /* dealing with virtual function device */ snprintf(name, sizeof(name), "%sphysfn/", dir); - if (!stat(name, &statbuf)) - pci_dev->need_emulate_cmd = 1; - else - pci_dev->need_emulate_cmd = 0; + if (!stat(name, &statbuf)) { + pci_dev->emulate_cmd_mask = 0xffff; + } dev->region_number = r; return 0; diff --git a/hw/device-assignment.h b/hw/device-assignment.h index 86af0a9..ae1bc58 100644 --- a/hw/device-assignment.h +++ b/hw/device-assignment.h @@ -109,7 +109,7 @@ typedef struct AssignedDevice { void *msix_table_page; target_phys_addr_t msix_table_addr; int mmio_index; - int need_emulate_cmd; + uint32_t emulate_cmd_mask; char *configfd_name; int32_t bootindex; QLIST_ENTRY(AssignedDevice) next; -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka ` (5 preceding siblings ...) 2011-04-29 9:05 ` [PATCH v2 6/6] pci-assign: Convert need_emulate_cmd into a bitmask Jan Kiszka @ 2011-05-02 14:11 ` Alex Williamson 2011-05-03 19:17 ` Marcelo Tosatti 7 siblings, 0 replies; 12+ messages in thread From: Alex Williamson @ 2011-05-02 14:11 UTC (permalink / raw) To: Jan Kiszka; +Cc: Avi Kivity, Marcelo Tosatti, kvm On Fri, 2011-04-29 at 11:05 +0200, Jan Kiszka wrote: > This round addresses the review comments and also add another fix to > ensure that non-virtualized config areas always hit the real device. > > Jan Kiszka (6): > pci-assign: Clean up assigned_dev_pci_read/write_config > pci-assign: Move merge_bits > pci-assign: Fix dword read at PCI_COMMAND > pci-assign: Properly handle more overlapping accesses > pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config > pci-assign: Convert need_emulate_cmd into a bitmask > > hw/device-assignment.c | 125 +++++++++++++++++++++++++++++++---------------- > hw/device-assignment.h | 2 +- > 2 files changed, 83 insertions(+), 44 deletions(-) > For patches 1-3,5,6: Acked-by: Alex Williamson <alex.williamson@redhat.com> v3 version of 4 acked separately ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka ` (6 preceding siblings ...) 2011-05-02 14:11 ` [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Alex Williamson @ 2011-05-03 19:17 ` Marcelo Tosatti 7 siblings, 0 replies; 12+ messages in thread From: Marcelo Tosatti @ 2011-05-03 19:17 UTC (permalink / raw) To: Jan Kiszka; +Cc: Avi Kivity, kvm, Alex Williamson On Fri, Apr 29, 2011 at 11:05:27AM +0200, Jan Kiszka wrote: > This round addresses the review comments and also add another fix to > ensure that non-virtualized config areas always hit the real device. > > Jan Kiszka (6): > pci-assign: Clean up assigned_dev_pci_read/write_config > pci-assign: Move merge_bits > pci-assign: Fix dword read at PCI_COMMAND > pci-assign: Properly handle more overlapping accesses > pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config > pci-assign: Convert need_emulate_cmd into a bitmask Applied, thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-05-03 19:20 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 1/6] pci-assign: Clean up assigned_dev_pci_read/write_config Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 2/6] pci-assign: Move merge_bits Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 3/6] pci-assign: Fix dword read at PCI_COMMAND Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses Jan Kiszka 2011-04-29 20:41 ` Alex Williamson 2011-05-02 10:29 ` [PATCH v3 " Jan Kiszka 2011-05-02 14:09 ` Alex Williamson 2011-04-29 9:05 ` [PATCH v2 5/6] pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config Jan Kiszka 2011-04-29 9:05 ` [PATCH v2 6/6] pci-assign: Convert need_emulate_cmd into a bitmask Jan Kiszka 2011-05-02 14:11 ` [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Alex Williamson 2011-05-03 19:17 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).