| Message ID | 20120319155650.GA6430@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/19/2012 05:56 PM, Michael S. Tsirkin wrote: > Currently virtio-pci is specified so that configuration of the device is > done through a PCI IO space (via BAR 0 of the virtual PCI device). > However, Linux guests happen to use ioread/iowrite/iomap primitives > for access, and these work uniformly across memory/io BARs. > > While PCI IO accesses are faster than MMIO on x86 kvm, > MMIO might be helpful on other systems which don't > implement PIO or where PIO is slower than MMIO. > > Add a property to make it possible to tweak the BAR type. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > This is harmless by default but causes segfaults in memory.c > when enabled. Thus an RFC until I figure out what's wrong. > Should be done via an extra BAR (with the same layout, perhaps extended) so compatibility is preserved.
On Mon, Mar 19, 2012 at 07:58:12PM +0200, Avi Kivity wrote: > On 03/19/2012 05:56 PM, Michael S. Tsirkin wrote: > > Currently virtio-pci is specified so that configuration of the device is > > done through a PCI IO space (via BAR 0 of the virtual PCI device). > > However, Linux guests happen to use ioread/iowrite/iomap primitives > > for access, and these work uniformly across memory/io BARs. > > > > While PCI IO accesses are faster than MMIO on x86 kvm, > > MMIO might be helpful on other systems which don't > > implement PIO or where PIO is slower than MMIO. > > > > Add a property to make it possible to tweak the BAR type. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > This is harmless by default but causes segfaults in memory.c > > when enabled. Thus an RFC until I figure out what's wrong. > > > > Should be done via an extra BAR (with the same layout, perhaps extended) > so compatibility is preserved. No, that would need guest changes to be of use. The point of this hack is to make things work for Linux guests where PIO does not work. > -- > error compiling committee.c: too many arguments to function
On 03/19/2012 10:56 AM, Michael S. Tsirkin wrote: > Currently virtio-pci is specified so that configuration of the device is > done through a PCI IO space (via BAR 0 of the virtual PCI device). > However, Linux guests happen to use ioread/iowrite/iomap primitives > for access, and these work uniformly across memory/io BARs. > > While PCI IO accesses are faster than MMIO on x86 kvm, > MMIO might be helpful on other systems which don't > implement PIO or where PIO is slower than MMIO. > > Add a property to make it possible to tweak the BAR type. > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > This is harmless by default but causes segfaults in memory.c > when enabled. Thus an RFC until I figure out what's wrong. Doesn't this violate the virtio-pci spec? Making the same vendor/device ID have different semantics depending on a magic flag in QEMU seems like a pretty bad idea to me. Regards, Anthony Liguori > > --- > hw/virtio-pci.c | 16 ++++++++++++++-- > hw/virtio-pci.h | 4 ++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 28498ec..6f338d2 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > { > uint8_t *config; > uint32_t size; > + uint8_t bar0_type; > > proxy->vdev = vdev; > > @@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > > memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy, > "virtio-pci", size); > - pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, > -&proxy->bar); > + > + if (proxy->flags& VIRTIO_PCI_FLAG_USE_MMIO) { > + bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY; > + } else { > + bar0_type = PCI_BASE_ADDRESS_SPACE_IO; > + } > + > + pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar); > > if (!kvm_has_many_ioeventfds()) { > proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; > @@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), > DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), > + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -856,6 +864,7 @@ static Property virtio_net_properties[] = { > DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL), > DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST), > DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx), > + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = { > DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), > DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), > DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31), > + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = { > > static Property virtio_balloon_properties[] = { > DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), > + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev) > static Property virtio_scsi_properties[] = { > DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), > + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h > index e560428..e6a8861 100644 > --- a/hw/virtio-pci.h > +++ b/hw/virtio-pci.h > @@ -24,6 +24,10 @@ > #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1 > #define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1<< VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT) > > +/* Some guests don't support port IO. Use MMIO instead. */ > +#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2 > +#define VIRTIO_PCI_FLAG_USE_MMIO (1<< VIRTIO_PCI_FLAG_USE_MMIO_BIT) > + > typedef struct { > PCIDevice pci_dev; > VirtIODevice *vdev;
On Mon, Mar 19, 2012 at 02:19:33PM -0500, Anthony Liguori wrote: > On 03/19/2012 10:56 AM, Michael S. Tsirkin wrote: > >Currently virtio-pci is specified so that configuration of the device is > >done through a PCI IO space (via BAR 0 of the virtual PCI device). > >However, Linux guests happen to use ioread/iowrite/iomap primitives > >for access, and these work uniformly across memory/io BARs. > > > >While PCI IO accesses are faster than MMIO on x86 kvm, > >MMIO might be helpful on other systems which don't > >implement PIO or where PIO is slower than MMIO. > > > >Add a property to make it possible to tweak the BAR type. > > > >Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > > >This is harmless by default but causes segfaults in memory.c > >when enabled. Thus an RFC until I figure out what's wrong. > > Doesn't this violate the virtio-pci spec? > The point is to change the BAR type depending on the architecture. IO is fastest on x86 but maybe not on other architectures. > Making the same vendor/device ID have different semantics depending > on a magic flag in QEMU seems like a pretty bad idea to me. > > Regards, > > Anthony Liguori We do this with MSI-X so why not the BAR type? > > > >--- > > hw/virtio-pci.c | 16 ++++++++++++++-- > > hw/virtio-pci.h | 4 ++++ > > 2 files changed, 18 insertions(+), 2 deletions(-) > > > >diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > >index 28498ec..6f338d2 100644 > >--- a/hw/virtio-pci.c > >+++ b/hw/virtio-pci.c > >@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > > { > > uint8_t *config; > > uint32_t size; > >+ uint8_t bar0_type; > > > > proxy->vdev = vdev; > > > >@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > > > > memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy, > > "virtio-pci", size); > >- pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, > >-&proxy->bar); > >+ > >+ if (proxy->flags& VIRTIO_PCI_FLAG_USE_MMIO) { > >+ bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY; > >+ } else { > >+ bar0_type = PCI_BASE_ADDRESS_SPACE_IO; > >+ } > >+ > >+ pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar); > > > > if (!kvm_has_many_ioeventfds()) { > > proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; > >@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = { > > DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), > > DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > > DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), > >+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > >@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = { > > DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL), > > DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST), > > DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx), > >+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > >@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = { > > DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), > > DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), > > DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31), > >+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > >@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = { > > > > static Property virtio_balloon_properties[] = { > > DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), > >+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > >@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev) > > static Property virtio_scsi_properties[] = { > > DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > > DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), > >+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > >diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h > >index e560428..e6a8861 100644 > >--- a/hw/virtio-pci.h > >+++ b/hw/virtio-pci.h > >@@ -24,6 +24,10 @@ > > #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1 > > #define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1<< VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT) > > > >+/* Some guests don't support port IO. Use MMIO instead. */ > >+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2 > >+#define VIRTIO_PCI_FLAG_USE_MMIO (1<< VIRTIO_PCI_FLAG_USE_MMIO_BIT) > >+ > > typedef struct { > > PCIDevice pci_dev; > > VirtIODevice *vdev;
On 03/19/2012 03:49 PM, Michael S. Tsirkin wrote: > On Mon, Mar 19, 2012 at 02:19:33PM -0500, Anthony Liguori wrote: >> On 03/19/2012 10:56 AM, Michael S. Tsirkin wrote: >>> Currently virtio-pci is specified so that configuration of the device is >>> done through a PCI IO space (via BAR 0 of the virtual PCI device). >>> However, Linux guests happen to use ioread/iowrite/iomap primitives >>> for access, and these work uniformly across memory/io BARs. >>> >>> While PCI IO accesses are faster than MMIO on x86 kvm, >>> MMIO might be helpful on other systems which don't >>> implement PIO or where PIO is slower than MMIO. >>> >>> Add a property to make it possible to tweak the BAR type. >>> >>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >>> >>> This is harmless by default but causes segfaults in memory.c >>> when enabled. Thus an RFC until I figure out what's wrong. >> >> Doesn't this violate the virtio-pci spec? >> > > The point is to change the BAR type depending on the architecture. > IO is fastest on x86 but maybe not on other architectures. Are we going to document that the BAR is X on architecture Y in the spec? I think the better way to do this is to use a separate device id range for MMIO virtio-pci. You can make the same driver hand both ranges and that way the device is presented consistently to the guest regardless of what the architecture is. >> Making the same vendor/device ID have different semantics depending >> on a magic flag in QEMU seems like a pretty bad idea to me. >> >> Regards, >> >> Anthony Liguori > > We do this with MSI-X so why not the BAR type? We extend the bar size with MSI-X and use a transport flag to indicate that it's available, right? Regards, Anthony LIguori > >>> >>> --- >>> hw/virtio-pci.c | 16 ++++++++++++++-- >>> hw/virtio-pci.h | 4 ++++ >>> 2 files changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c >>> index 28498ec..6f338d2 100644 >>> --- a/hw/virtio-pci.c >>> +++ b/hw/virtio-pci.c >>> @@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) >>> { >>> uint8_t *config; >>> uint32_t size; >>> + uint8_t bar0_type; >>> >>> proxy->vdev = vdev; >>> >>> @@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) >>> >>> memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy, >>> "virtio-pci", size); >>> - pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >>> -&proxy->bar); >>> + >>> + if (proxy->flags& VIRTIO_PCI_FLAG_USE_MMIO) { >>> + bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY; >>> + } else { >>> + bar0_type = PCI_BASE_ADDRESS_SPACE_IO; >>> + } >>> + >>> + pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar); >>> >>> if (!kvm_has_many_ioeventfds()) { >>> proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; >>> @@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = { >>> DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), >>> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), >>> DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), >>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> @@ -856,6 +864,7 @@ static Property virtio_net_properties[] = { >>> DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL), >>> DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST), >>> DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx), >>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> @@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = { >>> DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), >>> DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), >>> DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31), >>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> @@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = { >>> >>> static Property virtio_balloon_properties[] = { >>> DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), >>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> @@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev) >>> static Property virtio_scsi_properties[] = { >>> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), >>> DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), >>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >>> >>> diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h >>> index e560428..e6a8861 100644 >>> --- a/hw/virtio-pci.h >>> +++ b/hw/virtio-pci.h >>> @@ -24,6 +24,10 @@ >>> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1 >>> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1<< VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT) >>> >>> +/* Some guests don't support port IO. Use MMIO instead. */ >>> +#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2 >>> +#define VIRTIO_PCI_FLAG_USE_MMIO (1<< VIRTIO_PCI_FLAG_USE_MMIO_BIT) >>> + >>> typedef struct { >>> PCIDevice pci_dev; >>> VirtIODevice *vdev; >
On Mon, Mar 19, 2012 at 04:07:45PM -0500, Anthony Liguori wrote: > On 03/19/2012 03:49 PM, Michael S. Tsirkin wrote: > >On Mon, Mar 19, 2012 at 02:19:33PM -0500, Anthony Liguori wrote: > >>On 03/19/2012 10:56 AM, Michael S. Tsirkin wrote: > >>>Currently virtio-pci is specified so that configuration of the device is > >>>done through a PCI IO space (via BAR 0 of the virtual PCI device). > >>>However, Linux guests happen to use ioread/iowrite/iomap primitives > >>>for access, and these work uniformly across memory/io BARs. > >>> > >>>While PCI IO accesses are faster than MMIO on x86 kvm, > >>>MMIO might be helpful on other systems which don't > >>>implement PIO or where PIO is slower than MMIO. > >>> > >>>Add a property to make it possible to tweak the BAR type. > >>> > >>>Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > >>> > >>>This is harmless by default but causes segfaults in memory.c > >>>when enabled. Thus an RFC until I figure out what's wrong. > >> > >>Doesn't this violate the virtio-pci spec? > >> > > > >The point is to change the BAR type depending on the architecture. > >IO is fastest on x86 but maybe not on other architectures. > > Are we going to document that the BAR is X on architecture Y in the spec? > > I think the better way to do this is to use a separate device id > range for MMIO virtio-pci. You can make the same driver hand both > ranges and that way the device is presented consistently to the > guest regardless of what the architecture is. Yes there are endless ways to do this. This specific hack is good for making existing linux drivers on ppc, arm etc work. > >>Making the same vendor/device ID have different semantics depending > >>on a magic flag in QEMU seems like a pretty bad idea to me. > >> > >>Regards, > >> > >>Anthony Liguori > > > >We do this with MSI-X so why not the BAR type? > > We extend the bar size with MSI-X and use a transport flag to > indicate that it's available, right? No, we use regular pci capability. Just like BAR type is a regular PCI register :) > Regards, > > Anthony LIguori > > > > >>> > >>>--- > >>> hw/virtio-pci.c | 16 ++++++++++++++-- > >>> hw/virtio-pci.h | 4 ++++ > >>> 2 files changed, 18 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > >>>index 28498ec..6f338d2 100644 > >>>--- a/hw/virtio-pci.c > >>>+++ b/hw/virtio-pci.c > >>>@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > >>> { > >>> uint8_t *config; > >>> uint32_t size; > >>>+ uint8_t bar0_type; > >>> > >>> proxy->vdev = vdev; > >>> > >>>@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > >>> > >>> memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy, > >>> "virtio-pci", size); > >>>- pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, > >>>-&proxy->bar); > >>>+ > >>>+ if (proxy->flags& VIRTIO_PCI_FLAG_USE_MMIO) { > >>>+ bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY; > >>>+ } else { > >>>+ bar0_type = PCI_BASE_ADDRESS_SPACE_IO; > >>>+ } > >>>+ > >>>+ pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar); > >>> > >>> if (!kvm_has_many_ioeventfds()) { > >>> proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; > >>>@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = { > >>> DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), > >>> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > >>> DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = { > >>> DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL), > >>> DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST), > >>> DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = { > >>> DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), > >>> DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), > >>> DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = { > >>> > >>> static Property virtio_balloon_properties[] = { > >>> DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev) > >>> static Property virtio_scsi_properties[] = { > >>> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > >>> DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h > >>>index e560428..e6a8861 100644 > >>>--- a/hw/virtio-pci.h > >>>+++ b/hw/virtio-pci.h > >>>@@ -24,6 +24,10 @@ > >>> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1 > >>> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1<< VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT) > >>> > >>>+/* Some guests don't support port IO. Use MMIO instead. */ > >>>+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2 > >>>+#define VIRTIO_PCI_FLAG_USE_MMIO (1<< VIRTIO_PCI_FLAG_USE_MMIO_BIT) > >>>+ > >>> typedef struct { > >>> PCIDevice pci_dev; > >>> VirtIODevice *vdev; > >
On Mon, Mar 19, 2012 at 04:07:45PM -0500, Anthony Liguori wrote: > On 03/19/2012 03:49 PM, Michael S. Tsirkin wrote: > >On Mon, Mar 19, 2012 at 02:19:33PM -0500, Anthony Liguori wrote: > >>On 03/19/2012 10:56 AM, Michael S. Tsirkin wrote: > >>>Currently virtio-pci is specified so that configuration of the device is > >>>done through a PCI IO space (via BAR 0 of the virtual PCI device). > >>>However, Linux guests happen to use ioread/iowrite/iomap primitives > >>>for access, and these work uniformly across memory/io BARs. > >>> > >>>While PCI IO accesses are faster than MMIO on x86 kvm, > >>>MMIO might be helpful on other systems which don't > >>>implement PIO or where PIO is slower than MMIO. > >>> > >>>Add a property to make it possible to tweak the BAR type. > >>> > >>>Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > >>> > >>>This is harmless by default but causes segfaults in memory.c > >>>when enabled. Thus an RFC until I figure out what's wrong. > >> > >>Doesn't this violate the virtio-pci spec? > >> > > > >The point is to change the BAR type depending on the architecture. > >IO is fastest on x86 but maybe not on other architectures. > > Are we going to document that the BAR is X on architecture Y in the spec? > > I think the better way to do this is to use a separate device id > range for MMIO virtio-pci. You can make the same driver hand both > ranges and that way the device is presented consistently to the > guest regardless of what the architecture is. Maybe just make this a hidden option like x-miio? This will ensure people dont turn it on by mistake on e.g. x86. > >>Making the same vendor/device ID have different semantics depending > >>on a magic flag in QEMU seems like a pretty bad idea to me. > >> > >>Regards, > >> > >>Anthony Liguori > > > >We do this with MSI-X so why not the BAR type? > > We extend the bar size with MSI-X and use a transport flag to > indicate that it's available, right? > > Regards, > > Anthony LIguori > > > > >>> > >>>--- > >>> hw/virtio-pci.c | 16 ++++++++++++++-- > >>> hw/virtio-pci.h | 4 ++++ > >>> 2 files changed, 18 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > >>>index 28498ec..6f338d2 100644 > >>>--- a/hw/virtio-pci.c > >>>+++ b/hw/virtio-pci.c > >>>@@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > >>> { > >>> uint8_t *config; > >>> uint32_t size; > >>>+ uint8_t bar0_type; > >>> > >>> proxy->vdev = vdev; > >>> > >>>@@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > >>> > >>> memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy, > >>> "virtio-pci", size); > >>>- pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, > >>>-&proxy->bar); > >>>+ > >>>+ if (proxy->flags& VIRTIO_PCI_FLAG_USE_MMIO) { > >>>+ bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY; > >>>+ } else { > >>>+ bar0_type = PCI_BASE_ADDRESS_SPACE_IO; > >>>+ } > >>>+ > >>>+ pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar); > >>> > >>> if (!kvm_has_many_ioeventfds()) { > >>> proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; > >>>@@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = { > >>> DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), > >>> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > >>> DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>@@ -856,6 +864,7 @@ static Property virtio_net_properties[] = { > >>> DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL), > >>> DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST), > >>> DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>@@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = { > >>> DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), > >>> DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), > >>> DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>@@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = { > >>> > >>> static Property virtio_balloon_properties[] = { > >>> DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>@@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev) > >>> static Property virtio_scsi_properties[] = { > >>> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), > >>> DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), > >>>+ DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), > >>> DEFINE_PROP_END_OF_LIST(), > >>> }; > >>> > >>>diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h > >>>index e560428..e6a8861 100644 > >>>--- a/hw/virtio-pci.h > >>>+++ b/hw/virtio-pci.h > >>>@@ -24,6 +24,10 @@ > >>> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1 > >>> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1<< VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT) > >>> > >>>+/* Some guests don't support port IO. Use MMIO instead. */ > >>>+#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2 > >>>+#define VIRTIO_PCI_FLAG_USE_MMIO (1<< VIRTIO_PCI_FLAG_USE_MMIO_BIT) > >>>+ > >>> typedef struct { > >>> PCIDevice pci_dev; > >>> VirtIODevice *vdev; > >
On 03/19/2012 04:29 PM, Michael S. Tsirkin wrote: > On Mon, Mar 19, 2012 at 04:07:45PM -0500, Anthony Liguori wrote: >> On 03/19/2012 03:49 PM, Michael S. Tsirkin wrote: >>> On Mon, Mar 19, 2012 at 02:19:33PM -0500, Anthony Liguori wrote: >>>> On 03/19/2012 10:56 AM, Michael S. Tsirkin wrote: >>>>> Currently virtio-pci is specified so that configuration of the device is >>>>> done through a PCI IO space (via BAR 0 of the virtual PCI device). >>>>> However, Linux guests happen to use ioread/iowrite/iomap primitives >>>>> for access, and these work uniformly across memory/io BARs. >>>>> >>>>> While PCI IO accesses are faster than MMIO on x86 kvm, >>>>> MMIO might be helpful on other systems which don't >>>>> implement PIO or where PIO is slower than MMIO. >>>>> >>>>> Add a property to make it possible to tweak the BAR type. >>>>> >>>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >>>>> >>>>> This is harmless by default but causes segfaults in memory.c >>>>> when enabled. Thus an RFC until I figure out what's wrong. >>>> >>>> Doesn't this violate the virtio-pci spec? >>>> >>> >>> The point is to change the BAR type depending on the architecture. >>> IO is fastest on x86 but maybe not on other architectures. >> >> Are we going to document that the BAR is X on architecture Y in the spec? >> >> I think the better way to do this is to use a separate device id >> range for MMIO virtio-pci. You can make the same driver hand both >> ranges and that way the device is presented consistently to the >> guest regardless of what the architecture is. > > Maybe just make this a hidden option like x-miio? x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power? Really, aren't we just being too clever here? From a practical perspective, I doubt anyone is ever going to support a driver that has *never* been tested on the platform just because it was accidentally compiled and happens to be there. If we just do use a device PCI device id range for this, it's a 1-line patch that can be provided via an update to existing guests. Regards, Anthony Liguori > This will ensure people dont turn it on by mistake on e.g. x86. > >>>> Making the same vendor/device ID have different semantics depending >>>> on a magic flag in QEMU seems like a pretty bad idea to me. >>>> >>>> Regards, >>>> >>>> Anthony Liguori >>> >>> We do this with MSI-X so why not the BAR type? >> >> We extend the bar size with MSI-X and use a transport flag to >> indicate that it's available, right? >> >> Regards, >> >> Anthony LIguori >> >>> >>>>> >>>>> --- >>>>> hw/virtio-pci.c | 16 ++++++++++++++-- >>>>> hw/virtio-pci.h | 4 ++++ >>>>> 2 files changed, 18 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c >>>>> index 28498ec..6f338d2 100644 >>>>> --- a/hw/virtio-pci.c >>>>> +++ b/hw/virtio-pci.c >>>>> @@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) >>>>> { >>>>> uint8_t *config; >>>>> uint32_t size; >>>>> + uint8_t bar0_type; >>>>> >>>>> proxy->vdev = vdev; >>>>> >>>>> @@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) >>>>> >>>>> memory_region_init_io(&proxy->bar,&virtio_pci_config_ops, proxy, >>>>> "virtio-pci", size); >>>>> - pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >>>>> -&proxy->bar); >>>>> + >>>>> + if (proxy->flags& VIRTIO_PCI_FLAG_USE_MMIO) { >>>>> + bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY; >>>>> + } else { >>>>> + bar0_type = PCI_BASE_ADDRESS_SPACE_IO; >>>>> + } >>>>> + >>>>> + pci_register_bar(&proxy->pci_dev, 0, bar0_type,&proxy->bar); >>>>> >>>>> if (!kvm_has_many_ioeventfds()) { >>>>> proxy->flags&= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; >>>>> @@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = { >>>>> DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), >>>>> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), >>>>> DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), >>>>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> >>>>> @@ -856,6 +864,7 @@ static Property virtio_net_properties[] = { >>>>> DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL), >>>>> DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST), >>>>> DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx), >>>>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> >>>>> @@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = { >>>>> DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), >>>>> DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), >>>>> DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31), >>>>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> >>>>> @@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = { >>>>> >>>>> static Property virtio_balloon_properties[] = { >>>>> DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), >>>>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> >>>>> @@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev) >>>>> static Property virtio_scsi_properties[] = { >>>>> DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), >>>>> DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), >>>>> + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> >>>>> diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h >>>>> index e560428..e6a8861 100644 >>>>> --- a/hw/virtio-pci.h >>>>> +++ b/hw/virtio-pci.h >>>>> @@ -24,6 +24,10 @@ >>>>> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1 >>>>> #define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1<< VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT) >>>>> >>>>> +/* Some guests don't support port IO. Use MMIO instead. */ >>>>> +#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2 >>>>> +#define VIRTIO_PCI_FLAG_USE_MMIO (1<< VIRTIO_PCI_FLAG_USE_MMIO_BIT) >>>>> + >>>>> typedef struct { >>>>> PCIDevice pci_dev; >>>>> VirtIODevice *vdev; >>> >
On Mon, 19 Mar 2012 17:13:06 -0500, Anthony Liguori <anthony@codemonkey.ws> wrote: > > Maybe just make this a hidden option like x-miio? > > x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power? "To configure the device, we use the first I/O region of the PCI device." Meh, it does sound a little like we are specifying that it's an PCI I/O bar. Let's resurrect the PCI-v2 idea, which is ready to implement now, and a nice cleanup? Detach it from the change-of-ring-format idea which is turning out to be a tarpit. Thanks, Rusty.
On 03/19/2012 06:52 PM, Rusty Russell wrote: > On Mon, 19 Mar 2012 17:13:06 -0500, Anthony Liguori<anthony@codemonkey.ws> wrote: >>> Maybe just make this a hidden option like x-miio? >> >> x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power? > > "To configure the device, we use the first I/O region of the PCI > device." > > Meh, it does sound a little like we are specifying that it's an PCI I/O > bar. > > Let's resurrect the PCI-v2 idea, which is ready to implement now, and a > nice cleanup? Detach it from the change-of-ring-format idea which is > turning out to be a tarpit. I think that's a sensible approach. Regards, Anthony Liguori > Thanks, > Rusty.
On 03/19/2012 08:57 PM, Michael S. Tsirkin wrote: > > > > Should be done via an extra BAR (with the same layout, perhaps extended) > > so compatibility is preserved. > > No, that would need guest changes to be of use. The point of this hack > is to make things work for Linux guests where PIO does not work. It only works if all guest's PCI layer knows to deal with the bit flip correctly. I imagine it isn't true for at least the seabios virtio drivers.
On Tue, Mar 20, 2012 at 10:22:42AM +1030, Rusty Russell wrote: > On Mon, 19 Mar 2012 17:13:06 -0500, Anthony Liguori <anthony@codemonkey.ws> wrote: > > > Maybe just make this a hidden option like x-miio? > > > > x-violate-the-virtio-spec-to-trick-old-linux-drivers-into-working-on-power? > > "To configure the device, we use the first I/O region of the PCI > device." > > Meh, it does sound a little like we are specifying that it's an PCI I/O > bar. > > Let's resurrect the PCI-v2 idea, which is ready to implement now, and a > nice cleanup? Detach it from the change-of-ring-format idea which is > turning out to be a tarpit. > > Thanks, > Rusty. Yes. But it seems silly to even write code to play with device config in memory when we agreed the right thing to do is to use a config vq everywhere. Now a question: does a oconfig vq look like a PCI specific feature to you, a work-around for lack of multibyte atomic accesses? If yes it's sane to make it a PCI capability. Or is it something most transports would need? If yes we need a feature bit and this is a chicken and egg problem ... > -- > How could I marry someone with more hair than me? http://baldalex.org
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 28498ec..6f338d2 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -655,6 +655,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) { uint8_t *config; uint32_t size; + uint8_t bar0_type; proxy->vdev = vdev; @@ -684,8 +685,14 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) memory_region_init_io(&proxy->bar, &virtio_pci_config_ops, proxy, "virtio-pci", size); - pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, - &proxy->bar); + + if (proxy->flags & VIRTIO_PCI_FLAG_USE_MMIO) { + bar0_type = PCI_BASE_ADDRESS_SPACE_MEMORY; + } else { + bar0_type = PCI_BASE_ADDRESS_SPACE_IO; + } + + pci_register_bar(&proxy->pci_dev, 0, bar0_type, &proxy->bar); if (!kvm_has_many_ioeventfds()) { proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; @@ -823,6 +830,7 @@ static Property virtio_blk_properties[] = { DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true), DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features), + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), DEFINE_PROP_END_OF_LIST(), }; @@ -856,6 +864,7 @@ static Property virtio_net_properties[] = { DEFINE_PROP_UINT32("x-txtimer", VirtIOPCIProxy, net.txtimer, TX_TIMER_INTERVAL), DEFINE_PROP_INT32("x-txburst", VirtIOPCIProxy, net.txburst, TX_BURST), DEFINE_PROP_STRING("tx", VirtIOPCIProxy, net.tx), + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), DEFINE_PROP_END_OF_LIST(), }; @@ -888,6 +897,7 @@ static Property virtio_serial_properties[] = { DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0), DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, serial.max_virtserial_ports, 31), + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), DEFINE_PROP_END_OF_LIST(), }; @@ -915,6 +925,7 @@ static TypeInfo virtio_serial_info = { static Property virtio_balloon_properties[] = { DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features), + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), DEFINE_PROP_END_OF_LIST(), }; @@ -969,6 +980,7 @@ static int virtio_scsi_exit_pci(PCIDevice *pci_dev) static Property virtio_scsi_properties[] = { DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2), DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi), + DEFINE_PROP_BIT("mmio", VirtIOPCIProxy, flags, VIRTIO_PCI_FLAG_USE_MMIO_BIT, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h index e560428..e6a8861 100644 --- a/hw/virtio-pci.h +++ b/hw/virtio-pci.h @@ -24,6 +24,10 @@ #define VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT 1 #define VIRTIO_PCI_FLAG_USE_IOEVENTFD (1 << VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT) +/* Some guests don't support port IO. Use MMIO instead. */ +#define VIRTIO_PCI_FLAG_USE_MMIO_BIT 2 +#define VIRTIO_PCI_FLAG_USE_MMIO (1 << VIRTIO_PCI_FLAG_USE_MMIO_BIT) + typedef struct { PCIDevice pci_dev; VirtIODevice *vdev;
Currently virtio-pci is specified so that configuration of the device is done through a PCI IO space (via BAR 0 of the virtual PCI device). However, Linux guests happen to use ioread/iowrite/iomap primitives for access, and these work uniformly across memory/io BARs. While PCI IO accesses are faster than MMIO on x86 kvm, MMIO might be helpful on other systems which don't implement PIO or where PIO is slower than MMIO. Add a property to make it possible to tweak the BAR type. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> This is harmless by default but causes segfaults in memory.c when enabled. Thus an RFC until I figure out what's wrong. --- hw/virtio-pci.c | 16 ++++++++++++++-- hw/virtio-pci.h | 4 ++++ 2 files changed, 18 insertions(+), 2 deletions(-)