Re: [PATCH v2 8/9] hw/dma: Use dma_addr_t type definition when relevant
On 1/10/22 09:49, David Hildenbrand wrote:
> On 04.01.22 09:54, Philippe Mathieu-Daudé wrote:
>> From: Philippe Mathieu-Daudé
>>
>> Update the obvious places where dma_addr_t should be used
>> (instead of uint64_t, hwaddr, size_t, int32_t types).
>>
>> This allows to have _addr_t type portable on 32/64-bit
>> hosts.
>>
>> Move QEMUSGList declaration after dma_addr_t declaration
>> so this structure can use the new type.
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> include/sysemu/dma.h | 22 +++---
>> hw/nvme/ctrl.c| 2 +-
>> hw/rdma/rdma_utils.c | 2 +-
>> hw/scsi/megasas.c | 10 +-
>> softmmu/dma-helpers.c | 6 +++---
>> 5 files changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
>> index 0db2478a506..7a8ae4fcd0b 100644
>> --- a/include/sysemu/dma.h
>> +++ b/include/sysemu/dma.h
>> @@ -15,22 +15,11 @@
>> #include "block/block.h"
>> #include "block/accounting.h"
>>
>> -typedef struct ScatterGatherEntry ScatterGatherEntry;
>> -
>> typedef enum {
>> DMA_DIRECTION_TO_DEVICE = 0,
>> DMA_DIRECTION_FROM_DEVICE = 1,
>> } DMADirection;
>>
>> -struct QEMUSGList {
>> -ScatterGatherEntry *sg;
>> -int nsg;
>> -int nalloc;
>> -size_t size;
>> -DeviceState *dev;
>> -AddressSpace *as;
>> -};
>> -
>> /*
>> * When an IOMMU is present, bus addresses become distinct from
>> * CPU/memory physical addresses and may be a different size. Because
>> @@ -43,6 +32,17 @@ typedef uint64_t dma_addr_t;
>> #define DMA_ADDR_BITS 64
>> #define DMA_ADDR_FMT "%" PRIx64
>>
>> +typedef struct ScatterGatherEntry ScatterGatherEntry;
>> +
>> +struct QEMUSGList {
>> +ScatterGatherEntry *sg;
>> +int nsg;
>> +int nalloc;
>> +dma_addr_t size;
>> +DeviceState *dev;
>> +AddressSpace *as;
>> +};
>
> Changing one member while moving is sneaky. Why the move in this patch?
Because dma_addr_t is declared before QEMUSGList. I will add an
intermediate patch.
> Apart from that and Peters comment
>
> Reviewed-by: David Hildenbrand
Thanks (Peter's comment addressed).
Re: [PATCH v2 8/9] hw/dma: Use dma_addr_t type definition when relevant
On 04.01.22 09:54, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé
>
> Update the obvious places where dma_addr_t should be used
> (instead of uint64_t, hwaddr, size_t, int32_t types).
>
> This allows to have _addr_t type portable on 32/64-bit
> hosts.
>
> Move QEMUSGList declaration after dma_addr_t declaration
> so this structure can use the new type.
>
> Signed-off-by: Philippe Mathieu-Daudé
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> include/sysemu/dma.h | 22 +++---
> hw/nvme/ctrl.c| 2 +-
> hw/rdma/rdma_utils.c | 2 +-
> hw/scsi/megasas.c | 10 +-
> softmmu/dma-helpers.c | 6 +++---
> 5 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 0db2478a506..7a8ae4fcd0b 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -15,22 +15,11 @@
> #include "block/block.h"
> #include "block/accounting.h"
>
> -typedef struct ScatterGatherEntry ScatterGatherEntry;
> -
> typedef enum {
> DMA_DIRECTION_TO_DEVICE = 0,
> DMA_DIRECTION_FROM_DEVICE = 1,
> } DMADirection;
>
> -struct QEMUSGList {
> -ScatterGatherEntry *sg;
> -int nsg;
> -int nalloc;
> -size_t size;
> -DeviceState *dev;
> -AddressSpace *as;
> -};
> -
> /*
> * When an IOMMU is present, bus addresses become distinct from
> * CPU/memory physical addresses and may be a different size. Because
> @@ -43,6 +32,17 @@ typedef uint64_t dma_addr_t;
> #define DMA_ADDR_BITS 64
> #define DMA_ADDR_FMT "%" PRIx64
>
> +typedef struct ScatterGatherEntry ScatterGatherEntry;
> +
> +struct QEMUSGList {
> +ScatterGatherEntry *sg;
> +int nsg;
> +int nalloc;
> +dma_addr_t size;
> +DeviceState *dev;
> +AddressSpace *as;
> +};
Changing one member while moving is sneaky. Why the move in this patch?
Apart from that and Peters comment
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH v2 8/9] hw/dma: Use dma_addr_t type definition when relevant
On Tue, Jan 04, 2022 at 09:54:30AM +0100, Philippe Mathieu-Daudé wrote:
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 462f79a1f60..c3c49176110 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -1147,7 +1147,7 @@ static uint16_t nvme_tx(NvmeCtrl *n, NvmeSg *sg,
> uint8_t *ptr, uint32_t len,
>
> if (sg->flags & NVME_SG_DMA) {
> const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> -uint64_t residual;
> +dma_addr_t residual;
>
> if (dir == NVME_TX_DIRECTION_TO_DEVICE) {
> residual = dma_buf_write(ptr, len, >qsg, attrs);
If there's a new version: Maybe also change the return value types of
dma_buf_write|read() to dma_addr_t?
It'll be changed anyway in the next patch, so not a big deal.
The rest patches looks good to me. Thanks.
--
Peter Xu
[PATCH v2 8/9] hw/dma: Use dma_addr_t type definition when relevant
From: Philippe Mathieu-Daudé
Update the obvious places where dma_addr_t should be used
(instead of uint64_t, hwaddr, size_t, int32_t types).
This allows to have _addr_t type portable on 32/64-bit
hosts.
Move QEMUSGList declaration after dma_addr_t declaration
so this structure can use the new type.
Signed-off-by: Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé
---
include/sysemu/dma.h | 22 +++---
hw/nvme/ctrl.c| 2 +-
hw/rdma/rdma_utils.c | 2 +-
hw/scsi/megasas.c | 10 +-
softmmu/dma-helpers.c | 6 +++---
5 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
index 0db2478a506..7a8ae4fcd0b 100644
--- a/include/sysemu/dma.h
+++ b/include/sysemu/dma.h
@@ -15,22 +15,11 @@
#include "block/block.h"
#include "block/accounting.h"
-typedef struct ScatterGatherEntry ScatterGatherEntry;
-
typedef enum {
DMA_DIRECTION_TO_DEVICE = 0,
DMA_DIRECTION_FROM_DEVICE = 1,
} DMADirection;
-struct QEMUSGList {
-ScatterGatherEntry *sg;
-int nsg;
-int nalloc;
-size_t size;
-DeviceState *dev;
-AddressSpace *as;
-};
-
/*
* When an IOMMU is present, bus addresses become distinct from
* CPU/memory physical addresses and may be a different size. Because
@@ -43,6 +32,17 @@ typedef uint64_t dma_addr_t;
#define DMA_ADDR_BITS 64
#define DMA_ADDR_FMT "%" PRIx64
+typedef struct ScatterGatherEntry ScatterGatherEntry;
+
+struct QEMUSGList {
+ScatterGatherEntry *sg;
+int nsg;
+int nalloc;
+dma_addr_t size;
+DeviceState *dev;
+AddressSpace *as;
+};
+
static inline void dma_barrier(AddressSpace *as, DMADirection dir)
{
/*
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 462f79a1f60..c3c49176110 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1147,7 +1147,7 @@ static uint16_t nvme_tx(NvmeCtrl *n, NvmeSg *sg, uint8_t
*ptr, uint32_t len,
if (sg->flags & NVME_SG_DMA) {
const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
-uint64_t residual;
+dma_addr_t residual;
if (dir == NVME_TX_DIRECTION_TO_DEVICE) {
residual = dma_buf_write(ptr, len, >qsg, attrs);
diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c
index 61cb8ede0fd..5a7ef63ad28 100644
--- a/hw/rdma/rdma_utils.c
+++ b/hw/rdma/rdma_utils.c
@@ -20,7 +20,7 @@
void *rdma_pci_dma_map(PCIDevice *dev, dma_addr_t addr, dma_addr_t len)
{
void *p;
-hwaddr pci_len = len;
+dma_addr_t pci_len = len;
if (!addr) {
rdma_error_report("addr is NULL");
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index cb019549371..6c1ae6b980f 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1046,7 +1046,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev,
int lun,
uint16_t pd_id = ((sdev->id & 0xFF) << 8) | (lun & 0xFF);
uint8_t cmdbuf[6];
size_t len;
-size_t residual;
+dma_addr_t residual;
if (!cmd->iov_buf) {
cmd->iov_buf = g_malloc0(dcmd_size);
@@ -1152,7 +1152,7 @@ static int megasas_dcmd_ld_get_list(MegasasState *s,
MegasasCmd *cmd)
{
struct mfi_ld_list info;
size_t dcmd_size = sizeof(info);
-size_t residual;
+dma_addr_t residual;
uint32_t num_ld_disks = 0, max_ld_disks;
uint64_t ld_size;
BusChild *kid;
@@ -1198,7 +1198,7 @@ static int megasas_dcmd_ld_list_query(MegasasState *s,
MegasasCmd *cmd)
uint16_t flags;
struct mfi_ld_targetid_list info;
size_t dcmd_size = sizeof(info);
-size_t residual;
+dma_addr_t residual;
uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
BusChild *kid;
@@ -1251,7 +1251,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev,
int lun,
size_t dcmd_size = sizeof(struct mfi_ld_info);
uint8_t cdb[6];
ssize_t len;
-size_t residual;
+dma_addr_t residual;
uint16_t sdev_id = ((sdev->id & 0xFF) << 8) | (lun & 0xFF);
uint64_t ld_size;
@@ -1625,7 +1625,7 @@ static int megasas_handle_dcmd(MegasasState *s,
MegasasCmd *cmd)
}
static int megasas_finish_internal_dcmd(MegasasCmd *cmd,
-SCSIRequest *req, size_t residual)
+SCSIRequest *req, dma_addr_t residual)
{
int retval = MFI_STAT_OK;
int lun = req->lun;
diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
index 4563a775aa7..54340929334 100644
--- a/softmmu/dma-helpers.c
+++ b/softmmu/dma-helpers.c
@@ -294,12 +294,12 @@ BlockAIOCB *dma_blk_write(BlockBackend *blk,
}
-static MemTxResult dma_buf_rw(void *buf, int32_t len, uint64_t *residual,
+static MemTxResult dma_buf_rw(void *buf, dma_addr_t len, dma_addr_t *residual,
QEMUSGList *sg, DMADirection dir,
MemTxAttrs attrs)
{
uint8_t *ptr = buf;
-uint64_t xresidual;
+dma_addr_t xresidual;
int sg_cur_index;
MemTxResult res = MEMTX_OK;
@@ -308,7 +308,7 @@
