Re: [PATCH 08/25] block/nvme: Simplify device reset

[Stefan Hajnoczi]

On Tue, Oct 27, 2020 at 09:55:34AM -0700, Keith Busch wrote:
> On Tue, Oct 27, 2020 at 04:53:31PM +0100, Philippe Mathieu-Daudé wrote:
> > On 10/27/20 3:58 PM, Keith Busch wrote:
> > > On Tue, Oct 27, 2020 at 02:55:30PM +0100, Philippe Mathieu-Daudé wrote:
> > >> Avoid multiple endianess conversion by using device endianess.
> > >>
> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > >> ---
> > >>  block/nvme.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/block/nvme.c b/block/nvme.c
> > >> index e95d59d3126..be14350f959 100644
> > >> --- a/block/nvme.c
> > >> +++ b/block/nvme.c
> > >> @@ -755,7 +755,7 @@ static int nvme_init(BlockDriverState *bs, const 
> > >> char *device, int namespace,
> > >>      timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
> > >>  
> > >>      /* Reset device to get a clean state. */
> > >> -    regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
> > >> +    regs->cc &= const_le32(0xFE);
> > > 
> > > This doesn't look right. The 'regs' is an MMIO address, correct? Memory
> > > mappings use the CPU native access.
> > 
> > cc is little-endian uint32_t.
> 
> Well, yes and no. PCI is defined as a little endian transport, so all
> CPUs have to automatically convert from their native format when
> accessing memory mapped addresses over that transport, so you always use
> the arch native format from the host software.
> 
> This isn't just for CC. This includes all memory mapped registers, so
> this driver's CSTS, AQA, doorbells, etc... shouldn't have any endian
> swapping.
> 
> See also: every other nvme driver. :)

I don't see the opposite in Linux. The Linux NVMe drivers use byteswap
instructions because readl()/writel() and friends perform little-endian
memory accesses, not native endian memory accesses:

  static int nvme_pci_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
  {
        writel(val, to_nvme_dev(ctrl)->bar + off);
        return 0;
  }

arch/arm/include/asm/io.h:

  #define writel(v,c)     ({ __iowmb(); writel_relaxed(v,c); })

where the byteswap happens here:

  #define writel_relaxed(v,c) __raw_writel((__force u32) cpu_to_le32(v),c)

The CPU is using explicit byteswaps, which matches what the QEMU driver
is doing. Am I missing something?

Stefan

signature.asc
Description: PGP signature