Message ID | 1425390913-17726-1-git-send-email-kraxel@redhat.com |
---|---|
State | New |
Headers | show |
On Tue, Mar 03, 2015 at 02:55:13PM +0100, Gerd Hoffmann wrote: > This patch makes the bar layout for virtio pci devices configurable. > This is used to > (a) create different layouts for legacy/transitional vs. modern > devices. > (b) make sure we use unused pci bars, by checking > PCIDevice->io_regions > > VirtIOPCIProxy subclasses which need additional pci bars, such as > virtio-vga, just need to make sure they register the additinal bars > before initializing virtio-pci, so the superclass can see the registered > bars and shuffle around the virtio bars accordingly. I think I prefer we just DTRT and keep same layouts for everyone by default: isn't there a layout that is good for everybody? Maybe you could give two examples that require two incompatible layouts? I know this means we'll leave BAR0 unused for modern devices but that does not seem too bad. > The patch also makes sure the modern bar is properly aligned so it can > be a 64bit bar, and the prefetchable + 64bit attributes are enabled. This can be a separate patch? > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/virtio/virtio-pci.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 6b1f422..a2cfb1f 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -965,8 +965,6 @@ static void virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, > PCIDevice *dev = &proxy->pci_dev; > int offset; > > - cap->bar = 2; > - > offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len); > assert(offset > 0); > > @@ -1232,9 +1230,12 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr, > static void virtio_pci_device_plugged(DeviceState *d) > { > VirtIOPCIProxy *proxy = VIRTIO_PCI(d); > + PCIDevice *pci = PCI_DEVICE(d); > VirtioBusState *bus = &proxy->bus; > bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY); > bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); > + uint32_t modern_mem_bar; /* 64bit bar */ > + uint32_t msix_bar; /* 32bit bar */ > uint8_t *config; > uint32_t size; > > @@ -1248,6 +1249,12 @@ static void virtio_pci_device_plugged(DeviceState *d) > pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID, > pci_get_word(config + PCI_VENDOR_ID)); > pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus)); > + msix_bar = 1; > + modern_mem_bar = 2; > + while (pci->io_regions[modern_mem_bar].size || > + pci->io_regions[modern_mem_bar+1].size) { > + modern_mem_bar += 2; > + } > } else { > /* pure virtio-1.0 */ > pci_set_word(config + PCI_VENDOR_ID, > @@ -1255,7 +1262,18 @@ static void virtio_pci_device_plugged(DeviceState *d) > pci_set_word(config + PCI_DEVICE_ID, > 0x1040 + virtio_bus_get_vdev_id(bus)); > pci_config_set_revision(config, 1); > + modern_mem_bar = 0; > + while (pci->io_regions[modern_mem_bar].size || > + pci->io_regions[modern_mem_bar+1].size) { > + modern_mem_bar += 2; > + } > + msix_bar = modern_mem_bar + 2; > + while (pci->io_regions[msix_bar].size) { > + msix_bar++; > + } > } > + assert(modern_mem_bar <= 4); > + assert(msix_bar <= 5); > config[PCI_INTERRUPT_PIN] = 1; > > > @@ -1263,29 +1281,34 @@ static void virtio_pci_device_plugged(DeviceState *d) > struct virtio_pci_cap common = { > .cfg_type = VIRTIO_PCI_CAP_COMMON_CFG, > .cap_len = sizeof common, > + .bar = modern_mem_bar, > .offset = cpu_to_le32(0x0), > .length = cpu_to_le32(0x1000), > }; > struct virtio_pci_cap isr = { > .cfg_type = VIRTIO_PCI_CAP_ISR_CFG, > .cap_len = sizeof isr, > + .bar = modern_mem_bar, > .offset = cpu_to_le32(0x1000), > .length = cpu_to_le32(0x1000), > }; > struct virtio_pci_cap device = { > .cfg_type = VIRTIO_PCI_CAP_DEVICE_CFG, > .cap_len = sizeof device, > + .bar = modern_mem_bar, > .offset = cpu_to_le32(0x2000), > .length = cpu_to_le32(0x1000), > }; > struct virtio_pci_notify_cap notify = { > .cap.cfg_type = VIRTIO_PCI_CAP_NOTIFY_CFG, > .cap.cap_len = sizeof notify, > + .cap.bar = modern_mem_bar, > .cap.offset = cpu_to_le32(0x3000), > .cap.length = cpu_to_le32(QEMU_VIRTIO_PCI_QUEUE_MEM_MULT * > VIRTIO_PCI_QUEUE_MAX), > .notify_off_multiplier = cpu_to_le32(QEMU_VIRTIO_PCI_QUEUE_MEM_MULT), > }; > + uint32_t modern_mem_attr; > > static const MemoryRegionOps common_ops = { > .read = virtio_pci_common_read, > @@ -1359,12 +1382,15 @@ static void virtio_pci_device_plugged(DeviceState *d) > QEMU_VIRTIO_PCI_QUEUE_MEM_MULT * > VIRTIO_PCI_QUEUE_MAX); > memory_region_add_subregion(&proxy->modern_bar, 0x3000, &proxy->notify); > - pci_register_bar(&proxy->pci_dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, > - &proxy->modern_bar); > + modern_mem_attr = (PCI_BASE_ADDRESS_SPACE_MEMORY | > + PCI_BASE_ADDRESS_MEM_PREFETCH | > + PCI_BASE_ADDRESS_MEM_TYPE_64); > + pci_register_bar(&proxy->pci_dev, modern_mem_bar, > + modern_mem_attr, &proxy->modern_bar); > } > > if (proxy->nvectors && > - msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) { > + msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, msix_bar)) { > error_report("unable to init msix vectors to %" PRIu32, > proxy->nvectors); > proxy->nvectors = 0; > -- > 1.8.3.1
Hi, > > VirtIOPCIProxy subclasses which need additional pci bars, such as > > virtio-vga, just need to make sure they register the additinal bars > > before initializing virtio-pci, so the superclass can see the registered > > bars and shuffle around the virtio bars accordingly. > > I think I prefer we just DTRT and keep same layouts for everyone by > default: isn't there a layout that is good for everybody? I want bar #2 for the vga framebuffer for virtio-vga. Which conflicts with bar #2 being used for the modern bar in todays code. We can move the modern bar to #4 for everybody, then we'll have: #0 -- legacy i/o #1 -- msix #2 -- unused (by virtio) #3 -- unused (by virtio) #4 -- modern mem #5 -- modern mem too (because it's 64bit). That'll leave bars #2 + #3 free, for additional bars (1x 64bit or 2x 32bit) such as vga framebuffer if needed. > I know this means we'll leave BAR0 unused for modern devices but that > does not seem too bad. Yep, no technical reason against it, although it IMHO looks nicer. cheers, Gerd
On Wed, Mar 04, 2015 at 11:55:50AM +0100, Gerd Hoffmann wrote: > Hi, > > > > VirtIOPCIProxy subclasses which need additional pci bars, such as > > > virtio-vga, just need to make sure they register the additinal bars > > > before initializing virtio-pci, so the superclass can see the registered > > > bars and shuffle around the virtio bars accordingly. > > > > I think I prefer we just DTRT and keep same layouts for everyone by > > default: isn't there a layout that is good for everybody? > > I want bar #2 for the vga framebuffer for virtio-vga. Which conflicts > with bar #2 being used for the modern bar in todays code. OK, and IIUC you are saying that using bar #2 for that is a requirement for vga to work? > We can move the modern bar to #4 for everybody, then we'll have: > > #0 -- legacy i/o > #1 -- msix > #2 -- unused (by virtio) > #3 -- unused (by virtio) > #4 -- modern mem > #5 -- modern mem too (because it's 64bit). > > That'll leave bars #2 + #3 free, for additional bars (1x 64bit or 2x > 32bit) such as vga framebuffer if needed. Yes, that's fine with me. > > I know this means we'll leave BAR0 unused for modern devices but that > > does not seem too bad. > > Yep, no technical reason against it, although it IMHO looks nicer. > > cheers, > Gerd If we run into a problem, we'll be able to reshuffle it all easily.
On Mi, 2015-03-04 at 12:04 +0100, Michael S. Tsirkin wrote: > On Wed, Mar 04, 2015 at 11:55:50AM +0100, Gerd Hoffmann wrote: > > Hi, > > > > > > VirtIOPCIProxy subclasses which need additional pci bars, such as > > > > virtio-vga, just need to make sure they register the additinal bars > > > > before initializing virtio-pci, so the superclass can see the registered > > > > bars and shuffle around the virtio bars accordingly. > > > > > > I think I prefer we just DTRT and keep same layouts for everyone by > > > default: isn't there a layout that is good for everybody? > > > > I want bar #2 for the vga framebuffer for virtio-vga. Which conflicts > > with bar #2 being used for the modern bar in todays code. > > OK, and IIUC you are saying that using bar #2 for that is a requirement for > vga to work? Makes things alot easier as seavgabios has support for virtio-vga (in vga compat mode) already and expects the framebuffer being in bar #2. It's not a _really_ hard requirement though. cheers, Gerd
On Wed, Mar 04, 2015 at 12:46:50PM +0100, Gerd Hoffmann wrote: > On Mi, 2015-03-04 at 12:04 +0100, Michael S. Tsirkin wrote: > > On Wed, Mar 04, 2015 at 11:55:50AM +0100, Gerd Hoffmann wrote: > > > Hi, > > > > > > > > VirtIOPCIProxy subclasses which need additional pci bars, such as > > > > > virtio-vga, just need to make sure they register the additinal bars > > > > > before initializing virtio-pci, so the superclass can see the registered > > > > > bars and shuffle around the virtio bars accordingly. > > > > > > > > I think I prefer we just DTRT and keep same layouts for everyone by > > > > default: isn't there a layout that is good for everybody? > > > > > > I want bar #2 for the vga framebuffer for virtio-vga. Which conflicts > > > with bar #2 being used for the modern bar in todays code. > > > > OK, and IIUC you are saying that using bar #2 for that is a requirement for > > vga to work? > > Makes things alot easier as seavgabios has support for virtio-vga (in > vga compat mode) already and expects the framebuffer being in bar #2. > > It's not a _really_ hard requirement though. > > cheers, > Gerd > Easy is good, I think we don't really care where the modern bar is, let's just move it for everyone, and add a comment explaining the motivation.
On Wed, Mar 04, 2015 at 12:51:35PM +0100, Michael S. Tsirkin wrote: > On Wed, Mar 04, 2015 at 12:46:50PM +0100, Gerd Hoffmann wrote: > > On Mi, 2015-03-04 at 12:04 +0100, Michael S. Tsirkin wrote: > > > On Wed, Mar 04, 2015 at 11:55:50AM +0100, Gerd Hoffmann wrote: > > > > Hi, > > > > > > > > > > VirtIOPCIProxy subclasses which need additional pci bars, such as > > > > > > virtio-vga, just need to make sure they register the additinal bars > > > > > > before initializing virtio-pci, so the superclass can see the registered > > > > > > bars and shuffle around the virtio bars accordingly. > > > > > > > > > > I think I prefer we just DTRT and keep same layouts for everyone by > > > > > default: isn't there a layout that is good for everybody? > > > > > > > > I want bar #2 for the vga framebuffer for virtio-vga. Which conflicts > > > > with bar #2 being used for the modern bar in todays code. > > > > > > OK, and IIUC you are saying that using bar #2 for that is a requirement for > > > vga to work? > > > > Makes things alot easier as seavgabios has support for virtio-vga (in > > vga compat mode) already and expects the framebuffer being in bar #2. > > > > It's not a _really_ hard requirement though. > > > > cheers, > > Gerd > > > > Easy is good, I think we don't really care where the modern bar is, > let's just move it for everyone, and add a comment explaining the > motivation. OTOH if we really want to make it flexible, we can add a new virtio pci capability, with a new cfg_type value, say 0x6, and use that to describe BAR and offset for vga use. I'm not sure I understand all the implications though. > -- > MST
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 6b1f422..a2cfb1f 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -965,8 +965,6 @@ static void virtio_pci_add_mem_cap(VirtIOPCIProxy *proxy, PCIDevice *dev = &proxy->pci_dev; int offset; - cap->bar = 2; - offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, 0, cap->cap_len); assert(offset > 0); @@ -1232,9 +1230,12 @@ static void virtio_pci_device_write(void *opaque, hwaddr addr, static void virtio_pci_device_plugged(DeviceState *d) { VirtIOPCIProxy *proxy = VIRTIO_PCI(d); + PCIDevice *pci = PCI_DEVICE(d); VirtioBusState *bus = &proxy->bus; bool legacy = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY); bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN); + uint32_t modern_mem_bar; /* 64bit bar */ + uint32_t msix_bar; /* 32bit bar */ uint8_t *config; uint32_t size; @@ -1248,6 +1249,12 @@ static void virtio_pci_device_plugged(DeviceState *d) pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID, pci_get_word(config + PCI_VENDOR_ID)); pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus)); + msix_bar = 1; + modern_mem_bar = 2; + while (pci->io_regions[modern_mem_bar].size || + pci->io_regions[modern_mem_bar+1].size) { + modern_mem_bar += 2; + } } else { /* pure virtio-1.0 */ pci_set_word(config + PCI_VENDOR_ID, @@ -1255,7 +1262,18 @@ static void virtio_pci_device_plugged(DeviceState *d) pci_set_word(config + PCI_DEVICE_ID, 0x1040 + virtio_bus_get_vdev_id(bus)); pci_config_set_revision(config, 1); + modern_mem_bar = 0; + while (pci->io_regions[modern_mem_bar].size || + pci->io_regions[modern_mem_bar+1].size) { + modern_mem_bar += 2; + } + msix_bar = modern_mem_bar + 2; + while (pci->io_regions[msix_bar].size) { + msix_bar++; + } } + assert(modern_mem_bar <= 4); + assert(msix_bar <= 5); config[PCI_INTERRUPT_PIN] = 1; @@ -1263,29 +1281,34 @@ static void virtio_pci_device_plugged(DeviceState *d) struct virtio_pci_cap common = { .cfg_type = VIRTIO_PCI_CAP_COMMON_CFG, .cap_len = sizeof common, + .bar = modern_mem_bar, .offset = cpu_to_le32(0x0), .length = cpu_to_le32(0x1000), }; struct virtio_pci_cap isr = { .cfg_type = VIRTIO_PCI_CAP_ISR_CFG, .cap_len = sizeof isr, + .bar = modern_mem_bar, .offset = cpu_to_le32(0x1000), .length = cpu_to_le32(0x1000), }; struct virtio_pci_cap device = { .cfg_type = VIRTIO_PCI_CAP_DEVICE_CFG, .cap_len = sizeof device, + .bar = modern_mem_bar, .offset = cpu_to_le32(0x2000), .length = cpu_to_le32(0x1000), }; struct virtio_pci_notify_cap notify = { .cap.cfg_type = VIRTIO_PCI_CAP_NOTIFY_CFG, .cap.cap_len = sizeof notify, + .cap.bar = modern_mem_bar, .cap.offset = cpu_to_le32(0x3000), .cap.length = cpu_to_le32(QEMU_VIRTIO_PCI_QUEUE_MEM_MULT * VIRTIO_PCI_QUEUE_MAX), .notify_off_multiplier = cpu_to_le32(QEMU_VIRTIO_PCI_QUEUE_MEM_MULT), }; + uint32_t modern_mem_attr; static const MemoryRegionOps common_ops = { .read = virtio_pci_common_read, @@ -1359,12 +1382,15 @@ static void virtio_pci_device_plugged(DeviceState *d) QEMU_VIRTIO_PCI_QUEUE_MEM_MULT * VIRTIO_PCI_QUEUE_MAX); memory_region_add_subregion(&proxy->modern_bar, 0x3000, &proxy->notify); - pci_register_bar(&proxy->pci_dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, - &proxy->modern_bar); + modern_mem_attr = (PCI_BASE_ADDRESS_SPACE_MEMORY | + PCI_BASE_ADDRESS_MEM_PREFETCH | + PCI_BASE_ADDRESS_MEM_TYPE_64); + pci_register_bar(&proxy->pci_dev, modern_mem_bar, + modern_mem_attr, &proxy->modern_bar); } if (proxy->nvectors && - msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) { + msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, msix_bar)) { error_report("unable to init msix vectors to %" PRIu32, proxy->nvectors); proxy->nvectors = 0;
This patch makes the bar layout for virtio pci devices configurable. This is used to (a) create different layouts for legacy/transitional vs. modern devices. (b) make sure we use unused pci bars, by checking PCIDevice->io_regions VirtIOPCIProxy subclasses which need additional pci bars, such as virtio-vga, just need to make sure they register the additinal bars before initializing virtio-pci, so the superclass can see the registered bars and shuffle around the virtio bars accordingly. The patch also makes sure the modern bar is properly aligned so it can be a 64bit bar, and the prefetchable + 64bit attributes are enabled. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/virtio/virtio-pci.c | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-)