Re: [PATCH 17/17] hw/block/nvme: change controller pci id

From: Klaus Jensen <its@irrelevant.dk>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	qemu-block@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>,
	"Klaus Jensen" <k.jensen@samsung.com>,
	qemu-devel@nongnu.org, "Max Reitz" <mreitz@redhat.com>,
	"Keith Busch" <kbusch@kernel.org>,
	"Maxim Levitsky" <mlevitsk@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH 17/17] hw/block/nvme: change controller pci id
Date: Mon, 7 Sep 2020 13:02:16 +0200	[thread overview]
Message-ID: <20200907110216.GB732668@apples.localdomain> (raw)
In-Reply-To: <20200907105257.GH2682@work-vm>

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

On Sep  7 11:52, Dr. David Alan Gilbert wrote:
> * Klaus Jensen (its@irrelevant.dk) wrote:
> > On Sep  7 11:37, Dr. David Alan Gilbert wrote:
> > > * Philippe Mathieu-DaudÃƒÂ© (philmd@redhat.com) wrote:
> > > > +David in case
> > > > 
> > > > On 9/4/20 4:19 PM, Klaus Jensen wrote:
> > > > > From: Klaus Jensen <k.jensen@samsung.com>
> > > > > 
> > > > > There are two reasons for changing this:
> > > > > 
> > > > >   1. The nvme device currently uses an internal Intel device id.
> > > > > 
> > > > >   2. Since commits "nvme: fix write zeroes offset and count" and "nvme:
> > > > >      support multiple namespaces" the controller device no longer has
> > > > >      the quirks that the Linux kernel think it has.
> > > > > 
> > > > >      As the quirks are applied based on pci vendor and device id, change
> > > > >      them to get rid of the quirks.
> > > > > 
> > > > > To keep backward compatibility, add a new 'x-use-intel-id' parameter to
> > > > > the nvme device to force use of the Intel vendor and device id. This is
> > > > > off by default but add a compat property to set this for 5.1 machines
> > > > > and older.
> > > > 
> > > > So now what happens if you start a 5.1 machine with a recent kernel?
> > > > Simply the kernel will use unnecessary quirks, or are there more
> > > > changes in behavior?
> > > 
> > > Seems reasonable to me...but...
> > > 
> > > > > 
> > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > > Reviewed-by: Keith Busch <kbusch@kernel.org>
> > > > > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > > ---
> > > > >  hw/block/nvme.c   | 12 ++++++++++--
> > > > >  hw/block/nvme.h   |  1 +
> > > > >  hw/core/machine.c |  1 +
> > > > >  3 files changed, 12 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > > > index 453d3a89d475..8018f8679366 100644
> > > > > --- a/hw/block/nvme.c
> > > > > +++ b/hw/block/nvme.c
> > > > > @@ -2749,6 +2749,15 @@ static void nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
> > > > >  
> > > > >      pci_conf[PCI_INTERRUPT_PIN] = 1;
> > > > >      pci_config_set_prog_interface(pci_conf, 0x2);
> > > > > +
> > > > > +    if (n->params.use_intel_id) {
> > > > > +        pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
> > > > > +        pci_config_set_device_id(pci_conf, 0x5846);
> > > 
> > > Wasn't that magic number 5845 down there ?
> > > 
> > 
> > Argh! My first version of this just bumbed the intel device id and it
> > got left there.
> > 
> > Good find! Thank you!
> 
> It may be best to turn it into a constant in include/hw/pci/pci_ids.h if
> it corresponds to some real Intel device.
> 

Yes, but that is just the thing - it does not correspond to an
officially allocated device; which is why I think we should leave it out
of pci_ids.h.

The end goal is to get rid of its use in the code by deprecating the
use-intel-id parameter in the future. I guess the parameter should just
be deprecated immediately? Then we can get rid of it in, what, 5.4?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]