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!
