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.
> }
>
> - 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
