Re: [PATCH v2 5/9] nvram: add nrf51_soc flash read method

[Philippe Mathieu-Daudé]

On 7/21/20 8:47 AM, P J P wrote:
> +-- On Thu, 16 Jul 2020, Peter Maydell wrote --+
> | > P J P <ppandit@redhat.com> ���2020���6���25��������� ������3:01���������
> | > > +static uint64_t flash_read(void *opaque, hwaddr offset, unsigned size)
> | > > +{
> | > > +    NRF51NVMState *s = NRF51_NVM(opaque);
> | > > +
> | > > +    assert(offset + size <= s->flash_size);
> | > > +    return ldl_le_p(s->storage + offset);
> | > > +}
> | >
> | > The 'flash_ops' is for ROM, though I don't see where it calls 
> | > 'memory_region_rom_device_set_romd' to ROMD, so this MR is in MMIO mode 
> | > and it needs a read callback.
> | 
> | I think that 'romd mode' (ie reads-go-directly-to-RAM) is the default: 
> | memory_region_initfn() sets romd_mode to true. So unless the device 
> actively 
> | calls memory_region_rom_device_set_romd(mr, false) then the read callback 
> | can't be reached.
> 
> So, we go with g_assert_not_reached() ? We seem to have differing opinions 
> about these callbacks.

- Callback missing because we neglected to implement the
  hardware behavior:

  => qemu_log_mask(LOG_UNIMP, ...)

- Callback missing because the access is illegal on hardware
  (write on read-only register, read on write-only register):

  => qemu_log_mask(LOG_GUEST_ERROR, ...)

- Impossible situation unrelated to the hardware/guest behavior
  (problem in QEMU design)

  => g_assert_not_reached()


Note, when we runs QEMU with LOG_UNIMP/LOG_GUEST_ERROR enabled,
we are usually interested in what address the guest is accessing,
and in the write case, what value is written.

> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>