Re: [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi property type

[Marcel Apfelbaum <marcel@redhat.com>]

On 05/06/2016 07:20 AM, Cao jin wrote:
>  From bit to enum OnOffAuto.
>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Markus Armbruster <armbru@redhat.com>
> cc: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>
> Actually, I am not quite sure this device need this change, RFC.
>

Well, it already has the 'msi' property, so we may want to make it standard 'OnOffAuto'.
One problem I can see is the change of semantics. Until now msi=on means 'auto'. From now on
it means 'force msi=on', fail otherwise. If I got this right,  old machines having msi=on
will failed to start on platforms with msibroken=true, right?

Maybe we should preserve the semantics for old machines? (this patch does not actually
affect the semantics, but patch 11/11 should, otherwise why change it to OnOffAuto, right? )

Thanks,
Marcel


>   hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 32f4daa..9e31f0e 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -41,9 +41,10 @@ struct PCIBridgeDev {
>
>       MemoryRegion bar;
>       uint8_t chassis_nr;
> -#define PCI_BRIDGE_DEV_F_MSI_REQ 0
> -#define PCI_BRIDGE_DEV_F_SHPC_REQ 1
> +#define PCI_BRIDGE_DEV_F_SHPC_REQ 0
>       uint32_t flags;
> +
> +    OnOffAuto msi;
>   };
>   typedef struct PCIBridgeDev PCIBridgeDev;
>
> @@ -65,7 +66,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>           }
>       } else {
>           /* MSI is not applicable without SHPC */
> -        bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ);
> +        bridge_dev->msi = ON_OFF_AUTO_OFF;
>       }
>
>       err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
> @@ -73,7 +74,8 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>           goto slotid_error;
>       }
>
> -    if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
> +    if ((bridge_dev->msi == ON_OFF_AUTO_AUTO ||
> +                bridge_dev->msi == ON_OFF_AUTO_ON) &&
>           msi_nonbroken) {
>           err = msi_init(dev, 0, 1, true, true);
>           if (err < 0) {
> @@ -146,8 +148,8 @@ static Property pci_bridge_dev_properties[] = {
>                       /* Note: 0 is not a legal chassis number. */
>       DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr,
>                         0),
> -    DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags,
> -                    PCI_BRIDGE_DEV_F_MSI_REQ, true),
> +    DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi,
> +                            ON_OFF_AUTO_AUTO),
>       DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
>                       PCI_BRIDGE_DEV_F_SHPC_REQ, true),
>       DEFINE_PROP_END_OF_LIST(),
>