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

[Keith Busch]

On Wed, Oct 28, 2020 at 03:02:09PM +0000, Stefan Hajnoczi wrote:
> 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?

You're not missing anything. I just made a mistake. Looks like I never
followed the implementation that far along for the BE archs.