diff mbox

hw/iommu: Fix problems reported by Coverity scan

Message ID 1475337462-31629-2-git-send-email-davidkiarie4@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Kiarie Oct. 1, 2016, 3:57 p.m. UTC 
Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
---
 hw/i386/amd_iommu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Stefan Weil Oct. 1, 2016, 4:29 p.m. UTC | #1 
Hi,

On 10/01/16 17:57, David Kiarie wrote:
> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
> ---
>  hw/i386/amd_iommu.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 023de52..815d45f 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -144,7 +144,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>  static void amdvi_generate_msi_interrupt(AMDVIState *s)
>  {
>      MSIMessage msg;
> -    MemTxAttrs attrs;
> +    MemTxAttrs attrs = {0, 0};
>
>      attrs.requester_id = pci_requester_id(&s->pci.dev);
>
> @@ -185,7 +185,7 @@ static void amdvi_setevent_bits(uint64_t *buffer, uint64_t value, int start,
>                                  int length)
>  {
>      int index = start / 64, bitpos = start % 64;
> -    uint64_t mask = ((1 << length) - 1) << bitpos;
> +    uint64_t mask = ((1UL << length) - 1) << bitpos;
>      buffer[index] &= ~mask;
>      buffer[index] |= (value << bitpos) & mask;
>  }
> @@ -334,7 +334,7 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
>                                 uint16_t domid)
>  {
>      AMDVIIOTLBEntry *entry = g_malloc(sizeof(*entry));
> -    uint64_t *key = g_malloc(sizeof(key));
> +    uint64_t *key = g_malloc(sizeof(*key));

I suggest using g_new(uint64_t, 1) here.

>      uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
>
>      /* don't cache erroneous translations */
> @@ -1135,6 +1135,7 @@ static void amdvi_reset(DeviceState *dev)
>
>  static void amdvi_realize(DeviceState *dev, Error **err)
>  {
> +    int ret = 0;
>      AMDVIState *s = AMD_IOMMU_DEVICE(dev);
>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>      PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
> @@ -1147,8 +1148,11 @@ static void amdvi_realize(DeviceState *dev, Error **err)
>      object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
>      s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
>                                           AMDVI_CAPAB_SIZE);
> -    pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
> +    assert(s->capab_offset > 0);
> +    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
> +    assert(ret > 0);
>      pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);

Did you forget to assign the result to ret here? Without that, the 
following assertion does not make sense, and coverity will still complain.

> +    assert(ret > 0);
>
>      /* set up MMIO */
>      memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",
>

Stefan
David Kiarie Oct. 1, 2016, 4:53 p.m. UTC | #2 
On Sat, Oct 1, 2016 at 7:29 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Hi,
>
>
> On 10/01/16 17:57, David Kiarie wrote:
>>
>> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
>> ---
>>  hw/i386/amd_iommu.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 023de52..815d45f 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -144,7 +144,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr
>> addr, uint64_t val)
>>  static void amdvi_generate_msi_interrupt(AMDVIState *s)
>>  {
>>      MSIMessage msg;
>> -    MemTxAttrs attrs;
>> +    MemTxAttrs attrs = {0, 0};
>>
>>      attrs.requester_id = pci_requester_id(&s->pci.dev);
>>
>> @@ -185,7 +185,7 @@ static void amdvi_setevent_bits(uint64_t *buffer,
>> uint64_t value, int start,
>>                                  int length)
>>  {
>>      int index = start / 64, bitpos = start % 64;
>> -    uint64_t mask = ((1 << length) - 1) << bitpos;
>> +    uint64_t mask = ((1UL << length) - 1) << bitpos;
>>      buffer[index] &= ~mask;
>>      buffer[index] |= (value << bitpos) & mask;
>>  }
>> @@ -334,7 +334,7 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t
>> devid,
>>                                 uint16_t domid)
>>  {
>>      AMDVIIOTLBEntry *entry = g_malloc(sizeof(*entry));
>> -    uint64_t *key = g_malloc(sizeof(key));
>> +    uint64_t *key = g_malloc(sizeof(*key));
>
>
> I suggest using g_new(uint64_t, 1) here.
>
>>      uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
>>
>>      /* don't cache erroneous translations */
>> @@ -1135,6 +1135,7 @@ static void amdvi_reset(DeviceState *dev)
>>
>>  static void amdvi_realize(DeviceState *dev, Error **err)
>>  {
>> +    int ret = 0;
>>      AMDVIState *s = AMD_IOMMU_DEVICE(dev);
>>      X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>>      PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
>> @@ -1147,8 +1148,11 @@ static void amdvi_realize(DeviceState *dev, Error
>> **err)
>>      object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
>>      s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC,
>> 0,
>>                                           AMDVI_CAPAB_SIZE);
>> -    pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
>> AMDVI_CAPAB_REG_SIZE);
>> +    assert(s->capab_offset > 0);
>> +    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
>> AMDVI_CAPAB_REG_SIZE);
>> +    assert(ret > 0);
>>      pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
>> AMDVI_CAPAB_REG_SIZE);
>
>
> Did you forget to assign the result to ret here? Without that, the following
> assertion does not make sense, and coverity will still complain.

Yeah, missed something here.

>
>
>> +    assert(ret > 0);
>>
>>      /* set up MMIO */
>>      memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s,
>> "amdvi-mmio",
>>
>
> Stefan
>
Markus Armbruster Oct. 4, 2016, 8:06 a.m. UTC | #3 
Stefan Weil <sw@weilnetz.de> writes:

> Hi,
>
> On 10/01/16 17:57, David Kiarie wrote:
>> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
>> ---
>>  hw/i386/amd_iommu.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index 023de52..815d45f 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -144,7 +144,7 @@ static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
>>  static void amdvi_generate_msi_interrupt(AMDVIState *s)
>>  {
>>      MSIMessage msg;
>> -    MemTxAttrs attrs;
>> +    MemTxAttrs attrs = {0, 0};
>>
>>      attrs.requester_id = pci_requester_id(&s->pci.dev);
>>
>> @@ -185,7 +185,7 @@ static void amdvi_setevent_bits(uint64_t *buffer, uint64_t value, int start,
>>                                  int length)
>>  {
>>      int index = start / 64, bitpos = start % 64;
>> -    uint64_t mask = ((1 << length) - 1) << bitpos;
>> +    uint64_t mask = ((1UL << length) - 1) << bitpos;
>>      buffer[index] &= ~mask;
>>      buffer[index] |= (value << bitpos) & mask;
>>  }
>> @@ -334,7 +334,7 @@ static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
>>                                 uint16_t domid)
>>  {
>>      AMDVIIOTLBEntry *entry = g_malloc(sizeof(*entry));
>> -    uint64_t *key = g_malloc(sizeof(key));
>> +    uint64_t *key = g_malloc(sizeof(*key));
>
> I suggest using g_new(uint64_t, 1) here.

Either way is fine.

g_new(T, 1) is clearly superior to g_malloc(sizeof(T)), because it's
terser, and yields a more useful type.

v = g_new(T, 1) vs. v = g_malloc(sizeof(*v)) is less clear.  The g_new()
is more tightly typed, but the typing doesn't buy much here.  The
g_malloc() is idiomatic usage.  Matter of taste.

[...]
diff mbox

Patch

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 023de52..815d45f 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -144,7 +144,7 @@  static void amdvi_assign_andq(AMDVIState *s, hwaddr addr, uint64_t val)
 static void amdvi_generate_msi_interrupt(AMDVIState *s)
 {
     MSIMessage msg;
-    MemTxAttrs attrs;
+    MemTxAttrs attrs = {0, 0};
 
     attrs.requester_id = pci_requester_id(&s->pci.dev);
 
@@ -185,7 +185,7 @@  static void amdvi_setevent_bits(uint64_t *buffer, uint64_t value, int start,
                                 int length)
 {
     int index = start / 64, bitpos = start % 64;
-    uint64_t mask = ((1 << length) - 1) << bitpos;
+    uint64_t mask = ((1UL << length) - 1) << bitpos;
     buffer[index] &= ~mask;
     buffer[index] |= (value << bitpos) & mask;
 }
@@ -334,7 +334,7 @@  static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
                                uint16_t domid)
 {
     AMDVIIOTLBEntry *entry = g_malloc(sizeof(*entry));
-    uint64_t *key = g_malloc(sizeof(key));
+    uint64_t *key = g_malloc(sizeof(*key));
     uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
 
     /* don't cache erroneous translations */
@@ -1135,6 +1135,7 @@  static void amdvi_reset(DeviceState *dev)
 
 static void amdvi_realize(DeviceState *dev, Error **err)
 {
+    int ret = 0;
     AMDVIState *s = AMD_IOMMU_DEVICE(dev);
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
     PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
@@ -1147,8 +1148,11 @@  static void amdvi_realize(DeviceState *dev, Error **err)
     object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
     s->capab_offset = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
                                          AMDVI_CAPAB_SIZE);
-    pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
+    assert(s->capab_offset > 0);
+    ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
+    assert(ret > 0);
     pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
+    assert(ret > 0);
 
     /* set up MMIO */
     memory_region_init_io(&s->mmio, OBJECT(s), &mmio_mem_ops, s, "amdvi-mmio",