Re: [Qemu-devel] [PATCH v4 1/8] fw_cfg: max access size and region size are the same for MMIO data reg

[Andrew Jones]

On Fri, Dec 12, 2014 at 04:58:45PM +0100, Laszlo Ersek wrote:
> Make it clear that the maximum access size to the MMIO data register
> determines the full size of the memory region.
> 
> Currently the max access size is 1. Ensure that if a larger size were used
> in "fw_cfg_data_mem_ops.valid.max_access_size", the memory subsystem would
> split the access to byte-sized accesses internally, in increasing address
> order.
> 
> fw_cfg_data_mem_read() and fw_cfg_data_mem_write() don't care about
> "address" or "size"; they just call the sequential fw_cfg_read() and
> fw_cfg_write() functions, correspondingly. Therefore the automatic
> splitting is just right. (The endianness of "fw_cfg_data_mem_ops" is
> native.)

This 'is native' caught my eye. Laszlo's
Documentation/devicetree/bindings/arm/fw-cfg.txt patch states that the
selector register is LE and 

"
The data register allows accesses with 8, 16, 32 and 64-bit width.  Accesses
larger than a byte are interpreted as arrays, bundled together only for better
performance. The bytes constituting such a word, in increasing address order,
correspond to the bytes that would have been transferred by byte-wide accesses
in chronological order.
"

I chatted with Laszlo to make sure the host-is-BE case was considered.
It looks like there may be an issue there that Laszlo promised to look
into. Laszlo had already noticed that the selector was defined as native
in qemu as well, but should be LE. Now that we support > 1 byte reads
and writes from the data port, then maybe we should look into changing
that as well.

drew

> 
> This patch doesn't change behavior.
> 
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
> 
> Notes:
>     v4:
>     - unchanged
>     
>     v3:
>     - new in v3 [Drew Jones]
> 
>  hw/nvram/fw_cfg.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index c4b78ed..7f6031c 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -30,9 +30,8 @@
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  
>  #define FW_CFG_SIZE 2
> -#define FW_CFG_DATA_SIZE 1
>  #define TYPE_FW_CFG "fw_cfg"
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  #define FW_CFG(obj) OBJECT_CHECK(FWCfgState, (obj), TYPE_FW_CFG)
> @@ -323,8 +322,9 @@ static const MemoryRegionOps fw_cfg_data_mem_ops = {
>      .valid = {
>          .min_access_size = 1,
>          .max_access_size = 1,
>      },
> +    .impl.max_access_size = 1,
>  };
>  
>  static const MemoryRegionOps fw_cfg_comb_mem_ops = {
>      .read = fw_cfg_comb_read,
> @@ -608,9 +608,10 @@ static void fw_cfg_initfn(Object *obj)
>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops, s,
>                            "fwcfg.ctl", FW_CFG_SIZE);
>      sysbus_init_mmio(sbd, &s->ctl_iomem);
>      memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops, s,
> -                          "fwcfg.data", FW_CFG_DATA_SIZE);
> +                          "fwcfg.data",
> +                          fw_cfg_data_mem_ops.valid.max_access_size);
>      sysbus_init_mmio(sbd, &s->data_iomem);
>      /* In case ctl and data overlap: */
>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops, s,
>                            "fwcfg", FW_CFG_SIZE);
> -- 
> 1.8.3.1
> 
>