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

[Philippe Mathieu-Daudé]

+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);
>  
>