[PATCH 00/13] Categorize some uncategorized devices

[PATCH 00/13] Categorize some uncategorized devices

[Gan Qixin]

Hi all,

By running "qemu-system-x86_64 -device help" and "qemu-system-arm -M none
-device help", I found many uncategorized devices. Therefore, I categorized them.

Thanks,
Gan Qixin

Gan Qixin (13):
  pc-dimm: put it into the 'storage' category
  virtio-pmem: put it into the 'storage' category
  vmmouse: put it into the 'input' category
  nvdimm: put it into the 'storage' category
  mc146818rtc: put it into the 'misc' category
  ipmi: put some ipmi devices into the correct category
  tpm: put some tpm devices into the correct category
  AMDVI-PCI: put it into the 'misc' category
  u2f-passthru: put it into the 'usb' category
  tosa-ssp: put it into the 'misc' category
  spitz: put some Spitz-family devices into the correct category
  SPI flash devices: put them into the 'storage' category
  bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc'
    category

 hw/arm/spitz.c            | 2 ++
 hw/arm/tosa.c             | 2 ++
 hw/block/m25p80.c         | 1 +
 hw/i386/amd_iommu.c       | 8 ++++++++
 hw/i386/vmmouse.c         | 1 +
 hw/ipmi/ipmi_bmc_extern.c | 1 +
 hw/ipmi/ipmi_bmc_sim.c    | 1 +
 hw/ipmi/isa_ipmi_bt.c     | 1 +
 hw/ipmi/isa_ipmi_kcs.c    | 1 +
 hw/ipmi/pci_ipmi_bt.c     | 1 +
 hw/ipmi/pci_ipmi_kcs.c    | 1 +
 hw/mem/nvdimm.c           | 1 +
 hw/mem/pc-dimm.c          | 1 +
 hw/misc/bcm2835_cprman.c  | 4 ++++
 hw/rtc/mc146818rtc.c      | 1 +
 hw/tpm/tpm_tis_isa.c      | 1 +
 hw/tpm/tpm_tis_sysbus.c   | 1 +
 hw/usb/u2f-passthru.c     | 1 +
 hw/virtio/virtio-pmem.c   | 1 +
 19 files changed, 31 insertions(+)

-- 
2.23.0

[PATCH 01/13] pc-dimm: put it into the 'storage' category

[Gan Qixin]

The category of the pc-dimm device is not set, put it into the 'storage'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Michael S. Tsirkin <mst@redhat.com>
---
 hw/mem/pc-dimm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 2ffc986734..017146e3d1 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -282,6 +282,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
     mdc->get_plugged_size = memory_device_get_region_size;
     mdc->get_memory_region = pc_dimm_md_get_memory_region;
     mdc->fill_device_info = pc_dimm_md_fill_device_info;
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
 static TypeInfo pc_dimm_info = {
-- 
2.23.0

[PATCH 02/13] virtio-pmem: put it into the 'storage' category

[Gan Qixin]

The category of the virtio-pmem device is not set, put it into the 'storage'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-pmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index ddb0125901..98b3139cd0 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -175,6 +175,7 @@ static void virtio_pmem_class_init(ObjectClass *klass, void *data)
 
     vpc->fill_device_info = virtio_pmem_fill_device_info;
     vpc->get_memory_region = virtio_pmem_get_memory_region;
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
 static TypeInfo virtio_pmem_info = {
-- 
2.23.0

Re: [PATCH 02/13] virtio-pmem: put it into the 'storage' category

[Pankaj Gupta]

> The category of the virtio-pmem device is not set, put it into the 'storage'
> category.
>
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> ---
> Cc: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio-pmem.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
> index ddb0125901..98b3139cd0 100644
> --- a/hw/virtio/virtio-pmem.c
> +++ b/hw/virtio/virtio-pmem.c
> @@ -175,6 +175,7 @@ static void virtio_pmem_class_init(ObjectClass *klass, void *data)
>
>      vpc->fill_device_info = virtio_pmem_fill_device_info;
>      vpc->get_memory_region = virtio_pmem_get_memory_region;
> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
>
>  static TypeInfo virtio_pmem_info = {

Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>

Re: [PATCH 02/13] virtio-pmem: put it into the 'storage' category

[Pankaj Gupta]

> The category of the virtio-pmem device is not set, put it into the 'storage'
> category.
>
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> ---
> Cc: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/virtio/virtio-pmem.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
> index ddb0125901..98b3139cd0 100644
> --- a/hw/virtio/virtio-pmem.c
> +++ b/hw/virtio/virtio-pmem.c
> @@ -175,6 +175,7 @@ static void virtio_pmem_class_init(ObjectClass *klass, void *data)
>
>      vpc->fill_device_info = virtio_pmem_fill_device_info;
>      vpc->get_memory_region = virtio_pmem_get_memory_region;
> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);

Just noticed "virtio_pmem_pci_class_init" is marked as DEVICE_CATEGORY_MISC.
So, both virtio-pmem & virtio_pmem_pci have the same device category?

Thanks,
Pankaj

>  }
>
>  static TypeInfo virtio_pmem_info = {
> --
> 2.23.0
>
>

[PATCH 03/13] vmmouse: put it into the 'input' category

[Gan Qixin]

The category of the vmmouse device is not set, put it into the 'input'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/vmmouse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index a3556438f0..df4798f502 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -308,6 +308,7 @@ static void vmmouse_class_initfn(ObjectClass *klass, void *data)
     dc->reset = vmmouse_reset;
     dc->vmsd = &vmstate_vmmouse;
     device_class_set_props(dc, vmmouse_properties);
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
 static const TypeInfo vmmouse_info = {
-- 
2.23.0

Re: [PATCH 03/13] vmmouse: put it into the 'input' category

[Philippe Mathieu-Daudé]

On 11/15/20 7:48 PM, Gan Qixin wrote:
> The category of the vmmouse device is not set, put it into the 'input'
> category.
> 
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> ---
> Cc: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/vmmouse.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 04/13] nvdimm: put it into the 'storage' category

[Gan Qixin]

The category of the nvdimm device is not set, put it into the 'storage'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>
---
 hw/mem/nvdimm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index e1574bc07c..e30695b2ce 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -236,6 +236,7 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
 
     nvc->read_label_data = nvdimm_read_label_data;
     nvc->write_label_data = nvdimm_write_label_data;
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
 static TypeInfo nvdimm_info = {
-- 
2.23.0

[PATCH 05/13] mc146818rtc: put it into the 'misc' category

[Gan Qixin]

The category of the mc146818rtc device is not set, put it into the 'misc'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Michael S. Tsirkin <mst@redhat.com>
---
 hw/rtc/mc146818rtc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index 7a38540cb9..39edca0996 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -1039,6 +1039,7 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_rtc;
     isa->build_aml = rtc_build_aml;
     device_class_set_props(dc, mc146818rtc_properties);
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo mc146818rtc_info = {
-- 
2.23.0

[PATCH 06/13] ipmi: put some ipmi devices into the correct category

[Gan Qixin]

Some ipmi devices have no category, put them into the correct category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Corey Minyard <minyard@acm.org>
---
 hw/ipmi/ipmi_bmc_extern.c | 1 +
 hw/ipmi/ipmi_bmc_sim.c    | 1 +
 hw/ipmi/isa_ipmi_bt.c     | 1 +
 hw/ipmi/isa_ipmi_kcs.c    | 1 +
 hw/ipmi/pci_ipmi_bt.c     | 1 +
 hw/ipmi/pci_ipmi_kcs.c    | 1 +
 6 files changed, 6 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index c3f3306e66..aa8faf9a9b 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -529,6 +529,7 @@ static void ipmi_bmc_extern_class_init(ObjectClass *oc, void *data)
     dc->hotpluggable = false;
     dc->realize = ipmi_bmc_extern_realize;
     device_class_set_props(dc, ipmi_bmc_extern_properties);
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo ipmi_bmc_extern_type = {
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index f78e92d3d5..611650f79e 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -2214,6 +2214,7 @@ static void ipmi_sim_class_init(ObjectClass *oc, void *data)
     dc->realize = ipmi_sim_realize;
     device_class_set_props(dc, ipmi_sim_properties);
     bk->handle_command = ipmi_sim_handle_command;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo ipmi_sim_type = {
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index b7c2ad557b..7fc6022f13 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -152,6 +152,7 @@ static void isa_ipmi_bt_class_init(ObjectClass *oc, void *data)
     iic->get_backend_data = isa_ipmi_bt_get_backend_data;
     ipmi_bt_class_init(iic);
     iic->get_fwinfo = isa_ipmi_bt_get_fwinfo;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
 static const TypeInfo isa_ipmi_bt_info = {
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index 7dd6bf0040..b50901fc3b 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -159,6 +159,7 @@ static void isa_ipmi_kcs_class_init(ObjectClass *oc, void *data)
     iic->get_backend_data = isa_ipmi_kcs_get_backend_data;
     ipmi_kcs_class_init(iic);
     iic->get_fwinfo = isa_ipmi_kcs_get_fwinfo;
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
 static const TypeInfo isa_ipmi_kcs_info = {
diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
index b6e52730d3..7d7435719a 100644
--- a/hw/ipmi/pci_ipmi_bt.c
+++ b/hw/ipmi/pci_ipmi_bt.c
@@ -125,6 +125,7 @@ static void pci_ipmi_bt_class_init(ObjectClass *oc, void *data)
 
     iic->get_backend_data = pci_ipmi_bt_get_backend_data;
     ipmi_bt_class_init(iic);
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
 static const TypeInfo pci_ipmi_bt_info = {
diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
index de13418862..0f1a1e4098 100644
--- a/hw/ipmi/pci_ipmi_kcs.c
+++ b/hw/ipmi/pci_ipmi_kcs.c
@@ -125,6 +125,7 @@ static void pci_ipmi_kcs_class_init(ObjectClass *oc, void *data)
 
     iic->get_backend_data = pci_ipmi_kcs_get_backend_data;
     ipmi_kcs_class_init(iic);
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
 static const TypeInfo pci_ipmi_kcs_info = {
-- 
2.23.0

[PATCH 07/13] tpm: put some tpm devices into the correct category

[Gan Qixin]

Some tpm devices have no category, put them into the correct category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/tpm/tpm_tis_isa.c    | 1 +
 hw/tpm/tpm_tis_sysbus.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
index 6fd876eebf..10d8a14f19 100644
--- a/hw/tpm/tpm_tis_isa.c
+++ b/hw/tpm/tpm_tis_isa.c
@@ -150,6 +150,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, void *data)
     dc->reset = tpm_tis_isa_reset;
     tc->request_completed = tpm_tis_isa_request_completed;
     tc->get_version = tpm_tis_isa_get_tpm_version;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo tpm_tis_isa_info = {
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 2c32aa7099..45e63efd63 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -139,6 +139,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
     dc->reset = tpm_tis_sysbus_reset;
     tc->request_completed = tpm_tis_sysbus_request_completed;
     tc->get_version = tpm_tis_sysbus_get_tpm_version;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo tpm_tis_sysbus_info = {
-- 
2.23.0

[PATCH 08/13] AMDVI-PCI: put it into the 'misc' category

[Gan Qixin]

The category of the AMDVI-PCI device is not set, put it into the 'misc'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/amd_iommu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 74a93a5d93..1d4e488f7f 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1621,6 +1621,13 @@ static const TypeInfo amdvi = {
     .class_init = amdvi_class_init
 };
 
+static void amdvi_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
 static const TypeInfo amdviPCI = {
     .name = TYPE_AMD_IOMMU_PCI,
     .parent = TYPE_PCI_DEVICE,
@@ -1629,6 +1636,7 @@ static const TypeInfo amdviPCI = {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
         { },
     },
+    .class_init = amdvi_pci_class_init
 };
 
 static void amdvi_iommu_memory_region_class_init(ObjectClass *klass, void *data)
-- 
2.23.0

[PATCH 09/13] u2f-passthru: put it into the 'usb' category

[Gan Qixin]

The category of the u2f-passthru device is not set, put it into the 'usb'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/u2f-passthru.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/u2f-passthru.c b/hw/usb/u2f-passthru.c
index ae00e93f35..000822f396 100644
--- a/hw/usb/u2f-passthru.c
+++ b/hw/usb/u2f-passthru.c
@@ -534,6 +534,7 @@ static void u2f_passthru_class_init(ObjectClass *klass, void *data)
     dc->desc = "QEMU U2F passthrough key";
     dc->vmsd = &u2f_passthru_vmstate;
     device_class_set_props(dc, u2f_passthru_properties);
+    set_bit(DEVICE_CATEGORY_USB, dc->categories);
 }
 
 static const TypeInfo u2f_key_passthru_info = {
-- 
2.23.0

Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category

[Philippe Mathieu-Daudé]

On 11/15/20 7:48 PM, Gan Qixin wrote:
> The category of the u2f-passthru device is not set, put it into the 'usb'
> category.

I guess we discussed this with Thomas 1 or 2 years ago
but I don't remember. I think it was about using set_bits()
so devices can appear in multiple categories.

Gerd, do you know what is the point of the "usb" category for
management apps? This is a bus accepting multiple better
categorized devices (display, storage, input, network, sound).

Unrelated but multiple devices are related to SECURITY.
Maybe it is time to introduce the DEVICE_CATEGORY_SECURITY?

> 
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> ---
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/u2f-passthru.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/usb/u2f-passthru.c b/hw/usb/u2f-passthru.c
> index ae00e93f35..000822f396 100644
> --- a/hw/usb/u2f-passthru.c
> +++ b/hw/usb/u2f-passthru.c
> @@ -534,6 +534,7 @@ static void u2f_passthru_class_init(ObjectClass *klass, void *data)
>      dc->desc = "QEMU U2F passthrough key";
>      dc->vmsd = &u2f_passthru_vmstate;
>      device_class_set_props(dc, u2f_passthru_properties);
> +    set_bit(DEVICE_CATEGORY_USB, dc->categories);
>  }
>  
>  static const TypeInfo u2f_key_passthru_info = {
> 

Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category

[Gerd Hoffmann]

On Mon, Nov 16, 2020 at 03:04:52PM +0100, Philippe Mathieu-Daudé wrote:
> On 11/15/20 7:48 PM, Gan Qixin wrote:
> > The category of the u2f-passthru device is not set, put it into the 'usb'
> > category.
> 
> I guess we discussed this with Thomas 1 or 2 years ago
> but I don't remember. I think it was about using set_bits()
> so devices can appear in multiple categories.
> 
> Gerd, do you know what is the point of the "usb" category for
> management apps? This is a bus accepting multiple better
> categorized devices (display, storage, input, network, sound).

Right now we have the host adapters collected in the usb category.
Maybe we should rename the category to make that clear.

And, yes, tagging the u2f devices as "usb" doesn't look useful.

> Unrelated but multiple devices are related to SECURITY.
> Maybe it is time to introduce the DEVICE_CATEGORY_SECURITY?

Either that, or place it in the "misc" category.

take care,
  Gerd

RE: [PATCH 09/13] u2f-passthru: put it into the 'usb' category

[ganqixin]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Tuesday, November 17, 2020 2:59 PM
> To: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: ganqixin <ganqixin@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org; Marc-André Lureau
> <marcandre.lureau@redhat.com>; thuth@redhat.com; Chenqun (kuhn)
> <kuhn.chenqun@huawei.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; Eduardo Habkost
> <ehabkost@redhat.com>
> Subject: Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category
> 
> On Mon, Nov 16, 2020 at 03:04:52PM +0100, Philippe Mathieu-Daudé
> wrote:
> > On 11/15/20 7:48 PM, Gan Qixin wrote:
> > > The category of the u2f-passthru device is not set, put it into the 'usb'
> > > category.
> >
> > I guess we discussed this with Thomas 1 or 2 years ago but I don't
> > remember. I think it was about using set_bits() so devices can appear
> > in multiple categories.
> >
> > Gerd, do you know what is the point of the "usb" category for
> > management apps? This is a bus accepting multiple better categorized
> > devices (display, storage, input, network, sound).
> 
> Right now we have the host adapters collected in the usb category.
> Maybe we should rename the category to make that clear.
> 
> And, yes, tagging the u2f devices as "usb" doesn't look useful.
> 
> > Unrelated but multiple devices are related to SECURITY.
> > Maybe it is time to introduce the DEVICE_CATEGORY_SECURITY?
> 
> Either that, or place it in the "misc" category.

Thanks for your advice, I will put it into 'misc' category.

Gan Qixin

RE: [PATCH 09/13] u2f-passthru: put it into the 'usb' category

[ganqixin]

> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> Sent: Monday, November 16, 2020 10:05 PM
> To: ganqixin <ganqixin@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org; Marc-André Lureau
> <marcandre.lureau@redhat.com>; thuth@redhat.com
> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Eduardo Habkost <ehabkost@redhat.com>
> Subject: Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category
> 
> On 11/15/20 7:48 PM, Gan Qixin wrote:
> > The category of the u2f-passthru device is not set, put it into the 'usb'
> > category.
> 
> I guess we discussed this with Thomas 1 or 2 years ago but I don't remember.
> I think it was about using set_bits() so devices can appear in multiple
> categories.
> 
> Gerd, do you know what is the point of the "usb" category for management
> apps? This is a bus accepting multiple better categorized devices (display,
> storage, input, network, sound).
> 
> Unrelated but multiple devices are related to SECURITY.
> Maybe it is time to introduce the DEVICE_CATEGORY_SECURITY?

Hi, Philippe
Thanks for your reply, but I'm not sure if it is appropriate to add a security category to place this device.
(Maybe it's because I don't know much about these device, I haven't find many safety-related devices in 'misc' category or uncategorized devices)

Thanks,
Gan Qixin

Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category

[Philippe Mathieu-Daudé]

On 11/17/20 2:37 PM, ganqixin wrote:
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>> Sent: Monday, November 16, 2020 10:05 PM
>> To: ganqixin <ganqixin@huawei.com>; qemu-devel@nongnu.org;
>> qemu-trivial@nongnu.org; Marc-André Lureau
>> <marcandre.lureau@redhat.com>; thuth@redhat.com
>> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
>> <zhang.zhanghailiang@huawei.com>; Gerd Hoffmann <kraxel@redhat.com>;
>> Eduardo Habkost <ehabkost@redhat.com>
>> Subject: Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category
>>
>> On 11/15/20 7:48 PM, Gan Qixin wrote:
>>> The category of the u2f-passthru device is not set, put it into the 'usb'
>>> category.
>>
>> I guess we discussed this with Thomas 1 or 2 years ago but I don't remember.
>> I think it was about using set_bits() so devices can appear in multiple
>> categories.
>>
>> Gerd, do you know what is the point of the "usb" category for management
>> apps? This is a bus accepting multiple better categorized devices (display,
>> storage, input, network, sound).
>>
>> Unrelated but multiple devices are related to SECURITY.
>> Maybe it is time to introduce the DEVICE_CATEGORY_SECURITY?
> 
> Hi, Philippe
> Thanks for your reply, but I'm not sure if it is appropriate to add a security category to place this device.
> (Maybe it's because I don't know much about these device, I haven't find many safety-related devices in 'misc' category or uncategorized devices)

What is the difference between 'misc' and 'uncategorized'?

> 
> Thanks,
> Gan Qixin
> 

RE: [PATCH 09/13] u2f-passthru: put it into the 'usb' category

[ganqixin]

> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> Sent: Friday, December 4, 2020 9:50 PM
> To: ganqixin <ganqixin@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; Gerd Hoffmann
> <kraxel@redhat.com>; thuth@redhat.com
> Subject: Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category
> 
> On 11/17/20 2:37 PM, ganqixin wrote:
> >> -----Original Message-----
> >> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> >> Sent: Monday, November 16, 2020 10:05 PM
> >> To: ganqixin <ganqixin@huawei.com>; qemu-devel@nongnu.org;
> >> qemu-trivial@nongnu.org; Marc-André Lureau
> >> <marcandre.lureau@redhat.com>; thuth@redhat.com
> >> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
> >> <zhang.zhanghailiang@huawei.com>; Gerd Hoffmann
> <kraxel@redhat.com>;
> >> Eduardo Habkost <ehabkost@redhat.com>
> >> Subject: Re: [PATCH 09/13] u2f-passthru: put it into the 'usb'
> >> category
> >>
> >> On 11/15/20 7:48 PM, Gan Qixin wrote:
> >>> The category of the u2f-passthru device is not set, put it into the 'usb'
> >>> category.
> >>
> >> I guess we discussed this with Thomas 1 or 2 years ago but I don't
> remember.
> >> I think it was about using set_bits() so devices can appear in
> >> multiple categories.
> >>
> >> Gerd, do you know what is the point of the "usb" category for
> >> management apps? This is a bus accepting multiple better categorized
> >> devices (display, storage, input, network, sound).
> >>
> >> Unrelated but multiple devices are related to SECURITY.
> >> Maybe it is time to introduce the DEVICE_CATEGORY_SECURITY?
> >
> > Hi, Philippe
> > Thanks for your reply, but I'm not sure if it is appropriate to add a
> security category to place this device.
> > (Maybe it's because I don't know much about these device, I haven't
> > find many safety-related devices in 'misc' category or uncategorized
> > devices)
> 
> What is the difference between 'misc' and 'uncategorized'?

In my opinion, devices that we forgot to classify will appear in "uncategorized" list, and devices that don't have a suitable category to choose will be put into "misc" category.

> 
> >
> > Thanks,
> > Gan Qixin
> >

Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category

[Thomas Huth]

On 07/12/2020 09.05, ganqixin wrote:
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>> Sent: Friday, December 4, 2020 9:50 PM
>> To: ganqixin <ganqixin@huawei.com>; qemu-devel@nongnu.org;
>> qemu-trivial@nongnu.org
>> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
>> <zhang.zhanghailiang@huawei.com>; Gerd Hoffmann
>> <kraxel@redhat.com>; thuth@redhat.com
>> Subject: Re: [PATCH 09/13] u2f-passthru: put it into the 'usb' category
>>
>> On 11/17/20 2:37 PM, ganqixin wrote:
>>>> -----Original Message-----
>>>> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
>>>> Sent: Monday, November 16, 2020 10:05 PM
>>>> To: ganqixin <ganqixin@huawei.com>; qemu-devel@nongnu.org;
>>>> qemu-trivial@nongnu.org; Marc-André Lureau
>>>> <marcandre.lureau@redhat.com>; thuth@redhat.com
>>>> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
>>>> <zhang.zhanghailiang@huawei.com>; Gerd Hoffmann
>> <kraxel@redhat.com>;
>>>> Eduardo Habkost <ehabkost@redhat.com>
>>>> Subject: Re: [PATCH 09/13] u2f-passthru: put it into the 'usb'
>>>> category
>>>>
>>>> On 11/15/20 7:48 PM, Gan Qixin wrote:
>>>>> The category of the u2f-passthru device is not set, put it into the 'usb'
>>>>> category.
>>>>
>>>> I guess we discussed this with Thomas 1 or 2 years ago but I don't
>> remember.
>>>> I think it was about using set_bits() so devices can appear in
>>>> multiple categories.
>>>>
>>>> Gerd, do you know what is the point of the "usb" category for
>>>> management apps? This is a bus accepting multiple better categorized
>>>> devices (display, storage, input, network, sound).
>>>>
>>>> Unrelated but multiple devices are related to SECURITY.
>>>> Maybe it is time to introduce the DEVICE_CATEGORY_SECURITY?
>>>
>>> Hi, Philippe
>>> Thanks for your reply, but I'm not sure if it is appropriate to add a
>> security category to place this device.
>>> (Maybe it's because I don't know much about these device, I haven't
>>> find many safety-related devices in 'misc' category or uncategorized
>>> devices)
>>
>> What is the difference between 'misc' and 'uncategorized'?
> 
> In my opinion, devices that we forgot to classify will appear in "uncategorized" list, and devices that don't have a suitable category to choose will be put into "misc" category.

That's right. User-creatable devices should have a category, and the
uncategorized devices just have not been categorized yet.

So thanks for tackling this, Gan Qixin!

 Thomas

[PATCH 10/13] tosa-ssp: put it into the 'misc' category

[Gan Qixin]

The category of the tosa-ssp device is not set, put it into the 'misc'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/tosa.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index fe88ed89fe..0e3461c8ed 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -292,10 +292,12 @@ static const TypeInfo tosa_dac_info = {
 
 static void tosa_ssp_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
 
     k->realize = tosa_ssp_realize;
     k->transfer = tosa_ssp_tansfer;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo tosa_ssp_info = {
-- 
2.23.0

[PATCH 11/13] spitz: put some Spitz-family devices into the correct category

[Gan Qixin]

Some Spitz-family devices have no category, put them into the correct category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/spitz.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 32bdeacfd3..0e5e8a4634 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -1218,6 +1218,7 @@ static void corgi_ssp_class_init(ObjectClass *klass, void *data)
     k->realize = corgi_ssp_realize;
     k->transfer = corgi_ssp_transfer;
     dc->vmsd = &vmstate_corgi_ssp_regs;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo corgi_ssp_info = {
@@ -1247,6 +1248,7 @@ static void spitz_lcdtg_class_init(ObjectClass *klass, void *data)
     k->realize = spitz_lcdtg_realize;
     k->transfer = spitz_lcdtg_transfer;
     dc->vmsd = &vmstate_spitz_lcdtg_regs;
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 }
 
 static const TypeInfo spitz_lcdtg_info = {
-- 
2.23.0

[PATCH 12/13] SPI flash devices: put them into the 'storage' category

[Gan Qixin]

SPI flash devices have no category, put them into the 'storage' category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Alistair Francis <alistair@alistair23.me>
---
 hw/block/m25p80.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 483925f57a..8dd3ef8559 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1397,6 +1397,7 @@ static void m25p80_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, m25p80_properties);
     dc->reset = m25p80_reset;
     mc->pi = data;
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
 static const TypeInfo m25p80_info = {
-- 
2.23.0

Re: [PATCH 12/13] SPI flash devices: put them into the 'storage' category

[Philippe Mathieu-Daudé]

On 11/15/20 7:49 PM, Gan Qixin wrote:
> SPI flash devices have no category, put them into the 'storage' category.
> 
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> ---
> Cc: Alistair Francis <alistair@alistair23.me>
> ---
>  hw/block/m25p80.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 12/13] SPI flash devices: put them into the 'storage' category

[Alistair Francis]

On Mon, Nov 16, 2020 at 1:30 AM Gan Qixin <ganqixin@huawei.com> wrote:
>
> SPI flash devices have no category, put them into the 'storage' category.
>
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Cc: Alistair Francis <alistair@alistair23.me>
> ---
>  hw/block/m25p80.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 483925f57a..8dd3ef8559 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -1397,6 +1397,7 @@ static void m25p80_class_init(ObjectClass *klass, void *data)
>      device_class_set_props(dc, m25p80_properties);
>      dc->reset = m25p80_reset;
>      mc->pi = data;
> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
>
>  static const TypeInfo m25p80_info = {
> --
> 2.23.0
>
>

[PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Gan Qixin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 1490 bytes --]

Some peripherals of bcm2835 cprman have no category, put them into the 'misc'
category.

Signed-off-by: Gan Qixin <ganqixin@huawei.com>
---
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/bcm2835_cprman.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
index 7e415a017c..c62958a99e 100644
--- a/hw/misc/bcm2835_cprman.c
+++ b/hw/misc/bcm2835_cprman.c
@@ -136,6 +136,7 @@ static void pll_class_init(ObjectClass *klass, void *data)
 
     dc->reset = pll_reset;
     dc->vmsd = &pll_vmstate;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo cprman_pll_info = {
@@ -239,6 +240,7 @@ static void pll_channel_class_init(ObjectClass *klass, void *data)
 
     dc->reset = pll_channel_reset;
     dc->vmsd = &pll_channel_vmstate;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo cprman_pll_channel_info = {
@@ -359,6 +361,7 @@ static void clock_mux_class_init(ObjectClass *klass, void *data)
 
     dc->reset = clock_mux_reset;
     dc->vmsd = &clock_mux_vmstate;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo cprman_clock_mux_info = {
@@ -411,6 +414,7 @@ static void dsi0hsck_mux_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &dsi0hsck_mux_vmstate;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
 }
 
 static const TypeInfo cprman_dsi0hsck_mux_info = {
-- 
2.23.0

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Philippe Mathieu-Daudé]

Hi Gan,

On 11/15/20 7:49 PM, Gan Qixin wrote:
> Some peripherals of bcm2835 cprman have no category, put them into the 'misc'
> category.
> 
> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> ---
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/misc/bcm2835_cprman.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> index 7e415a017c..c62958a99e 100644
> --- a/hw/misc/bcm2835_cprman.c
> +++ b/hw/misc/bcm2835_cprman.c
> @@ -136,6 +136,7 @@ static void pll_class_init(ObjectClass *klass, void *data)
>  
>      dc->reset = pll_reset;
>      dc->vmsd = &pll_vmstate;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);

Well, this is not an usable device but a part of a bigger device,
so here we want the opposite: not list this device in any category.

Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM
types so management apps can filter them out? (And so we are sure
all QOM is classified).

Thomas, you already dealt with categorizing devices in the past,
what do you think about this? Who else could help? Maybe add
someone from libvirt in the thread?

>  }
>  
>  static const TypeInfo cprman_pll_info = {
> @@ -239,6 +240,7 @@ static void pll_channel_class_init(ObjectClass *klass, void *data)
>  
>      dc->reset = pll_channel_reset;
>      dc->vmsd = &pll_channel_vmstate;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
>  static const TypeInfo cprman_pll_channel_info = {
> @@ -359,6 +361,7 @@ static void clock_mux_class_init(ObjectClass *klass, void *data)
>  
>      dc->reset = clock_mux_reset;
>      dc->vmsd = &clock_mux_vmstate;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
>  static const TypeInfo cprman_clock_mux_info = {
> @@ -411,6 +414,7 @@ static void dsi0hsck_mux_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->vmsd = &dsi0hsck_mux_vmstate;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>  }
>  
>  static const TypeInfo cprman_dsi0hsck_mux_info = {
> 

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Peter Maydell]

On Mon, 16 Nov 2020 at 13:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Well, this is not an usable device but a part of a bigger device,
> so here we want the opposite: not list this device in any category.
>
> Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM
> types so management apps can filter them out? (And so we are sure
> all QOM is classified).
>
> Thomas, you already dealt with categorizing devices in the past,
> what do you think about this? Who else could help? Maybe add
> someone from libvirt in the thread?

If we could get to the point where we can assert() that
dc->categories is non-zero in class init, we would be able
to avoid further "forgot to categorize device" bugs getting
into the tree in future, which seems like an argument for
having some way of marking "really just an implementation
detail" devices I guess?

thanks
-- PMM

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Thomas Huth]

On 16/11/2020 14.31, Peter Maydell wrote:
> On Mon, 16 Nov 2020 at 13:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Well, this is not an usable device but a part of a bigger device,
>> so here we want the opposite: not list this device in any category.
>>
>> Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM
>> types so management apps can filter them out? (And so we are sure
>> all QOM is classified).
>>
>> Thomas, you already dealt with categorizing devices in the past,
>> what do you think about this? Who else could help? Maybe add
>> someone from libvirt in the thread?
> 
> If we could get to the point where we can assert() that
> dc->categories is non-zero in class init, we would be able
> to avoid further "forgot to categorize device" bugs getting
> into the tree in future, which seems like an argument for
> having some way of marking "really just an implementation
> detail" devices I guess?

IMHO we need:

  assert(dc->user_creatable == false ||  categories != 0)

then we don't need something like DEVICE_CATEGORY_COMPOSITE.

 Thomas

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Thomas Huth]

On 16/11/2020 14.25, Philippe Mathieu-Daudé wrote:
> Hi Gan,
> 
> On 11/15/20 7:49 PM, Gan Qixin wrote:
>> Some peripherals of bcm2835 cprman have no category, put them into the 'misc'
>> category.
>>
>> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
>> ---
>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/misc/bcm2835_cprman.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
>> index 7e415a017c..c62958a99e 100644
>> --- a/hw/misc/bcm2835_cprman.c
>> +++ b/hw/misc/bcm2835_cprman.c
>> @@ -136,6 +136,7 @@ static void pll_class_init(ObjectClass *klass, void *data)
>>  
>>      dc->reset = pll_reset;
>>      dc->vmsd = &pll_vmstate;
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> 
> Well, this is not an usable device but a part of a bigger device,
> so here we want the opposite: not list this device in any category.
> 
> Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM
> types so management apps can filter them out? (And so we are sure
> all QOM is classified).
> 
> Thomas, you already dealt with categorizing devices in the past,
> what do you think about this? Who else could help? Maybe add
> someone from libvirt in the thread?

My 0.02 € : Mark the device as user_creatable = false if it can not really
be used by the user with the -device CLI parameter. Then it also does not
need a category. I know Markus will likely have a different opinion, but in
my eyes it's just ugly if we present devices to the users that they can not use.
(By the way, this device here seems to be a decendant of TYPE_SYS_BUS_DEVICE
... shouldn't these show up as user_creatable = false automatically?)

 Thomas

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 16/11/2020 14.25, Philippe Mathieu-Daudé wrote:
>> Hi Gan,
>> 
>> On 11/15/20 7:49 PM, Gan Qixin wrote:
>>> Some peripherals of bcm2835 cprman have no category, put them into the 'misc'
>>> category.
>>>
>>> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
>>> ---
>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/misc/bcm2835_cprman.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
>>> index 7e415a017c..c62958a99e 100644
>>> --- a/hw/misc/bcm2835_cprman.c
>>> +++ b/hw/misc/bcm2835_cprman.c
>>> @@ -136,6 +136,7 @@ static void pll_class_init(ObjectClass *klass, void *data)
>>>  
>>>      dc->reset = pll_reset;
>>>      dc->vmsd = &pll_vmstate;
>>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> 
>> Well, this is not an usable device but a part of a bigger device,
>> so here we want the opposite: not list this device in any category.
>> 
>> Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM
>> types so management apps can filter them out? (And so we are sure
>> all QOM is classified).
>> 
>> Thomas, you already dealt with categorizing devices in the past,
>> what do you think about this? Who else could help? Maybe add
>> someone from libvirt in the thread?
>
> My 0.02 € : Mark the device as user_creatable = false if it can not really
> be used by the user with the -device CLI parameter. Then it also does not
> need a category. I know Markus will likely have a different opinion, but in

You're hurting my feelings!  ;-P

> my eyes it's just ugly if we present devices to the users that they can not use.

If we believe a device should only ever be used from C, then we should
keep it away from the UI.

However, I'm wary of overloading user_creatable.  Even though it has
shifted shape a number of times (cannot_instantiate_with_device_add_yet,
no_user, and now user_creatable), its purpose has always been focused:
distinguishing devices that can be instantiated by generic code from the
ones that need device-specific code.  See user_creatable's comment in
qdev-core.h.

I don't want to lose that distinction.  That's all.

> (By the way, this device here seems to be a decendant of TYPE_SYS_BUS_DEVICE
> ... shouldn't these show up as user_creatable = false automatically?)

Yes, unless it is a dynamic sysbus device (which I consider a flawed
concept).

But TYPE_CPRMAN_PLL is *not* a descendant of TYPE_SYS_BUS_DEVICE, it's a
bus-less device:

    static const TypeInfo cprman_pll_info = {
        .name = TYPE_CPRMAN_PLL,
--->    .parent = TYPE_DEVICE,
        .instance_size = sizeof(CprmanPllState),
        .class_init = pll_class_init,
        .instance_init = pll_init,
    };

Unless bus-less devices are somehow usable with -device, they should
have user_creatable = false.

qdev_device_add() looks like a bus-less device is usable if the machine
provides a hotplug handler for it.  Commit 03fcbd9dc5 "qdev: Check for
the availability of a hotplug controller before adding a device" seems
to be pertinent.

Are there any hotplug handlers for this device?  If yes, which machines
provide one?

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Peter Maydell]

On Mon, 16 Nov 2020 at 17:09, Markus Armbruster <armbru@redhat.com> wrote:
> But TYPE_CPRMAN_PLL is *not* a descendant of TYPE_SYS_BUS_DEVICE, it's a
> bus-less device:
>
>     static const TypeInfo cprman_pll_info = {
>         .name = TYPE_CPRMAN_PLL,
> --->    .parent = TYPE_DEVICE,
>         .instance_size = sizeof(CprmanPllState),
>         .class_init = pll_class_init,
>         .instance_init = pll_init,
>     };

I'm really dubious of devices that directly inherit from
TYPE_DEVICE, because their reset method won't be automatically
called. In this case it looks like the TYPE_BCM2835_CPRMAN
device's reset method manually calls reset on these devices,
though, so it isn't actually buggy, just confusing.

thanks
-- PMM

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 16 Nov 2020 at 17:09, Markus Armbruster <armbru@redhat.com> wrote:
>> But TYPE_CPRMAN_PLL is *not* a descendant of TYPE_SYS_BUS_DEVICE, it's a
>> bus-less device:
>>
>>     static const TypeInfo cprman_pll_info = {
>>         .name = TYPE_CPRMAN_PLL,
>> --->    .parent = TYPE_DEVICE,
>>         .instance_size = sizeof(CprmanPllState),
>>         .class_init = pll_class_init,
>>         .instance_init = pll_init,
>>     };
>
> I'm really dubious of devices that directly inherit from
> TYPE_DEVICE, because their reset method won't be automatically
> called. In this case it looks like the TYPE_BCM2835_CPRMAN
> device's reset method manually calls reset on these devices,
> though, so it isn't actually buggy, just confusing.

I guess this is a trap for unwary implementers, aggravated by our usual
dearth of qdev documentation.  I can see ~100 .parent = TYPE_DEVICE
lines.  Checking them all manually won't be fun.  Any automation ideas?

The concept "bus-less device" is sane.  We used not to have it, and the
resulting need for pseudo-busses was annoying enough to make us add
bus-less devices.

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Peter Maydell]

On Tue, 17 Nov 2020 at 05:41, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
> > I'm really dubious of devices that directly inherit from
> > TYPE_DEVICE, because their reset method won't be automatically
> > called. In this case it looks like the TYPE_BCM2835_CPRMAN
> > device's reset method manually calls reset on these devices,
> > though, so it isn't actually buggy, just confusing.
>
> I guess this is a trap for unwary implementers, aggravated by our usual
> dearth of qdev documentation.  I can see ~100 .parent = TYPE_DEVICE
> lines.  Checking them all manually won't be fun.  Any automation ideas?
>
> The concept "bus-less device" is sane.  We used not to have it, and the
> resulting need for pseudo-busses was annoying enough to make us add
> bus-less devices.

Yeah, the problem really is that our reset handling remains a
mess. I'm not sure (a) what the right model for reset is and
(b) what would be a feasible transition plan to get from
here to there...

thanks
-- PMM

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Thomas Huth]

On 16/11/2020 18.00, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 16/11/2020 14.25, Philippe Mathieu-Daudé wrote:
>>> Hi Gan,
>>>
>>> On 11/15/20 7:49 PM, Gan Qixin wrote:
>>>> Some peripherals of bcm2835 cprman have no category, put them into the 'misc'
>>>> category.
>>>>
>>>> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
>>>> ---
>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  hw/misc/bcm2835_cprman.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
>>>> index 7e415a017c..c62958a99e 100644
>>>> --- a/hw/misc/bcm2835_cprman.c
>>>> +++ b/hw/misc/bcm2835_cprman.c
>>>> @@ -136,6 +136,7 @@ static void pll_class_init(ObjectClass *klass, void *data)
>>>>  
>>>>      dc->reset = pll_reset;
>>>>      dc->vmsd = &pll_vmstate;
>>>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>
>>> Well, this is not an usable device but a part of a bigger device,
>>> so here we want the opposite: not list this device in any category.
>>>
>>> Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM
>>> types so management apps can filter them out? (And so we are sure
>>> all QOM is classified).
>>>
>>> Thomas, you already dealt with categorizing devices in the past,
>>> what do you think about this? Who else could help? Maybe add
>>> someone from libvirt in the thread?
>>
>> My 0.02 € : Mark the device as user_creatable = false if it can not really
>> be used by the user with the -device CLI parameter. Then it also does not
>> need a category. I know Markus will likely have a different opinion, but in
> 
> You're hurting my feelings!  ;-P
> 
>> my eyes it's just ugly if we present devices to the users that they can not use.
> 
> If we believe a device should only ever be used from C, then we should
> keep it away from the UI.
> 
> However, I'm wary of overloading user_creatable.  Even though it has
> shifted shape a number of times (cannot_instantiate_with_device_add_yet,
> no_user, and now user_creatable), its purpose has always been focused:
> distinguishing devices that can be instantiated by generic code from the
> ones that need device-specific code.  See user_creatable's comment in
> qdev-core.h.
> 
> I don't want to lose that distinction.  That's all.

Well, currently we have the user_creatable flag and the hotpluggable flag. I
guess that's simply not enough.

I think in the long run, we should maybe replace the two flags with a
"creatable" type instead that could take the following values:

 CREATABLE_AS_SUBDEVICE  /* Device is part of another device and
                            can only by added by code */
 CREATABLE_BY_QOM        /* Some fancy new QOM function can be
                            used to e.g. create this as part of
                            a machine */
 CREATABLE_BY_COLDPLUG   /* For cold-plugging via -device */
 CREATABLE_BY_HOTPLUG    /* For hot-plugging via device_add */

... but that's likely something for the distant future...

>> (By the way, this device here seems to be a decendant of TYPE_SYS_BUS_DEVICE
>> ... shouldn't these show up as user_creatable = false automatically?)
> 
> Yes, unless it is a dynamic sysbus device (which I consider a flawed
> concept).
> 
> But TYPE_CPRMAN_PLL is *not* a descendant of TYPE_SYS_BUS_DEVICE, it's a
> bus-less device:

Oops, I obviously looked at the wrong device in that file
(TYPE_BCM2835_CPRMAN instead of TYPE_CPRMAN_PLL) - thanks for the clarification!

 Thomas

Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 16/11/2020 18.00, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 16/11/2020 14.25, Philippe Mathieu-Daudé wrote:
>>>> Hi Gan,
>>>>
>>>> On 11/15/20 7:49 PM, Gan Qixin wrote:
>>>>> Some peripherals of bcm2835 cprman have no category, put them into the 'misc'
>>>>> category.
>>>>>
>>>>> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
>>>>> ---
>>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>>  hw/misc/bcm2835_cprman.c | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
>>>>> index 7e415a017c..c62958a99e 100644
>>>>> --- a/hw/misc/bcm2835_cprman.c
>>>>> +++ b/hw/misc/bcm2835_cprman.c
>>>>> @@ -136,6 +136,7 @@ static void pll_class_init(ObjectClass *klass, void *data)
>>>>>  
>>>>>      dc->reset = pll_reset;
>>>>>      dc->vmsd = &pll_vmstate;
>>>>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>>>
>>>> Well, this is not an usable device but a part of a bigger device,
>>>> so here we want the opposite: not list this device in any category.
>>>>
>>>> Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM
>>>> types so management apps can filter them out? (And so we are sure
>>>> all QOM is classified).
>>>>
>>>> Thomas, you already dealt with categorizing devices in the past,
>>>> what do you think about this? Who else could help? Maybe add
>>>> someone from libvirt in the thread?
>>>
>>> My 0.02 € : Mark the device as user_creatable = false if it can not really
>>> be used by the user with the -device CLI parameter. Then it also does not
>>> need a category. I know Markus will likely have a different opinion, but in
>> 
>> You're hurting my feelings!  ;-P
>> 
>>> my eyes it's just ugly if we present devices to the users that they can not use.
>> 
>> If we believe a device should only ever be used from C, then we should
>> keep it away from the UI.
>> 
>> However, I'm wary of overloading user_creatable.  Even though it has
>> shifted shape a number of times (cannot_instantiate_with_device_add_yet,
>> no_user, and now user_creatable), its purpose has always been focused:
>> distinguishing devices that can be instantiated by generic code from the
>> ones that need device-specific code.  See user_creatable's comment in
>> qdev-core.h.
>> 
>> I don't want to lose that distinction.  That's all.
>
> Well, currently we have the user_creatable flag and the hotpluggable flag. I
> guess that's simply not enough.
>
> I think in the long run, we should maybe replace the two flags with a
> "creatable" type instead that could take the following values:
>
>  CREATABLE_AS_SUBDEVICE  /* Device is part of another device and
>                             can only by added by code */
>  CREATABLE_BY_QOM        /* Some fancy new QOM function can be
>                             used to e.g. create this as part of
>                             a machine */
>  CREATABLE_BY_COLDPLUG   /* For cold-plugging via -device */
>  CREATABLE_BY_HOTPLUG    /* For hot-plugging via device_add */

For hot-plug to actually work, both the plug / device and the socket /
bus need to support it.  CREATABLE_BY_HOTPLUG would be about the former.
The latter could depend on bus or machine state.

> ... but that's likely something for the distant future...

The future arrives when the need for it overcomes inertia :)

[...]

Should bus-less devices default to .user_creatable = false? (was: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category)

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

[...]
> qdev_device_add() looks like a bus-less device is usable if the machine
> provides a hotplug handler for it.  Commit 03fcbd9dc5 "qdev: Check for
> the availability of a hotplug controller before adding a device" seems
> to be pertinent.

Nope.  A hotplug handler is only required for hot plug (d'oh!), not for
cold plug.  

I wonder whether bus-less devices should default to .user_creatable =
false like sysbus devices, and for the same reasons.

To actually *do* something, a physical device requires some connection
to the rest of the world.  Same for a virtual device (at least the ones
that model physical devices).

sysbus_device_class_init():

    /*
     * device_add plugs devices into a suitable bus.  For "real" buses,
     * that actually connects the device.  For sysbus, the connections
     * need to be made separately, and device_add can't do that.  The
     * device would be left unconnected, and will probably not work
     *
     * However, a few machines can handle device_add/-device with
     * a few specific sysbus devices. In those cases, the device
     * subclass needs to override it and set user_creatable=true.
     */
    k->user_creatable = false;

The reasoning applies to bus-less devices just as well, doesn't it?

[...]

Re: Should bus-less devices default to .user_creatable = false? (was: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category)

[Thomas Huth]

On 18/11/2020 09.50, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> [...]
>> qdev_device_add() looks like a bus-less device is usable if the machine
>> provides a hotplug handler for it.  Commit 03fcbd9dc5 "qdev: Check for
>> the availability of a hotplug controller before adding a device" seems
>> to be pertinent.
> 
> Nope.  A hotplug handler is only required for hot plug (d'oh!), not for
> cold plug.  
> 
> I wonder whether bus-less devices should default to .user_creatable =
> false like sysbus devices, and for the same reasons.
> 
> To actually *do* something, a physical device requires some connection
> to the rest of the world.  Same for a virtual device (at least the ones
> that model physical devices).

I know at least two virtual devices that are bus-less and cold-pluggable:
hw/ppc/spapr_rng.c and hw/watchdog/wdt_diag288.c ... but we could certainly
mark them with user_creatable = true manually if we decide that bus-less
devices should be handled differently by default.

 Thomas

RE: [PATCH 13/13] bcm2835_cprman: put some peripherals of bcm2835 cprman into the 'misc' category

[ganqixin]

> -----Original Message-----
> From: Thomas Huth [mailto:thuth@redhat.com]
> Sent: Monday, November 16, 2020 10:30 PM
> To: Philippe Mathieu-Daudé <f4bug@amsat.org>; ganqixin
> <ganqixin@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; Markus Armbruster
> <armbru@redhat.com>
> Subject: Re: [PATCH 13/13] bcm2835_cprman: put some peripherals of
> bcm2835 cprman into the 'misc' category
> 
> On 16/11/2020 14.25, Philippe Mathieu-Daudé wrote:
> > Hi Gan,
> >
> > On 11/15/20 7:49 PM, Gan Qixin wrote:
> >> Some peripherals of bcm2835 cprman have no category, put them into the
> 'misc'
> >> category.
> >>
> >> Signed-off-by: Gan Qixin <ganqixin@huawei.com>
> >> ---
> >> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>  hw/misc/bcm2835_cprman.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> >> index 7e415a017c..c62958a99e 100644
> >> --- a/hw/misc/bcm2835_cprman.c
> >> +++ b/hw/misc/bcm2835_cprman.c
> >> @@ -136,6 +136,7 @@ static void pll_class_init(ObjectClass *klass,
> >> void *data)
> >>
> >>      dc->reset = pll_reset;
> >>      dc->vmsd = &pll_vmstate;
> >> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> >
> > Well, this is not an usable device but a part of a bigger device, so
> > here we want the opposite: not list this device in any category.
> >
> > Maybe we could add a DEVICE_CATEGORY_COMPOSITE for all such QOM types
> > so management apps can filter them out? (And so we are sure all QOM is
> > classified).
> >
> > Thomas, you already dealt with categorizing devices in the past, what
> > do you think about this? Who else could help? Maybe add someone from
> > libvirt in the thread?
> 
> My 0.02 € : Mark the device as user_creatable = false if it can not really be used
> by the user with the -device CLI parameter. Then it also does not need a
> category. I know Markus will likely have a different opinion, but in my eyes it's
> just ugly if we present devices to the users that they can not use.
> (By the way, this device here seems to be a decendant of
> TYPE_SYS_BUS_DEVICE ... shouldn't these show up as user_creatable = false
> automatically?)

I agree, but marking user_creatable as false seems to make it deviate from its own meaning (like markus said).
Is there any other way to avoid presenting devices to the users in "-device help"?

Thanks,
Gan Qixin