Re: [PATCH 4/4] dma: Let ld*_dma() propagate MemTxResult

[Cédric Le Goater]

On 12/18/21 15:51, Philippe Mathieu-Daudé wrote:
> dma_memory_read() returns a MemTxResult type. Do not discard
> it, return it to the caller.

Good ! We should be using it in XIVE.

> Update the few callers.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.
    

> ---
>   include/hw/pci/pci.h       |  6 ++++--
>   include/hw/ppc/spapr_vio.h |  6 +++++-
>   include/sysemu/dma.h       | 25 ++++++++++++-------------
>   hw/intc/pnv_xive.c         |  8 ++++----
>   hw/usb/hcd-xhci.c          |  7 ++++---
>   5 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 0613308b1b6..8c5f2ed5054 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -854,8 +854,10 @@ static inline MemTxResult pci_dma_write(PCIDevice *dev, 
> dma_addr_t addr,
>       static inline uint##_bits##_t ld##_l##_pci_dma(PCIDevice *dev,      \
>                                                      dma_addr_t addr)     \
>       {                                                                   \
> -        return ld##_l##_dma(pci_get_address_space(dev), addr,           \
> -                            MEMTXATTRS_UNSPECIFIED);                    \
> +        uint##_bits##_t val; \
> +        ld##_l##_dma(pci_get_address_space(dev), addr, &val, \
> +                     MEMTXATTRS_UNSPECIFIED); \
> +        return val; \
>       }                                                                   \
>       static inline void st##_s##_pci_dma(PCIDevice *dev,                 \
>                                           dma_addr_t addr, uint##_bits##_t val) 
> \
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index d2ec9b0637f..7eae1a48478 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -127,7 +127,11 @@ static inline int spapr_vio_dma_set(SpaprVioDevice *dev, 
> uint64_t taddr,
>   #define vio_stq(_dev, _addr, _val) \
>           (stq_be_dma(&(_dev)->as, (_addr), (_val), MEMTXATTRS_UNSPECIFIED))
>   #define vio_ldq(_dev, _addr) \
> -        (ldq_be_dma(&(_dev)->as, (_addr), MEMTXATTRS_UNSPECIFIED))
> +        ({ \
> +            uint64_t _val; \
> +            ldq_be_dma(&(_dev)->as, (_addr), &_val, MEMTXATTRS_UNSPECIFIED); \
> +            _val; \
> +        })
>   
>   int spapr_vio_send_crq(SpaprVioDevice *dev, uint8_t *crq);
>   
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 191cf0b271a..589f5ba52a2 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -241,14 +241,15 @@ static inline void dma_memory_unmap(AddressSpace *as,
>   }
>   
>   #define DEFINE_LDST_DMA(_lname, _sname, _bits, _end) \
> -    static inline uint##_bits##_t ld##_lname##_##_end##_dma(AddressSpace *as, \
> -                                                            dma_addr_t addr, \
> -                                                            MemTxAttrs attrs) \
> -    {                                                                   \
> -        uint##_bits##_t val;                                            \
> -        dma_memory_read(as, addr, &val, (_bits) / 8, attrs); \
> -        return _end##_bits##_to_cpu(val);                               \
> -    }                                                                   \
> +    static inline MemTxResult ld##_lname##_##_end##_dma(AddressSpace *as, \
> +                                                        dma_addr_t addr, \
> +                                                        uint##_bits##_t *pval, 
> \
> +                                                        MemTxAttrs attrs) \
> +    { \
> +        MemTxResult res = dma_memory_read(as, addr, pval, (_bits) / 8, attrs); 
> \
> +        _end##_bits##_to_cpus(pval); \
> +        return res; \
> +    } \
>       static inline MemTxResult st##_sname##_##_end##_dma(AddressSpace *as, \
>                                                           dma_addr_t addr, \
>                                                           uint##_bits##_t val, \
> @@ -258,12 +259,10 @@ static inline void dma_memory_unmap(AddressSpace *as,
>           return dma_memory_write(as, addr, &val, (_bits) / 8, attrs); \
>       }
>   
> -static inline uint8_t ldub_dma(AddressSpace *as, dma_addr_t addr, MemTxAttrs attrs)
> +static inline MemTxResult ldub_dma(AddressSpace *as, dma_addr_t addr,
> +                                   uint8_t *val, MemTxAttrs attrs)
>   {
> -    uint8_t val;
> -
> -    dma_memory_read(as, addr, &val, 1, attrs);
> -    return val;
> +    return dma_memory_read(as, addr, val, 1, attrs);
>   }
>   
>   static inline MemTxResult stb_dma(AddressSpace *as, dma_addr_t addr,
> diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
> index d9249bbc0c1..bb207514f2d 100644
> --- a/hw/intc/pnv_xive.c
> +++ b/hw/intc/pnv_xive.c
> @@ -172,7 +172,7 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, 
> uint32_t type,
>   
>       /* Get the page size of the indirect table. */
>       vsd_addr = vsd & VSD_ADDRESS_MASK;
> -    vsd = ldq_be_dma(&address_space_memory, vsd_addr, MEMTXATTRS_UNSPECIFIED);
> +    ldq_be_dma(&address_space_memory, vsd_addr, &vsd, MEMTXATTRS_UNSPECIFIED);
>   
>       if (!(vsd & VSD_ADDRESS_MASK)) {
>   #ifdef XIVE_DEBUG
> @@ -195,8 +195,8 @@ static uint64_t pnv_xive_vst_addr_indirect(PnvXive *xive, 
> uint32_t type,
>       /* Load the VSD we are looking for, if not already done */
>       if (vsd_idx) {
>           vsd_addr = vsd_addr + vsd_idx * XIVE_VSD_SIZE;
> -        vsd = ldq_be_dma(&address_space_memory, vsd_addr,
> -                         MEMTXATTRS_UNSPECIFIED);
> +        ldq_be_dma(&address_space_memory, vsd_addr, &vsd,
> +                   MEMTXATTRS_UNSPECIFIED);
>   
>           if (!(vsd & VSD_ADDRESS_MASK)) {
>   #ifdef XIVE_DEBUG
> @@ -543,7 +543,7 @@ static uint64_t pnv_xive_vst_per_subpage(PnvXive *xive, 
> uint32_t type)
>   
>       /* Get the page size of the indirect table. */
>       vsd_addr = vsd & VSD_ADDRESS_MASK;
> -    vsd = ldq_be_dma(&address_space_memory, vsd_addr, MEMTXATTRS_UNSPECIFIED);
> +    ldq_be_dma(&address_space_memory, vsd_addr, &vsd, MEMTXATTRS_UNSPECIFIED);
>   
>       if (!(vsd & VSD_ADDRESS_MASK)) {
>   #ifdef XIVE_DEBUG
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index d960b814587..da5a4072107 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -2062,7 +2062,7 @@ static TRBCCode xhci_address_slot(XHCIState *xhci, 
> unsigned int slotid,
>       assert(slotid >= 1 && slotid <= xhci->numslots);
>   
>       dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
> -    poctx = ldq_le_dma(xhci->as, dcbaap + 8 * slotid, MEMTXATTRS_UNSPECIFIED);
> +    ldq_le_dma(xhci->as, dcbaap + 8 * slotid, &poctx, MEMTXATTRS_UNSPECIFIED);
>       ictx = xhci_mask64(pictx);
>       octx = xhci_mask64(poctx);
>   
> @@ -3429,6 +3429,7 @@ static int usb_xhci_post_load(void *opaque, int version_id)
>       uint32_t slot_ctx[4];
>       uint32_t ep_ctx[5];
>       int slotid, epid, state;
> +    uint64_t addr;
>   
>       dcbaap = xhci_addr64(xhci->dcbaap_low, xhci->dcbaap_high);
>   
> @@ -3437,8 +3438,8 @@ static int usb_xhci_post_load(void *opaque, int version_id)
>           if (!slot->addressed) {
>               continue;
>           }
> -        slot->ctx = xhci_mask64(ldq_le_dma(xhci->as, dcbaap + 8 * slotid,
> -                                           MEMTXATTRS_UNSPECIFIED));
> +        ldq_le_dma(xhci->as, dcbaap + 8 * slotid, &addr, 
> MEMTXATTRS_UNSPECIFIED);
> +        slot->ctx = xhci_mask64(addr);
>           xhci_dma_read_u32s(xhci, slot->ctx, slot_ctx, sizeof(slot_ctx));
>           slot->uport = xhci_lookup_uport(xhci, slot_ctx);
>           if (!slot->uport) {