Re: [PATCH 1/4] dma: Let st*_dma() take MemTxAttrs argument

From: "Cédric Le Goater" <clg@kaod.org>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, qemu-devel@nongnu.org
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	David Hildenbrand <david@redhat.com>, Li Qiang <liq3ea@gmail.com>,
	Qiuhao Li <Qiuhao.Li@outlook.com>, Peter Xu <peterx@redhat.com>,
	Greg Kurz <groug@kaod.org>, Alexander Bulekov <alxndr@bu.edu>,
	qemu-ppc@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH 1/4] dma: Let st*_dma() take MemTxAttrs argument
Date: Wed, 22 Dec 2021 07:44:01 +0100	[thread overview]
Message-ID: <17de7e50-1683-1819-34e3-421881bbafc3@kaod.org> (raw)
In-Reply-To: <20211218145111.1540114-2-philmd@redhat.com>

On 12/18/21 15:51, Philippe Mathieu-Daudé wrote:
> Let devices specify transaction attributes when calling st*_dma().
> 
> Keep the default MEMTXATTRS_UNSPECIFIED in 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       |  3 ++-
>   include/hw/ppc/spapr_vio.h | 12 ++++++++----
>   include/sysemu/dma.h       | 10 ++++++----
>   hw/nvram/fw_cfg.c          |  4 ++--
>   4 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index a751ab5a75d..d07e9707b48 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -859,7 +859,8 @@ static inline MemTxResult pci_dma_write(PCIDevice *dev, dma_addr_t addr,
>       static inline void st##_s##_pci_dma(PCIDevice *dev,                 \
>                                           dma_addr_t addr, uint##_bits##_t val) \
>       {                                                                   \
> -        st##_s##_dma(pci_get_address_space(dev), addr, val);            \
> +        st##_s##_dma(pci_get_address_space(dev), addr, val, \
> +                     MEMTXATTRS_UNSPECIFIED); \
>       }
>   
>   PCI_DMA_DEFINE_LDST(ub, b, 8);
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index 5d2ea8e6656..e87f8e6f596 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -118,10 +118,14 @@ static inline int spapr_vio_dma_set(SpaprVioDevice *dev, uint64_t taddr,
>           H_DEST_PARM : H_SUCCESS;
>   }
>   
> -#define vio_stb(_dev, _addr, _val) (stb_dma(&(_dev)->as, (_addr), (_val)))
> -#define vio_sth(_dev, _addr, _val) (stw_be_dma(&(_dev)->as, (_addr), (_val)))
> -#define vio_stl(_dev, _addr, _val) (stl_be_dma(&(_dev)->as, (_addr), (_val)))
> -#define vio_stq(_dev, _addr, _val) (stq_be_dma(&(_dev)->as, (_addr), (_val)))
> +#define vio_stb(_dev, _addr, _val) \
> +        (stb_dma(&(_dev)->as, (_addr), (_val), MEMTXATTRS_UNSPECIFIED))
> +#define vio_sth(_dev, _addr, _val) \
> +        (stw_be_dma(&(_dev)->as, (_addr), (_val), MEMTXATTRS_UNSPECIFIED))
> +#define vio_stl(_dev, _addr, _val) \
> +        (stl_be_dma(&(_dev)->as, (_addr), (_val), MEMTXATTRS_UNSPECIFIED))
> +#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)))
>   
>   int spapr_vio_send_crq(SpaprVioDevice *dev, uint8_t *crq);
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 9f998edbea4..2a60d2c5d61 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -250,10 +250,11 @@ static inline void dma_memory_unmap(AddressSpace *as,
>       }                                                                   \
>       static inline void st##_sname##_##_end##_dma(AddressSpace *as,      \
>                                                    dma_addr_t addr,       \
> -                                                 uint##_bits##_t val)   \
> +                                                 uint##_bits##_t val,   \
> +                                                 MemTxAttrs attrs)      \
>       {                                                                   \
>           val = cpu_to_##_end##_bits(val);                                \
> -        dma_memory_write(as, addr, &val, (_bits) / 8, MEMTXATTRS_UNSPECIFIED); \
> +        dma_memory_write(as, addr, &val, (_bits) / 8, attrs); \
>       }
>   
>   static inline uint8_t ldub_dma(AddressSpace *as, dma_addr_t addr)
> @@ -264,9 +265,10 @@ static inline uint8_t ldub_dma(AddressSpace *as, dma_addr_t addr)
>       return val;
>   }
>   
> -static inline void stb_dma(AddressSpace *as, dma_addr_t addr, uint8_t val)
> +static inline void stb_dma(AddressSpace *as, dma_addr_t addr,
> +                           uint8_t val, MemTxAttrs attrs)
>   {
> -    dma_memory_write(as, addr, &val, 1, MEMTXATTRS_UNSPECIFIED);
> +    dma_memory_write(as, addr, &val, 1, attrs);
>   }
>   
>   DEFINE_LDST_DMA(uw, w, 16, le);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 9b91b15cb08..e5f3c981841 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -360,7 +360,7 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>       if (dma_memory_read(s->dma_as, dma_addr,
>                           &dma, sizeof(dma), MEMTXATTRS_UNSPECIFIED)) {
>           stl_be_dma(s->dma_as, dma_addr + offsetof(FWCfgDmaAccess, control),
> -                   FW_CFG_DMA_CTL_ERROR);
> +                   FW_CFG_DMA_CTL_ERROR, MEMTXATTRS_UNSPECIFIED);
>           return;
>       }
>   
> @@ -446,7 +446,7 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
>       }
>   
>       stl_be_dma(s->dma_as, dma_addr + offsetof(FWCfgDmaAccess, control),
> -                dma.control);
> +                dma.control, MEMTXATTRS_UNSPECIFIED);
>   
>       trace_fw_cfg_read(s, 0);
>   }
>