diff mbox series

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

Message ID 20200904141956.576630-18-its@irrelevant.dk
State New, archived
Headers show
Series hw/block/nvme: multiple namespaces support | expand

Commit Message

Klaus Jensen Sept. 4, 2020, 2:19 p.m. UTC
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.

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(-)

Comments

Philippe Mathieu-Daudé Sept. 7, 2020, 2:28 a.m. UTC | #1
+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?

> 
> 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);
> +    } else {
> +        pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REDHAT);
> +        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REDHAT_NVME);
> +    }
> +
>      pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS);
>      pcie_endpoint_cap_init(pci_dev, 0x80);
>  
> @@ -2903,6 +2912,7 @@ static Property nvme_props[] = {
>      DEFINE_PROP_UINT8("aerl", NvmeCtrl, params.aerl, 3),
>      DEFINE_PROP_UINT32("aer_max_queued", NvmeCtrl, params.aer_max_queued, 64),
>      DEFINE_PROP_UINT8("mdts", NvmeCtrl, params.mdts, 7),
> +    DEFINE_PROP_BOOL("x-use-intel-id", NvmeCtrl, params.use_intel_id, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2919,8 +2929,6 @@ static void nvme_class_init(ObjectClass *oc, void *data)
>      pc->realize = nvme_realize;
>      pc->exit = nvme_exit;
>      pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
> -    pc->vendor_id = PCI_VENDOR_ID_INTEL;
> -    pc->device_id = 0x5845;
>      pc->revision = 2;
>  
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 72260f2e8ea9..a734a5e1370d 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -15,6 +15,7 @@ typedef struct NvmeParams {
>      uint8_t  aerl;
>      uint32_t aer_max_queued;
>      uint8_t  mdts;
> +    bool     use_intel_id;
>  } NvmeParams;
>  
>  typedef struct NvmeAsyncEvent {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ea26d612374d..67990232528c 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -34,6 +34,7 @@ GlobalProperty hw_compat_5_1[] = {
>      { "vhost-user-scsi", "num_queues", "1"},
>      { "virtio-blk-device", "num-queues", "1"},
>      { "virtio-scsi-device", "num_queues", "1"},
> +    { "nvme", "x-use-intel-id", "on"},
>  };
>  const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);
>  
>
Klaus Jensen Sept. 7, 2020, 7:23 a.m. UTC | #2
On Sep  7 04:28, Philippe Mathieu-Daudé 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?
> 

Yes, the kernel will then just apply unneccesary quirks, these are:

  1. NVME_QUIRK_IDENTIFY_CNS which says that the device does not support
     anything else than values 0x0 and 0x1 for CNS (Identify Namespace and
     Identify Namespace). With multiple namespace support, this just
     means that the kernel will "scan" namespaces instead of using
     "Active Namespace ID list" (CNS 0x2).

  2. NVME_QUIRK_DISABLE_WRITE_ZEROES. The nvme device started out with a
     broken Write Zeroes implementation which has since been fixed in
     commit 9d6459d21a6e ("nvme: fix write zeroes offset and count").
Philippe Mathieu-Daudé Sept. 7, 2020, 8:36 a.m. UTC | #3
On 9/7/20 9:23 AM, Klaus Jensen wrote:
> On Sep  7 04:28, Philippe Mathieu-Daudé 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?
>>
> 
> Yes, the kernel will then just apply unneccesary quirks, these are:
> 
>   1. NVME_QUIRK_IDENTIFY_CNS which says that the device does not support
>      anything else than values 0x0 and 0x1 for CNS (Identify Namespace and
>      Identify Namespace). With multiple namespace support, this just
>      means that the kernel will "scan" namespaces instead of using
>      "Active Namespace ID list" (CNS 0x2).
> 
>   2. NVME_QUIRK_DISABLE_WRITE_ZEROES. The nvme device started out with a
>      broken Write Zeroes implementation which has since been fixed in
>      commit 9d6459d21a6e ("nvme: fix write zeroes offset and count").

OK thanks. Can you amend that information in the commit
description please?

Thanks,

Phil.
Klaus Jensen Sept. 7, 2020, 8:58 a.m. UTC | #4
On Sep  7 10:36, Philippe Mathieu-Daudé wrote:
> On 9/7/20 9:23 AM, Klaus Jensen wrote:
> > On Sep  7 04:28, Philippe Mathieu-Daudé 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?
> >>
> > 
> > Yes, the kernel will then just apply unneccesary quirks, these are:
> > 
> >   1. NVME_QUIRK_IDENTIFY_CNS which says that the device does not support
> >      anything else than values 0x0 and 0x1 for CNS (Identify Namespace and
> >      Identify Namespace). With multiple namespace support, this just
> >      means that the kernel will "scan" namespaces instead of using
> >      "Active Namespace ID list" (CNS 0x2).
> > 
> >   2. NVME_QUIRK_DISABLE_WRITE_ZEROES. The nvme device started out with a
> >      broken Write Zeroes implementation which has since been fixed in
> >      commit 9d6459d21a6e ("nvme: fix write zeroes offset and count").
> 
> OK thanks. Can you amend that information in the commit
> description please?
> 

Yes, absolutely.
Klaus Jensen Sept. 7, 2020, 9:20 a.m. UTC | #5
On Sep  7 10:58, Klaus Jensen wrote:
> On Sep  7 10:36, Philippe Mathieu-Daudé wrote:
> > On 9/7/20 9:23 AM, Klaus Jensen wrote:
> > > On Sep  7 04:28, Philippe Mathieu-Daudé 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?
> > >>
> > > 
> > > Yes, the kernel will then just apply unneccesary quirks, these are:
> > > 
> > >   1. NVME_QUIRK_IDENTIFY_CNS which says that the device does not support
> > >      anything else than values 0x0 and 0x1 for CNS (Identify Namespace and
> > >      Identify Namespace). With multiple namespace support, this just
> > >      means that the kernel will "scan" namespaces instead of using
> > >      "Active Namespace ID list" (CNS 0x2).
> > > 
> > >   2. NVME_QUIRK_DISABLE_WRITE_ZEROES. The nvme device started out with a
> > >      broken Write Zeroes implementation which has since been fixed in
> > >      commit 9d6459d21a6e ("nvme: fix write zeroes offset and count").
> > 
> > OK thanks. Can you amend that information in the commit
> > description please?
> > 
> 
> Yes, absolutely.

By the way. Is it correct use of an 'x-' parameter here - since it is
something that we might remove in the future? I was unable to find any
documentation on the purpose of the 'x-' prefix, but I was guessing it
was for stuff like this.
Philippe Mathieu-Daudé Sept. 7, 2020, 9:33 a.m. UTC | #6
On 9/7/20 11:20 AM, Klaus Jensen wrote:
> On Sep  7 10:58, Klaus Jensen wrote:
>> On Sep  7 10:36, Philippe Mathieu-Daudé wrote:
>>> On 9/7/20 9:23 AM, Klaus Jensen wrote:
>>>> On Sep  7 04:28, Philippe Mathieu-Daudé 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?
>>>>>
>>>>
>>>> Yes, the kernel will then just apply unneccesary quirks, these are:
>>>>
>>>>   1. NVME_QUIRK_IDENTIFY_CNS which says that the device does not support
>>>>      anything else than values 0x0 and 0x1 for CNS (Identify Namespace and
>>>>      Identify Namespace). With multiple namespace support, this just
>>>>      means that the kernel will "scan" namespaces instead of using
>>>>      "Active Namespace ID list" (CNS 0x2).
>>>>
>>>>   2. NVME_QUIRK_DISABLE_WRITE_ZEROES. The nvme device started out with a
>>>>      broken Write Zeroes implementation which has since been fixed in
>>>>      commit 9d6459d21a6e ("nvme: fix write zeroes offset and count").
>>>
>>> OK thanks. Can you amend that information in the commit
>>> description please?
>>>
>>
>> Yes, absolutely.
> 
> By the way. Is it correct use of an 'x-' parameter here - since it is
> something that we might remove in the future? I was unable to find any
> documentation on the purpose of the 'x-' prefix, but I was guessing it
> was for stuff like this.

Probably not. 'x-' is for unstable debugging/testing features.
Dr. David Alan Gilbert Sept. 7, 2020, 10:37 a.m. UTC | #7
* 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 ?

Dave

> > +    } else {
> > +        pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REDHAT);
> > +        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REDHAT_NVME);
> > +    }
> > +
> >      pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS);
> >      pcie_endpoint_cap_init(pci_dev, 0x80);
> >  
> > @@ -2903,6 +2912,7 @@ static Property nvme_props[] = {
> >      DEFINE_PROP_UINT8("aerl", NvmeCtrl, params.aerl, 3),
> >      DEFINE_PROP_UINT32("aer_max_queued", NvmeCtrl, params.aer_max_queued, 64),
> >      DEFINE_PROP_UINT8("mdts", NvmeCtrl, params.mdts, 7),
> > +    DEFINE_PROP_BOOL("x-use-intel-id", NvmeCtrl, params.use_intel_id, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -2919,8 +2929,6 @@ static void nvme_class_init(ObjectClass *oc, void *data)
> >      pc->realize = nvme_realize;
> >      pc->exit = nvme_exit;
> >      pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
> > -    pc->vendor_id = PCI_VENDOR_ID_INTEL;
> > -    pc->device_id = 0x5845;
> >      pc->revision = 2;
> >  
> >      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 72260f2e8ea9..a734a5e1370d 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -15,6 +15,7 @@ typedef struct NvmeParams {
> >      uint8_t  aerl;
> >      uint32_t aer_max_queued;
> >      uint8_t  mdts;
> > +    bool     use_intel_id;
> >  } NvmeParams;
> >  
> >  typedef struct NvmeAsyncEvent {
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index ea26d612374d..67990232528c 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -34,6 +34,7 @@ GlobalProperty hw_compat_5_1[] = {
> >      { "vhost-user-scsi", "num_queues", "1"},
> >      { "virtio-blk-device", "num-queues", "1"},
> >      { "virtio-scsi-device", "num_queues", "1"},
> > +    { "nvme", "x-use-intel-id", "on"},
> >  };
> >  const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);
> >  
> > 
>
Klaus Jensen Sept. 7, 2020, 10:50 a.m. UTC | #8
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!
Dr. David Alan Gilbert Sept. 7, 2020, 10:52 a.m. UTC | #9
* 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.

Dave
Klaus Jensen Sept. 7, 2020, 11:02 a.m. UTC | #10
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?
Keith Busch Sept. 8, 2020, 3:39 p.m. UTC | #11
On Mon, Sep 07, 2020 at 01:02:16PM +0200, Klaus Jensen wrote:
> On Sep  7 11:52, Dr. David Alan Gilbert wrote:
>
> > 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?

It's not a real device yet, but it likely will not be an nvme device
once a product does release with this identifier. There may be trouble
with driver binding when that happens, so deprecating it here sooner
than later is my preference.
diff mbox series

Patch

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);
+    } else {
+        pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REDHAT);
+        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REDHAT_NVME);
+    }
+
     pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_EXPRESS);
     pcie_endpoint_cap_init(pci_dev, 0x80);
 
@@ -2903,6 +2912,7 @@  static Property nvme_props[] = {
     DEFINE_PROP_UINT8("aerl", NvmeCtrl, params.aerl, 3),
     DEFINE_PROP_UINT32("aer_max_queued", NvmeCtrl, params.aer_max_queued, 64),
     DEFINE_PROP_UINT8("mdts", NvmeCtrl, params.mdts, 7),
+    DEFINE_PROP_BOOL("x-use-intel-id", NvmeCtrl, params.use_intel_id, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2919,8 +2929,6 @@  static void nvme_class_init(ObjectClass *oc, void *data)
     pc->realize = nvme_realize;
     pc->exit = nvme_exit;
     pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
-    pc->vendor_id = PCI_VENDOR_ID_INTEL;
-    pc->device_id = 0x5845;
     pc->revision = 2;
 
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 72260f2e8ea9..a734a5e1370d 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -15,6 +15,7 @@  typedef struct NvmeParams {
     uint8_t  aerl;
     uint32_t aer_max_queued;
     uint8_t  mdts;
+    bool     use_intel_id;
 } NvmeParams;
 
 typedef struct NvmeAsyncEvent {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index ea26d612374d..67990232528c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,6 +34,7 @@  GlobalProperty hw_compat_5_1[] = {
     { "vhost-user-scsi", "num_queues", "1"},
     { "virtio-blk-device", "num-queues", "1"},
     { "virtio-scsi-device", "num_queues", "1"},
+    { "nvme", "x-use-intel-id", "on"},
 };
 const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);