Re: [PATCH v6 23/42] nvme: add mapping helpers

[Maxim Levitsky]

On Tue, 2020-03-31 at 07:44 +0200, Klaus Birkelund Jensen wrote:
> On Mar 25 12:45, Maxim Levitsky wrote:
> > On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > > From: Klaus Jensen <address@hidden>
> > > 
> > > Add nvme_map_addr, nvme_map_addr_cmb and nvme_addr_to_cmb helpers and
> > > use them in nvme_map_prp.
> > > 
> > > This fixes a bug where in the case of a CMB transfer, the device would
> > > map to the buffer with a wrong length.
> > > 
> > > Fixes: b2b2b67a00574 ("nvme: Add support for Read Data and Write Data in 
> > > CMBs.")
> > > Signed-off-by: Klaus Jensen <address@hidden>
> > > ---
> > >  hw/block/nvme.c       | 97 +++++++++++++++++++++++++++++++++++--------
> > >  hw/block/trace-events |  1 +
> > >  2 files changed, 81 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 08267e847671..187c816eb6ad 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -153,29 +158,79 @@ static void nvme_irq_deassert(NvmeCtrl *n, 
> > > NvmeCQueue *cq)
> > >      }
> > >  }
> > >  
> > > +static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, QEMUIOVector *iov, hwaddr 
> > > addr,
> > > +                                  size_t len)
> > > +{
> > > +    if (!nvme_addr_is_cmb(n, addr) || !nvme_addr_is_cmb(n, addr + len)) {
> > > +        return NVME_DATA_TRAS_ERROR;
> > > +    }
> > 
> > I just noticed that
> > in theory (not that it really matters) but addr+len refers to the byte 
> > which is already 
> > not the part of the transfer.
> > 
> 
> Oh. Good catch - and I think that it does matter? Can't we end up
> rejecting a valid access? Anyway, I fixed it with a '- 1'.

Actually thinking again about it, we can indeed reject the access if the data 
happens
to to include last byte of CMB. That can absolutely happen.

When I wrote this I was thinking the other way around that we might reject data
that is in regular ram and 'touches' the CMB, which indeed won't happen since
RAM usually don't come close to MMIO ranges.

Anyway there is not reason to not fix such issues.

> 
> > 
> > > +
> > > +    qemu_iovec_add(iov, nvme_addr_to_cmb(n, addr), len);
> > 
> > Also intersting is we can add 0 sized iovec.
> > 
> 
> I added a check on len. This also makes sure the above '- 1' fix doesn't
> cause an 'addr + 0 - 1' to be done.
Yes that is what I was thinking, len=0 needs a special case here.


Best regards,
        Maxim Levitsky