[PATCH kvmtool 0/4] kvmtool: clang/GCC9 fixes

[PATCH kvmtool 0/4] kvmtool: clang/GCC9 fixes

[Andre Przywara]

When compiling kvmtool with clang (works only on aarch64/arm), it turned
up some interesting warnings. One of those is also issued by GCC9.

This series fixes them. More details in each commit message.

Please have look!

Cheers,
Andre.

Andre Przywara (4):
  vfio: remove spurious ampersand
  vfio: remove unneeded test
  vfio: rework vfio_irq_set payload setting
  virtio/blk: Avoid taking pointer to packed struct

 vfio/pci.c   | 28 ++++++++++++++--------------
 virtio/blk.c |  4 ++--
 2 files changed, 16 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH kvmtool 1/4] vfio: remove spurious ampersand

[Andre Przywara]

As clang rightfully pointed out, the ampersand in front of this member
looks wrong.

Remove it so we actually really compare against the count being 0.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
 vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vfio/pci.c b/vfio/pci.c
index f17498ea..10aa87b1 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -952,7 +952,7 @@ static int vfio_pci_init_msis(struct kvm *kvm, struct vfio_device *vdev,
 	size_t nr_entries = msis->nr_entries;
 
 	ret = ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO, &msis->info);
-	if (ret || &msis->info.count == 0) {
+	if (ret || msis->info.count == 0) {
 		vfio_dev_err(vdev, "no MSI reported by VFIO");
 		return -ENODEV;
 	}
-- 
2.17.1

[PATCH kvmtool 2/4] vfio: remove unneeded test

[Andre Przywara]

clang complained that the comparison of an u8 variable against 256 is
somewhat pointless.

Just remove the check, as the condition will never hit.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
 vfio/pci.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/vfio/pci.c b/vfio/pci.c
index 10aa87b1..a4086326 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -557,11 +557,6 @@ static int vfio_pci_parse_caps(struct vfio_device *vdev)
 	pdev->hdr.capabilities = 0;
 
 	for (; pos; pos = next) {
-		if (pos >= PCI_DEV_CFG_SIZE) {
-			vfio_dev_warn(vdev, "ignoring cap outside of config space");
-			return -EINVAL;
-		}
-
 		cap = PCI_CAP(&pdev->hdr, pos);
 		next = cap->next;
 
-- 
2.17.1

[PATCH kvmtool 3/4] vfio: rework vfio_irq_set payload setting

[Andre Przywara]

struct vfio_irq_set from the kernel headers contains a variable sized
array to hold a payload. The vfio_irq_eventfd struct puts the "fd"
member right after this, hoping it to automatically fit in the payload slot.
But having a variable sized type not at the end of a struct is a GNU C
extension, so clang will refuse to compile this.

Solve this by somewhat doing the compiler's job and place the payload
manually at the end of the structure.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 vfio/pci.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/vfio/pci.c b/vfio/pci.c
index a4086326..76e24c15 100644
--- a/vfio/pci.c
+++ b/vfio/pci.c
@@ -9,11 +9,16 @@
 #include <sys/time.h>
 
 /* Wrapper around UAPI vfio_irq_set */
-struct vfio_irq_eventfd {
+union vfio_irq_eventfd {
 	struct vfio_irq_set	irq;
-	int			fd;
+	u8 buffer[sizeof(struct vfio_irq_set) + sizeof(int)];
 };
 
+static void set_vfio_irq_eventd_payload(union vfio_irq_eventfd *evfd, int fd)
+{
+	memcpy(&evfd->irq.data, &fd, sizeof(fd));
+}
+
 #define msi_is_enabled(state)		((state) & VFIO_PCI_MSI_STATE_ENABLED)
 #define msi_is_masked(state)		((state) & VFIO_PCI_MSI_STATE_MASKED)
 #define msi_is_empty(state)		((state) & VFIO_PCI_MSI_STATE_EMPTY)
@@ -38,7 +43,7 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
 	int *eventfds;
 	struct vfio_pci_device *pdev = &vdev->pci;
 	struct vfio_pci_msi_common *msis = msix ? &pdev->msix : &pdev->msi;
-	struct vfio_irq_eventfd single = {
+	union vfio_irq_eventfd single = {
 		.irq = {
 			.argsz	= sizeof(single),
 			.flags	= VFIO_IRQ_SET_DATA_EVENTFD |
@@ -117,7 +122,7 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
 			continue;
 
 		single.irq.start = i;
-		single.fd = fd;
+		set_vfio_irq_eventd_payload(&single, fd);
 
 		ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &single);
 		if (ret < 0) {
@@ -1021,8 +1026,8 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 {
 	int ret;
 	int trigger_fd, unmask_fd;
-	struct vfio_irq_eventfd	trigger;
-	struct vfio_irq_eventfd	unmask;
+	union vfio_irq_eventfd	trigger;
+	union vfio_irq_eventfd	unmask;
 	struct vfio_pci_device *pdev = &vdev->pci;
 	int gsi = pdev->intx_gsi;
 
@@ -1058,7 +1063,7 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 		.start	= 0,
 		.count	= 1,
 	};
-	trigger.fd = trigger_fd;
+	set_vfio_irq_eventd_payload(&trigger, trigger_fd);
 
 	ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &trigger);
 	if (ret < 0) {
@@ -1073,7 +1078,7 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
 		.start	= 0,
 		.count	= 1,
 	};
-	unmask.fd = unmask_fd;
+	set_vfio_irq_eventd_payload(&unmask, unmask_fd);
 
 	ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &unmask);
 	if (ret < 0) {
-- 
2.17.1

[PATCH kvmtool 4/4] virtio/blk: Avoid taking pointer to packed struct

[Andre Przywara]

clang and GCC9 refuse to compile virtio/blk.c with the following message:
virtio/blk.c:161:37: error: taking address of packed member 'geometry' of class
      or structure 'virtio_blk_config' may result in an unaligned pointer value
      [-Werror,-Waddress-of-packed-member]
        struct virtio_blk_geometry *geo = &conf->geometry;

Since struct virtio_blk_geometry is in a kernel header, we can't do much
about the packed attribute, but as Peter pointed out, the solution is
rather simple: just get rid of the convenience variable and use the
original struct member directly.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virtio/blk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/virtio/blk.c b/virtio/blk.c
index 50db6f5f..f267be15 100644
--- a/virtio/blk.c
+++ b/virtio/blk.c
@@ -161,7 +161,6 @@ static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
 {
 	struct blk_dev *bdev = dev;
 	struct virtio_blk_config *conf = &bdev->blk_config;
-	struct virtio_blk_geometry *geo = &conf->geometry;
 
 	bdev->features = features;
 
@@ -170,7 +169,8 @@ static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
 	conf->seg_max = virtio_host_to_guest_u32(&bdev->vdev, conf->seg_max);
 
 	/* Geometry */
-	geo->cylinders = virtio_host_to_guest_u16(&bdev->vdev, geo->cylinders);
+	conf->geometry.cylinders = virtio_host_to_guest_u16(&bdev->vdev,
+						conf->geometry.cylinders);
 
 	conf->blk_size = virtio_host_to_guest_u32(&bdev->vdev, conf->blk_size);
 	conf->min_io_size = virtio_host_to_guest_u16(&bdev->vdev, conf->min_io_size);
-- 
2.17.1

Re: [PATCH kvmtool 3/4] vfio: rework vfio_irq_set payload setting

[Jean-Philippe Brucker]

On 03/05/2019 18:15, Andre Przywara wrote:
> struct vfio_irq_set from the kernel headers contains a variable sized
> array to hold a payload. The vfio_irq_eventfd struct puts the "fd"
> member right after this, hoping it to automatically fit in the payload slot.
> But having a variable sized type not at the end of a struct is a GNU C
> extension, so clang will refuse to compile this.
> 
> Solve this by somewhat doing the compiler's job and place the payload
> manually at the end of the structure.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

It's odd that clang still warns about it even when passing -std=gnuXX.
The resulting code doesn't look particularly nice, but it would be good
to have the other patches upstream and this one is harmless, so:

Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>

> ---
>  vfio/pci.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/vfio/pci.c b/vfio/pci.c
> index a4086326..76e24c15 100644
> --- a/vfio/pci.c
> +++ b/vfio/pci.c
> @@ -9,11 +9,16 @@
>  #include <sys/time.h>
>  
>  /* Wrapper around UAPI vfio_irq_set */
> -struct vfio_irq_eventfd {
> +union vfio_irq_eventfd {
>  	struct vfio_irq_set	irq;
> -	int			fd;
> +	u8 buffer[sizeof(struct vfio_irq_set) + sizeof(int)];
>  };
>  
> +static void set_vfio_irq_eventd_payload(union vfio_irq_eventfd *evfd, int fd)
> +{
> +	memcpy(&evfd->irq.data, &fd, sizeof(fd));
> +}
> +
>  #define msi_is_enabled(state)		((state) & VFIO_PCI_MSI_STATE_ENABLED)
>  #define msi_is_masked(state)		((state) & VFIO_PCI_MSI_STATE_MASKED)
>  #define msi_is_empty(state)		((state) & VFIO_PCI_MSI_STATE_EMPTY)
> @@ -38,7 +43,7 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
>  	int *eventfds;
>  	struct vfio_pci_device *pdev = &vdev->pci;
>  	struct vfio_pci_msi_common *msis = msix ? &pdev->msix : &pdev->msi;
> -	struct vfio_irq_eventfd single = {
> +	union vfio_irq_eventfd single = {
>  		.irq = {
>  			.argsz	= sizeof(single),
>  			.flags	= VFIO_IRQ_SET_DATA_EVENTFD |
> @@ -117,7 +122,7 @@ static int vfio_pci_enable_msis(struct kvm *kvm, struct vfio_device *vdev,
>  			continue;
>  
>  		single.irq.start = i;
> -		single.fd = fd;
> +		set_vfio_irq_eventd_payload(&single, fd);
>  
>  		ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &single);
>  		if (ret < 0) {
> @@ -1021,8 +1026,8 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  {
>  	int ret;
>  	int trigger_fd, unmask_fd;
> -	struct vfio_irq_eventfd	trigger;
> -	struct vfio_irq_eventfd	unmask;
> +	union vfio_irq_eventfd	trigger;
> +	union vfio_irq_eventfd	unmask;
>  	struct vfio_pci_device *pdev = &vdev->pci;
>  	int gsi = pdev->intx_gsi;
>  
> @@ -1058,7 +1063,7 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  		.start	= 0,
>  		.count	= 1,
>  	};
> -	trigger.fd = trigger_fd;
> +	set_vfio_irq_eventd_payload(&trigger, trigger_fd);
>  
>  	ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &trigger);
>  	if (ret < 0) {
> @@ -1073,7 +1078,7 @@ static int vfio_pci_enable_intx(struct kvm *kvm, struct vfio_device *vdev)
>  		.start	= 0,
>  		.count	= 1,
>  	};
> -	unmask.fd = unmask_fd;
> +	set_vfio_irq_eventd_payload(&unmask, unmask_fd);
>  
>  	ret = ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &unmask);
>  	if (ret < 0) {
> 

Re: [PATCH kvmtool 4/4] virtio/blk: Avoid taking pointer to packed struct

[Jean-Philippe Brucker]

On 03/05/2019 18:15, Andre Przywara wrote:
> clang and GCC9 refuse to compile virtio/blk.c with the following message:
> virtio/blk.c:161:37: error: taking address of packed member 'geometry' of class
>       or structure 'virtio_blk_config' may result in an unaligned pointer value
>       [-Werror,-Waddress-of-packed-member]
>         struct virtio_blk_geometry *geo = &conf->geometry;
> 
> Since struct virtio_blk_geometry is in a kernel header, we can't do much
> about the packed attribute, but as Peter pointed out, the solution is
> rather simple: just get rid of the convenience variable and use the
> original struct member directly.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>

I just encountered this one with GCC 9.1

> ---
>  virtio/blk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/virtio/blk.c b/virtio/blk.c
> index 50db6f5f..f267be15 100644
> --- a/virtio/blk.c
> +++ b/virtio/blk.c
> @@ -161,7 +161,6 @@ static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
>  {
>  	struct blk_dev *bdev = dev;
>  	struct virtio_blk_config *conf = &bdev->blk_config;
> -	struct virtio_blk_geometry *geo = &conf->geometry;
>  
>  	bdev->features = features;
>  
> @@ -170,7 +169,8 @@ static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
>  	conf->seg_max = virtio_host_to_guest_u32(&bdev->vdev, conf->seg_max);
>  
>  	/* Geometry */
> -	geo->cylinders = virtio_host_to_guest_u16(&bdev->vdev, geo->cylinders);
> +	conf->geometry.cylinders = virtio_host_to_guest_u16(&bdev->vdev,
> +						conf->geometry.cylinders);
>  
>  	conf->blk_size = virtio_host_to_guest_u32(&bdev->vdev, conf->blk_size);
>  	conf->min_io_size = virtio_host_to_guest_u16(&bdev->vdev, conf->min_io_size);
> 

Re: [PATCH kvmtool 0/4] kvmtool: clang/GCC9 fixes

[Will Deacon]

On Fri, May 03, 2019 at 06:15:40PM +0100, Andre Przywara wrote:
> When compiling kvmtool with clang (works only on aarch64/arm), it turned
> up some interesting warnings. One of those is also issued by GCC9.
> 
> This series fixes them. More details in each commit message.

Thanks, pushed out now.

Will