Re: [PATCH v3 3/8] dp8393x: Only shift the device registers mapping by 1 bit

[Mark Cave-Ayland]

On 10/07/2021 18:49, Philippe Mathieu-Daudé wrote:

> The SONIC device only allows 16/32-bit accesses. From the machine
> view (from the bus), it is only shifted by 1 bit. Another bit is
> shifted, but it is an implementation detail of the QEMU model.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/m68k/q800.c   | 2 +-
>   hw/mips/jazz.c   | 2 +-
>   hw/net/dp8393x.c | 2 ++
>   3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 6817c8b5d1a..d78edfe40e8 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -316,7 +316,7 @@ static void q800_init(MachineState *machine)
>   
>       dev = qdev_new("dp8393x");
>       qdev_set_nic_properties(dev, &nd_table[0]);
> -    qdev_prop_set_uint8(dev, "it_shift", 2);
> +    qdev_prop_set_uint8(dev, "it_shift", 1);
>       qdev_prop_set_bit(dev, "big_endian", true);
>       object_property_set_link(OBJECT(dev), "dma_mr",
>                                OBJECT(get_system_memory()), &error_abort);
> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> index d6183e18821..7f8680a189d 100644
> --- a/hw/mips/jazz.c
> +++ b/hw/mips/jazz.c
> @@ -295,7 +295,7 @@ static void mips_jazz_init(MachineState *machine,
>   
>               dev = qdev_new("dp8393x");
>               qdev_set_nic_properties(dev, nd);
> -            qdev_prop_set_uint8(dev, "it_shift", 2);
> +            qdev_prop_set_uint8(dev, "it_shift", 1);
>               qdev_prop_set_bit(dev, "big_endian", big_endian > 0);
>               object_property_set_link(OBJECT(dev), "dma_mr",
>                                        OBJECT(rc4030_dma_mr), &error_abort);
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index d1e147a82a6..64f3b0fc3ea 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -971,6 +971,8 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
>   {
>       dp8393xState *s = DP8393X(dev);
>   
> +    s->it_shift += 1; /* Registers are 16-bit wide */
> +
>       address_space_init(&s->as, s->dma_mr, "dp8393x");
>       memory_region_init_io(&s->mmio, OBJECT(dev), &dp8393x_ops, s,
>                             "dp8393x-regs", SONIC_REG_COUNT << s->it_shift);

This patch looks wrong to me: by convention it_shift is used to implement the 
register stride, so by reducing this from 2 to 1 you've immediately broken the POLA 
regarding the it_shift property. There could be an argument that there is an 
intermediate bus that could be modelled for 16-bit accesses here, but this is worked 
around (and clearly commented) in patch 8.


ATB,

Mark.