qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 01/22] ppc/xics: remove set_nr_irqs() handler


From: Cédric Le Goater
Subject: Re: [Qemu-devel] [PATCH v2 01/22] ppc/xics: remove set_nr_irqs() handler from XICSStateClass
Date: Fri, 24 Feb 2017 11:46:12 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0

On 02/22/2017 04:21 AM, David Gibson wrote:
> On Thu, Feb 16, 2017 at 02:47:24PM +0100, Cédric Le Goater wrote:
>> Today, the ICS (Interrupt Controller Source) object is created and
>> realized by the init and realize routines of the XICS object, but some
>> of the parameters are only known at the machine level.
>>
>> These parameters are passed from the sPAPR machine to the ICS object
>> in a rather convoluted way using property handlers and a class handler
>> of the XICS object. The number of irqs required to allocate the IRQ
>> state objects in the ICS realize routine is one of them.
>>
>> Let's simplify the process by creating the ICS object along with the
>> XICS object at the machine level and link the ICS into the XICS list
>> of ICSs at this level also. In the sPAPR machine, there is only a
>> single ICS but that will change with the PowerNV machine.
>>
>> Also, QOMify the creation of the objects and get rid of the
>> superfluous code.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>
>>  Changes since v1:
>>
>>  - added a XICS link to the ICS object
>>
>>  hw/intc/xics.c        | 51 ++++++++++++++++---------------------------------
>>  hw/intc/xics_kvm.c    | 44 ++++++++++--------------------------------
>>  hw/intc/xics_spapr.c  | 34 ---------------------------------
>>  hw/ppc/spapr.c        | 53 
>> +++++++++++++++++++++++++++++++++++----------------
>>  include/hw/ppc/xics.h |  2 --
>>  5 files changed, 63 insertions(+), 121 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 372b8311fb8b..e70d3b8b1095 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -151,38 +151,6 @@ static void xics_common_reset(DeviceState *d)
>>      }
>>  }
>>  
>> -static void xics_prop_get_nr_irqs(Object *obj, Visitor *v, const char *name,
>> -                                  void *opaque, Error **errp)
>> -{
>> -    XICSState *xics = XICS_COMMON(obj);
>> -    int64_t value = xics->nr_irqs;
>> -
>> -    visit_type_int(v, name, &value, errp);
>> -}
>> -
>> -static void xics_prop_set_nr_irqs(Object *obj, Visitor *v, const char *name,
>> -                                  void *opaque, Error **errp)
>> -{
>> -    XICSState *xics = XICS_COMMON(obj);
>> -    XICSStateClass *info = XICS_COMMON_GET_CLASS(xics);
>> -    Error *error = NULL;
>> -    int64_t value;
>> -
>> -    visit_type_int(v, name, &value, &error);
>> -    if (error) {
>> -        error_propagate(errp, error);
>> -        return;
>> -    }
>> -    if (xics->nr_irqs) {
>> -        error_setg(errp, "Number of interrupts is already set to %u",
>> -                   xics->nr_irqs);
>> -        return;
>> -    }
>> -
>> -    assert(info->set_nr_irqs);
>> -    info->set_nr_irqs(xics, value, errp);
>> -}
>> -
>>  void xics_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>>                           const char *typename, Error **errp)
>>  {
>> @@ -241,9 +209,6 @@ static void xics_common_initfn(Object *obj)
>>      XICSState *xics = XICS_COMMON(obj);
>>  
>>      QLIST_INIT(&xics->ics);
>> -    object_property_add(obj, "nr_irqs", "int",
>> -                        xics_prop_get_nr_irqs, xics_prop_set_nr_irqs,
>> -                        NULL, NULL, NULL);
>>      object_property_add(obj, "nr_servers", "int",
>>                          xics_prop_get_nr_servers, xics_prop_set_nr_servers,
>>                          NULL, NULL, NULL);
>> @@ -737,6 +702,16 @@ static void ics_simple_initfn(Object *obj)
>>  static void ics_simple_realize(DeviceState *dev, Error **errp)
>>  {
>>      ICSState *ics = ICS_SIMPLE(dev);
>> +    Object *obj;
>> +    Error *err = NULL;
>> +
>> +    obj = object_property_get_link(OBJECT(dev), "xics", &err);
>> +    if (!obj) {
>> +        error_setg(errp, "%s: required link 'xics' not found: %s",
>> +                   __func__, error_get_pretty(err));
>> +        return;
>> +    }
>> +    ics->xics = XICS_COMMON(obj);
>>  
>>      if (!ics->nr_irqs) {
>>          error_setg(errp, "Number of interrupts needs to be greater 0");
>> @@ -746,12 +721,18 @@ static void ics_simple_realize(DeviceState *dev, Error 
>> **errp)
>>      ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
>>  }
>>  
>> +static Property ics_simple_properties[] = {
>> +    DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>  static void ics_simple_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>      ICSStateClass *isc = ICS_BASE_CLASS(klass);
>>  
>>      dc->realize = ics_simple_realize;
>> +    dc->props = ics_simple_properties;
>>      dc->vmsd = &vmstate_ics_simple;
>>      dc->reset = ics_simple_reset;
>>      isc->post_load = ics_simple_post_load;
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> index 17694eaa8709..4a6c0522feb6 100644
>> --- a/hw/intc/xics_kvm.c
>> +++ b/hw/intc/xics_kvm.c
>> @@ -294,6 +294,16 @@ static void ics_kvm_reset(DeviceState *dev)
>>  static void ics_kvm_realize(DeviceState *dev, Error **errp)
>>  {
>>      ICSState *ics = ICS_SIMPLE(dev);
>> +    Object *obj;
>> +    Error *err = NULL;
>> +
>> +    obj = object_property_get_link(OBJECT(dev), "xics", &err);
>> +    if (!obj) {
>> +        error_setg(errp, "%s: required link 'xics' not found: %s",
>> +                   __func__, error_get_pretty(err));
>> +        return;
>> +    }
>> +    ics->xics = XICS_COMMON(obj);
> 
> Couldn't you have this logic in the base class rather than repeating
> it in ics simple and ics kvm?

Yes we could. But, as I am getting rid of the backlinks at the end of 
the patchset, the redundancy is not much of a problem.

Thanks,

C.  

>>      if (!ics->nr_irqs) {
>>          error_setg(errp, "Number of interrupts needs to be greater 0");
>> @@ -358,18 +368,6 @@ static void xics_kvm_cpu_setup(XICSState *xics, 
>> PowerPCCPU *cpu)
>>      ss->cap_irq_xics_enabled = true;
>>  }
>>  
>> -static void xics_kvm_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
>> -                                 Error **errp)
>> -{
>> -    ICSState *ics = QLIST_FIRST(&xics->ics);
>> -
>> -    /* This needs to be deprecated ... */
>> -    xics->nr_irqs = nr_irqs;
>> -    if (ics) {
>> -        ics->nr_irqs = nr_irqs;
>> -    }
>> -}
>> -
>>  static void xics_kvm_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>>                                      Error **errp)
>>  {
>> @@ -389,7 +387,6 @@ static void xics_kvm_realize(DeviceState *dev, Error 
>> **errp)
>>  {
>>      KVMXICSState *xicskvm = XICS_SPAPR_KVM(dev);
>>      XICSState *xics = XICS_COMMON(dev);
>> -    ICSState *ics;
>>      int i, rc;
>>      Error *error = NULL;
>>      struct kvm_create_device xics_create_device = {
>> @@ -441,14 +438,6 @@ static void xics_kvm_realize(DeviceState *dev, Error 
>> **errp)
>>  
>>      xicskvm->kernel_xics_fd = xics_create_device.fd;
>>  
>> -    QLIST_FOREACH(ics, &xics->ics, list) {
>> -        object_property_set_bool(OBJECT(ics), true, "realized", &error);
>> -        if (error) {
>> -            error_propagate(errp, error);
>> -            goto fail;
>> -        }
>> -    }
>> -
>>      assert(xics->nr_servers);
>>      for (i = 0; i < xics->nr_servers; i++) {
>>          object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
>> @@ -472,17 +461,6 @@ fail:
>>      kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
>>  }
>>  
>> -static void xics_kvm_initfn(Object *obj)
>> -{
>> -    XICSState *xics = XICS_COMMON(obj);
>> -    ICSState *ics;
>> -
>> -    ics = ICS_SIMPLE(object_new(TYPE_ICS_KVM));
>> -    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
>> -    ics->xics = xics;
>> -    QLIST_INSERT_HEAD(&xics->ics, ics, list);
>> -}
>> -
>>  static void xics_kvm_class_init(ObjectClass *oc, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(oc);
>> @@ -490,7 +468,6 @@ static void xics_kvm_class_init(ObjectClass *oc, void 
>> *data)
>>  
>>      dc->realize = xics_kvm_realize;
>>      xsc->cpu_setup = xics_kvm_cpu_setup;
>> -    xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
>>      xsc->set_nr_servers = xics_kvm_set_nr_servers;
>>  }
>>  
>> @@ -499,7 +476,6 @@ static const TypeInfo xics_spapr_kvm_info = {
>>      .parent        = TYPE_XICS_COMMON,
>>      .instance_size = sizeof(KVMXICSState),
>>      .class_init    = xics_kvm_class_init,
>> -    .instance_init = xics_kvm_initfn,
>>  };
>>  
>>  static void xics_kvm_register_types(void)
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 2e3f1c5e95b2..03e42a866603 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -239,18 +239,6 @@ static void rtas_int_on(PowerPCCPU *cpu, 
>> sPAPRMachineState *spapr,
>>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>  }
>>  
>> -static void xics_spapr_set_nr_irqs(XICSState *xics, uint32_t nr_irqs,
>> -                                   Error **errp)
>> -{
>> -    ICSState *ics = QLIST_FIRST(&xics->ics);
>> -
>> -    /* This needs to be deprecated ... */
>> -    xics->nr_irqs = nr_irqs;
>> -    if (ics) {
>> -        ics->nr_irqs = nr_irqs;
>> -    }
>> -}
>> -
>>  static void xics_spapr_set_nr_servers(XICSState *xics, uint32_t nr_servers,
>>                                        Error **errp)
>>  {
>> @@ -260,7 +248,6 @@ static void xics_spapr_set_nr_servers(XICSState *xics, 
>> uint32_t nr_servers,
>>  static void xics_spapr_realize(DeviceState *dev, Error **errp)
>>  {
>>      XICSState *xics = XICS_SPAPR(dev);
>> -    ICSState *ics;
>>      Error *error = NULL;
>>      int i;
>>  
>> @@ -282,14 +269,6 @@ static void xics_spapr_realize(DeviceState *dev, Error 
>> **errp)
>>      spapr_register_hypercall(H_EOI, h_eoi);
>>      spapr_register_hypercall(H_IPOLL, h_ipoll);
>>  
>> -    QLIST_FOREACH(ics, &xics->ics, list) {
>> -        object_property_set_bool(OBJECT(ics), true, "realized", &error);
>> -        if (error) {
>> -            error_propagate(errp, error);
>> -            return;
>> -        }
>> -    }
>> -
>>      for (i = 0; i < xics->nr_servers; i++) {
>>          object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
>>                                   &error);
>> @@ -300,24 +279,12 @@ static void xics_spapr_realize(DeviceState *dev, Error 
>> **errp)
>>      }
>>  }
>>  
>> -static void xics_spapr_initfn(Object *obj)
>> -{
>> -    XICSState *xics = XICS_SPAPR(obj);
>> -    ICSState *ics;
>> -
>> -    ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
>> -    object_property_add_child(obj, "ics", OBJECT(ics), NULL);
>> -    ics->xics = xics;
>> -    QLIST_INSERT_HEAD(&xics->ics, ics, list);
>> -}
>> -
>>  static void xics_spapr_class_init(ObjectClass *oc, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(oc);
>>      XICSStateClass *xsc = XICS_SPAPR_CLASS(oc);
>>  
>>      dc->realize = xics_spapr_realize;
>> -    xsc->set_nr_irqs = xics_spapr_set_nr_irqs;
>>      xsc->set_nr_servers = xics_spapr_set_nr_servers;
>>  }
>>  
>> @@ -327,7 +294,6 @@ static const TypeInfo xics_spapr_info = {
>>      .instance_size = sizeof(XICSState),
>>      .class_size = sizeof(XICSStateClass),
>>      .class_init    = xics_spapr_class_init,
>> -    .instance_init = xics_spapr_initfn,
>>  };
>>  
>>  #define ICS_IRQ_FREE(ics, srcno)   \
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 9b6ad0f2fd77..94b1e8e3227a 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -95,22 +95,42 @@
>>  
>>  #define HTAB_SIZE(spapr)        (1ULL << ((spapr)->htab_shift))
>>  
>> -static XICSState *try_create_xics(const char *type, int nr_servers,
>> -                                  int nr_irqs, Error **errp)
>> -{
>> -    Error *err = NULL;
>> -    DeviceState *dev;
>> +static XICSState *try_create_xics(const char *type, const char *type_ics,
>> +                                  int nr_servers, int nr_irqs, Error **errp)
>> +{
>> +    Error *err = NULL, *local_err = NULL;
>> +    XICSState *xics;
>> +    ICSState *ics = NULL;
>> +
>> +    xics = XICS_COMMON(object_new(type));
>> +    qdev_set_parent_bus(DEVICE(xics), sysbus_get_default());
>> +    object_property_set_int(OBJECT(xics), nr_servers, "nr_servers", &err);
>> +    object_property_set_bool(OBJECT(xics), true, "realized", &local_err);
>> +    error_propagate(&err, local_err);
>> +    if (err) {
>> +        goto error;
>> +    }
>>  
>> -    dev = DEVICE(object_new(type));
>> -    qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
>> -    qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
>> -    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>> +    ics = ICS_SIMPLE(object_new(type_ics));
>> +    object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
>> +    object_property_set_int(OBJECT(ics), nr_irqs, "nr-irqs", &err);
>> +    object_property_add_const_link(OBJECT(ics), "xics", OBJECT(xics), NULL);
>> +    object_property_set_bool(OBJECT(ics), true, "realized", &local_err);
>> +    error_propagate(&err, local_err);
>>      if (err) {
>> -        error_propagate(errp, err);
>> -        object_unparent(OBJECT(dev));
>> -        return NULL;
>> +        goto error;
>>      }
>> -    return XICS_COMMON(dev);
>> +    QLIST_INSERT_HEAD(&xics->ics, ics, list);
>> +
>> +    return xics;
>> +
>> +error:
>> +    error_propagate(errp, err);
>> +    if (ics) {
>> +        object_unparent(OBJECT(ics));
>> +    }
>> +    object_unparent(OBJECT(xics));
>> +    return NULL;
>>  }
>>  
>>  static XICSState *xics_system_init(MachineState *machine,
>> @@ -122,8 +142,8 @@ static XICSState *xics_system_init(MachineState *machine,
>>          Error *err = NULL;
>>  
>>          if (machine_kernel_irqchip_allowed(machine)) {
>> -            xics = try_create_xics(TYPE_XICS_SPAPR_KVM, nr_servers, nr_irqs,
>> -                                   &err);
>> +            xics = try_create_xics(TYPE_XICS_SPAPR_KVM, TYPE_ICS_KVM,
>> +                                   nr_servers, nr_irqs, &err);
>>          }
>>          if (machine_kernel_irqchip_required(machine) && !xics) {
>>              error_reportf_err(err,
>> @@ -134,7 +154,8 @@ static XICSState *xics_system_init(MachineState *machine,
>>      }
>>  
>>      if (!xics) {
>> -        xics = try_create_xics(TYPE_XICS_SPAPR, nr_servers, nr_irqs, errp);
>> +        xics = try_create_xics(TYPE_XICS_SPAPR, TYPE_ICS_SIMPLE, nr_servers,
>> +                               nr_irqs, errp);
>>      }
>>  
>>      return xics;
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 3f0c31610aa4..8fe3c4d2ceab 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -74,7 +74,6 @@ struct XICSStateClass {
>>      DeviceClass parent_class;
>>  
>>      void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
>> -    void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
>>      void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers, Error 
>> **errp);
>>  };
>>  
>> @@ -83,7 +82,6 @@ struct XICSState {
>>      SysBusDevice parent_obj;
>>      /*< public >*/
>>      uint32_t nr_servers;
>> -    uint32_t nr_irqs;
>>      ICPState *ss;
>>      QLIST_HEAD(, ICSState) ics;
>>  };
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]