Re: [PATCH v2 4/5] hw/nvme: fix mmio read

[Klaus Jensen]

On Jul 14 00:18, Philippe Mathieu-Daudé wrote:
> On 7/13/21 9:24 PM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > The new PMR test unearthed a long-standing issue with MMIO reads on
> > big-endian hosts.
> > 
> > Fix this by unconditionally storing all controller registers in little
> > endian.
> > 
> > Cc: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/nvme/ctrl.c | 304 ++++++++++++++++++++++++++++---------------------
> >  1 file changed, 174 insertions(+), 130 deletions(-)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 0449cc4dee9b..ddac9344a74e 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -439,10 +439,12 @@ static uint8_t nvme_sq_empty(NvmeSQueue *sq)
> >  
> >  static void nvme_irq_check(NvmeCtrl *n)
> >  {
> > +    uint32_t intms = le32_to_cpu(n->bar.intms);
> > +
> >      if (msix_enabled(&(n->parent_obj))) {
> >          return;
> >      }
> > -    if (~n->bar.intms & n->irq_status) {
> > +    if (~intms & n->irq_status) {
> >          pci_irq_assert(&n->parent_obj);
> >      } else {
> >          pci_irq_deassert(&n->parent_obj);
> > @@ -1289,7 +1291,7 @@ static void nvme_post_cqes(void *opaque)
> >          if (ret) {
> >              trace_pci_nvme_err_addr_write(addr);
> >              trace_pci_nvme_err_cfs();
> > -            n->bar.csts = NVME_CSTS_FAILED;
> > +            n->bar.csts = cpu_to_le64(NVME_CSTS_FAILED);
> 
> The load/store API is safer than the cpu_to_X() one because
> it removes alignment problems. Here it becomes:
> 
>                stq_le_p(&n->bar.csts, NVME_CSTS_FAILED);
> 
> >              break;
> >          }
> >          QTAILQ_REMOVE(&cq->req_list, req, entry);
> > @@ -4022,7 +4024,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, 
> > NvmeRequest *req)
> >          trace_pci_nvme_err_invalid_create_sq_sqid(sqid);
> >          return NVME_INVALID_QID | NVME_DNR;
> >      }
> > -    if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
> > +    if (unlikely(!qsize || qsize > 
> > NVME_CAP_MQES(le64_to_cpu(n->bar.cap)))) {
> 
> And here:
> 
>  if (unlikely(!qsize || qsize > NVME_CAP_MQES(ldq_le_p(&n->bar.cap)))) {
> 
> >          trace_pci_nvme_err_invalid_create_sq_size(qsize);
> >          return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
> >      }
> 
> However for the BAR is likely aligned, so using the cpu_to() API
> shouldn't be a problem until we try to deprecate/remove it.
> 
> 

Right. It makes sense to use that API for new code - I will amend it :)

signature.asc
Description: PGP signature