Re: [Qemu-devel] [RfC PATCH] virtio-pci: place msix regions in modern virtio bar

[Michael S. Tsirkin]

On Fri, Jan 23, 2015 at 03:48:28PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > >  Also seabios wouldn't map the bar
> > > above 4G.
> 
> > We can teach it to do that, and even if not, OS can do it on hotplug.
> 
> Just to clarify: seabios can do it in the general case, it just wouldn't
> do it for xhci specifically because otherwise the seabios xhci driver
> wouldn't be able to reach the registers any more ...
> 
> For non-transitional virtio-{blk,scsi,net} devices seabios (and ipxe)
> would have a simliar problem btw:  When mapping the bars above 4G it
> can't handle the devices any more.

You can always use the config cap for this.
I didn't implement it yet, but I will.

> But we'll probably stick to transitional by default for a while (where
> seabios can just use the legacy registers in io address space),

io *is* very crowded. I think we should try and code up modern drivers
when we have the time.

> and
> there also is ovmf coming which runs in 64bit mode so mapping the bars
> high isn't a problem there.
> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [RfC PATCH] virtio-pci: place msix regions in modern virtio bar

[Gerd Hoffmann]

  Hi,

> >  Also seabios wouldn't map the bar
> > above 4G.

> We can teach it to do that, and even if not, OS can do it on hotplug.

Just to clarify: seabios can do it in the general case, it just wouldn't
do it for xhci specifically because otherwise the seabios xhci driver
wouldn't be able to reach the registers any more ...

For non-transitional virtio-{blk,scsi,net} devices seabios (and ipxe)
would have a simliar problem btw:  When mapping the bars above 4G it
can't handle the devices any more.

But we'll probably stick to transitional by default for a while (where
seabios can just use the legacy registers in io address space), and
there also is ovmf coming which runs in 64bit mode so mapping the bars
high isn't a problem there.

cheers,
  Gerd

Re: [Qemu-devel] [RfC PATCH] virtio-pci: place msix regions in modern virtio bar

[Michael S. Tsirkin]

On Fri, Jan 23, 2015 at 02:53:27PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > I wanted to make the modern BAR prefetcheable, so it can
> > be a full 64-bit one,
> 
> It's not the case right now though, the two bars have identical
> attributes, and there is plenty of unused space in the modern virtio
> bar ...

It's just POC, I'll change it.

> > this is impossible with the MSI-X
> > BAR.
> 
> What exactly is impossible?  msi-x being in a 64bit bar?  msi-x being in
> a prefetchable bar?

This.

>  Also why the bar needs to be prefetchable to be
> 64bit bit?

It doesn't but bridges only have 32 bit windows for non prefetcheable
bars. So even if you try to make it 64 bit, only the low 32 bit are used.

> xhci looks this way right now:
> 
> 00:0f.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
> Controller (rev 03) (prog-if 30 [XHCI])
>   Subsystem: Red Hat, Inc QEMU Virtual Machine
>   Physical Slot: 15
>   Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> Stepping- SERR+ FastB2B- DisINTx+
>   Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-  SERR-Latency: 0, Cache Line Size: 64 bytes
>   Interrupt: pin A routed to IRQ 10
>   Region 0: Memory at febd (64-bit, non-prefetchable) [size=16K]
>   Capabilities: [90] MSI-X: Enable+ Count=16 Masked-
>   Vector table: BAR=0 offset=3000
>   PBA: BAR=0 offset=3800
>   Capabilities: [70] MSI: Enable- Count=1/16 Maskable- 64bit+
>   Address:   Data: 
>   Kernel driver in use: xhci_hcd
> 
> Which works just fine even with msi-x in a 64-bit (non-prefetchable)
> bar, but maybe just by pure luck?

xhci is integrated in a root complex so it can work there.

>  Also seabios wouldn't map the bar
> above 4G.
> 
> cheers,
>   Gerd

We can teach it to do that, and even if not, OS can do it on hotplug.

Re: [Qemu-devel] [RfC PATCH] virtio-pci: place msix regions in modern virtio bar

[Gerd Hoffmann]

  Hi,

> I wanted to make the modern BAR prefetcheable, so it can
> be a full 64-bit one,

It's not the case right now though, the two bars have identical
attributes, and there is plenty of unused space in the modern virtio
bar ...

> this is impossible with the MSI-X
> BAR.

What exactly is impossible?  msi-x being in a 64bit bar?  msi-x being in
a prefetchable bar?  Also why the bar needs to be prefetchable to be
64bit bit?

xhci looks this way right now:

00:0f.0 USB controller: NEC Corporation uPD720200 USB 3.0 Host
Controller (rev 03) (prog-if 30 [XHCI])
Subsystem: Red Hat, Inc QEMU Virtual Machine
Physical Slot: 15
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR+ FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- SERR- 

Re: [Qemu-devel] [RfC PATCH] virtio-pci: place msix regions in modern virtio bar

[Michael S. Tsirkin]

On Fri, Jan 23, 2015 at 01:29:43PM +0100, Gerd Hoffmann wrote:
> Only for legacy-free virtio devices, to avoid unpleasent
> surprises with old drivers.
> 
> mtree snippet:
> 
> fea0-fea7 (prio 1, RW): virtio-pci
>   fea0-fea00fff (prio 0, RW): virtio-pci-common
>   fea01000-fea01fff (prio 0, RW): virtio-pci-isr
>   fea02000-fea02fff (prio 0, RW): virtio-pci-device
>   fea03000-fea42fff (prio 0, RW): virtio-pci-notify
>   fea7e000-fea7e02f (prio 0, RW): msix-table
>   fea7f000-fea7f007 (prio 0, RW): msix-pba
> 
> Applies on top of mst's virtio-1.0 branch, plus my three virtio-pci
> patches posted to the list yesterday.
> 
> Signed-off-by: Gerd Hoffmann 

Hmm what's the point here?

I wanted to make the modern BAR prefetcheable, so it can
be a full 64-bit one, this is impossible with the MSI-X
BAR.

This requires keeping it separate though.

Again, if we start running of BARs we'll consider our options
then, these things are easy to change.


> ---
>  hw/virtio/virtio-pci.c | 39 +++
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ba1405f..934ca7a 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1235,7 +1235,7 @@ static void virtio_pci_device_plugged(DeviceState *d)
>  bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
>  int modern_mem_bar;
>  uint8_t *config;
> -uint32_t size;
> +uint32_t modern_mem_size, legacy_io_size;
>  
>  config = proxy->pci_dev.config;
>  if (proxy->class_code) {
> @@ -1339,9 +1339,10 @@ static void virtio_pci_device_plugged(DeviceState *d)
>  virtio_pci_add_mem_cap(proxy, ¬ify.cap);
>  
>  virtio_add_feature(&proxy->host_features, VIRTIO_F_VERSION_1);
> +modern_mem_size = 2 * QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
> +VIRTIO_PCI_QUEUE_MAX;
>  memory_region_init(&proxy->modern_bar, OBJECT(proxy), "virtio-pci",
> -   2 * QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
> -   VIRTIO_PCI_QUEUE_MAX);
> +   modern_mem_size);
>  memory_region_init_io(&proxy->common, OBJECT(proxy),
>&common_ops,
>proxy,
> @@ -1369,25 +1370,39 @@ static void virtio_pci_device_plugged(DeviceState *d)
>   &proxy->modern_bar);
>  }
>  
> -if (proxy->nvectors &&
> -msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
> -error_report("unable to init msix vectors to %" PRIu32,
> - proxy->nvectors);
> -proxy->nvectors = 0;
> +if (proxy->nvectors) {
> +if (modern && modern_mem_bar == 0) {
> +if (msix_init(&proxy->pci_dev, proxy->nvectors,
> +  &proxy->modern_bar, modern_mem_bar,
> +  modern_mem_size - 0x2000,
> +  &proxy->modern_bar, modern_mem_bar,
> +  modern_mem_size - 0x1000,
> +  0x90)) {
> +error_report("unable to init msix vectors to %" PRIu32,
> + proxy->nvectors);
> +proxy->nvectors = 0;
> +}
> +} else {
> +if (msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 
> 1)) {
> +error_report("unable to init msix vectors to %" PRIu32,
> + proxy->nvectors);
> +proxy->nvectors = 0;
> +}
> +}
>  }
>  
>  proxy->pci_dev.config_write = virtio_write_config;
>  
>  if (legacy) {
> -size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> +legacy_io_size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
>  + virtio_bus_get_vdev_config_len(bus);
> -if (size & (size - 1)) {
> -size = 1 << qemu_fls(size);
> +if (legacy_io_size & (legacy_io_size - 1)) {
> +legacy_io_size = 1 << qemu_fls(legacy_io_size);
>  }
>  
>  memory_region_init_io(&proxy->bar, OBJECT(proxy),
>&virtio_pci_config_ops,
> -  proxy, "virtio-pci", size);
> +  proxy, "virtio-pci", legacy_io_size);
>  
>  pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
>   &proxy->bar);
> -- 
> 1.8.3.1

[Qemu-devel] [RfC PATCH] virtio-pci: place msix regions in modern virtio bar

[Gerd Hoffmann]

Only for legacy-free virtio devices, to avoid unpleasent
surprises with old drivers.

mtree snippet:

fea0-fea7 (prio 1, RW): virtio-pci
  fea0-fea00fff (prio 0, RW): virtio-pci-common
  fea01000-fea01fff (prio 0, RW): virtio-pci-isr
  fea02000-fea02fff (prio 0, RW): virtio-pci-device
  fea03000-fea42fff (prio 0, RW): virtio-pci-notify
  fea7e000-fea7e02f (prio 0, RW): msix-table
  fea7f000-fea7f007 (prio 0, RW): msix-pba

Applies on top of mst's virtio-1.0 branch, plus my three virtio-pci
patches posted to the list yesterday.

Signed-off-by: Gerd Hoffmann 
---
 hw/virtio/virtio-pci.c | 39 +++
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ba1405f..934ca7a 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1235,7 +1235,7 @@ static void virtio_pci_device_plugged(DeviceState *d)
 bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
 int modern_mem_bar;
 uint8_t *config;
-uint32_t size;
+uint32_t modern_mem_size, legacy_io_size;
 
 config = proxy->pci_dev.config;
 if (proxy->class_code) {
@@ -1339,9 +1339,10 @@ static void virtio_pci_device_plugged(DeviceState *d)
 virtio_pci_add_mem_cap(proxy, ¬ify.cap);
 
 virtio_add_feature(&proxy->host_features, VIRTIO_F_VERSION_1);
+modern_mem_size = 2 * QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
+VIRTIO_PCI_QUEUE_MAX;
 memory_region_init(&proxy->modern_bar, OBJECT(proxy), "virtio-pci",
-   2 * QEMU_VIRTIO_PCI_QUEUE_MEM_MULT *
-   VIRTIO_PCI_QUEUE_MAX);
+   modern_mem_size);
 memory_region_init_io(&proxy->common, OBJECT(proxy),
   &common_ops,
   proxy,
@@ -1369,25 +1370,39 @@ static void virtio_pci_device_plugged(DeviceState *d)
  &proxy->modern_bar);
 }
 
-if (proxy->nvectors &&
-msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
-error_report("unable to init msix vectors to %" PRIu32,
- proxy->nvectors);
-proxy->nvectors = 0;
+if (proxy->nvectors) {
+if (modern && modern_mem_bar == 0) {
+if (msix_init(&proxy->pci_dev, proxy->nvectors,
+  &proxy->modern_bar, modern_mem_bar,
+  modern_mem_size - 0x2000,
+  &proxy->modern_bar, modern_mem_bar,
+  modern_mem_size - 0x1000,
+  0x90)) {
+error_report("unable to init msix vectors to %" PRIu32,
+ proxy->nvectors);
+proxy->nvectors = 0;
+}
+} else {
+if (msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) {
+error_report("unable to init msix vectors to %" PRIu32,
+ proxy->nvectors);
+proxy->nvectors = 0;
+}
+}
 }
 
 proxy->pci_dev.config_write = virtio_write_config;
 
 if (legacy) {
-size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
+legacy_io_size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
 + virtio_bus_get_vdev_config_len(bus);
-if (size & (size - 1)) {
-size = 1 << qemu_fls(size);
+if (legacy_io_size & (legacy_io_size - 1)) {
+legacy_io_size = 1 << qemu_fls(legacy_io_size);
 }
 
 memory_region_init_io(&proxy->bar, OBJECT(proxy),
   &virtio_pci_config_ops,
-  proxy, "virtio-pci", size);
+  proxy, "virtio-pci", legacy_io_size);
 
 pci_register_bar(&proxy->pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO,
  &proxy->bar);
-- 
1.8.3.1