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

[Niklas Cassel]

On Thu, Oct 01, 2020 at 08:59:31AM -0700, Keith Busch wrote:
> 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.

I simply added the second if-statement, (if (NVME_CC_EN(n->bar.cc))),
the rest of the NVME_CC_CSS was written by someone else.

But I see your point, all of this code:

        if (NVME_CC_CSS(data) != NVME_CC_CSS(n->bar.cc)) {
            if (NVME_CC_EN(n->bar.cc)) {
                NVME_GUEST_ERR(pci_nvme_err_change_css_when_enabled,
                               "changing selected command set when enabled");
            } else {
                switch (NVME_CC_CSS(data)) {
                case CSS_NVM_ONLY:
                    trace_pci_nvme_css_nvm_cset_selected_by_host(data &
                                                                 0xffffffff);
                break;
                case CSS_CSI:
                    NVME_SET_CC_CSS(n->bar.cc, CSS_CSI);
                    trace_pci_nvme_css_all_csets_sel_by_host(data &
                                                             0xffffffff);
                    break;
                case CSS_ADMIN_ONLY:
                    break;
                default:
                    NVME_GUEST_ERR(pci_nvme_ub_unknown_css_value,
                                   "unknown value in CC.CSS field");
                }
            }
        }

should simply be dropped.

No need to call NVME_SET_CC_CSS() explicitly.

CC.CSS bit will be set futher down in this function anyway:

        if (NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc)) {
            n->bar.cc = data;


Kind regards,
Niklas