Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument

[Christian Borntraeger]

On 18.02.20 12:24, Peter Maydell wrote:
> The address_space_rw() function allows either reads or writes
> depending on the is_write argument passed to it; this is useful
> when the direction of the access is determined programmatically
> (as for instance when handling the KVM_EXIT_MMIO exit reason).
> Under the hood it just calls either address_space_write() or
> address_space_read_full().
> 
> We also use it a lot with a constant is_write argument, though,
> which has two issues:
>  * when reading "address_space_rw(..., 1)" this is less
>    immediately clear to the reader as being a write than
>    "address_space_write(...)"
>  * calling address_space_rw() bypasses the optimization
>    in address_space_read() that fast-paths reads of a
>    fixed length
> 
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/as-rw-const.patch.
> 
> Two lines in hw/net/dp8393x.c that Coccinelle produced that
> were over 80 characters were re-wrapped by hand.
> 
> Signed-off-by: Peter Maydell <address@hidden>

s390 part 
Acked-by: Christian Borntraeger <address@hidden>

> ---
> I could break this down into separate patches by submaintainer,
> but the patch is not that large and I would argue that it's
> better for the project if we can try to avoid introducing too
> much friction into the process of doing 'safe' tree-wide
> minor refactorings.
> 
> v1->v2: put the coccinelle script in scripts/coccinelle rather
> than just in the commit message.
[...]
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -874,18 +874,18 @@ static inline int ida_read_next_idaw(CcwDataStream *cds)
>          if (idaw_addr & 0x07 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
>              return -EINVAL; /* channel program check */
>          }
> -        ret = address_space_rw(&address_space_memory, idaw_addr,
> -                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt2,
> -                               sizeof(idaw.fmt2), false);
> +        ret = address_space_read(&address_space_memory, idaw_addr,
> +                                 MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt2,
> +                                 sizeof(idaw.fmt2));
>          cds->cda = be64_to_cpu(idaw.fmt2);
>      } else {
>          idaw_addr = cds->cda_orig + sizeof(idaw.fmt1) * cds->at_idaw;
>          if (idaw_addr & 0x03 || !cds_ccw_addrs_ok(idaw_addr, 0, ccw_fmt1)) {
>              return -EINVAL; /* channel program check */
>          }
> -        ret = address_space_rw(&address_space_memory, idaw_addr,
> -                               MEMTXATTRS_UNSPECIFIED, (void *) &idaw.fmt1,
> -                               sizeof(idaw.fmt1), false);
> +        ret = address_space_read(&address_space_memory, idaw_addr,
> +                                 MEMTXATTRS_UNSPECIFIED, (void *)&idaw.fmt1,
> +                                 sizeof(idaw.fmt1));
>          cds->cda = be64_to_cpu(idaw.fmt1);
>          if (cds->cda & 0x80000000) {
>              return -EINVAL; /* channel program check */