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@xxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxx>
Acked-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
> 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) {
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
