Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
Cédric Le Goater writes:
> On 09/20/2016 11:41 AM, Nikunj A Dadhania wrote:
>> Cédric Le Goater writes:
>>> On 09/20/2016 10:10 AM, Nikunj A Dadhania wrote:
>> -static void ics_reject(ICSState *ics, int nr);
>> -static void ics_resend(ICSState *ics, int server);
>> -static void ics_eoi(ICSState *ics, int nr);
>> +static void ics_reject(ICSState *ics, uint32_t nr)
>> +{
>> +ICSStateClass *k = ICS_GET_CLASS(ics);
>
> Shouldn't that be ICS_BASE_GET_CLASS()
The class hierarchy is something like this:
ICS_BASE -> ICS_SIMPLE -> ICS_KVM
>>>
>>> yes. but if we called ics_* with an instance of an ics class which is
>>> not a ICS_SIMPLE class that will break.
>>
>> Correct
>>
>>> ICSStateClass is the baseclass, whenever we call methods on a ICSState*
>>> object, we should use the method defines in ICSStateClass.
>>
>> Hmm, in that case I need to initialize base class methods in
>> instance_init of ics_simple
>
> yes but this is done, no ? I see :
>
> static void ics_simple_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> ICSStateClass *isc = ICS_BASE_CLASS(klass);
Right.
Currently, we have this:
+ICSStateClass *isc = ICS_CLASS(klass);
>>>
>>> oh yes and same in the kvm ICS. The name are bit confusing as we are
>>> introducing SIMPLE but not the associated macros.
>>>
>>> You can check on my 2.8 github branch, I got something working there :
>>>
>>>
>>> https://github.com/legoater/qemu/commit/d0b492707debc7192f0860e2c5fc2daa9fd453ac
>>
>> Cool. :-)
>>
>> Thanks for testing.
>
> I did some tests for pnv, pseries tcg and kvm.
>
> It didn't see any migration issues, yet. Do you have some scenario
> to reproduce the fix you are trying to add ? That's under tcg with
> a e1000 ?
Oh that is pretty complex:
Here is how I was reproducing the issue, it does not reproduce always,
but at least 1 in 3 times, and sometimes when lucky 3 in 3 times as well.
* Start a guest with single cpu, kernel-irqchip=off, e1000 card
* From the guest start a flood ping (ssh to guest)
* From the guest console trigger xmon
* Migrate to the target (localhost)
* Once the vm migrates to the target, exit xmon
* In success case, i get the console, and the flood ping continues
* In case of failure, console does not respond and within few seconds I
get a crash
Regards,
Nikunj
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
On 09/20/2016 11:41 AM, Nikunj A Dadhania wrote:
> Cédric Le Goater writes:
>> On 09/20/2016 10:10 AM, Nikunj A Dadhania wrote:
> -static void ics_reject(ICSState *ics, int nr);
> -static void ics_resend(ICSState *ics, int server);
> -static void ics_eoi(ICSState *ics, int nr);
> +static void ics_reject(ICSState *ics, uint32_t nr)
> +{
> +ICSStateClass *k = ICS_GET_CLASS(ics);
Shouldn't that be ICS_BASE_GET_CLASS()
>>>
>>> The class hierarchy is something like this:
>>>
>>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM
>>
>> yes. but if we called ics_* with an instance of an ics class which is
>> not a ICS_SIMPLE class that will break.
>
> Correct
>
>> ICSStateClass is the baseclass, whenever we call methods on a ICSState*
>> object, we should use the method defines in ICSStateClass.
>
> Hmm, in that case I need to initialize base class methods in
> instance_init of ics_simple
yes but this is done, no ? I see :
static void ics_simple_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
ICSStateClass *isc = ICS_BASE_CLASS(klass);
>>>
>>> Right.
>>>
>>> Currently, we have this:
>>>
>>> +ICSStateClass *isc = ICS_CLASS(klass);
>>
>> oh yes and same in the kvm ICS. The name are bit confusing as we are
>> introducing SIMPLE but not the associated macros.
>>
>> You can check on my 2.8 github branch, I got something working there :
>>
>>
>> https://github.com/legoater/qemu/commit/d0b492707debc7192f0860e2c5fc2daa9fd453ac
>
> Cool. :-)
>
> Thanks for testing.
I did some tests for pnv, pseries tcg and kvm.
It didn't see any migration issues, yet. Do you have some scenario
to reproduce the fix you are trying to add ? That's under tcg with
a e1000 ?
Thanks,
C.
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
Cédric Le Goater writes:
> On 09/20/2016 10:10 AM, Nikunj A Dadhania wrote:
-static void ics_reject(ICSState *ics, int nr);
-static void ics_resend(ICSState *ics, int server);
-static void ics_eoi(ICSState *ics, int nr);
+static void ics_reject(ICSState *ics, uint32_t nr)
+{
+ICSStateClass *k = ICS_GET_CLASS(ics);
>>>
>>> Shouldn't that be ICS_BASE_GET_CLASS()
>>
>> The class hierarchy is something like this:
>>
>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM
>
> yes. but if we called ics_* with an instance of an ics class which is
> not a ICS_SIMPLE class that will break.
Correct
> ICSStateClass is the baseclass, whenever we call methods on a ICSState*
> object, we should use the method defines in ICSStateClass.
Hmm, in that case I need to initialize base class methods in
instance_init of ics_simple
>>>
>>> yes but this is done, no ? I see :
>>>
>>> static void ics_simple_class_init(ObjectClass *klass, void *data)
>>> {
>>> DeviceClass *dc = DEVICE_CLASS(klass);
>>> ICSStateClass *isc = ICS_BASE_CLASS(klass);
>>
>> Right.
>>
>> Currently, we have this:
>>
>> +ICSStateClass *isc = ICS_CLASS(klass);
>
> oh yes and same in the kvm ICS. The name are bit confusing as we are
> introducing SIMPLE but not the associated macros.
>
> You can check on my 2.8 github branch, I got something working there :
>
>
> https://github.com/legoater/qemu/commit/d0b492707debc7192f0860e2c5fc2daa9fd453ac
Cool. :-)
Thanks for testing.
Regards,
Nikunj
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
On 09/20/2016 10:10 AM, Nikunj A Dadhania wrote:
> Cédric Le Goater writes:
>
>> On 09/20/2016 08:29 AM, Nikunj A Dadhania wrote:
>>> Cédric Le Goater writes:
>>>
On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote:
> Cédric Le Goater writes:
>
>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
>>> From: Benjamin Herrenschmidt
>>>
>>> The existing implementation remains same and ics-base is introduced. The
>>> type name "ics" is retained, and all the related functions renamed as
>>> ics_simple_*
>>>
>>> This will allow different implementations for the source controllers
>>> such as the MSI support of PHB3 on Power8 which uses in-memory state
>>> tables for example.
>>
>> A couple of issues below regarding the class helpers,
>>
>>> Signed-off-by: Benjamin Herrenschmidt
>>> Signed-off-by: Nikunj A Dadhania
>>> ---
>>> hw/intc/trace-events | 10 ++--
>>> hw/intc/xics.c| 143
>>> +++---
>>> hw/intc/xics_kvm.c| 10 ++--
>>> hw/intc/xics_spapr.c | 28 +-
>>> include/hw/ppc/xics.h | 23 +---
>>> 5 files changed, 131 insertions(+), 83 deletions(-)
>>
>>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
>>> #define XISR(ss) (((ss)->xirr) & XISR_MASK)
>>> #define CPPR(ss) (((ss)->xirr) >> 24)
>>>
>>> -static void ics_reject(ICSState *ics, int nr);
>>> -static void ics_resend(ICSState *ics, int server);
>>> -static void ics_eoi(ICSState *ics, int nr);
>>> +static void ics_reject(ICSState *ics, uint32_t nr)
>>> +{
>>> +ICSStateClass *k = ICS_GET_CLASS(ics);
>>
>> Shouldn't that be ICS_BASE_GET_CLASS()
>
> The class hierarchy is something like this:
>
> ICS_BASE -> ICS_SIMPLE -> ICS_KVM
yes. but if we called ics_* with an instance of an ics class which is
not a ICS_SIMPLE class that will break.
>>>
>>> Correct
>>>
ICSStateClass is the baseclass, whenever we call methods on a ICSState*
object, we should use the method defines in ICSStateClass.
>>>
>>> Hmm, in that case I need to initialize base class methods in
>>> instance_init of ics_simple
>>
>> yes but this is done, no ? I see :
>>
>> static void ics_simple_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> ICSStateClass *isc = ICS_BASE_CLASS(klass);
>
> Right.
>
> Currently, we have this:
>
> +ICSStateClass *isc = ICS_CLASS(klass);
oh yes and same in the kvm ICS. The name are bit confusing as we are
introducing SIMPLE but not the associated macros.
You can check on my 2.8 github branch, I got something working there :
https://github.com/legoater/qemu/commit/d0b492707debc7192f0860e2c5fc2daa9fd453ac
Thanks,
C.
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
Cédric Le Goater writes:
> On 09/20/2016 08:29 AM, Nikunj A Dadhania wrote:
>> Cédric Le Goater writes:
>>
>>> On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote:
Cédric Le Goater writes:
> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
>> From: Benjamin Herrenschmidt
>>
>> The existing implementation remains same and ics-base is introduced. The
>> type name "ics" is retained, and all the related functions renamed as
>> ics_simple_*
>>
>> This will allow different implementations for the source controllers
>> such as the MSI support of PHB3 on Power8 which uses in-memory state
>> tables for example.
>
> A couple of issues below regarding the class helpers,
>
>> Signed-off-by: Benjamin Herrenschmidt
>> Signed-off-by: Nikunj A Dadhania
>> ---
>> hw/intc/trace-events | 10 ++--
>> hw/intc/xics.c| 143
>> +++---
>> hw/intc/xics_kvm.c| 10 ++--
>> hw/intc/xics_spapr.c | 28 +-
>> include/hw/ppc/xics.h | 23 +---
>> 5 files changed, 131 insertions(+), 83 deletions(-)
>
>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
>> #define XISR(ss) (((ss)->xirr) & XISR_MASK)
>> #define CPPR(ss) (((ss)->xirr) >> 24)
>>
>> -static void ics_reject(ICSState *ics, int nr);
>> -static void ics_resend(ICSState *ics, int server);
>> -static void ics_eoi(ICSState *ics, int nr);
>> +static void ics_reject(ICSState *ics, uint32_t nr)
>> +{
>> +ICSStateClass *k = ICS_GET_CLASS(ics);
>
> Shouldn't that be ICS_BASE_GET_CLASS()
The class hierarchy is something like this:
ICS_BASE -> ICS_SIMPLE -> ICS_KVM
>>>
>>> yes. but if we called ics_* with an instance of an ics class which is
>>> not a ICS_SIMPLE class that will break.
>>
>> Correct
>>
>>> ICSStateClass is the baseclass, whenever we call methods on a ICSState*
>>> object, we should use the method defines in ICSStateClass.
>>
>> Hmm, in that case I need to initialize base class methods in
>> instance_init of ics_simple
>
> yes but this is done, no ? I see :
>
> static void ics_simple_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> ICSStateClass *isc = ICS_BASE_CLASS(klass);
Right.
Currently, we have this:
+ICSStateClass *isc = ICS_CLASS(klass);
>
> dc->realize = ics_simple_realize;
> dc->vmsd = &vmstate_ics_simple;
> dc->reset = ics_simple_reset;
> isc->post_load = ics_simple_post_load;
> isc->reject = ics_simple_reject;
> isc->resend = ics_simple_resend;
> isc->eoi = ics_simple_eoi;
Regards
Nikunj
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
On 09/20/2016 08:29 AM, Nikunj A Dadhania wrote:
> Cédric Le Goater writes:
>
>> On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote:
>>> Cédric Le Goater writes:
>>>
On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
> From: Benjamin Herrenschmidt
>
> The existing implementation remains same and ics-base is introduced. The
> type name "ics" is retained, and all the related functions renamed as
> ics_simple_*
>
> This will allow different implementations for the source controllers
> such as the MSI support of PHB3 on Power8 which uses in-memory state
> tables for example.
A couple of issues below regarding the class helpers,
> Signed-off-by: Benjamin Herrenschmidt
> Signed-off-by: Nikunj A Dadhania
> ---
> hw/intc/trace-events | 10 ++--
> hw/intc/xics.c| 143
> +++---
> hw/intc/xics_kvm.c| 10 ++--
> hw/intc/xics_spapr.c | 28 +-
> include/hw/ppc/xics.h | 23 +---
> 5 files changed, 131 insertions(+), 83 deletions(-)
> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
> #define XISR(ss) (((ss)->xirr) & XISR_MASK)
> #define CPPR(ss) (((ss)->xirr) >> 24)
>
> -static void ics_reject(ICSState *ics, int nr);
> -static void ics_resend(ICSState *ics, int server);
> -static void ics_eoi(ICSState *ics, int nr);
> +static void ics_reject(ICSState *ics, uint32_t nr)
> +{
> +ICSStateClass *k = ICS_GET_CLASS(ics);
Shouldn't that be ICS_BASE_GET_CLASS()
>>>
>>> The class hierarchy is something like this:
>>>
>>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM
>>
>> yes. but if we called ics_* with an instance of an ics class which is
>> not a ICS_SIMPLE class that will break.
>
> Correct
>
>> ICSStateClass is the baseclass, whenever we call methods on a ICSState*
>> object, we should use the method defines in ICSStateClass.
>
> Hmm, in that case I need to initialize base class methods in
> instance_init of ics_simple
yes but this is done, no ? I see :
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->vmsd = &vmstate_ics_simple;
dc->reset = ics_simple_reset;
isc->post_load = ics_simple_post_load;
isc->reject = ics_simple_reject;
isc->resend = ics_simple_resend;
isc->eoi = ics_simple_eoi;
}
Thanks,
C.
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
Cédric Le Goater writes:
> On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote:
>> Cédric Le Goater writes:
>>
>>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
From: Benjamin Herrenschmidt
The existing implementation remains same and ics-base is introduced. The
type name "ics" is retained, and all the related functions renamed as
ics_simple_*
This will allow different implementations for the source controllers
such as the MSI support of PHB3 on Power8 which uses in-memory state
tables for example.
>>>
>>> A couple of issues below regarding the class helpers,
>>>
Signed-off-by: Benjamin Herrenschmidt
Signed-off-by: Nikunj A Dadhania
---
hw/intc/trace-events | 10 ++--
hw/intc/xics.c| 143
+++---
hw/intc/xics_kvm.c| 10 ++--
hw/intc/xics_spapr.c | 28 +-
include/hw/ppc/xics.h | 23 +---
5 files changed, 131 insertions(+), 83 deletions(-)
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index aa342a8..a367b46 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr)
"icp_accept: XIRR %#"PRIx3
xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi:
server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32
xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to
deliver irq %#"PRIx32" priority %#x"
xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new
XIRR=%#x new pending priority=%#x"
-xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
+xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d
[irq %#x]"
xics_masked_pending(void) "set_irq_msi: masked pending"
-xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
-xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority)
"ics_write_xive: irq %#x [src %d] server %#x prio %#x"
-xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
-xics_ics_eoi(int nr) "ics_eoi: irq %#x"
+xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d
[irq %#x]"
+xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t
priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
+xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]"
+xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x"
xics_alloc(int irq) "irq %d"
xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d,
%d irqs, lsi=%d, alignnum %d"
xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d
irqs"
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index c7901c4..b15751e 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics)
{
ICSState *ics;
-ics = ICS(object_new(TYPE_ICS));
+ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
ics->xics = xics;
QLIST_INSERT_HEAD(&xics->ics, ics, list);
@@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
#define XISR(ss) (((ss)->xirr) & XISR_MASK)
#define CPPR(ss) (((ss)->xirr) >> 24)
-static void ics_reject(ICSState *ics, int nr);
-static void ics_resend(ICSState *ics, int server);
-static void ics_eoi(ICSState *ics, int nr);
+static void ics_reject(ICSState *ics, uint32_t nr)
+{
+ICSStateClass *k = ICS_GET_CLASS(ics);
>>>
>>> Shouldn't that be ICS_BASE_GET_CLASS()
>>
>> The class hierarchy is something like this:
>>
>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM
>
> yes. but if we called ics_* with an instance of an ics class which is
> not a ICS_SIMPLE class that will break.
Correct
> ICSStateClass is the baseclass, whenever we call methods on a ICSState*
> object, we should use the method defines in ICSStateClass.
Hmm, in that case I need to initialize base class methods in
instance_init of ics_simple
>> We have an instance init for ICS_SIMPLE where we set these pointers.
>>
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index a7a1e54..2231f2a 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -119,22 +119,29 @@ struct ICPState {
bool cap_irq_xics_enabled;
};
-#define TYPE_ICS "ics"
-#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
+#define TYPE_ICS_BASE "ics-base"
+#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
>>>
>>> #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE)
>>
>> Yes, that is a bug. Will correct.
>>
>>>
-#define TYPE_KVM_ICS "icskvm"
-#define KVM_ICS(obj)
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote:
> Cédric Le Goater writes:
>
>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
>>> From: Benjamin Herrenschmidt
>>>
>>> The existing implementation remains same and ics-base is introduced. The
>>> type name "ics" is retained, and all the related functions renamed as
>>> ics_simple_*
>>>
>>> This will allow different implementations for the source controllers
>>> such as the MSI support of PHB3 on Power8 which uses in-memory state
>>> tables for example.
>>
>> A couple of issues below regarding the class helpers,
>>
>>> Signed-off-by: Benjamin Herrenschmidt
>>> Signed-off-by: Nikunj A Dadhania
>>> ---
>>> hw/intc/trace-events | 10 ++--
>>> hw/intc/xics.c| 143
>>> +++---
>>> hw/intc/xics_kvm.c| 10 ++--
>>> hw/intc/xics_spapr.c | 28 +-
>>> include/hw/ppc/xics.h | 23 +---
>>> 5 files changed, 131 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>>> index aa342a8..a367b46 100644
>>> --- a/hw/intc/trace-events
>>> +++ b/hw/intc/trace-events
>>> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr)
>>> "icp_accept: XIRR %#"PRIx3
>>> xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi:
>>> server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32
>>> xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to
>>> deliver irq %#"PRIx32" priority %#x"
>>> xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new
>>> XIRR=%#x new pending priority=%#x"
>>> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
>>> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq
>>> %#x]"
>>> xics_masked_pending(void) "set_irq_msi: masked pending"
>>> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
>>> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority)
>>> "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>>> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
>>> -xics_ics_eoi(int nr) "ics_eoi: irq %#x"
>>> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq
>>> %#x]"
>>> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t
>>> priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>>> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]"
>>> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x"
>>> xics_alloc(int irq) "irq %d"
>>> xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d,
>>> %d irqs, lsi=%d, alignnum %d"
>>> xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index c7901c4..b15751e 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>>> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics)
>>> {
>>> ICSState *ics;
>>>
>>> -ics = ICS(object_new(TYPE_ICS));
>>> +ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
>>> object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
>>> ics->xics = xics;
>>> QLIST_INSERT_HEAD(&xics->ics, ics, list);
>>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
>>> #define XISR(ss) (((ss)->xirr) & XISR_MASK)
>>> #define CPPR(ss) (((ss)->xirr) >> 24)
>>>
>>> -static void ics_reject(ICSState *ics, int nr);
>>> -static void ics_resend(ICSState *ics, int server);
>>> -static void ics_eoi(ICSState *ics, int nr);
>>> +static void ics_reject(ICSState *ics, uint32_t nr)
>>> +{
>>> +ICSStateClass *k = ICS_GET_CLASS(ics);
>>
>> Shouldn't that be ICS_BASE_GET_CLASS()
>
> The class hierarchy is something like this:
>
> ICS_BASE -> ICS_SIMPLE -> ICS_KVM
yes. but if we called ics_* with an instance of an ics class which is
not a ICS_SIMPLE class that will break.
ICSStateClass is the baseclass, whenever we call methods on a ICSState*
object, we should use the method defines in ICSStateClass.
> We have an instance init for ICS_SIMPLE where we set these pointers.
>
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index a7a1e54..2231f2a 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -119,22 +119,29 @@ struct ICPState {
>>> bool cap_irq_xics_enabled;
>>> };
>>>
>>> -#define TYPE_ICS "ics"
>>> -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>>> +#define TYPE_ICS_BASE "ics-base"
>>> +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
>>
>> #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE)
>
> Yes, that is a bug. Will correct.
>
>>
>>> -#define TYPE_KVM_ICS "icskvm"
>>> -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
>>> +/* Retain ics for sPAPR for migration from existing sPAPR guests */
>>> +#define TYPE_ICS_SIMPLE "ics"
>>> +#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
>>> +
>>> +#define TYP
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
Cédric Le Goater writes:
> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
>> From: Benjamin Herrenschmidt
>>
>> The existing implementation remains same and ics-base is introduced. The
>> type name "ics" is retained, and all the related functions renamed as
>> ics_simple_*
>>
>> This will allow different implementations for the source controllers
>> such as the MSI support of PHB3 on Power8 which uses in-memory state
>> tables for example.
>
> A couple of issues below regarding the class helpers,
>
>> Signed-off-by: Benjamin Herrenschmidt
>> Signed-off-by: Nikunj A Dadhania
>> ---
>> hw/intc/trace-events | 10 ++--
>> hw/intc/xics.c| 143
>> +++---
>> hw/intc/xics_kvm.c| 10 ++--
>> hw/intc/xics_spapr.c | 28 +-
>> include/hw/ppc/xics.h | 23 +---
>> 5 files changed, 131 insertions(+), 83 deletions(-)
>>
>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>> index aa342a8..a367b46 100644
>> --- a/hw/intc/trace-events
>> +++ b/hw/intc/trace-events
>> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr)
>> "icp_accept: XIRR %#"PRIx3
>> xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server
>> %d given XIRR %#"PRIx32" new XIRR %#"PRIx32
>> xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to
>> deliver irq %#"PRIx32" priority %#x"
>> xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new
>> XIRR=%#x new pending priority=%#x"
>> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
>> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq
>> %#x]"
>> xics_masked_pending(void) "set_irq_msi: masked pending"
>> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
>> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority)
>> "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
>> -xics_ics_eoi(int nr) "ics_eoi: irq %#x"
>> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq
>> %#x]"
>> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority)
>> "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]"
>> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x"
>> xics_alloc(int irq) "irq %d"
>> xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d
>> irqs, lsi=%d, alignnum %d"
>> xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index c7901c4..b15751e 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics)
>> {
>> ICSState *ics;
>>
>> -ics = ICS(object_new(TYPE_ICS));
>> +ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
>> object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
>> ics->xics = xics;
>> QLIST_INSERT_HEAD(&xics->ics, ics, list);
>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
>> #define XISR(ss) (((ss)->xirr) & XISR_MASK)
>> #define CPPR(ss) (((ss)->xirr) >> 24)
>>
>> -static void ics_reject(ICSState *ics, int nr);
>> -static void ics_resend(ICSState *ics, int server);
>> -static void ics_eoi(ICSState *ics, int nr);
>> +static void ics_reject(ICSState *ics, uint32_t nr)
>> +{
>> +ICSStateClass *k = ICS_GET_CLASS(ics);
>
> Shouldn't that be ICS_BASE_GET_CLASS()
The class hierarchy is something like this:
ICS_BASE -> ICS_SIMPLE -> ICS_KVM
We have an instance init for ICS_SIMPLE where we set these pointers.
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index a7a1e54..2231f2a 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -119,22 +119,29 @@ struct ICPState {
>> bool cap_irq_xics_enabled;
>> };
>>
>> -#define TYPE_ICS "ics"
>> -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>> +#define TYPE_ICS_BASE "ics-base"
>> +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
>
> #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE)
Yes, that is a bug. Will correct.
>
>> -#define TYPE_KVM_ICS "icskvm"
>> -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
>> +/* Retain ics for sPAPR for migration from existing sPAPR guests */
>> +#define TYPE_ICS_SIMPLE "ics"
>> +#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
>> +
>> +#define TYPE_ICS_KVM "icskvm"
>> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
>>
>> #define ICS_CLASS(klass) \
>> - OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
>> + OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE)
>> #define ICS_GET_CLASS(obj) \
>> - OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS)
>> + OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_SIMPLE)
>
> #d
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
Cédric Le Goater writes: > On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote: >> From: Benjamin Herrenschmidt >> >> The existing implementation remains same and ics-base is introduced. The >> type name "ics" is retained, and all the related functions renamed as >> ics_simple_* >> >> This will allow different implementations for the source controllers >> such as the MSI support of PHB3 on Power8 which uses in-memory state >> tables for example. >> >> Signed-off-by: Benjamin Herrenschmidt >> Signed-off-by: Nikunj A Dadhania > > This patch had a Reviewed-by from David. Did you change anything > in v4 ? No changes, I missed adding reviewed-by in v4. Regards Nikunj
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
> From: Benjamin Herrenschmidt
>
> The existing implementation remains same and ics-base is introduced. The
> type name "ics" is retained, and all the related functions renamed as
> ics_simple_*
>
> This will allow different implementations for the source controllers
> such as the MSI support of PHB3 on Power8 which uses in-memory state
> tables for example.
A couple of issues below regarding the class helpers,
> Signed-off-by: Benjamin Herrenschmidt
> Signed-off-by: Nikunj A Dadhania
> ---
> hw/intc/trace-events | 10 ++--
> hw/intc/xics.c| 143
> +++---
> hw/intc/xics_kvm.c| 10 ++--
> hw/intc/xics_spapr.c | 28 +-
> include/hw/ppc/xics.h | 23 +---
> 5 files changed, 131 insertions(+), 83 deletions(-)
>
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index aa342a8..a367b46 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr)
> "icp_accept: XIRR %#"PRIx3
> xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server
> %d given XIRR %#"PRIx32" new XIRR %#"PRIx32
> xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver
> irq %#"PRIx32" priority %#x"
> xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new
> XIRR=%#x new pending priority=%#x"
> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq
> %#x]"
> xics_masked_pending(void) "set_irq_msi: masked pending"
> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority)
> "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
> -xics_ics_eoi(int nr) "ics_eoi: irq %#x"
> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq
> %#x]"
> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority)
> "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]"
> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x"
> xics_alloc(int irq) "irq %d"
> xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d
> irqs, lsi=%d, alignnum %d"
> xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c7901c4..b15751e 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics)
> {
> ICSState *ics;
>
> -ics = ICS(object_new(TYPE_ICS));
> +ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
> object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
> ics->xics = xics;
> QLIST_INSERT_HEAD(&xics->ics, ics, list);
> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
> #define XISR(ss) (((ss)->xirr) & XISR_MASK)
> #define CPPR(ss) (((ss)->xirr) >> 24)
>
> -static void ics_reject(ICSState *ics, int nr);
> -static void ics_resend(ICSState *ics, int server);
> -static void ics_eoi(ICSState *ics, int nr);
> +static void ics_reject(ICSState *ics, uint32_t nr)
> +{
> +ICSStateClass *k = ICS_GET_CLASS(ics);
Shouldn't that be ICS_BASE_GET_CLASS()
> +if (k->reject) {
> +k->reject(ics, nr);
> +}
> +}
> +
> +static void ics_resend(ICSState *ics, int server)
> +{
> +ICSStateClass *k = ICS_GET_CLASS(ics);
here also
> +
> +if (k->resend) {
> +k->resend(ics, server);
> +}
> +}
> +
> +static void ics_eoi(ICSState *ics, int nr)
> +{
> +ICSStateClass *k = ICS_GET_CLASS(ics);
and here.
> +
> +if (k->eoi) {
> +k->eoi(ics, nr);
> +}
> +}
>
> static void icp_check_ipi(ICPState *ss)
> {
> @@ -462,7 +485,7 @@ static const TypeInfo icp_info = {
> /*
> * ICS: Source layer
> */
> -static void resend_msi(ICSState *ics, int srcno)
> +static void ics_simple_resend_msi(ICSState *ics, int srcno)
> {
> ICSIRQState *irq = ics->irqs + srcno;
>
> @@ -475,7 +498,7 @@ static void resend_msi(ICSState *ics, int srcno)
> }
> }
>
> -static void resend_lsi(ICSState *ics, int srcno)
> +static void ics_simple_resend_lsi(ICSState *ics, int srcno)
> {
> ICSIRQState *irq = ics->irqs + srcno;
>
> @@ -487,11 +510,11 @@ static void resend_lsi(ICSState *ics, int srcno)
> }
> }
>
> -static void set_irq_msi(ICSState *ics, int srcno, int val)
> +static void ics_simple_set_irq_msi(ICSState *ics, int srcno, int val)
> {
> ICSIRQState *irq = ics->irqs + srcno;
>
> -trace_xics_set_irq_msi(srcno, srcno + ics->offset);
> +trace_xics_ics_simple_set_irq_msi(srcno, srcno + ics->offset);
>
> if (val) {
> if (irq->priority == 0xff) {
> @@ -503,31 +526,31 @@ static void set_irq_
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class
On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
> From: Benjamin Herrenschmidt
>
> The existing implementation remains same and ics-base is introduced. The
> type name "ics" is retained, and all the related functions renamed as
> ics_simple_*
>
> This will allow different implementations for the source controllers
> such as the MSI support of PHB3 on Power8 which uses in-memory state
> tables for example.
>
> Signed-off-by: Benjamin Herrenschmidt
> Signed-off-by: Nikunj A Dadhania
This patch had a Reviewed-by from David. Did you change anything
in v4 ?
Thanks,
C.
> ---
> hw/intc/trace-events | 10 ++--
> hw/intc/xics.c| 143
> +++---
> hw/intc/xics_kvm.c| 10 ++--
> hw/intc/xics_spapr.c | 28 +-
> include/hw/ppc/xics.h | 23 +---
> 5 files changed, 131 insertions(+), 83 deletions(-)
>
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index aa342a8..a367b46 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr)
> "icp_accept: XIRR %#"PRIx3
> xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: server
> %d given XIRR %#"PRIx32" new XIRR %#"PRIx32
> xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to deliver
> irq %#"PRIx32" priority %#x"
> xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new
> XIRR=%#x new pending priority=%#x"
> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq
> %#x]"
> xics_masked_pending(void) "set_irq_msi: masked pending"
> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority)
> "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
> -xics_ics_eoi(int nr) "ics_eoi: irq %#x"
> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq
> %#x]"
> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t priority)
> "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]"
> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x"
> xics_alloc(int irq) "irq %d"
> xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, %d
> irqs, lsi=%d, alignnum %d"
> xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c7901c4..b15751e 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics)
> {
> ICSState *ics;
>
> -ics = ICS(object_new(TYPE_ICS));
> +ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
> object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
> ics->xics = xics;
> QLIST_INSERT_HEAD(&xics->ics, ics, list);
> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
> #define XISR(ss) (((ss)->xirr) & XISR_MASK)
> #define CPPR(ss) (((ss)->xirr) >> 24)
>
> -static void ics_reject(ICSState *ics, int nr);
> -static void ics_resend(ICSState *ics, int server);
> -static void ics_eoi(ICSState *ics, int nr);
> +static void ics_reject(ICSState *ics, uint32_t nr)
> +{
> +ICSStateClass *k = ICS_GET_CLASS(ics);
> +
> +if (k->reject) {
> +k->reject(ics, nr);
> +}
> +}
> +
> +static void ics_resend(ICSState *ics, int server)
> +{
> +ICSStateClass *k = ICS_GET_CLASS(ics);
> +
> +if (k->resend) {
> +k->resend(ics, server);
> +}
> +}
> +
> +static void ics_eoi(ICSState *ics, int nr)
> +{
> +ICSStateClass *k = ICS_GET_CLASS(ics);
> +
> +if (k->eoi) {
> +k->eoi(ics, nr);
> +}
> +}
>
> static void icp_check_ipi(ICPState *ss)
> {
> @@ -462,7 +485,7 @@ static const TypeInfo icp_info = {
> /*
> * ICS: Source layer
> */
> -static void resend_msi(ICSState *ics, int srcno)
> +static void ics_simple_resend_msi(ICSState *ics, int srcno)
> {
> ICSIRQState *irq = ics->irqs + srcno;
>
> @@ -475,7 +498,7 @@ static void resend_msi(ICSState *ics, int srcno)
> }
> }
>
> -static void resend_lsi(ICSState *ics, int srcno)
> +static void ics_simple_resend_lsi(ICSState *ics, int srcno)
> {
> ICSIRQState *irq = ics->irqs + srcno;
>
> @@ -487,11 +510,11 @@ static void resend_lsi(ICSState *ics, int srcno)
> }
> }
>
> -static void set_irq_msi(ICSState *ics, int srcno, int val)
> +static void ics_simple_set_irq_msi(ICSState *ics, int srcno, int val)
> {
> ICSIRQState *irq = ics->irqs + srcno;
>
> -trace_xics_set_irq_msi(srcno, srcno + ics->offset);
> +trace_xics_ics_simple_set_irq_msi(srcno, srcno + ics->offset);
>
> if (val) {
> if (irq->priority == 0xff) {
> @@ -503,31 +526,31 @@ static void set_irq_msi(ICSState *ics, int
