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

[Ernest Esene]

On Mon, Mar 25, 2019 at 07:42:27PM -0500, Corey Minyard wrote:
> 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/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
> 
Thank you corey.

Ernest

signature.asc
Description: PGP signature