[PATCH] virtio-mmio: read[wl]()/write[wl] are already little-endian

[PATCH] virtio-mmio: read[wl]()/write[wl] are already little-endian

[Laurent Vivier]

read[wl]()/write[wl] already access memory in little-endian mode.
No need to convert the value with cpu_to_leXX()/leXX_to_cpu()

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 drivers/virtio/virtio_mmio.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index a286d22b6551..3f6a5588f77d 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -168,17 +168,17 @@ static void vm_get(struct virtio_device *vdev, unsigned offset,
 		memcpy(buf, &b, sizeof b);
 		break;
 	case 2:
-		w = cpu_to_le16(readw(base + offset));
+		w = readw(base + offset);
 		memcpy(buf, &w, sizeof w);
 		break;
 	case 4:
-		l = cpu_to_le32(readl(base + offset));
+		l = readl(base + offset);
 		memcpy(buf, &l, sizeof l);
 		break;
 	case 8:
-		l = cpu_to_le32(readl(base + offset));
+		l = readl(base + offset);
 		memcpy(buf, &l, sizeof l);
-		l = cpu_to_le32(ioread32(base + offset + sizeof l));
+		l = ioread32(base + offset + sizeof l);
 		memcpy(buf + sizeof l, &l, sizeof l);
 		break;
 	default:
@@ -212,17 +212,17 @@ static void vm_set(struct virtio_device *vdev, unsigned offset,
 		break;
 	case 2:
 		memcpy(&w, buf, sizeof w);
-		writew(le16_to_cpu(w), base + offset);
+		writew(w, base + offset);
 		break;
 	case 4:
 		memcpy(&l, buf, sizeof l);
-		writel(le32_to_cpu(l), base + offset);
+		writel(l, base + offset);
 		break;
 	case 8:
 		memcpy(&l, buf, sizeof l);
-		writel(le32_to_cpu(l), base + offset);
+		writel(l, base + offset);
 		memcpy(&l, buf + sizeof l, sizeof l);
-		writel(le32_to_cpu(l), base + offset + sizeof l);
+		writel(l, base + offset + sizeof l);
 		break;
 	default:
 		BUG();
-- 
2.29.2

Re: [PATCH] virtio-mmio: read[wl]()/write[wl] are already little-endian

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5687 bytes --]

Hi Laurent,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.12-rc2 next-20210309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Laurent-Vivier/virtio-mmio-read-wl-write-wl-are-already-little-endian/20210310-064527
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
config: x86_64-randconfig-s022-20210310 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-262-g5e674421-dirty
        # https://github.com/0day-ci/linux/commit/1fd3d4da486545f554eb33663c6afe068bbcbcf8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Laurent-Vivier/virtio-mmio-read-wl-write-wl-are-already-little-endian/20210310-064527
        git checkout 1fd3d4da486545f554eb33663c6afe068bbcbcf8
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   drivers/virtio/virtio_mmio.c:171:19: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le16 [usertype] w @@     got unsigned short @@
   drivers/virtio/virtio_mmio.c:171:19: sparse:     expected restricted __le16 [usertype] w
   drivers/virtio/virtio_mmio.c:171:19: sparse:     got unsigned short
   drivers/virtio/virtio_mmio.c:175:19: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [usertype] l @@     got unsigned int @@
   drivers/virtio/virtio_mmio.c:175:19: sparse:     expected restricted __le32 [usertype] l
   drivers/virtio/virtio_mmio.c:175:19: sparse:     got unsigned int
   drivers/virtio/virtio_mmio.c:179:19: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [addressable] [usertype] l @@     got unsigned int @@
   drivers/virtio/virtio_mmio.c:179:19: sparse:     expected restricted __le32 [addressable] [usertype] l
   drivers/virtio/virtio_mmio.c:179:19: sparse:     got unsigned int
   drivers/virtio/virtio_mmio.c:181:19: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __le32 [addressable] [usertype] l @@     got unsigned int @@
   drivers/virtio/virtio_mmio.c:181:19: sparse:     expected restricted __le32 [addressable] [usertype] l
   drivers/virtio/virtio_mmio.c:181:19: sparse:     got unsigned int
>> drivers/virtio/virtio_mmio.c:215:24: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned short val @@     got restricted __le16 [addressable] [usertype] w @@
   drivers/virtio/virtio_mmio.c:215:24: sparse:     expected unsigned short val
   drivers/virtio/virtio_mmio.c:215:24: sparse:     got restricted __le16 [addressable] [usertype] w
>> drivers/virtio/virtio_mmio.c:219:24: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int val @@     got restricted __le32 [addressable] [usertype] l @@
   drivers/virtio/virtio_mmio.c:219:24: sparse:     expected unsigned int val
   drivers/virtio/virtio_mmio.c:219:24: sparse:     got restricted __le32 [addressable] [usertype] l
   drivers/virtio/virtio_mmio.c:223:24: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int val @@     got restricted __le32 [addressable] [usertype] l @@
   drivers/virtio/virtio_mmio.c:223:24: sparse:     expected unsigned int val
   drivers/virtio/virtio_mmio.c:223:24: sparse:     got restricted __le32 [addressable] [usertype] l
   drivers/virtio/virtio_mmio.c:225:24: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned int val @@     got restricted __le32 [addressable] [usertype] l @@
   drivers/virtio/virtio_mmio.c:225:24: sparse:     expected unsigned int val
   drivers/virtio/virtio_mmio.c:225:24: sparse:     got restricted __le32 [addressable] [usertype] l

vim +215 drivers/virtio/virtio_mmio.c

   188	
   189	static void vm_set(struct virtio_device *vdev, unsigned offset,
   190			   const void *buf, unsigned len)
   191	{
   192		struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
   193		void __iomem *base = vm_dev->base + VIRTIO_MMIO_CONFIG;
   194		u8 b;
   195		__le16 w;
   196		__le32 l;
   197	
   198		if (vm_dev->version == 1) {
   199			const u8 *ptr = buf;
   200			int i;
   201	
   202			for (i = 0; i < len; i++)
   203				writeb(ptr[i], base + offset + i);
   204	
   205			return;
   206		}
   207	
   208		switch (len) {
   209		case 1:
   210			memcpy(&b, buf, sizeof b);
   211			writeb(b, base + offset);
   212			break;
   213		case 2:
   214			memcpy(&w, buf, sizeof w);
 > 215			writew(w, base + offset);
   216			break;
   217		case 4:
   218			memcpy(&l, buf, sizeof l);
 > 219			writel(l, base + offset);
   220			break;
   221		case 8:
   222			memcpy(&l, buf, sizeof l);
   223			writel(l, base + offset);
   224			memcpy(&l, buf + sizeof l, sizeof l);
   225			writel(l, base + offset + sizeof l);
   226			break;
   227		default:
   228			BUG();
   229		}
   230	}
   231	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34366 bytes --]

Re: [PATCH] virtio-mmio: read[wl]()/write[wl] are already little-endian

[Michael S. Tsirkin]

On Tue, Mar 09, 2021 at 11:43:13PM +0100, Laurent Vivier wrote:
> read[wl]()/write[wl] already access memory in little-endian mode.

But then they convert it to CPU right? We just convert it back ...

> No need to convert the value with cpu_to_leXX()/leXX_to_cpu()
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  drivers/virtio/virtio_mmio.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index a286d22b6551..3f6a5588f77d 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -168,17 +168,17 @@ static void vm_get(struct virtio_device *vdev, unsigned offset,
>  		memcpy(buf, &b, sizeof b);
>  		break;
>  	case 2:
> -		w = cpu_to_le16(readw(base + offset));
> +		w = readw(base + offset);
>  		memcpy(buf, &w, sizeof w);
>  		break;
>  	case 4:
> -		l = cpu_to_le32(readl(base + offset));
> +		l = readl(base + offset);
>  		memcpy(buf, &l, sizeof l);
>  		break;
>  	case 8:
> -		l = cpu_to_le32(readl(base + offset));
> +		l = readl(base + offset);
>  		memcpy(buf, &l, sizeof l);
> -		l = cpu_to_le32(ioread32(base + offset + sizeof l));
> +		l = ioread32(base + offset + sizeof l);
>  		memcpy(buf + sizeof l, &l, sizeof l);
>  		break;
>  	default:
> @@ -212,17 +212,17 @@ static void vm_set(struct virtio_device *vdev, unsigned offset,
>  		break;
>  	case 2:
>  		memcpy(&w, buf, sizeof w);
> -		writew(le16_to_cpu(w), base + offset);
> +		writew(w, base + offset);
>  		break;
>  	case 4:
>  		memcpy(&l, buf, sizeof l);
> -		writel(le32_to_cpu(l), base + offset);
> +		writel(l, base + offset);
>  		break;
>  	case 8:
>  		memcpy(&l, buf, sizeof l);
> -		writel(le32_to_cpu(l), base + offset);
> +		writel(l, base + offset);
>  		memcpy(&l, buf + sizeof l, sizeof l);
> -		writel(le32_to_cpu(l), base + offset + sizeof l);
> +		writel(l, base + offset + sizeof l);
>  		break;
>  	default:
>  		BUG();
> -- 
> 2.29.2

Re: [PATCH] virtio-mmio: read[wl]()/write[wl] are already little-endian

[Laurent Vivier]

Le 11/03/2021 à 16:44, Michael S. Tsirkin a écrit :
> On Tue, Mar 09, 2021 at 11:43:13PM +0100, Laurent Vivier wrote:
>> read[wl]()/write[wl] already access memory in little-endian mode.
> 
> But then they convert it to CPU right? We just convert it back ...

Yes, you're right.

But there's a real problem with a big-endian guest using directly virtio-mmio without using virtio-pci.

It seems there is one too many little-endian conversion somewhere, and this has not been detected
before because there is no big-endian guest that uses virtio-mmio directly, and with little-endian
guests the conversion is a no-op. I'm going to have a closer look to the code path... perhaps the
problem is in QEMU not in the kernel.

Thanks,
Laurent

Re: [PATCH] virtio-mmio: read[wl]()/write[wl] are already little-endian

[Laurent Vivier]

Le 11/03/2021 à 16:44, Michael S. Tsirkin a écrit :
> On Tue, Mar 09, 2021 at 11:43:13PM +0100, Laurent Vivier wrote:
>> read[wl]()/write[wl] already access memory in little-endian mode.
> 
> But then they convert it to CPU right? We just convert it back ...

In fact the problem is in QEMU.

On a big-endian guest, the readw() returns a byte-swapped value, This means QEMU doesn't provide a
little-endian value.

I found in QEMU virtio_mmio_read() provides a value with byte-swapped bytes.

The problem comes from virtio_config_readX() functions that read the value using ldX_p accessors.

Is it normal not to use the modern variant here if we are not in legacy mode?

I think we should have something like this in virtio_mmio_read (and write):

--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -112,15 +112,28 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)

     if (offset >= VIRTIO_MMIO_CONFIG) {
         offset -= VIRTIO_MMIO_CONFIG;
-        switch (size) {
-        case 1:
-            return virtio_config_readb(vdev, offset);
-        case 2:
-            return virtio_config_readw(vdev, offset);
-        case 4:
-            return virtio_config_readl(vdev, offset);
-        default:
-            abort();
+        if (proxy->legacy) {
+            switch (size) {
+            case 1:
+                return virtio_config_readb(vdev, offset);
+            case 2:
+                return virtio_config_readw(vdev, offset);
+            case 4:
+                return virtio_config_readl(vdev, offset);
+            default:
+                abort();
+            }
+        } else {
+            switch (size) {
+            case 1:
+                return virtio_config_modern_readb(vdev, offset);
+            case 2:
+                return virtio_config_modern_readw(vdev, offset);
+            case 4:
+                return virtio_config_modern_readl(vdev, offset);
+            default:
+                abort();
+            }
         }
     }
     if (size != 4) {

And we need the same thing in virtio_pci_config_read() (and write).

And this could explain why it works with virtio-pci and not with virtio-mmio with the big-endian guest:

with virtio-pci the bytes are swapped twice (once in virtio-mmio and then in virtio-pci), so they
are restored to the initial value, whereas with direct virtio-mmio they are swapped only once.

Thanks,
Laurent

Re: [PATCH] virtio-mmio: read[wl]()/write[wl] are already little-endian

[Michael S. Tsirkin]

On Sat, Mar 13, 2021 at 06:10:29PM +0100, Laurent Vivier wrote:
> Le 11/03/2021 à 16:44, Michael S. Tsirkin a écrit :
> > On Tue, Mar 09, 2021 at 11:43:13PM +0100, Laurent Vivier wrote:
> >> read[wl]()/write[wl] already access memory in little-endian mode.
> > 
> > But then they convert it to CPU right? We just convert it back ...
> 
> In fact the problem is in QEMU.
> 
> On a big-endian guest, the readw() returns a byte-swapped value, This means QEMU doesn't provide a
> little-endian value.
> 
> I found in QEMU virtio_mmio_read() provides a value with byte-swapped bytes.
> 
> The problem comes from virtio_config_readX() functions that read the value using ldX_p accessors.
> 
> Is it normal not to use the modern variant here if we are not in legacy mode?
> 
> I think we should have something like this in virtio_mmio_read (and write):
> 
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -112,15 +112,28 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
> 
>      if (offset >= VIRTIO_MMIO_CONFIG) {
>          offset -= VIRTIO_MMIO_CONFIG;
> -        switch (size) {
> -        case 1:
> -            return virtio_config_readb(vdev, offset);
> -        case 2:
> -            return virtio_config_readw(vdev, offset);
> -        case 4:
> -            return virtio_config_readl(vdev, offset);
> -        default:
> -            abort();
> +        if (proxy->legacy) {
> +            switch (size) {
> +            case 1:
> +                return virtio_config_readb(vdev, offset);
> +            case 2:
> +                return virtio_config_readw(vdev, offset);
> +            case 4:
> +                return virtio_config_readl(vdev, offset);
> +            default:
> +                abort();
> +            }
> +        } else {
> +            switch (size) {
> +            case 1:
> +                return virtio_config_modern_readb(vdev, offset);
> +            case 2:
> +                return virtio_config_modern_readw(vdev, offset);
> +            case 4:
> +                return virtio_config_modern_readl(vdev, offset);
> +            default:
> +                abort();
> +            }
>          }
>      }
>      if (size != 4) {

Sounds reasonable ...


> And we need the same thing in virtio_pci_config_read() (and write).

We already have it, see below.

> And this could explain why it works with virtio-pci and not with virtio-mmio with the big-endian guest:
> 
> with virtio-pci the bytes are swapped twice (once in virtio-mmio and then in virtio-pci), so they
> are restored to the initial value, whereas with direct virtio-mmio they are swapped only once.
> 
> Thanks,
> Laurent

virtio pci does this: modern accesses use:

	virtio_pci_device_read

static uint64_t virtio_pci_device_read(void *opaque, hwaddr addr,
                                       unsigned size)
{
    VirtIOPCIProxy *proxy = opaque;
    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
    uint64_t val = 0;

    if (vdev == NULL) {
        return val;
    }

    switch (size) {
    case 1:
        val = virtio_config_modern_readb(vdev, addr);
        break;
    case 2:
        val = virtio_config_modern_readw(vdev, addr);
        break;
    case 4:
        val = virtio_config_modern_readl(vdev, addr);
        break;
    }
    return val;
}


while legacy accesses use:

	virtio_pci_config_read

static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
                                       unsigned size)
{
    VirtIOPCIProxy *proxy = opaque;
    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
    uint32_t config = VIRTIO_PCI_CONFIG_SIZE(&proxy->pci_dev);
    uint64_t val = 0;
    if (addr < config) {
        return virtio_ioport_read(proxy, addr);
    }
    addr -= config;

    switch (size) {
    case 1:
        val = virtio_config_readb(vdev, addr);
        break;
    case 2:
        val = virtio_config_readw(vdev, addr);
        if (virtio_is_big_endian(vdev)) {
            val = bswap16(val);
        }
        break;
    case 4:
        val = virtio_config_readl(vdev, addr);
        if (virtio_is_big_endian(vdev)) {
            val = bswap32(val);
        }
        break;
    }
    return val;
}


the naming is configing but there you are.

virtio_pci_config_read is also calling virtio_is_big_endian.

        
static inline bool virtio_is_big_endian(VirtIODevice *vdev)
{       
    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
        assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
    }   
    /* Devices conforming to VIRTIO 1.0 or later are always LE. */
    return false;
}       
            

I note that
virtio_is_big_endian is kind of wrong for modern config accesses since it
checks guest feature bits - config accesses are done before features are
acknowledged.
Luckily this function is never called for a modern guest ...
It would be nice to add virtio_legacy_is_big_endian and call that
when we know it's a legacy interface.


-- 
MST