From: Klaus Jensen <k.jensen@samsung.com>

The new PMR test unearthed a long-standing issue with MMIO reads on
big-endian hosts.

Fix by using the ldn_he_p helper instead of memcpy.

Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2f0524e12a36..dd81c3b19c7e 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5951,7 +5951,6 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
 {
     NvmeCtrl *n = (NvmeCtrl *)opaque;
     uint8_t *ptr = (uint8_t *)&n->bar;
-    uint64_t val = 0;
 
     trace_pci_nvme_mmio_read(addr, size);
 
@@ -5977,14 +5976,15 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
             (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
             memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
         }
-        memcpy(&val, ptr + addr, size);
-    } else {
-        NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
-                       "MMIO read beyond last register,"
-                       " offset=0x%"PRIx64", returning 0", addr);
+
+        return ldn_he_p(ptr + addr, size);
     }
 
-    return val;
+    NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
+                   "MMIO read beyond last register,"
+                   " offset=0x%"PRIx64", returning 0", addr);
+
+    return 0;
 }
 
 static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
-- 
2.32.0

On Tue, Jul 13, 2021 at 07:43:59AM +0200, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>The new PMR test unearthed a long-standing issue with MMIO reads on
>big-endian hosts.
>
>Fix by using the ldn_he_p helper instead of memcpy.
>
>Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>Reported-by: Peter Maydell <peter.maydell@linaro.org>
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> hw/nvme/ctrl.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 2f0524e12a36..dd81c3b19c7e 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -5951,7 +5951,6 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
> {
>     NvmeCtrl *n = (NvmeCtrl *)opaque;
>     uint8_t *ptr = (uint8_t *)&n->bar;
>-    uint64_t val = 0;
>
>     trace_pci_nvme_mmio_read(addr, size);
>
>@@ -5977,14 +5976,15 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
>             (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
>             memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
>         }
>-        memcpy(&val, ptr + addr, size);
>-    } else {
>-        NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
>-                       "MMIO read beyond last register,"
>-                       " offset=0x%"PRIx64", returning 0", addr);
>+
>+        return ldn_he_p(ptr + addr, size);
>     }
>
>-    return val;
>+    NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
>+                   "MMIO read beyond last register,"
>+                   " offset=0x%"PRIx64", returning 0", addr);
>+
>+    return 0;
> }
>
> static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
>-- 
>2.32.0
>
>
LGTM.

Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>

On Tue, 13 Jul 2021 at 06:44, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@samsung.com>
>
> The new PMR test unearthed a long-standing issue with MMIO reads on
> big-endian hosts.
>
> Fix by using the ldn_he_p helper instead of memcpy.
>
> Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/nvme/ctrl.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 2f0524e12a36..dd81c3b19c7e 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -5951,7 +5951,6 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      NvmeCtrl *n = (NvmeCtrl *)opaque;
>      uint8_t *ptr = (uint8_t *)&n->bar;
> -    uint64_t val = 0;
>
>      trace_pci_nvme_mmio_read(addr, size);
>
> @@ -5977,14 +5976,15 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
>              (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
>              memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
>          }
> -        memcpy(&val, ptr + addr, size);
> -    } else {
> -        NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
> -                       "MMIO read beyond last register,"
> -                       " offset=0x%"PRIx64", returning 0", addr);
> +
> +        return ldn_he_p(ptr + addr, size);

I don't think this will do the right thing for accesses which aren't
of the same width as whatever the field in NvmeBar is defined as.
For instance, if the guest does a 32-bit access to offset 0,
because the first field is defined as 'uint64_t cap', on a
big-endian host they will end up reading the high 4 bytes of the
64-bit value, and on a little-endian host they will get the low 4 bytes.

>      }
>
> -    return val;
> +    NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
> +                   "MMIO read beyond last register,"
> +                   " offset=0x%"PRIx64", returning 0", addr);
> +
> +    return 0;
>  }

Looking at the surrounding code, I notice that we guard this "read size bytes
from &n->bar + addr" with
    if (addr < sizeof(n->bar)) {

but that doesn't account for 'size', so if the guest asks to read
4 bytes starting at offset sizeof(n->bar)-1 then we'll still read
3 bytes beyond the end of the buffer...

thanks
-- PMM

On Jul 13 11:07, Peter Maydell wrote:
> On Tue, 13 Jul 2021 at 06:44, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > From: Klaus Jensen <k.jensen@samsung.com>
> >
> > The new PMR test unearthed a long-standing issue with MMIO reads on
> > big-endian hosts.
> >
> > Fix by using the ldn_he_p helper instead of memcpy.
> >
> > Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/nvme/ctrl.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 2f0524e12a36..dd81c3b19c7e 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -5951,7 +5951,6 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
> >  {
> >      NvmeCtrl *n = (NvmeCtrl *)opaque;
> >      uint8_t *ptr = (uint8_t *)&n->bar;
> > -    uint64_t val = 0;
> >
> >      trace_pci_nvme_mmio_read(addr, size);
> >
> > @@ -5977,14 +5976,15 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size)
> >              (NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
> >              memory_region_msync(&n->pmr.dev->mr, 0, n->pmr.dev->size);
> >          }
> > -        memcpy(&val, ptr + addr, size);
> > -    } else {
> > -        NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
> > -                       "MMIO read beyond last register,"
> > -                       " offset=0x%"PRIx64", returning 0", addr);
> > +
> > +        return ldn_he_p(ptr + addr, size);
> 
> I don't think this will do the right thing for accesses which aren't
> of the same width as whatever the field in NvmeBar is defined as.
> For instance, if the guest does a 32-bit access to offset 0,
> because the first field is defined as 'uint64_t cap', on a
> big-endian host they will end up reading the high 4 bytes of the
> 64-bit value, and on a little-endian host they will get the low 4 bytes.
> 

Thanks for taking a look Peter, I wondered if I actually fixed it
correctly or not, and I obviously didnt.

I guess I can't get around handling 64 bit registers explicitly and
convert them to little endian explicitly then.

> >      }
> >
> > -    return val;
> > +    NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
> > +                   "MMIO read beyond last register,"
> > +                   " offset=0x%"PRIx64", returning 0", addr);
> > +
> > +    return 0;
> >  }
> 
> Looking at the surrounding code, I notice that we guard this "read size bytes
> from &n->bar + addr" with
>     if (addr < sizeof(n->bar)) {
> 
> but that doesn't account for 'size', so if the guest asks to read
> 4 bytes starting at offset sizeof(n->bar)-1 then we'll still read
> 3 bytes beyond the end of the buffer...

The buffer is at least sizeof(n->bar) + 8 bytes (there are two doorbell
registers following the controller registers). It is wrong for the host
to read those, but as per the spec it is undefined behavior. I did
consider reversing the condition to `(addr > sizeof(n->bar) - size)`. I
guess that would be the proper thing to do.

On Tue, 13 Jul 2021 at 11:19, Klaus Jensen <its@irrelevant.dk> wrote:
>
> On Jul 13 11:07, Peter Maydell wrote:
> > Looking at the surrounding code, I notice that we guard this "read size bytes
> > from &n->bar + addr" with
> >     if (addr < sizeof(n->bar)) {
> >
> > but that doesn't account for 'size', so if the guest asks to read
> > 4 bytes starting at offset sizeof(n->bar)-1 then we'll still read
> > 3 bytes beyond the end of the buffer...
>
> The buffer is at least sizeof(n->bar) + 8 bytes (there are two doorbell
> registers following the controller registers). It is wrong for the host
> to read those, but as per the spec it is undefined behavior.

I don't know about the doorbell registers, but with this code
(or the old memcpy()) you'll access whatever the next thing after
"NvmeBar bar" in the NvmeCtrl struct is, which looks like it's the
first part of 'struct NvmeParams".

thanks
-- PMM

On Jul 13 11:31, Peter Maydell wrote:
> On Tue, 13 Jul 2021 at 11:19, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > On Jul 13 11:07, Peter Maydell wrote:
> > > Looking at the surrounding code, I notice that we guard this "read size bytes
> > > from &n->bar + addr" with
> > >     if (addr < sizeof(n->bar)) {
> > >
> > > but that doesn't account for 'size', so if the guest asks to read
> > > 4 bytes starting at offset sizeof(n->bar)-1 then we'll still read
> > > 3 bytes beyond the end of the buffer...
> >
> > The buffer is at least sizeof(n->bar) + 8 bytes (there are two doorbell
> > registers following the controller registers). It is wrong for the host
> > to read those, but as per the spec it is undefined behavior.
> 
> I don't know about the doorbell registers, but with this code
> (or the old memcpy()) you'll access whatever the next thing after
> "NvmeBar bar" in the NvmeCtrl struct is, which looks like it's the
> first part of 'struct NvmeParams".
> 

Sorry, you are of course right. I remembered how the bar was allocated
incorrectly.