Re: [Qemu-devel] [PATCH] audio: intel-hda: do not use old_mmio accesses

[Peter Maydell]

On 21 August 2017 at 22:13, Matt Parker <address@hidden> wrote:
> intel-hda is still using the old_mmio accessors for io.
> This updates the device to use .read and .write accessors instead.

Hi; thanks for this patch.

It looks like you forgot to provide your Signed-off-by: line;
we can't accept patches without one, I'm afraid.

> ---
>  hw/audio/intel-hda.c | 56 
> +++++++++-------------------------------------------
>  1 file changed, 9 insertions(+), 47 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 06acc98f7b..c0f002f744 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1043,66 +1043,28 @@ static void intel_hda_regs_reset(IntelHDAState *d)
>
>  /* --------------------------------------------------------------------- */
>
> -static void intel_hda_mmio_writeb(void *opaque, hwaddr addr, uint32_t val)
> +static void intel_hda_mmio_write(void *opaque, hwaddr addr, uint64_t val, 
> unsigned size)
>  {
>      IntelHDAState *d = opaque;
>      const IntelHDAReg *reg = intel_hda_reg_find(d, addr);
>
> -    intel_hda_reg_write(d, reg, val, 0xff);
> +    intel_hda_reg_write(d, reg, val, (1UL << (size * 8)) - 1);

If size is 4 and 'unsigned long' is 32 bits (which it usually
is) then this will shift off the end of the value, which is
undefined behaviour.

You could fix that by using '1ULL' instead of '1UL', but
I'd suggest using MAKE_64BIT_MASK(0, size * 8) for this.
(that macro is in "qemu/bitops.h", which you'll probably need
to add a #include for.) Using the macro makes the intent a bit
clearer and means nobody has to think about whether the bit
shifting is doing what it's supposed to.

I recommend putting a newline in the prototype to keep
it below 80 lines and please the checkpatch script, too.

thanks
-- PMM