From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:38767)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <alex.williamson@redhat.com>) id 1YLDqi-0001cT-3d
	for qemu-devel@nongnu.org; Tue, 10 Feb 2015 11:39:44 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <alex.williamson@redhat.com>) id 1YLDqc-0000Ud-Jb
	for qemu-devel@nongnu.org; Tue, 10 Feb 2015 11:39:44 -0500
Received: from mx1.redhat.com ([209.132.183.28]:42640)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <alex.williamson@redhat.com>) id 1YLDqc-0000UV-D6
	for qemu-devel@nongnu.org; Tue, 10 Feb 2015 11:39:38 -0500
Message-ID: <1423586371.22865.812.camel@redhat.com>
From: Alex Williamson <alex.williamson@redhat.com>
Date: Tue, 10 Feb 2015 09:39:31 -0700
In-Reply-To: <d5318d00525c9347ca49b7922a35381d6a84afb5.1423551266.git.chen.fan.fnst@cn.fujitsu.com>
References: <cover.1423551266.git.chen.fan.fnst@cn.fujitsu.com>
	<d5318d00525c9347ca49b7922a35381d6a84afb5.1423551266.git.chen.fan.fnst@cn.fujitsu.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [RFC v3 07/10] vfio_pci: change vfio device
 features bit macro to enum definition
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Cc: izumi.taku@jp.fujitsu.com, qemu-devel@nongnu.org

On Tue, 2015-02-10 at 15:03 +0800, Chen Fan wrote:
> Introduce an independent enum structure to define the features bitmap,
> it would be good for adding new features definition.
> 
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  hw/vfio/pci.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 75c932b..bf314a1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -134,6 +134,12 @@ typedef struct VFIOMSIXInfo {
>      void *mmap;
>  } VFIOMSIXInfo;
>  
> +/* Bits in VFIOPCIDevice features field. */
> +enum {
> +#define VFIO_FEATURE_ENABLE_VGA_BIT 0
> +    VFIO_FEATURE_ENABLE_VGA = (1 << VFIO_FEATURE_ENABLE_VGA_BIT),
> +};
> +

As commented last time, I don't see why making this an enum helps
anything.  If anything, the bits could be an enum, but it makes no sense
to me to put the feature mask into an enum.

>  typedef struct VFIOPCIDevice {
>      PCIDevice pdev;
>      VFIODevice vbasedev;
> @@ -154,8 +160,6 @@ typedef struct VFIOPCIDevice {
>      PCIHostDeviceAddress host;
>      EventNotifier err_notifier;
>      uint32_t features;
> -#define VFIO_FEATURE_ENABLE_VGA_BIT 0
> -#define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>      int32_t bootindex;
>      uint8_t pm_cap;
>      bool has_vga;