Re: [Qemu-devel] [PATCH] Categorize devices

[Corey Minyard]

On Sun, Mar 24, 2019 at 07:05:23PM +0100, Ernest Esene wrote:
> Categorize devices in "uncategorised devices" section
> This patch is based on BiteSizedTask.
> 
> Signed-off-by: Ernest Esene <address@hidden>

I'm not 100% sure the use of this field.  A couple
of comments on the IPMI one inline.

> ---
>  hw/dma/i82374.c           | 2 ++
>  hw/i386/amd_iommu.c       | 2 ++
>  hw/i386/intel_iommu.c     | 2 ++
>  hw/i386/pc_piix.c         | 1 +
>  hw/ipmi/ipmi_bmc_extern.c | 2 ++
>  hw/ipmi/ipmi_bmc_sim.c    | 2 ++
>  hw/ipmi/isa_ipmi_bt.c     | 2 ++
>  hw/ipmi/isa_ipmi_kcs.c    | 2 ++
>  hw/mem/nvdimm.c           | 1 +
>  hw/mem/pc-dimm.c          | 1 +
>  hw/tpm/tpm_tis.c          | 3 +++
>  11 files changed, 20 insertions(+)
> 
> diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
> index 892f655a..5b42dd1b 100644
> --- a/hw/dma/i82374.c
> +++ b/hw/dma/i82374.c
> @@ -147,6 +147,8 @@ static void i82374_class_init(ObjectClass *klass, void 
> *data)
>      dc->realize = i82374_realize;
>      dc->vmsd = &vmstate_i82374;
>      dc->props = i82374_properties;
> +    dc->desc = "Intel Enhanced DMA controller";
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>  }
>  
>  static const TypeInfo i82374_info = {
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 6eabdf99..4a4e2c7f 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1601,6 +1601,8 @@ static void amdvi_class_init(ObjectClass *klass, void* 
> data)
>      dc_class->int_remap = amdvi_int_remap;
>      /* Supported by the pc-q35-* machine types */
>      dc->user_creatable = true;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    dc->desc = "AMD IOMMU (AMD-Vi) DMA Remapping device";
>  }
>  
>  static const TypeInfo amdvi = {
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index b90de6c6..4d0e6042 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3702,6 +3702,8 @@ static void vtd_class_init(ObjectClass *klass, void 
> *data)
>      x86_class->int_remap = vtd_int_remap;
>      /* Supported by the pc-q35-* machine types */
>      dc->user_creatable = true;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    dc->desc = "Intel IOMMU (VT-d) DMA Remapping device";
>  }
>  
>  static const TypeInfo vtd_info = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 8ad8e885..03a9cb8a 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -911,6 +911,7 @@ static void isa_bridge_class_init(ObjectClass *klass, 
> void *data)
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
>      dc->desc        = "ISA bridge faked to support IGD PT";
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      k->vendor_id    = PCI_VENDOR_ID_INTEL;
>      k->class_id     = PCI_CLASS_BRIDGE_ISA;
>  };
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index bf0b7ee0..39049c4d 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -526,6 +526,8 @@ static void ipmi_bmc_extern_class_init(ObjectClass *oc, 
> void *data)
>      dc->hotpluggable = false;
>      dc->realize = ipmi_bmc_extern_realize;
>      dc->props = ipmi_bmc_extern_properties;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->desc = "IPMI Baseboard management controller";

This is not exactly a bridge.  None of the categories seem
to fit, though, a management device would be the best
category, but that's not available.  misc is probably the
best.

Also, the description might be betters a: "IPMI external
baseboard management controller" to distinguish it from
the next one...

>  }
>  
>  static const TypeInfo ipmi_bmc_extern_type = {
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 9b509f82..95a096fa 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -2016,6 +2016,8 @@ static void ipmi_sim_class_init(ObjectClass *oc, void 
> *data)
>      dc->realize = ipmi_sim_realize;
>      dc->props = ipmi_sim_properties;
>      bk->handle_command = ipmi_sim_handle_command;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->desc = "IPMI Baseboard management controller";

This is definitely not a bridge, same basic comment as above,
but this is an internal simulator of the device.  For the
description, perhaps: "IPMI simulated baseboard management
controller"

>  }
>  
>  static const TypeInfo ipmi_sim_type = {
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index 8bbb1fa7..9ca3402e 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -541,6 +541,8 @@ static void isa_ipmi_bt_class_init(ObjectClass *oc, void 
> *data)
>  
>      dc->realize = isa_ipmi_bt_realize;
>      dc->props = ipmi_isa_properties;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->desc = "ISA IPMI BT System Interface";

I'm ok with these being bridges, that seem accurate, and the
description looks good.  Same for KCS below.

Thanks,

-corey

>  
>      iic->get_backend_data = isa_ipmi_bt_get_backend_data;
>      ipmi_bt_class_init(iic);
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index a7943155..818d59d1 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -524,6 +524,8 @@ static void isa_ipmi_kcs_class_init(ObjectClass *oc, void 
> *data)
>  
>      dc->realize = ipmi_isa_realize;
>      dc->props = ipmi_isa_properties;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->desc = "ISA IPMI KCS System Interface";
>  
>      iic->get_backend_data = isa_ipmi_kcs_get_backend_data;
>      ipmi_kcs_class_init(iic);
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index bf2adf5e..a334dbe1 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -200,6 +200,7 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
>      ddc->realize = nvdimm_realize;
>      mdc->get_memory_region = nvdimm_md_get_memory_region;
>      dc->props = nvdimm_properties;
> +    dc->desc = "NVDIMM memory module";
>  
>      nvc->read_label_data = nvdimm_read_label_data;
>      nvc->write_label_data = nvdimm_write_label_data;
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 152400b1..19b9c0f4 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -259,6 +259,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void 
> *data)
>      dc->unrealize = pc_dimm_unrealize;
>      dc->props = pc_dimm_properties;
>      dc->desc = "DIMM memory module";
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  
>      ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
>  
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index fd183e8d..36aaaa82 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -1020,6 +1020,9 @@ static void tpm_tis_class_init(ObjectClass *klass, void 
> *data)
>      dc->props = tpm_tis_properties;
>      dc->reset = tpm_tis_reset;
>      dc->vmsd  = &vmstate_tpm_tis;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    dc->desc = "Trusted Platform Module (TPM) TIS Interface";
> +
>      tc->model = TPM_MODEL_TPM_TIS;
>      tc->get_version = tpm_tis_get_tpm_version;
>      tc->request_completed = tpm_tis_request_completed;
> -- 
> 2.14.2
>