Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes
On Mon, Jan 18, 2021 at 08:53:24PM +0100, Klaus Jensen wrote: > On Jan 18 21:59, Minwoo Im wrote: > > > 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. > > > > Agreed. > > > > On the other hand. > > The spec just allows undefined behavior if the alignment or size > requirement is violated. So falling through is not wrong. > > Keith, any thoughts on this? If the host's access is unaligned and breaks something, the host gets to keep both pieces.
Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes
On 21-01-18 20:53:24, Klaus Jensen wrote: > On Jan 18 21:59, Minwoo Im wrote: > > > 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. > > > > Agreed. > > > > On the other hand. > > The spec just allows undefined behavior if the alignment or size > requirement is violated. So falling through is not wrong. If so, maybe we just can make this MMIO region support under 4bytes access with error messaging. I don't think we should not support them on purpose because spec says it just results in undefined behaviors which is just undefined how to handle them.
Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes
On Jan 18 21:59, Minwoo Im wrote: > > 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. > > Agreed. > On the other hand. The spec just allows undefined behavior if the alignment or size requirement is violated. So falling through is not wrong. Keith, any thoughts on this? signature.asc Description: PGP signature
Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes
> 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. Agreed.
Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes
On Jan 18 21:41, Minwoo Im wrote:
> On 21-01-18 10:46:55, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > 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
> > ---
> > 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 & 0x);
> > break;
> > case 0x28: /* ASQ */
> > -n->bar.asq = data;
> > +n->bar.asq = size == 8 ? data :
> > +(n->bar.asq & ~0x) | (data & 0x);
> ^^^
> If this one is to unmask lower 32bits other than higher 32bits, then
> it should be:
>
> (n->bar.asq & ~0xULL)
>
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
Re: [PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes
On 21-01-18 10:46:55, Klaus Jensen wrote:
> From: Klaus Jensen
>
> 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
> ---
> 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 & 0x);
> break;
> case 0x28: /* ASQ */
> -n->bar.asq = data;
> +n->bar.asq = size == 8 ? data :
> +(n->bar.asq & ~0x) | (data & 0x);
^^^
If this one is to unmask lower 32bits other than higher 32bits, then
it should be:
(n->bar.asq & ~0xULL)
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.
Thanks!
[PATCH v2 02/12] hw/block/nvme: fix 64 bit register hi/lo split writes
From: Klaus Jensen 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 --- 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 & 0x); break; case 0x28: /* ASQ */ -n->bar.asq = data; +n->bar.asq = size == 8 ? data : +(n->bar.asq & ~0x) | (data & 0x); trace_pci_nvme_mmio_asqaddr(data); break; case 0x2c: /* ASQ hi */ -n->bar.asq |= data << 32; +n->bar.asq = (n->bar.asq & 0x) | (data << 32); trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq); break; case 0x30: /* ACQ */ trace_pci_nvme_mmio_acqaddr(data); -n->bar.acq = data; +n->bar.acq = size == 8 ? data : +(n->bar.acq & ~0x) | (data & 0x); break; case 0x34: /* ACQ hi */ -n->bar.acq |= data << 32; +n->bar.acq = (n->bar.acq & 0x) | (data << 32); trace_pci_nvme_mmio_acqaddr_hi(data, n->bar.acq); break; case 0x38: /* CMBLOC */ -- 2.30.0
