Re: [PATCH v5 05/14] hw/block/nvme: Add support for Namespace Types

[Keith Busch]

On Thu, Oct 01, 2020 at 03:50:35PM +0000, Niklas Cassel wrote:
> On Thu, Oct 01, 2020 at 09:29:22AM -0600, Keith Busch wrote:
> > On Thu, Oct 01, 2020 at 11:22:46AM +0000, Niklas Cassel wrote:
> > > On Mon, Sep 28, 2020 at 11:35:19AM +0900, Dmitry Fomichev wrote:
> > > > From: Niklas Cassel <niklas.cassel@wdc.com>
> > > > @@ -2222,6 +2328,30 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr 
> > > > offset, uint64_t data,
> > > >          break;
> > > >      case 0x14:  /* CC */
> > > >          trace_pci_nvme_mmio_cfg(data & 0xffffffff);
> > > > +
> > > > +        if (NVME_CC_CSS(data) != NVME_CC_CSS(n->bar.cc)) {
> > > > +            if (NVME_CC_EN(n->bar.cc)) {
> > > 
> > > I just saw this print when doing controller reset on a live system.
> > > 
> > > Added a debug print:
> > > nvme_write_bar WRITING: 0x0 previous: 0x464061
> > > 
> > > so the second if-statement has to be:
> > > 
> > >     if (NVME_CC_EN(n->bar.cc) && NVME_CC_EN(data)) {
> > > 
> > > Sorry for introducing the bug in the first place.
> > 
> > No worries.
> > 
> > I don't think the check should be here at all, really. The only check for 
> > valid
> > CSS should be in nvme_start_ctrl(), which I posted yesterday.
> 
> The reasoning for this additional check is this:
> 
> From CC.CC register description:
> 
> "This field shall only be changed when the controller
> is disabled (CC.EN is cleared to ‘0’)."
> 
> In the QEMU model, we have functions, e.g. nvme_cmd_effects(),
> that uses n->bar.cc "at runtime".
> 
> So I don't think that simply checking for valid CSS in
> nvme_start_ctrl() is sufficient.
> 
> Thoughts?

The qemu controller accepts host register writes only for valid enable
and shutdown  bit transitions. Or at least it should. If not, then we
need to fix that, but that's not specific to the CSS bits.