On Jul 13 11:07, Peter Maydell wrote:
> On Tue, 13 Jul 2021 at 06:44, Klaus Jensen <i...@irrelevant.dk> wrote:
> >
> > From: Klaus Jensen <k.jen...@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.go...@samsung.com>
> > Reported-by: Peter Maydell <peter.mayd...@linaro.org>
> > Signed-off-by: Klaus Jensen <k.jen...@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.
signature.asc
Description: PGP signature
