Re: [Qemu-devel] [PATCH V3] hw/virtio-pci: fix virtio behaviour

[Cornelia Huck]

On Wed, 20 Jul 2016 18:28:21 +0300
Marcel Apfelbaum <address@hidden> wrote:

> Enable transitional virtio devices by default.
> Enable virtio-1.0 for devices plugged into
> PCIe ports (Root ports or Downstream ports).

Add "by default", as this can still be overridden?

> 
> Using the virtio-1 mode will remove the limitation

s/Using the virtio-1 mode/Disabling the legacy mode/

?

> of the number of devices that can be attached to a machine
> by removing the need for the IO BAR.
> 
> Signed-off-by: Marcel Apfelbaum <address@hidden>

(...)

> +static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
> +{
> +    return !proxy->disable_modern;
> +}
> +
> +static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy)
> +{
> +    return proxy->disable_legacy == ON_OFF_AUTO_OFF;
> +}
> +
> +static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)

One thing I still find a bit confusing is that you refer to 'modern'
above, but force to 'virtio_1' here... but that's a minor thing.

> +{
> +    proxy->disable_modern = false;
> +    proxy->disable_legacy = ON_OFF_AUTO_ON;
> +}
> 
>  /*
>   * virtio-scsi-pci: This extends VirtioPCIProxy.
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 9914e7a..1531399 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -6,6 +6,14 @@
>          .driver   = "virtio-mmio",\
>          .property = "format_transport_address",\
>          .value    = "off",\
> +    },{\
> +        .driver   = "virtio-pci",\
> +        .property = "disable-modern",\
> +        .value    = "on",\
> +    },{\
> +        .driver   = "virtio-pci",\
> +        .property = "disable-legacy",\
> +        .value    = "off",\

After looking at the code, I think this will work - did you test this
with a compat machine, though?

>      },
> 
>  #define HW_COMPAT_2_5 \

But generally, looks good and I think this is also an improvement in
readability.