[Qemu-devel] (no subject)

[Qemu-devel] (no subject)

[David Gibson]

These are patches that I've sent in before but don't seem to have been
picked up by anybody yet.  Resending so they don't fall off the radar.
1, 2, and 3 of 4 in particular fix real, observed bugs on powerpc.

[Qemu-devel] [PATCH 1/4] kvm: Comparison with ioctl number macros needs to be unsigned

[David Gibson]

In kvm-all.c we store an ioctl cmd number in the irqchip_inject_ioctl field
of KVMState, which has type 'int'.  This seems to make sense since the
ioctl() man page says that the cmd parameter has type int.

However, the kernel treats ioctl numbers as unsigned - sys_ioctl() takes an
unsigned int, and the macros which generate ioctl numbers expand to
unsigned expressions.  Furthermore, some ioctls (IOC_READ ioctls on x86
and IOC_WRITE ioctls on powerpc) have bit 31 set, and so would be negative
if interpreted as an int. This has the surprising and compile-breaking
consequence that in kvm_irqchip_set_irq() where we do:
    return (s->irqchip_inject_ioctl == KVM_IRQ_LINE) ? 1 : event.status;
We will get a "comparison is always false due to limited range of data
type" warning from gcc if KVM_IRQ_LINE is one of the bit-31-set ioctls,
which it is on powerpc.

So, despite the fact that the man page and posix say ioctl numbers are
signed, they're actually unsigned.  The kernel uses unsigned, the glibc
header uses unsigned long, and FreeBSD, NetBSD and OSX also use unsigned
long ioctl numbers in the code.

Therefore, this patch changes the variable to be unsigned, fixing the
compile.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tossatti <mtossatti@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 kvm-all.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 77eadf6..a5727a3 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -78,7 +78,10 @@ struct KVMState
     int pit_in_kernel;
     int xsave, xcrs;
     int many_ioeventfds;
-    int irqchip_inject_ioctl;
+    /* The man page (and posix) say ioctl numbers are signed int, but
+     * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
+     * unsigned, and treating them as signed here can break things */
+    unsigned irqchip_inject_ioctl;
 #ifdef KVM_CAP_IRQ_ROUTING
     struct kvm_irq_routing *irq_routes;
     int nr_allocated_irq_routes;
-- 
1.7.9.1

[Qemu-devel] [PATCH 2/4] Endian fix an assertion in usb-msd

[David Gibson]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This fixes a broken endian assumption in an assertion in usb-msd.

Cc: Gerd Hoffman <kraxel@redhat.com>

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/usb-msd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index c6f08a0..c18e76d 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -195,7 +195,7 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
     DPRINTF("Command status %d tag 0x%x, len %zd\n",
             s->csw.status, s->csw.tag, p->iov.size);
 
-    assert(s->csw.sig == 0x53425355);
+    assert(s->csw.sig == cpu_to_le32(0x53425355));
     len = MIN(sizeof(s->csw), p->iov.size);
     usb_packet_copy(p, &s->csw, len);
     memset(&s->csw, 0, sizeof(s->csw));
-- 
1.7.9.1

[Qemu-devel] [PATCH 3/4] kvm: Fix dirty tracking with large kernel page size

[David Gibson]

If the kernel page size is larger than TARGET_PAGE_SIZE, which
happens for example on ppc64 with kernels compiled for 64K pages,
the dirty tracking doesn't work.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tossatti <mtossatti@redhat.com>

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 kvm-all.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index a5727a3..eb0e3b0 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -349,10 +349,11 @@ static int kvm_set_migration_log(int enable)
 static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
                                          unsigned long *bitmap)
 {
-    unsigned int i, j;
+  unsigned int i, j;
     unsigned long page_number, c;
     target_phys_addr_t addr, addr1;
     unsigned int len = ((section->size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
+    unsigned long hpratio = getpagesize() / TARGET_PAGE_SIZE;
 
     /*
      * bitmap-traveling is faster than memory-traveling (for addr...)
@@ -364,10 +365,11 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
             do {
                 j = ffsl(c) - 1;
                 c &= ~(1ul << j);
-                page_number = i * HOST_LONG_BITS + j;
+                page_number = (i * HOST_LONG_BITS + j) * hpratio;
                 addr1 = page_number * TARGET_PAGE_SIZE;
                 addr = section->offset_within_region + addr1;
-                memory_region_set_dirty(section->mr, addr, TARGET_PAGE_SIZE);
+                memory_region_set_dirty(section->mr, addr,
+                                        TARGET_PAGE_SIZE * hpratio);
             } while (c != 0);
         }
     }
-- 
1.7.9.1

[Qemu-devel] [PATCH 4/4] pci: Factor out bounds checking on config space accesses

[David Gibson]

There are several paths into the code to emulate PCI config space accesses:
one for MMIO to a plain old PCI bridge one for MMIO to a PCIe bridge and
one for the pseries machine which provides para-virtualized access to PCI
config space.  Each of these functions does their own bounds checking
against the size of config space to check for addresses outside the
size of config space.  The pci_host_config_{read,write}_common() (sort
of) checks for partial overruns, that is where the address is within
the size of config space, but address + length is not, it takes a
limit parameter for this purpose.

As well as being a small code duplication, and it being weird to
separate the checks for partial and total overruns, this checking
currently has a few buglets:

    * For non PCI-Express we assume that the size of config space is
      PCI_CONFIG_SPACE_SIZE.  That's true for everything we emulate
      now, but is not necessarily true (e.g. PCI-X devices can have
      extended config space)

    * The limit parameter is not necessary, since the size of config
      space can be obtained using pci_config_size()

    * Partial overruns could only occur with a misaligned access,
      which should have already been dealt with by this point

    * Partial overruns are handled as a partial read or write, which
      is very unlikely behaviour for real hardware

    * Furthermore, partial reads are 0x0 padded, whereas returning
      0xff for unimplemented addresses us much more common.

    * The partial reads/writes only work correctly by assuming
      little-endian byte layout.  While that is always true for PCI
      config space, it's an awfully subtle thing to rely on without
      comment.

This patch, therefore, moves the bounds checking wholly into
pci_host_config_{read,write}_common().  No partial reads or writes are
performed, instead any out-of-bounds write is simply ignored and an
out-of-bounds read returns 0xff.

This simplifies all the callers, and makes the overall semantics saner
for edge cases.

Cc: Michael S. Tsirkin <mst@redhat.com>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci_host.c  |   26 ++++++++++++++------------
 hw/pci_host.h  |    4 ++--
 hw/pcie_host.c |   18 ++----------------
 hw/spapr_pci.c |   27 ++++-----------------------
 4 files changed, 22 insertions(+), 53 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 44c6c20..829d797 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -48,48 +48,50 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
 }
 
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len)
+                                  uint32_t val, uint32_t len)
 {
     assert(len <= 4);
-    pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
+    if ((addr + len) <= pci_config_size(pci_dev)) {
+        pci_dev->config_write(pci_dev, addr, val, len);
+    }
 }
 
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len)
+                                     uint32_t len)
 {
     assert(len <= 4);
-    return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
+    if ((addr + len) <= pci_config_size(pci_dev)) {
+        return pci_dev->config_read(pci_dev, addr, len);
+    } else {
+        return ~0x0;
+    }
 }
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 {
     PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
-    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
 
     if (!pci_dev) {
         return;
     }
 
     PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n",
-                __func__, pci_dev->name, config_addr, val, len);
-    pci_host_config_write_common(pci_dev, config_addr, PCI_CONFIG_SPACE_SIZE,
-                                 val, len);
+                __func__, pci_dev->name, addr, val, len);
+    pci_host_config_write_common(pci_dev, addr, val, len);
 }
 
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
 {
     PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
-    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
     uint32_t val;
 
     if (!pci_dev) {
         return ~0x0;
     }
 
-    val = pci_host_config_read_common(pci_dev, config_addr,
-                                      PCI_CONFIG_SPACE_SIZE, len);
+    val = pci_host_config_read_common(pci_dev, addr, len);
     PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
-                __func__, pci_dev->name, config_addr, val, len);
+                __func__, pci_dev->name, addr, val, len);
 
     return val;
 }
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 359e38f..4bb0838 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -42,9 +42,9 @@ struct PCIHostState {
 
 /* common internal helpers for PCI/PCIe hosts, cut off overflows */
 void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
-                                  uint32_t limit, uint32_t val, uint32_t len);
+                                  uint32_t val, uint32_t len);
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t len);
+                                     uint32_t len);
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
diff --git a/hw/pcie_host.c b/hw/pcie_host.c
index 28bbe72..3af8610 100644
--- a/hw/pcie_host.c
+++ b/hw/pcie_host.c
@@ -60,19 +60,12 @@ static void pcie_mmcfg_data_write(void *opaque, target_phys_addr_t mmcfg_addr,
     PCIBus *s = e->pci.bus;
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t addr;
-    uint32_t limit;
 
     if (!pci_dev) {
         return;
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-    limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return;
-    }
-    pci_host_config_write_common(pci_dev, addr, limit, val, len);
+    pci_host_config_write_common(pci_dev, addr, val, len);
 }
 
 static uint64_t pcie_mmcfg_data_read(void *opaque,
@@ -83,19 +76,12 @@ static uint64_t pcie_mmcfg_data_read(void *opaque,
     PCIBus *s = e->pci.bus;
     PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
     uint32_t addr;
-    uint32_t limit;
 
     if (!pci_dev) {
         return ~0x0;
     }
     addr = PCIE_MMCFG_CONFOFFSET(mmcfg_addr);
-    limit = pci_config_size(pci_dev);
-    if (limit <= addr) {
-        /* conventional pci device can be behind pcie-to-pci bridge.
-           256 <= addr < 4K has no effects. */
-        return ~0x0;
-    }
-    return pci_host_config_read_common(pci_dev, addr, limit, len);
+    return pci_host_config_read_common(pci_dev, addr, len);
 }
 
 static const MemoryRegionOps pcie_mmcfg_ops = {
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index cfdd9dd..1e8d03e 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -67,25 +67,6 @@ static uint32_t rtas_pci_cfgaddr(uint32_t arg)
     return ((arg >> 20) & 0xf00) | (arg & 0xff);
 }
 
-static uint32_t rtas_read_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
-                                        uint32_t limit, uint32_t len)
-{
-    if ((addr + len) <= limit) {
-        return pci_host_config_read_common(pci_dev, addr, limit, len);
-    } else {
-        return ~0x0;
-    }
-}
-
-static void rtas_write_pci_config_do(PCIDevice *pci_dev, uint32_t addr,
-                                     uint32_t limit, uint32_t val,
-                                     uint32_t len)
-{
-    if ((addr + len) <= limit) {
-        pci_host_config_write_common(pci_dev, addr, limit, val, len);
-    }
-}
-
 static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
                                      uint32_t token, uint32_t nargs,
                                      target_ulong args,
@@ -101,7 +82,7 @@ static void rtas_ibm_read_pci_config(sPAPREnvironment *spapr,
     }
     size = rtas_ld(args, 3);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
+    val = pci_host_config_read_common(dev, addr, size);
     rtas_st(rets, 0, 0);
     rtas_st(rets, 1, val);
 }
@@ -120,7 +101,7 @@ static void rtas_read_pci_config(sPAPREnvironment *spapr,
     }
     size = rtas_ld(args, 1);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    val = rtas_read_pci_config_do(dev, addr, pci_config_size(dev), size);
+    val = pci_host_config_read_common(dev, addr, size);
     rtas_st(rets, 0, 0);
     rtas_st(rets, 1, val);
 }
@@ -141,7 +122,7 @@ static void rtas_ibm_write_pci_config(sPAPREnvironment *spapr,
     val = rtas_ld(args, 4);
     size = rtas_ld(args, 3);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
+    pci_host_config_write_common(dev, addr, val, size);
     rtas_st(rets, 0, 0);
 }
 
@@ -160,7 +141,7 @@ static void rtas_write_pci_config(sPAPREnvironment *spapr,
     val = rtas_ld(args, 2);
     size = rtas_ld(args, 1);
     addr = rtas_pci_cfgaddr(rtas_ld(args, 0));
-    rtas_write_pci_config_do(dev, addr, pci_config_size(dev), val, size);
+    pci_host_config_write_common(dev, addr, val, size);
     rtas_st(rets, 0, 0);
 }
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 2/4] Endian fix an assertion in usb-msd

[Gerd Hoffmann]

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

On 03/08/12 01:41, David Gibson wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> -    assert(s->csw.sig == 0x53425355);
> +    assert(s->csw.sig == cpu_to_le32(0x53425355));

Correct but incomplete.  residue is sent in host endian instead of
little endian too.  It is zero most of the time though, so it easily
goes unnoticed.

Can you try the attached patch instead?

cheers,
  Gerd


[-- Attachment #2: 0001-usb-storage-fix-byteorder-issues.patch --]
[-- Type: text/plain, Size: 5473 bytes --]

>From a6ac421d0aa1bcfd42601a655e4995c67f6c903c Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 9 Mar 2012 13:19:28 +0100
Subject: [PATCH] usb-storage: fix byteorder issues.

Store the command word in MSDStatus.  Add the function
usb_msd_recv_command to copy the command, simliar to
usb_msd_send_status which copyes the status.

Do all byteswapping in usb_msd_recv_command and usb_msd_send_status
functions where the copying from/to the usb packets happens, so all
other places can just use native endian when accessing command and
status packet data.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-storage.c |   76 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 6ffaf70..7da6205 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -45,6 +45,16 @@ struct usb_msd_csw {
     uint8_t status;
 };
 
+struct usb_msd_cbw {
+    uint32_t sig;
+    uint32_t tag;
+    uint32_t data_len;
+    uint8_t flags;
+    uint8_t lun;
+    uint8_t cmd_len;
+    uint8_t cmd[16];
+};
+
 typedef struct {
     USBDevice dev;
     enum USBMSDMode mode;
@@ -52,6 +62,7 @@ typedef struct {
     uint8_t *scsi_buf;
     uint32_t data_len;
     uint32_t residue;
+    struct usb_msd_cbw cbw;
     struct usb_msd_csw csw;
     SCSIRequest *req;
     SCSIBus bus;
@@ -63,16 +74,6 @@ typedef struct {
     USBPacket *packet;
 } MSDState;
 
-struct usb_msd_cbw {
-    uint32_t sig;
-    uint32_t tag;
-    uint32_t data_len;
-    uint8_t flags;
-    uint8_t lun;
-    uint8_t cmd_len;
-    uint8_t cmd[16];
-};
-
 enum {
     STR_MANUFACTURER = 1,
     STR_PRODUCT,
@@ -188,6 +189,26 @@ static void usb_msd_copy_data(MSDState *s, USBPacket *p)
     }
 }
 
+static int usb_msd_recv_command(MSDState *s, USBPacket *p)
+{
+    if (p->iov.size != 31) {
+        fprintf(stderr, "usb-msd: Bad CBW size");
+        return -1;
+    }
+
+    usb_packet_copy(p, &s->cbw, 31);
+    s->cbw.sig = le32_to_cpu(s->cbw.sig);
+    s->cbw.tag = le32_to_cpu(s->cbw.tag);
+    s->cbw.data_len = le32_to_cpu(s->cbw.data_len);
+
+    if (s->cbw.sig != 0x43425355) {
+        fprintf(stderr, "usb-msd: Bad signature %08x\n",
+                s->cbw.sig);
+        return -1;
+    }
+    return 0;
+}
+
 static void usb_msd_send_status(MSDState *s, USBPacket *p)
 {
     int len;
@@ -197,7 +218,12 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
 
     assert(s->csw.sig == 0x53425355);
     len = MIN(sizeof(s->csw), p->iov.size);
+
+    s->csw.sig = cpu_to_le32(s->csw.sig);
+    s->csw.tag = cpu_to_le32(s->csw.tag);
+    s->csw.residue = cpu_to_le32(s->csw.residue);
     usb_packet_copy(p, &s->csw, len);
+
     memset(&s->csw, 0, sizeof(s->csw));
 }
 
@@ -231,8 +257,8 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status, size_t r
     DPRINTF("Command complete %d tag 0x%x\n", status, req->tag);
     s->residue = s->data_len;
 
-    s->csw.sig = cpu_to_le32(0x53425355);
-    s->csw.tag = cpu_to_le32(req->tag);
+    s->csw.sig = 0x53425355;
+    s->csw.tag = req->tag;
     s->csw.residue = s->residue;
     s->csw.status = status != 0;
 
@@ -338,9 +364,7 @@ static void usb_msd_cancel_io(USBDevice *dev, USBPacket *p)
 static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
 {
     MSDState *s = (MSDState *)dev;
-    uint32_t tag;
     int ret = 0;
-    struct usb_msd_cbw cbw;
     uint8_t devep = p->ep->nr;
 
     switch (p->pid) {
@@ -350,35 +374,27 @@ static int usb_msd_handle_data(USBDevice *dev, USBPacket *p)
 
         switch (s->mode) {
         case USB_MSDM_CBW:
-            if (p->iov.size != 31) {
-                fprintf(stderr, "usb-msd: Bad CBW size");
-                goto fail;
-            }
-            usb_packet_copy(p, &cbw, 31);
-            if (le32_to_cpu(cbw.sig) != 0x43425355) {
-                fprintf(stderr, "usb-msd: Bad signature %08x\n",
-                        le32_to_cpu(cbw.sig));
+            if (usb_msd_recv_command(s, p) != 0) {
                 goto fail;
             }
-            DPRINTF("Command on LUN %d\n", cbw.lun);
-            if (cbw.lun != 0) {
-                fprintf(stderr, "usb-msd: Bad LUN %d\n", cbw.lun);
+            DPRINTF("Command on LUN %d\n", s->cbw.lun);
+            if (s->cbw.lun != 0) {
+                fprintf(stderr, "usb-msd: Bad LUN %d\n", s->cbw.lun);
                 goto fail;
             }
-            tag = le32_to_cpu(cbw.tag);
-            s->data_len = le32_to_cpu(cbw.data_len);
+            s->data_len = s->cbw.data_len;
             if (s->data_len == 0) {
                 s->mode = USB_MSDM_CSW;
-            } else if (cbw.flags & 0x80) {
+            } else if (s->cbw.flags & 0x80) {
                 s->mode = USB_MSDM_DATAIN;
             } else {
                 s->mode = USB_MSDM_DATAOUT;
             }
             DPRINTF("Command tag 0x%x flags %08x len %d data %d\n",
-                    tag, cbw.flags, cbw.cmd_len, s->data_len);
+                    tag, s->cbw.flags, s->cbw.cmd_len, s->data_len);
             s->residue = 0;
             s->scsi_len = 0;
-            s->req = scsi_req_new(s->scsi_dev, tag, 0, cbw.cmd, NULL);
+            s->req = scsi_req_new(s->scsi_dev, s->cbw.tag, 0, s->cbw.cmd, NULL);
             scsi_req_enqueue(s->req);
             if (s->req && s->req->cmd.xfer != SCSI_XFER_NONE) {
                 scsi_req_continue(s->req);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/4] kvm: Comparison with ioctl number macros needs to be unsigned

[Alexander Graf]

On 08.03.2012, at 01:41, David Gibson wrote:

> In kvm-all.c we store an ioctl cmd number in the irqchip_inject_ioctl field
> of KVMState, which has type 'int'.  This seems to make sense since the
> ioctl() man page says that the cmd parameter has type int.
> 
> However, the kernel treats ioctl numbers as unsigned - sys_ioctl() takes an
> unsigned int, and the macros which generate ioctl numbers expand to
> unsigned expressions.  Furthermore, some ioctls (IOC_READ ioctls on x86
> and IOC_WRITE ioctls on powerpc) have bit 31 set, and so would be negative
> if interpreted as an int. This has the surprising and compile-breaking
> consequence that in kvm_irqchip_set_irq() where we do:
>    return (s->irqchip_inject_ioctl == KVM_IRQ_LINE) ? 1 : event.status;
> We will get a "comparison is always false due to limited range of data
> type" warning from gcc if KVM_IRQ_LINE is one of the bit-31-set ioctls,
> which it is on powerpc.
> 
> So, despite the fact that the man page and posix say ioctl numbers are
> signed, they're actually unsigned.  The kernel uses unsigned, the glibc
> header uses unsigned long, and FreeBSD, NetBSD and OSX also use unsigned
> long ioctl numbers in the code.
> 
> Therefore, this patch changes the variable to be unsigned, fixing the
> compile.
> 
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tossatti <mtossatti@redhat.com>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

I picked that one into ppc-next, as without compilation on ppc breaks for me. I haven't touched the other 3 patches in the set though.


Alex

PS: These 4 patches don't target a single maintainer. Thus it's better to not send them as a patch set :).

Re: [Qemu-devel] [PATCH 2/4] Endian fix an assertion in usb-msd

[Benjamin Herrenschmidt]

On Fri, 2012-03-09 at 13:46 +0100, Gerd Hoffmann wrote:
> On 03/08/12 01:41, David Gibson wrote:
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> > -    assert(s->csw.sig == 0x53425355);
> > +    assert(s->csw.sig == cpu_to_le32(0x53425355));
> 
> Correct but incomplete.  residue is sent in host endian instead of
> little endian too.  It is zero most of the time though, so it easily
> goes unnoticed.
> 
> Can you try the attached patch instead?

I dislike the patch as it does an in-place swap of the structure. I tend
to prefer a given structure to have a known endian, never change in
place. For example it would make it harder to ever implement endian
annotations like the kernel does (to have sparse find the bugs for us).

As such I would rather fix the access locations instead. This basically
means adding this small patch on top of my previous one (I haven't
spotted any other error) :

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index c18e76d..d5d7f74 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -193,7 +193,7 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
     int len;
 
     DPRINTF("Command status %d tag 0x%x, len %zd\n",
-            s->csw.status, s->csw.tag, p->iov.size);
+            s->csw.status, le32_to_cpu(s->csw.tag), p->iov.size);
 
     assert(s->csw.sig == cpu_to_le32(0x53425355));
     len = MIN(sizeof(s->csw), p->iov.size);
@@ -233,7 +233,7 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status, size_t r
 
     s->csw.sig = cpu_to_le32(0x53425355);
     s->csw.tag = cpu_to_le32(req->tag);
-    s->csw.residue = s->residue;
+    s->csw.residue = cpu_to_le32(s->residue);
     s->csw.status = status != 0;
 
     if (s->packet) {

Re: [Qemu-devel] [PATCH 2/4] Endian fix an assertion in usb-msd

[Gerd Hoffmann]

On 03/13/12 05:22, Benjamin Herrenschmidt wrote:
> As such I would rather fix the access locations instead. This basically
> means adding this small patch on top of my previous one (I haven't
> spotted any other error) :

Squashed into the original patch and added to the usb patch queue.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/4] Endian fix an assertion in usb-msd

[Benjamin Herrenschmidt]

On Tue, 2012-03-13 at 10:57 +0100, Gerd Hoffmann wrote:
> On 03/13/12 05:22, Benjamin Herrenschmidt wrote:
> > As such I would rather fix the access locations instead. This basically
> > means adding this small patch on top of my previous one (I haven't
> > spotted any other error) :
> 
> Squashed into the original patch and added to the usb patch queue.

Thanks !

Cheers,
Ben.

Re: [Qemu-devel] [PATCH 2/4] Endian fix an assertion in usb-msd

[David Gibson]

On Tue, Mar 13, 2012 at 10:57:37AM +0100, Gerd Hoffmann wrote:
> On 03/13/12 05:22, Benjamin Herrenschmidt wrote:
> > As such I would rather fix the access locations instead. This basically
> > means adding this small patch on top of my previous one (I haven't
> > spotted any other error) :
> 
> Squashed into the original patch and added to the usb patch queue.

Excellent, thanks.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson