Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes

[Klaus Jensen]

On Jan 18 21:41, Minwoo Im wrote:
> On 21-01-18 10:46:55, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > 64 bit registers like ASQ and ACQ should be writable by both a hi/lo 32
> > bit write combination as well as a plain 64 bit write. The spec does not
> > define ordering on the hi/lo split, but the code currently assumes that
> > the low order bits are written first. Additionally, the code does not
> > consider that another address might already have been written into the
> > register, causing the OR'ing to result in a bad address.
> > 
> > Fix this by explicitly overwriting only the low or high order bits for
> > 32 bit writes.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index bd7e258c3c26..40b9f8ccfc0e 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -3781,19 +3781,21 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr 
> > offset, uint64_t data,
> >          trace_pci_nvme_mmio_aqattr(data & 0xffffffff);
> >          break;
> >      case 0x28:  /* ASQ */
> > -        n->bar.asq = data;
> > +        n->bar.asq = size == 8 ? data :
> > +            (n->bar.asq & ~0xffffffff) | (data & 0xffffffff);
>                              ^^^^^^^^^^^
> If this one is to unmask lower 32bits other than higher 32bits, then
> it should be:
> 
>       (n->bar.asq & ~0xffffffffULL)
> 

Ouch. Yes, thanks!

> Also, maybe we should take care of lower than 4bytes as:
> 
>       .min_access_size = 2,
>       .max_access_size = 8,
> 
> Even we have some error messages up there with:
> 
>       if (unlikely(size < sizeof(uint32_t))) {
>           NVME_GUEST_ERR(pci_nvme_ub_mmiowr_toosmall,
>                          "MMIO write smaller than 32-bits,"
>                          " offset=0x%"PRIx64", size=%u",
>                          offset, size);
>           /* should be ignored, fall through for now */
>       }
> 
> It's a fall-thru error, and that's it.  I would prefer not to have this
> error and support for 2bytes access also, OR do not support for 2bytes
> access for this MMIO area.
> 

The spec requires aligned 32 bit accesses (or the size of the register),
so maybe it's about time we actually ignore instead of falling through.

signature.asc
Description: PGP signature