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

2020-07-21 Thread P J P 
+-- On Tue, 21 Jul 2020, Philippe Mathieu-Daudé wrote --+
| On 7/21/20 8:47 AM, P J P wrote:
| > +-- On Thu, 16 Jul 2020, Peter Maydell wrote --+
| > | > 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.

Okay, preparing a revised patch series.

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

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

2020-07-21 Thread 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  ���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
>

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

2020-07-20 Thread P J P 
+-- On Thu, 16 Jul 2020, Peter Maydell wrote --+
| > P J P  
篋\x8E2020綛\xB46\xE6\x9C\x8825\xE6\x97ュ\x91\xA8\xE5\x9B\x9B 
筝\x8A\xE5\x8D\x883:01\xE5\x86\x99\xE9\x81\x93鐚\x9A
| > > +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.

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

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

2020-07-16 Thread Peter Maydell 
On Mon, 29 Jun 2020 at 12:18, Li Qiang  wrote:
>
> P J P  于2020年6月25日周四 上午3:01写道：
> >
> > From: Prasad J Pandit 
> >
> > Add nrf51_soc mmio read method to avoid NULL pointer dereference
> > issue.
> >
> > Reported-by: Lei Sun 
> > Signed-off-by: Prasad J Pandit 
> > ---
> >  hw/nvram/nrf51_nvm.c | 8 
> >  1 file changed, 8 insertions(+)
> >
> > Update v2: return ldl_le_p()
> >   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html
> >
> > diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
> > index f2283c1a8d..8000ed530a 100644
> > --- a/hw/nvram/nrf51_nvm.c
> > +++ b/hw/nvram/nrf51_nvm.c
> > @@ -273,6 +273,13 @@ static const MemoryRegionOps io_ops = {
> >  .endianness = DEVICE_LITTLE_ENDIAN,
> >  };
> >
> > +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.

thanks
-- PMM

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

2020-06-29 Thread Li Qiang 
Paolo Bonzini  于2020年6月29日周一 下午11:32写道：
>
> On 29/06/20 13:55, P J P wrote:
> > |
> > | I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n",
> > | __func__);' as other patches does.
> >
> > Earlier patch v1 did that. It was suggested to return ldl_le_p(), as that's 
> > a
> > valid return IIUC, instead of a zero(0), in case flash_read() is called.
> >
> > Thanks so much for the reviews. I'll send a revised series with due updates.
>
> I think abort() is preferable (while LOG_UNIMP is wrong as it implies
> there is something to do that QEMU is not doing).
>

Oh, here the UNIMP I understand as it will not be implemented, not the
thing 'should do but not do now'.

If. we use. abort(), the guest also. can trigger this abort(crash).
Though there is also other places we use this I think it is not very good.

In fact I would like to silent ignore(callback do nothing) if the
developer doesn't provide callback at the beginning.
But there are some cases we use LOG_UNIMP:
-->https://git.qemu.org/?p=qemu.git;a=commit;h=158b659451


Thanks,
Li Qiang


> Paolo
>

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

2020-06-29 Thread Paolo Bonzini 
On 29/06/20 13:55, P J P wrote:
> | 
> | I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n", 
> | __func__);' as other patches does.
> 
> Earlier patch v1 did that. It was suggested to return ldl_le_p(), as that's a 
> valid return IIUC, instead of a zero(0), in case flash_read() is called.
> 
> Thanks so much for the reviews. I'll send a revised series with due updates.

I think abort() is preferable (while LOG_UNIMP is wrong as it implies
there is something to do that QEMU is not doing).

Paolo

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

2020-06-29 Thread Paolo Bonzini 
On 29/06/20 13:17, Li Qiang wrote:
> 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.
> 
> However as the origin code doesn't provide a read callback. So why
> here we return something?
> 
> I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n",
> __func__);' as other
> patches does.

Even abort() would do (with a comment).

Paolo

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

2020-06-29 Thread P J P 
  Hello Li,

+-- On Mon, 29 Jun 2020, Li Qiang wrote --+
| P J P  于2020年6月25日周四 上午3:01写道：
| > Update v2: return ldl_le_p()
| >   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html
| >
| > +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);
| > +}
| 
| However as the origin code doesn't provide a read callback. So why here we 
| return something?
| 
| I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n", 
| __func__);' as other patches does.

Earlier patch v1 did that. It was suggested to return ldl_le_p(), as that's a 
valid return IIUC, instead of a zero(0), in case flash_read() is called.

Thanks so much for the reviews. I'll send a revised series with due updates.

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

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

2020-06-29 Thread Li Qiang 
P J P  于2020年6月25日周四 上午3:01写道：
>
> From: Prasad J Pandit 
>
> Add nrf51_soc mmio read method to avoid NULL pointer dereference
> issue.
>
> Reported-by: Lei Sun 
> Signed-off-by: Prasad J Pandit 
> ---
>  hw/nvram/nrf51_nvm.c | 8 
>  1 file changed, 8 insertions(+)
>
> Update v2: return ldl_le_p()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html
>
> diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
> index f2283c1a8d..8000ed530a 100644
> --- a/hw/nvram/nrf51_nvm.c
> +++ b/hw/nvram/nrf51_nvm.c
> @@ -273,6 +273,13 @@ static const MemoryRegionOps io_ops = {
>  .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>
> +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.

However as the origin code doesn't provide a read callback. So why
here we return something?

I prefer here just 'qemu_log_mask(LOG_UNIMP, "%s not implemented\n",
__func__);' as other
patches does.

Thanks,
Li Qiang


>
>  static void flash_write(void *opaque, hwaddr offset, uint64_t value,
>  unsigned int size)
> @@ -300,6 +307,7 @@ static void flash_write(void *opaque, hwaddr offset, 
> uint64_t value,
>
>
>  static const MemoryRegionOps flash_ops = {
> +.read = flash_read,
>  .write = flash_write,
>  .valid.min_access_size = 4,
>  .valid.max_access_size = 4,
> --
> 2.26.2
>

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

2020-06-24 Thread P J P 
From: Prasad J Pandit 

Add nrf51_soc mmio read method to avoid NULL pointer dereference
issue.

Reported-by: Lei Sun 
Signed-off-by: Prasad J Pandit 
---
 hw/nvram/nrf51_nvm.c | 8 
 1 file changed, 8 insertions(+)

Update v2: return ldl_le_p()
  -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04972.html

diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
index f2283c1a8d..8000ed530a 100644
--- a/hw/nvram/nrf51_nvm.c
+++ b/hw/nvram/nrf51_nvm.c
@@ -273,6 +273,13 @@ static const MemoryRegionOps io_ops = {
 .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+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);
+}
 
 static void flash_write(void *opaque, hwaddr offset, uint64_t value,
 unsigned int size)
@@ -300,6 +307,7 @@ static void flash_write(void *opaque, hwaddr offset, 
uint64_t value,
 
 
 static const MemoryRegionOps flash_ops = {
+.read = flash_read,
 .write = flash_write,
 .valid.min_access_size = 4,
 .valid.max_access_size = 4,
-- 
2.26.2