| Message ID | 1312135082-31985-21-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jul 31, 2011 at 08:57:43PM +0300, Avi Kivity wrote: > @@ -491,30 +473,26 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val) > virtio_config_writel(proxy->vdev, addr, val); > } > > -static void virtio_map(PCIDevice *pci_dev, int region_num, > - pcibus_t addr, pcibus_t size, int type) > -{ > - VirtIOPCIProxy *proxy = container_of(pci_dev, VirtIOPCIProxy, pci_dev); > - VirtIODevice *vdev = proxy->vdev; > - unsigned config_len = VIRTIO_PCI_REGION_SIZE(pci_dev) + vdev->config_len; > - > - proxy->addr = addr; > - > - register_ioport_write(addr, config_len, 1, virtio_pci_config_writeb, proxy); > - register_ioport_write(addr, config_len, 2, virtio_pci_config_writew, proxy); > - register_ioport_write(addr, config_len, 4, virtio_pci_config_writel, proxy); > - register_ioport_read(addr, config_len, 1, virtio_pci_config_readb, proxy); > - register_ioport_read(addr, config_len, 2, virtio_pci_config_readw, proxy); > - register_ioport_read(addr, config_len, 4, virtio_pci_config_readl, proxy); > +const MemoryRegionPortio virtio_portio[] = { > + { 0, 0x10000, 1, .write = virtio_pci_config_writeb, }, > + { 0, 0x10000, 2, .write = virtio_pci_config_writew, }, > + { 0, 0x10000, 4, .write = virtio_pci_config_writel, }, > + { 0, 0x10000, 1, .read = virtio_pci_config_readb, }, > + { 0, 0x10000, 2, .read = virtio_pci_config_readw, }, > + { 0, 0x10000, 4, .read = virtio_pci_config_readl, }, > + PORTIO_END > +}; > > - if (vdev->config_len) > - vdev->get_config(vdev, vdev->config); > -} > +static const MemoryRegionOps virtio_pci_config_ops = { > + .old_portio = virtio_portio, > + .endianness = DEVICE_LITTLE_ENDIAN, > +}; > > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > uint32_t val, int len) > { > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > + VirtIODevice *vdev = proxy->vdev; > > if (PCI_COMMAND == address) { > if (!(val & PCI_COMMAND_MASTER)) { > @@ -525,6 +503,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > } > } > } > + if (address == PCI_BASE_ADDRESS_0 && vdev->config_len) { > + vdev->get_config(vdev, vdev->config); > + } > > pci_default_write_config(pci_dev, address, val, len); > msix_write_config(pci_dev, address, val, len); I'm not really sure why did we get the config on map, specifically - Anthony, do you know? But if we want to do that, memory space enable might be a better place. Or maybe we just want a callback on map.
On 08/01/2011 11:26 AM, Michael S. Tsirkin wrote: > > > > static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > uint32_t val, int len) > > { > > VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > > + VirtIODevice *vdev = proxy->vdev; > > > > if (PCI_COMMAND == address) { > > if (!(val& PCI_COMMAND_MASTER)) { > > @@ -525,6 +503,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > } > > } > > } > > + if (address == PCI_BASE_ADDRESS_0&& vdev->config_len) { > > + vdev->get_config(vdev, vdev->config); > > + } > > > > pci_default_write_config(pci_dev, address, val, len); > > msix_write_config(pci_dev, address, val, len); > > I'm not really sure why did we get the config on map, > specifically - Anthony, do you know? > But if we want to do that, memory space enable might > be a better place. Or maybe we just want a callback on > map. Just because a memory region becomes visible to the cpu is no reason to have a callback. From the device perspective, it can't tell that it happened.
On Mon, Aug 01, 2011 at 12:35:44PM +0300, Avi Kivity wrote: > On 08/01/2011 11:26 AM, Michael S. Tsirkin wrote: > >> > >> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > >> uint32_t val, int len) > >> { > >> VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > >> + VirtIODevice *vdev = proxy->vdev; > >> > >> if (PCI_COMMAND == address) { > >> if (!(val& PCI_COMMAND_MASTER)) { > >> @@ -525,6 +503,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > >> } > >> } > >> } > >> + if (address == PCI_BASE_ADDRESS_0&& vdev->config_len) { > >> + vdev->get_config(vdev, vdev->config); > >> + } > >> > >> pci_default_write_config(pci_dev, address, val, len); > >> msix_write_config(pci_dev, address, val, len); > > > >I'm not really sure why did we get the config on map, > >specifically - Anthony, do you know? > >But if we want to do that, memory space enable might > >be a better place. Or maybe we just want a callback on > >map. > > > Just because a memory region becomes visible to the cpu is no reason > to have a callback. From the device perspective, it can't tell that > it happened. Well, the reason we have this logic here, I think, is to make sure it runs before the guest accesses the configuration with a write access. I'm not sure why we don't do this in the init callback - Anthony? > -- > error compiling committee.c: too many arguments to function
On Mon, Aug 01, 2011 at 12:35:44PM +0300, Avi Kivity wrote: > On 08/01/2011 11:26 AM, Michael S. Tsirkin wrote: > >> > >> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > >> uint32_t val, int len) > >> { > >> VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); > >> + VirtIODevice *vdev = proxy->vdev; > >> > >> if (PCI_COMMAND == address) { > >> if (!(val& PCI_COMMAND_MASTER)) { > >> @@ -525,6 +503,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > >> } > >> } > >> } > >> + if (address == PCI_BASE_ADDRESS_0&& vdev->config_len) { > >> + vdev->get_config(vdev, vdev->config); > >> + } > >> > >> pci_default_write_config(pci_dev, address, val, len); > >> msix_write_config(pci_dev, address, val, len); > > > >I'm not really sure why did we get the config on map, > >specifically - Anthony, do you know? > >But if we want to do that, memory space enable might > >be a better place. Or maybe we just want a callback on > >map. > > > Just because a memory region becomes visible to the cpu is no reason > to have a callback. From the device perspective, it can't tell that > it happened. BTW this is what qxl does, too, conceptually: on config writes, it peeks at the bar to detect whether that got unmapped. > -- > error compiling committee.c: too many arguments to function
On 08/01/2011 03:26 AM, Michael S. Tsirkin wrote: > On Sun, Jul 31, 2011 at 08:57:43PM +0300, Avi Kivity wrote: >> @@ -491,30 +473,26 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val) >> virtio_config_writel(proxy->vdev, addr, val); >> } >> >> -static void virtio_map(PCIDevice *pci_dev, int region_num, >> - pcibus_t addr, pcibus_t size, int type) >> -{ >> - VirtIOPCIProxy *proxy = container_of(pci_dev, VirtIOPCIProxy, pci_dev); >> - VirtIODevice *vdev = proxy->vdev; >> - unsigned config_len = VIRTIO_PCI_REGION_SIZE(pci_dev) + vdev->config_len; >> - >> - proxy->addr = addr; >> - >> - register_ioport_write(addr, config_len, 1, virtio_pci_config_writeb, proxy); >> - register_ioport_write(addr, config_len, 2, virtio_pci_config_writew, proxy); >> - register_ioport_write(addr, config_len, 4, virtio_pci_config_writel, proxy); >> - register_ioport_read(addr, config_len, 1, virtio_pci_config_readb, proxy); >> - register_ioport_read(addr, config_len, 2, virtio_pci_config_readw, proxy); >> - register_ioport_read(addr, config_len, 4, virtio_pci_config_readl, proxy); >> +const MemoryRegionPortio virtio_portio[] = { >> + { 0, 0x10000, 1, .write = virtio_pci_config_writeb, }, >> + { 0, 0x10000, 2, .write = virtio_pci_config_writew, }, >> + { 0, 0x10000, 4, .write = virtio_pci_config_writel, }, >> + { 0, 0x10000, 1, .read = virtio_pci_config_readb, }, >> + { 0, 0x10000, 2, .read = virtio_pci_config_readw, }, >> + { 0, 0x10000, 4, .read = virtio_pci_config_readl, }, >> + PORTIO_END >> +}; >> >> - if (vdev->config_len) >> - vdev->get_config(vdev, vdev->config); >> -} >> +static const MemoryRegionOps virtio_pci_config_ops = { >> + .old_portio = virtio_portio, >> + .endianness = DEVICE_LITTLE_ENDIAN, >> +}; >> >> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, >> uint32_t val, int len) >> { >> VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); >> + VirtIODevice *vdev = proxy->vdev; >> >> if (PCI_COMMAND == address) { >> if (!(val& PCI_COMMAND_MASTER)) { >> @@ -525,6 +503,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, >> } >> } >> } >> + if (address == PCI_BASE_ADDRESS_0&& vdev->config_len) { >> + vdev->get_config(vdev, vdev->config); >> + } >> >> pci_default_write_config(pci_dev, address, val, len); >> msix_write_config(pci_dev, address, val, len); > > I'm not really sure why did we get the config on map, > specifically - Anthony, do you know? It's just a mechanism to lazily load the config. We could just as easily read the config whenever the (virtio) config space was accessed. Regards, Anthony Liguori > But if we want to do that, memory space enable might > be a better place. Or maybe we just want a callback on > map. >
On 08/01/2011 05:23 AM, Michael S. Tsirkin wrote: > On Mon, Aug 01, 2011 at 12:35:44PM +0300, Avi Kivity wrote: >> On 08/01/2011 11:26 AM, Michael S. Tsirkin wrote: >>>> >>>> static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, >>>> uint32_t val, int len) >>>> { >>>> VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); >>>> + VirtIODevice *vdev = proxy->vdev; >>>> >>>> if (PCI_COMMAND == address) { >>>> if (!(val& PCI_COMMAND_MASTER)) { >>>> @@ -525,6 +503,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, >>>> } >>>> } >>>> } >>>> + if (address == PCI_BASE_ADDRESS_0&& vdev->config_len) { >>>> + vdev->get_config(vdev, vdev->config); >>>> + } >>>> >>>> pci_default_write_config(pci_dev, address, val, len); >>>> msix_write_config(pci_dev, address, val, len); >>> >>> I'm not really sure why did we get the config on map, >>> specifically - Anthony, do you know? >>> But if we want to do that, memory space enable might >>> be a better place. Or maybe we just want a callback on >>> map. >> >> >> Just because a memory region becomes visible to the cpu is no reason >> to have a callback. From the device perspective, it can't tell that >> it happened. > > Well, the reason we have this logic here, I think, is > to make sure it runs before the guest accesses > the configuration with a write access. > > I'm not sure why we don't do this in the init > callback - Anthony? It could be done in init I think. I can't recall why it didn't start that way initially. Regards, Anthony Liguori > > >> -- >> error compiling committee.c: too many arguments to function >
On 08/01/2011 10:53 PM, Michael S. Tsirkin wrote: > > > > > > Just because a memory region becomes visible to the cpu is no reason > > to have a callback. From the device perspective, it can't tell that > > it happened. > > BTW this is what qxl does, too, conceptually: on config writes, it peeks > at the bar to detect whether that got unmapped. Can you elaborate? why? what does it do?
On Tue, Aug 02, 2011 at 12:17:06PM +0300, Avi Kivity wrote: > On 08/01/2011 10:53 PM, Michael S. Tsirkin wrote: > >> > >> > >> Just because a memory region becomes visible to the cpu is no reason > >> to have a callback. From the device perspective, it can't tell that > >> it happened. > > > >BTW this is what qxl does, too, conceptually: on config writes, it peeks > >at the bar to detect whether that got unmapped. > > Can you elaborate? why? what does it do? Disables vga mapping when it sees io has been disabled. What it really should do is check the io enable bit I think ... vga_dirty_log_stop(vga); pci_default_write_config(d, address, val, len); if (vga->map_addr && qxl->pci.io_regions[0].addr == -1) { vga->map_addr = 0; } vga_dirty_log_start(vga); > -- > error compiling committee.c: too many arguments to function
On 08/02/2011 12:34 PM, Michael S. Tsirkin wrote: > On Tue, Aug 02, 2011 at 12:17:06PM +0300, Avi Kivity wrote: > > On 08/01/2011 10:53 PM, Michael S. Tsirkin wrote: > > >> > > >> > > >> Just because a memory region becomes visible to the cpu is no reason > > >> to have a callback. From the device perspective, it can't tell that > > >> it happened. > > > > > >BTW this is what qxl does, too, conceptually: on config writes, it peeks > > >at the bar to detect whether that got unmapped. > > > > Can you elaborate? why? what does it do? > > > Disables vga mapping when it sees io has been disabled. > What it really should do is check the io enable bit I think ... > > vga_dirty_log_stop(vga); > pci_default_write_config(d, address, val, len); > if (vga->map_addr&& qxl->pci.io_regions[0].addr == -1) { > vga->map_addr = 0; > } > vga_dirty_log_start(vga); If the memory is not visible to the guest, logging has no effect.
On 08/02/11 11:17, Avi Kivity wrote: > On 08/01/2011 10:53 PM, Michael S. Tsirkin wrote: >> > >> > >> > Just because a memory region becomes visible to the cpu is no reason >> > to have a callback. From the device perspective, it can't tell that >> > it happened. >> >> BTW this is what qxl does, too, conceptually: on config writes, it peeks >> at the bar to detect whether that got unmapped. > > Can you elaborate? why? what does it do? Nothing special, just keep track of the map state and toggle logging if needed, especially make sure to (re-)enable logging after the guest mapped the bar somewhere. I'm pretty sure this can be killed during the conversion as the new memory slot manager will care about this so qxl doesn't has to. cheers, Gerd
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index d685243..c114e1a 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -161,7 +161,8 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, { VirtQueue *vq = virtio_get_queue(proxy->vdev, n); EventNotifier *notifier = virtio_queue_get_host_notifier(vq); - int r; + int r = 0; + if (assign) { r = event_notifier_init(notifier, 1); if (r < 0) { @@ -169,24 +170,11 @@ static int virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy, __func__, r); return r; } - r = kvm_set_ioeventfd_pio_word(event_notifier_get_fd(notifier), - proxy->addr + VIRTIO_PCI_QUEUE_NOTIFY, - n, assign); - if (r < 0) { - error_report("%s: unable to map ioeventfd: %d", - __func__, r); - event_notifier_cleanup(notifier); - } + memory_region_add_eventfd(&proxy->bar, VIRTIO_PCI_QUEUE_NOTIFY, 2, + true, n, event_notifier_get_fd(notifier)); } else { - r = kvm_set_ioeventfd_pio_word(event_notifier_get_fd(notifier), - proxy->addr + VIRTIO_PCI_QUEUE_NOTIFY, - n, assign); - if (r < 0) { - error_report("%s: unable to unmap ioeventfd: %d", - __func__, r); - return r; - } - + memory_region_del_eventfd(&proxy->bar, VIRTIO_PCI_QUEUE_NOTIFY, 2, + true, n, event_notifier_get_fd(notifier)); /* Handle the race condition where the guest kicked and we deassigned * before we got around to handling the kick. */ @@ -423,7 +411,6 @@ static uint32_t virtio_pci_config_readb(void *opaque, uint32_t addr) { VirtIOPCIProxy *proxy = opaque; uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev); - addr -= proxy->addr; if (addr < config) return virtio_ioport_read(proxy, addr); addr -= config; @@ -434,7 +421,6 @@ static uint32_t virtio_pci_config_readw(void *opaque, uint32_t addr) { VirtIOPCIProxy *proxy = opaque; uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev); - addr -= proxy->addr; if (addr < config) return virtio_ioport_read(proxy, addr); addr -= config; @@ -445,7 +431,6 @@ static uint32_t virtio_pci_config_readl(void *opaque, uint32_t addr) { VirtIOPCIProxy *proxy = opaque; uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev); - addr -= proxy->addr; if (addr < config) return virtio_ioport_read(proxy, addr); addr -= config; @@ -456,7 +441,6 @@ static void virtio_pci_config_writeb(void *opaque, uint32_t addr, uint32_t val) { VirtIOPCIProxy *proxy = opaque; uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev); - addr -= proxy->addr; if (addr < config) { virtio_ioport_write(proxy, addr, val); return; @@ -469,7 +453,6 @@ static void virtio_pci_config_writew(void *opaque, uint32_t addr, uint32_t val) { VirtIOPCIProxy *proxy = opaque; uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev); - addr -= proxy->addr; if (addr < config) { virtio_ioport_write(proxy, addr, val); return; @@ -482,7 +465,6 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val) { VirtIOPCIProxy *proxy = opaque; uint32_t config = VIRTIO_PCI_CONFIG(&proxy->pci_dev); - addr -= proxy->addr; if (addr < config) { virtio_ioport_write(proxy, addr, val); return; @@ -491,30 +473,26 @@ static void virtio_pci_config_writel(void *opaque, uint32_t addr, uint32_t val) virtio_config_writel(proxy->vdev, addr, val); } -static void virtio_map(PCIDevice *pci_dev, int region_num, - pcibus_t addr, pcibus_t size, int type) -{ - VirtIOPCIProxy *proxy = container_of(pci_dev, VirtIOPCIProxy, pci_dev); - VirtIODevice *vdev = proxy->vdev; - unsigned config_len = VIRTIO_PCI_REGION_SIZE(pci_dev) + vdev->config_len; - - proxy->addr = addr; - - register_ioport_write(addr, config_len, 1, virtio_pci_config_writeb, proxy); - register_ioport_write(addr, config_len, 2, virtio_pci_config_writew, proxy); - register_ioport_write(addr, config_len, 4, virtio_pci_config_writel, proxy); - register_ioport_read(addr, config_len, 1, virtio_pci_config_readb, proxy); - register_ioport_read(addr, config_len, 2, virtio_pci_config_readw, proxy); - register_ioport_read(addr, config_len, 4, virtio_pci_config_readl, proxy); +const MemoryRegionPortio virtio_portio[] = { + { 0, 0x10000, 1, .write = virtio_pci_config_writeb, }, + { 0, 0x10000, 2, .write = virtio_pci_config_writew, }, + { 0, 0x10000, 4, .write = virtio_pci_config_writel, }, + { 0, 0x10000, 1, .read = virtio_pci_config_readb, }, + { 0, 0x10000, 2, .read = virtio_pci_config_readw, }, + { 0, 0x10000, 4, .read = virtio_pci_config_readl, }, + PORTIO_END +}; - if (vdev->config_len) - vdev->get_config(vdev, vdev->config); -} +static const MemoryRegionOps virtio_pci_config_ops = { + .old_portio = virtio_portio, + .endianness = DEVICE_LITTLE_ENDIAN, +}; static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, uint32_t val, int len) { VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); + VirtIODevice *vdev = proxy->vdev; if (PCI_COMMAND == address) { if (!(val & PCI_COMMAND_MASTER)) { @@ -525,6 +503,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, } } } + if (address == PCI_BASE_ADDRESS_0 && vdev->config_len) { + vdev->get_config(vdev, vdev->config); + } pci_default_write_config(pci_dev, address, val, len); msix_write_config(pci_dev, address, val, len); @@ -678,8 +659,10 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) if (size & (size-1)) size = 1 << qemu_fls(size); - pci_register_bar(&proxy->pci_dev, 0, size, PCI_BASE_ADDRESS_SPACE_IO, - virtio_map); + memory_region_init_io(&proxy->bar, &virtio_pci_config_ops, proxy, + "virtio-pci", size); + pci_register_bar_region(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, + &proxy->bar); if (!kvm_has_many_ioeventfds()) { proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; @@ -714,6 +697,9 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev) static int virtio_exit_pci(PCIDevice *pci_dev) { + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); + + memory_region_destroy(&proxy->bar); return msix_uninit(pci_dev); } diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h index 1f0de56..5af1c8c 100644 --- a/hw/virtio-pci.h +++ b/hw/virtio-pci.h @@ -21,8 +21,8 @@ typedef struct { PCIDevice pci_dev; VirtIODevice *vdev; + MemoryRegion bar; uint32_t flags; - uint32_t addr; uint32_t class_code; uint32_t nvectors; BlockConf block;
except msix. [jan: fix build] Signed-off-by: Avi Kivity <avi@redhat.com> --- hw/virtio-pci.c | 74 ++++++++++++++++++++++-------------------------------- hw/virtio-pci.h | 2 +- 2 files changed, 31 insertions(+), 45 deletions(-)