Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Tue, Dec 04, 2018 at 06:04:13PM +0100, Cédric Le Goater wrote:
> On 12/4/18 2:54 AM, David Gibson wrote:
> > On Mon, Dec 03, 2018 at 06:05:12PM +0100, Cédric Le Goater wrote:
> >> I forgot to reply to this one.
> >>
> >> On 11/29/18 1:47 AM, David Gibson wrote:
> >>> On Wed, Nov 28, 2018 at 11:59:58AM +0100, Cédric Le Goater wrote:
> On 11/28/18 12:49 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
> >> The last sub-engine of the XIVE architecture is the Interrupt
> >> Virtualization Presentation Engine (IVPE). On HW, they share elements,
> >> the Power Bus interface (CQ), the routing table descriptors, and they
> >> can be combined in the same HW logic. We do the same in QEMU and
> >> combine both engines in the XiveRouter for simplicity.
> >
> > Ok, I'm not entirely convinced combining the IVPE and IVRE into a
> > single object is a good idea, but we can probably discuss that once
> > I've read further.
>
> We could introduce a simplified presenter for sPAPR but I am not even
> sure of that as it will get more complex if we support the EBB
> one day.
> >
> > [snip]
> >> +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t
> >> nvt_idx)
> >> +{
> >> +return (nvt_blk << 19) | nvt_idx;
> >
> > I'm guessing this formula is the standard way of combining the NVT
> > block and index into a single word?
>
> That number is the VP/NVT identifier which is written in the CAM value.
> The index is on 19 bits because of the NVT definition in the END
> structure. It is being increased to 24 bits on Power10
>
> > If so, I think we should
> > standardize on passing a single word "nvt_id" around and only
> > splitting it when we need to use the block separately.
>
> This is really the only place where we concatenate the two NVT values,
> block and index.
> >>>
> >>> Hm, ok. I know we don't model them (yet, maybe ever) but could
> >>> combined values appear in the PowerBUS messages that handle remote
> >>> notifications?
> >>
> >> They do.
> >>
> > Same goes for
> > the end_id, assuming there's a standard way of putting that into a
> > single word. That will address the point I raised earlier about lisn
> > being passed around as a single word, but these later stage ids being
> > split.
>
> Hmm, I am not sure this is a good option. It is not how the PowerNV
> model would use it, skiboot is very much aware of these blocks and
> indexes and for remote accesses chips are identified using the block.
> I will take a look at it but I am not found of it. I can add helpers
> in some places though.
> >>>
> >>> Hm, ok. Do the block and index appear as an (effectively) single
> >>> field in the EAS?
> >>
> >> no. In all XIVE structures, block and index are always distinct.
> >
> > Hm. Distinct in what sense? I get that the fields are labelled
> > separately in the documentation, but if the fields are adjacent, you
> > could equally well treat them as one.
>
> yes. Indeed. They are adjacent. The size of the index is subject to
> change in P10.
Ah, ok. If the boundary might change in P10 that is indeed a reason
to keep them separate.
> I am not sure that treating them as one will be of any help because
> we need to extract them from their XIVE structure with the *_INDEX
> and *_BLOCK masks first. I will take a look. May be not in v6.
Ok.
> >> +if (!match->tctx) {
> >> +qemu_log_mask(LOG_GUEST_ERROR, "XIVE: NVT %x/%x is not
> >> dispatched\n",
> >> + nvt_blk, nvt_idx);
> >> +return false;
> >
> > Hmm.. this isn't actually an error isn't it? At least not for powernv
>
> It is on sPAPR, it would mean the END was configured with an unknow CPU.
> >>>
> >>> Right.
> >>>
> It is not error on PowerNV, when we support escalations.
>
> > - that just means the NVT isn't currently dispatched, so we'll need to
> > trigger the escalation interrupt.
>
> Yes.
>
> > Does this get changed later in the series?
>
> No.
> >>>
> >>> But this code is common to PAPR and powernv, yes, so it will need to?
> >>
> >> When we add support for escalations, yes, it will change. Would you rather
> >> use an error_report() until then ?
> >
> > Ah, I guess leaving an error until we implement escalation makes
> > sense. It shouldn't be LOG_GUEST_ERROR, though, the guest didn't do
> > anything wrong, and error_report() doesn't really make sense for the
> > same reason.
> >
> > LOG_UNIMP, I guess?
>
> OK. will do.
>
> Thanks,
>
> C.
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On 12/4/18 2:54 AM, David Gibson wrote:
> On Mon, Dec 03, 2018 at 06:05:12PM +0100, Cédric Le Goater wrote:
>> I forgot to reply to this one.
>>
>> On 11/29/18 1:47 AM, David Gibson wrote:
>>> On Wed, Nov 28, 2018 at 11:59:58AM +0100, Cédric Le Goater wrote:
On 11/28/18 12:49 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
>> The last sub-engine of the XIVE architecture is the Interrupt
>> Virtualization Presentation Engine (IVPE). On HW, they share elements,
>> the Power Bus interface (CQ), the routing table descriptors, and they
>> can be combined in the same HW logic. We do the same in QEMU and
>> combine both engines in the XiveRouter for simplicity.
>
> Ok, I'm not entirely convinced combining the IVPE and IVRE into a
> single object is a good idea, but we can probably discuss that once
> I've read further.
We could introduce a simplified presenter for sPAPR but I am not even
sure of that as it will get more complex if we support the EBB
one day.
>
> [snip]
>> +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t
>> nvt_idx)
>> +{
>> +return (nvt_blk << 19) | nvt_idx;
>
> I'm guessing this formula is the standard way of combining the NVT
> block and index into a single word?
That number is the VP/NVT identifier which is written in the CAM value.
The index is on 19 bits because of the NVT definition in the END
structure. It is being increased to 24 bits on Power10
> If so, I think we should
> standardize on passing a single word "nvt_id" around and only
> splitting it when we need to use the block separately.
This is really the only place where we concatenate the two NVT values,
block and index.
>>>
>>> Hm, ok. I know we don't model them (yet, maybe ever) but could
>>> combined values appear in the PowerBUS messages that handle remote
>>> notifications?
>>
>> They do.
>>
> Same goes for
> the end_id, assuming there's a standard way of putting that into a
> single word. That will address the point I raised earlier about lisn
> being passed around as a single word, but these later stage ids being
> split.
Hmm, I am not sure this is a good option. It is not how the PowerNV
model would use it, skiboot is very much aware of these blocks and
indexes and for remote accesses chips are identified using the block.
I will take a look at it but I am not found of it. I can add helpers
in some places though.
>>>
>>> Hm, ok. Do the block and index appear as an (effectively) single
>>> field in the EAS?
>>
>> no. In all XIVE structures, block and index are always distinct.
>
> Hm. Distinct in what sense? I get that the fields are labelled
> separately in the documentation, but if the fields are adjacent, you
> could equally well treat them as one.
yes. Indeed. They are adjacent. The size of the index is subject to
change in P10.
I am not sure that treating them as one will be of any help because
we need to extract them from their XIVE structure with the *_INDEX
and *_BLOCK masks first. I will take a look. May be not in v6.
>> +if (!match->tctx) {
>> +qemu_log_mask(LOG_GUEST_ERROR, "XIVE: NVT %x/%x is not
>> dispatched\n",
>> + nvt_blk, nvt_idx);
>> +return false;
>
> Hmm.. this isn't actually an error isn't it? At least not for powernv
It is on sPAPR, it would mean the END was configured with an unknow CPU.
>>>
>>> Right.
>>>
It is not error on PowerNV, when we support escalations.
> - that just means the NVT isn't currently dispatched, so we'll need to
> trigger the escalation interrupt.
Yes.
> Does this get changed later in the series?
No.
>>>
>>> But this code is common to PAPR and powernv, yes, so it will need to?
>>
>> When we add support for escalations, yes, it will change. Would you rather
>> use an error_report() until then ?
>
> Ah, I guess leaving an error until we implement escalation makes
> sense. It shouldn't be LOG_GUEST_ERROR, though, the guest didn't do
> anything wrong, and error_report() doesn't really make sense for the
> same reason.
>
> LOG_UNIMP, I guess?
OK. will do.
Thanks,
C.
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Mon, Dec 03, 2018 at 06:05:12PM +0100, Cédric Le Goater wrote:
> I forgot to reply to this one.
>
> On 11/29/18 1:47 AM, David Gibson wrote:
> > On Wed, Nov 28, 2018 at 11:59:58AM +0100, Cédric Le Goater wrote:
> >> On 11/28/18 12:49 AM, David Gibson wrote:
> >>> On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
> The last sub-engine of the XIVE architecture is the Interrupt
> Virtualization Presentation Engine (IVPE). On HW, they share elements,
> the Power Bus interface (CQ), the routing table descriptors, and they
> can be combined in the same HW logic. We do the same in QEMU and
> combine both engines in the XiveRouter for simplicity.
> >>>
> >>> Ok, I'm not entirely convinced combining the IVPE and IVRE into a
> >>> single object is a good idea, but we can probably discuss that once
> >>> I've read further.
> >>
> >> We could introduce a simplified presenter for sPAPR but I am not even
> >> sure of that as it will get more complex if we support the EBB
> >> one day.
[snip]
> +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t
> nvt_idx)
> +{
> +return (nvt_blk << 19) | nvt_idx;
> >>>
> >>> I'm guessing this formula is the standard way of combining the NVT
> >>> block and index into a single word?
> >>
> >> That number is the VP/NVT identifier which is written in the CAM value.
> >> The index is on 19 bits because of the NVT definition in the END
> >> structure. It is being increased to 24 bits on Power10
> >>
> >>> If so, I think we should
> >>> standardize on passing a single word "nvt_id" around and only
> >>> splitting it when we need to use the block separately.
> >>
> >> This is really the only place where we concatenate the two NVT values,
> >> block and index.
> >
> > Hm, ok. I know we don't model them (yet, maybe ever) but could
> > combined values appear in the PowerBUS messages that handle remote
> > notifications?
>
> They do.
>
> >>> Same goes for
> >>> the end_id, assuming there's a standard way of putting that into a
> >>> single word. That will address the point I raised earlier about lisn
> >>> being passed around as a single word, but these later stage ids being
> >>> split.
> >>
> >> Hmm, I am not sure this is a good option. It is not how the PowerNV
> >> model would use it, skiboot is very much aware of these blocks and
> >> indexes and for remote accesses chips are identified using the block.
> >> I will take a look at it but I am not found of it. I can add helpers
> >> in some places though.
> >
> > Hm, ok. Do the block and index appear as an (effectively) single
> > field in the EAS?
>
> no. In all XIVE structures, block and index are always distinct.
Hm. Distinct in what sense? I get that the fields are labelled
separately in the documentation, but if the fields are adjacent, you
could equally well treat them as one.
> +if (!match->tctx) {
> +qemu_log_mask(LOG_GUEST_ERROR, "XIVE: NVT %x/%x is not
> dispatched\n",
> + nvt_blk, nvt_idx);
> +return false;
> >>>
> >>> Hmm.. this isn't actually an error isn't it? At least not for powernv
> >>
> >> It is on sPAPR, it would mean the END was configured with an unknow CPU.
> >
> > Right.
> >
> >> It is not error on PowerNV, when we support escalations.
> >>
> >>> - that just means the NVT isn't currently dispatched, so we'll need to
> >>> trigger the escalation interrupt.
> >>
> >> Yes.
> >>
> >>> Does this get changed later in the series?
> >>
> >> No.
> >
> > But this code is common to PAPR and powernv, yes, so it will need to?
>
> When we add support for escalations, yes, it will change. Would you rather
> use an error_report() until then ?
Ah, I guess leaving an error until we implement escalation makes
sense. It shouldn't be LOG_GUEST_ERROR, though, the guest didn't do
anything wrong, and error_report() doesn't really make sense for the
same reason.
LOG_UNIMP, I guess?
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
I forgot to reply to this one.
On 11/29/18 1:47 AM, David Gibson wrote:
> On Wed, Nov 28, 2018 at 11:59:58AM +0100, Cédric Le Goater wrote:
>> On 11/28/18 12:49 AM, David Gibson wrote:
>>> On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
The last sub-engine of the XIVE architecture is the Interrupt
Virtualization Presentation Engine (IVPE). On HW, they share elements,
the Power Bus interface (CQ), the routing table descriptors, and they
can be combined in the same HW logic. We do the same in QEMU and
combine both engines in the XiveRouter for simplicity.
>>>
>>> Ok, I'm not entirely convinced combining the IVPE and IVRE into a
>>> single object is a good idea, but we can probably discuss that once
>>> I've read further.
>>
>> We could introduce a simplified presenter for sPAPR but I am not even
>> sure of that as it will get more complex if we support the EBB one day.
>
> I wasn't really thinking about PAPR for this comment.
>
When the IVRE has completed its job of matching an event source with a
Notification Virtual Target (NVT) to notify, it forwards the event
notification to the IVPE sub-engine. The IVPE scans the thread
interrupt contexts of the Notification Virtual Targets (NVT)
dispatched on the HW processor threads and if a match is found, it
signals the thread. If not, the IVPE escalates the notification to
some other targets and records the notification in a backlog queue.
The IVPE maintains the thread interrupt context state for each of its
NVTs not dispatched on HW processor threads in the Notification
Virtual Target table (NVTT).
The model currently only supports single NVT notifications.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/xive.h | 13 +++
include/hw/ppc/xive_regs.h | 22
hw/intc/xive.c | 223 +
3 files changed, 258 insertions(+)
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 5987f26ddb98..e715a6c6923d 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -197,6 +197,10 @@ typedef struct XiveRouterClass {
XiveEND *end);
int (*set_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
XiveEND *end);
+int (*get_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
+ XiveNVT *nvt);
+int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
+ XiveNVT *nvt);
>>>
>>> As with the ENDs, I don't think get/set is a good interface for a
>>> bigger-than-word-size object.
>>
>> We need to agree on this interface before I respin. So you would like
>> to add a extra argument specifying the word being accessed ?
>
> Yes. Ok, 3 options I can see at this point:
>
> 1) read/write accessors which take a word number
>
> 2) A "get" accessor which copies the whole structure, but "write"
> accessor which takes a word number. The asymmetry is a bit ugly, but
> it's the non-atomic writeback of the whole structure which I'm most
> uncomfortable with.
>
> 3) A map/unmap interface which gives you / releases a pointer to the
> "live" structure. For powernv that would become
> address_space_map()/unmap(). For PAPR it would just be reutn pointer
> / no-op.
This discussion is in progress in another subthread.
>>
>>>
} XiveRouterClass;
void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
@@ -207,6 +211,10 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t
end_blk, uint32_t end_idx,
XiveEND *end);
int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t
end_idx,
XiveEND *end);
+int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t
nvt_idx,
+XiveNVT *nvt);
+int xive_router_set_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t
nvt_idx,
+XiveNVT *nvt);
/*
* XIVE END ESBs
@@ -274,4 +282,9 @@ extern const MemoryRegionOps xive_tm_ops;
void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
+static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t
nvt_idx)
+{
+return (nvt_blk << 19) | nvt_idx;
>>>
>>> I'm guessing this formula is the standard way of combining the NVT
>>> block and index into a single word?
>>
>> That number is the VP/NVT identifier which is written in the CAM value.
>> The index is on 19 bits because of the NVT definition in the END
>> structure. It is being increased to 24 bits on Power10
>>
>>> If so, I think we should
>>> standardize on passing a single word "nvt_id" around and only
>>> splitting it when we need to use the block separately.
>>
>> This is really the only
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Thu, Nov 29, 2018 at 06:51:53PM +0100, Cédric Le Goater wrote: > On 11/29/18 4:39 AM, Benjamin Herrenschmidt wrote: > > On Thu, 2018-11-29 at 11:47 +1100, David Gibson wrote: > >> > >> 1) read/write accessors which take a word number > > ok for single word updates of the structures. > > >> 2) A "get" accessor which copies the whole structure, > > ok > > >> but "write" > >> accessor which takes a word number. The asymmetry is a bit ugly, but > >> it's the non-atomic writeback of the whole structure which I'm most > >> uncomfortable with. > > And, how would you make the update of the whole structure in RAM look > "atomic" under QEMU ? So, the BQL means it actually is atomic now (at least for PAPR where the guest doesn't have access to it), but I don't want to rely on that always being the case - there are moves to put less stuff under the BQL, and with KVM we might be mapping some of these things such that real hardware can touch it. But the real point is that we don't *need* it to be atomic. Perhaps the individual field updates need to be atomic, but not writes to the END as a whole. Writing back the whole thing is also a whole heap of unnecessary stores. > > It shouldn't be a big deal though, there are HW facilities to access > > the structures "atomically" anyway, due to the caching done by the > > XIVE. > > Are you suggesting that the PowerNV model should update the VPC, EQC, > IVC in the VST accessors before updating the VSTs in RAM ? > >> 3) A map/unmap interface which gives you / releases a pointer to the > >> "live" structure. For powernv that would become > >> address_space_map()/unmap(). > > yes. > > >> For PAPR it would just be reutn pointer / no-op. > > ok > > I think I will introduce these handlers progressively in the patchset. > > Thanks, > > C. > > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On 11/29/18 4:39 AM, Benjamin Herrenschmidt wrote: > On Thu, 2018-11-29 at 11:47 +1100, David Gibson wrote: >> >> 1) read/write accessors which take a word number ok for single word updates of the structures. >> 2) A "get" accessor which copies the whole structure, ok >> but "write" >> accessor which takes a word number. The asymmetry is a bit ugly, but >> it's the non-atomic writeback of the whole structure which I'm most >> uncomfortable with. And, how would you make the update of the whole structure in RAM look "atomic" under QEMU ? > It shouldn't be a big deal though, there are HW facilities to access > the structures "atomically" anyway, due to the caching done by the > XIVE. Are you suggesting that the PowerNV model should update the VPC, EQC, IVC in the VST accessors before updating the VSTs in RAM ? >> 3) A map/unmap interface which gives you / releases a pointer to the >> "live" structure. For powernv that would become >> address_space_map()/unmap(). yes. >> For PAPR it would just be reutn pointer / no-op. ok I think I will introduce these handlers progressively in the patchset. Thanks, C.
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Thu, 2018-11-29 at 11:47 +1100, David Gibson wrote: > > 1) read/write accessors which take a word number > > 2) A "get" accessor which copies the whole structure, but "write" > accessor which takes a word number. The asymmetry is a bit ugly, but > it's the non-atomic writeback of the whole structure which I'm most > uncomfortable with. It shouldn't be a big deal though, there are HW facilities to access the structures "atomically" anyway, due to the caching done by the XIVE. > 3) A map/unmap interface which gives you / releases a pointer to the > "live" structure. For powernv that would become > address_space_map()/unmap(). For PAPR it would just be reutn pointer > / no-op.
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Wed, Nov 28, 2018 at 11:59:58AM +0100, Cédric Le Goater wrote:
> On 11/28/18 12:49 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
> >> The last sub-engine of the XIVE architecture is the Interrupt
> >> Virtualization Presentation Engine (IVPE). On HW, they share elements,
> >> the Power Bus interface (CQ), the routing table descriptors, and they
> >> can be combined in the same HW logic. We do the same in QEMU and
> >> combine both engines in the XiveRouter for simplicity.
> >
> > Ok, I'm not entirely convinced combining the IVPE and IVRE into a
> > single object is a good idea, but we can probably discuss that once
> > I've read further.
>
> We could introduce a simplified presenter for sPAPR but I am not even
> sure of that as it will get more complex if we support the EBB one day.
I wasn't really thinking about PAPR for this comment.
> >> When the IVRE has completed its job of matching an event source with a
> >> Notification Virtual Target (NVT) to notify, it forwards the event
> >> notification to the IVPE sub-engine. The IVPE scans the thread
> >> interrupt contexts of the Notification Virtual Targets (NVT)
> >> dispatched on the HW processor threads and if a match is found, it
> >> signals the thread. If not, the IVPE escalates the notification to
> >> some other targets and records the notification in a backlog queue.
> >>
> >> The IVPE maintains the thread interrupt context state for each of its
> >> NVTs not dispatched on HW processor threads in the Notification
> >> Virtual Target table (NVTT).
> >>
> >> The model currently only supports single NVT notifications.
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> include/hw/ppc/xive.h | 13 +++
> >> include/hw/ppc/xive_regs.h | 22
> >> hw/intc/xive.c | 223 +
> >> 3 files changed, 258 insertions(+)
> >>
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index 5987f26ddb98..e715a6c6923d 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -197,6 +197,10 @@ typedef struct XiveRouterClass {
> >> XiveEND *end);
> >> int (*set_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >> XiveEND *end);
> >> +int (*get_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> >> + XiveNVT *nvt);
> >> +int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> >> + XiveNVT *nvt);
> >
> > As with the ENDs, I don't think get/set is a good interface for a
> > bigger-than-word-size object.
>
> We need to agree on this interface before I respin. So you would like
> to add a extra argument specifying the word being accessed ?
Yes. Ok, 3 options I can see at this point:
1) read/write accessors which take a word number
2) A "get" accessor which copies the whole structure, but "write"
accessor which takes a word number. The asymmetry is a bit ugly, but
it's the non-atomic writeback of the whole structure which I'm most
uncomfortable with.
3) A map/unmap interface which gives you / releases a pointer to the
"live" structure. For powernv that would become
address_space_map()/unmap(). For PAPR it would just be reutn pointer
/ no-op.
>
> >
> >> } XiveRouterClass;
> >>
> >> void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
> >> @@ -207,6 +211,10 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t
> >> end_blk, uint32_t end_idx,
> >> XiveEND *end);
> >> int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t
> >> end_idx,
> >> XiveEND *end);
> >> +int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t
> >> nvt_idx,
> >> +XiveNVT *nvt);
> >> +int xive_router_set_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t
> >> nvt_idx,
> >> +XiveNVT *nvt);
> >>
> >> /*
> >> * XIVE END ESBs
> >> @@ -274,4 +282,9 @@ extern const MemoryRegionOps xive_tm_ops;
> >>
> >> void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
> >>
> >> +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t
> >> nvt_idx)
> >> +{
> >> +return (nvt_blk << 19) | nvt_idx;
> >
> > I'm guessing this formula is the standard way of combining the NVT
> > block and index into a single word?
>
> That number is the VP/NVT identifier which is written in the CAM value.
> The index is on 19 bits because of the NVT definition in the END
> structure. It is being increased to 24 bits on Power10
>
> > If so, I think we should
> > standardize on passing a single word "nvt_id" around and only
> > splitting it when we need to use the block separately.
>
> This is really the only place where we concatenate the two NVT values,
> block and index.
Hm, ok. I know we don't model them (yet, maybe ever) but could
combined values appear in the PowerBUS
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On 11/28/18 12:49 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
>> The last sub-engine of the XIVE architecture is the Interrupt
>> Virtualization Presentation Engine (IVPE). On HW, they share elements,
>> the Power Bus interface (CQ), the routing table descriptors, and they
>> can be combined in the same HW logic. We do the same in QEMU and
>> combine both engines in the XiveRouter for simplicity.
>
> Ok, I'm not entirely convinced combining the IVPE and IVRE into a
> single object is a good idea, but we can probably discuss that once
> I've read further.
We could introduce a simplified presenter for sPAPR but I am not even
sure of that as it will get more complex if we support the EBB one day.
>> When the IVRE has completed its job of matching an event source with a
>> Notification Virtual Target (NVT) to notify, it forwards the event
>> notification to the IVPE sub-engine. The IVPE scans the thread
>> interrupt contexts of the Notification Virtual Targets (NVT)
>> dispatched on the HW processor threads and if a match is found, it
>> signals the thread. If not, the IVPE escalates the notification to
>> some other targets and records the notification in a backlog queue.
>>
>> The IVPE maintains the thread interrupt context state for each of its
>> NVTs not dispatched on HW processor threads in the Notification
>> Virtual Target table (NVTT).
>>
>> The model currently only supports single NVT notifications.
>>
>> Signed-off-by: Cédric Le Goater
>> ---
>> include/hw/ppc/xive.h | 13 +++
>> include/hw/ppc/xive_regs.h | 22
>> hw/intc/xive.c | 223 +
>> 3 files changed, 258 insertions(+)
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 5987f26ddb98..e715a6c6923d 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -197,6 +197,10 @@ typedef struct XiveRouterClass {
>> XiveEND *end);
>> int (*set_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>> XiveEND *end);
>> +int (*get_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
>> + XiveNVT *nvt);
>> +int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
>> + XiveNVT *nvt);
>
> As with the ENDs, I don't think get/set is a good interface for a
> bigger-than-word-size object.
We need to agree on this interface before I respin. So you would like
to add a extra argument specifying the word being accessed ?
>
>> } XiveRouterClass;
>>
>> void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
>> @@ -207,6 +211,10 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t
>> end_blk, uint32_t end_idx,
>> XiveEND *end);
>> int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>> XiveEND *end);
>> +int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
>> +XiveNVT *nvt);
>> +int xive_router_set_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
>> +XiveNVT *nvt);
>>
>> /*
>> * XIVE END ESBs
>> @@ -274,4 +282,9 @@ extern const MemoryRegionOps xive_tm_ops;
>>
>> void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
>>
>> +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
>> +{
>> +return (nvt_blk << 19) | nvt_idx;
>
> I'm guessing this formula is the standard way of combining the NVT
> block and index into a single word?
That number is the VP/NVT identifier which is written in the CAM value.
The index is on 19 bits because of the NVT definition in the END
structure. It is being increased to 24 bits on Power10
> If so, I think we should
> standardize on passing a single word "nvt_id" around and only
> splitting it when we need to use the block separately.
This is really the only place where we concatenate the two NVT values,
block and index.
> Same goes for
> the end_id, assuming there's a standard way of putting that into a
> single word. That will address the point I raised earlier about lisn
> being passed around as a single word, but these later stage ids being
> split.
Hmm, I am not sure this is a good option. It is not how the PowerNV
model would use it, skiboot is very much aware of these blocks and
indexes and for remote accesses chips are identified using the block.
I will take a look at it but I am not found of it. I can add helpers
in some places though.
I agree we have some kind of issue linking the HW model with the sPAPR
machine. The guest interface is only about IRQ numbers, priorities and
cpu numbers. We really don't care about XIVE blocks and indexes in that
case. we can clarify the code by bypassing the XiveRouter interfaces
to the table and directly use the sPAPR interrupt controller. That
should help a bit for the hcalls but we would
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Wed, 2018-11-28 at 10:49 +1100, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
> > The last sub-engine of the XIVE architecture is the Interrupt
> > Virtualization Presentation Engine (IVPE). On HW, they share elements,
> > the Power Bus interface (CQ), the routing table descriptors, and they
> > can be combined in the same HW logic. We do the same in QEMU and
> > combine both engines in the XiveRouter for simplicity.
>
> Ok, I'm not entirely convinced combining the IVPE and IVRE into a
> single object is a good idea, but we can probably discuss that once
> I've read further.
Keep in mind that the communication between the two is a bit more hairy
than simple notifications, though. Especially once we start
implementing escalation interrupts or worse, groups...
> > When the IVRE has completed its job of matching an event source with a
> > Notification Virtual Target (NVT) to notify, it forwards the event
> > notification to the IVPE sub-engine. The IVPE scans the thread
> > interrupt contexts of the Notification Virtual Targets (NVT)
> > dispatched on the HW processor threads and if a match is found, it
> > signals the thread. If not, the IVPE escalates the notification to
> > some other targets and records the notification in a backlog queue.
> >
> > The IVPE maintains the thread interrupt context state for each of its
> > NVTs not dispatched on HW processor threads in the Notification
> > Virtual Target table (NVTT).
> >
> > The model currently only supports single NVT notifications.
> >
> > Signed-off-by: Cédric Le Goater
> > ---
> > include/hw/ppc/xive.h | 13 +++
> > include/hw/ppc/xive_regs.h | 22
> > hw/intc/xive.c | 223 +
> > 3 files changed, 258 insertions(+)
> >
> > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> > index 5987f26ddb98..e715a6c6923d 100644
> > --- a/include/hw/ppc/xive.h
> > +++ b/include/hw/ppc/xive.h
> > @@ -197,6 +197,10 @@ typedef struct XiveRouterClass {
> > XiveEND *end);
> > int (*set_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> > XiveEND *end);
> > +int (*get_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> > + XiveNVT *nvt);
> > +int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> > + XiveNVT *nvt);
>
> As with the ENDs, I don't think get/set is a good interface for a
> bigger-than-word-size object.
>
> > } XiveRouterClass;
> >
> > void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
> > @@ -207,6 +211,10 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t
> > end_blk, uint32_t end_idx,
> > XiveEND *end);
> > int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t
> > end_idx,
> > XiveEND *end);
> > +int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t
> > nvt_idx,
> > +XiveNVT *nvt);
> > +int xive_router_set_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t
> > nvt_idx,
> > +XiveNVT *nvt);
> >
> > /*
> > * XIVE END ESBs
> > @@ -274,4 +282,9 @@ extern const MemoryRegionOps xive_tm_ops;
> >
> > void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
> >
> > +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t
> > nvt_idx)
> > +{
> > +return (nvt_blk << 19) | nvt_idx;
>
> I'm guessing this formula is the standard way of combining the NVT
> block and index into a single word? If so, I think we should
> standardize on passing a single word "nvt_id" around and only
> splitting it when we need to use the block separately. Same goes for
> the end_id, assuming there's a standard way of putting that into a
> single word. That will address the point I raised earlier about lisn
> being passed around as a single word, but these later stage ids being
> split.
>
> We'll probably want some inlines or macros to build an
> nvt/end/lisn/whatever id from block and index as well.
>
> > +}
> > +
> > #endif /* PPC_XIVE_H */
> > diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
> > index 2e3d6cb507da..05cb992d2815 100644
> > --- a/include/hw/ppc/xive_regs.h
> > +++ b/include/hw/ppc/xive_regs.h
> > @@ -158,4 +158,26 @@ typedef struct XiveEND {
> > #define END_W7_F1_LOG_SERVER_ID PPC_BITMASK32(1, 31)
> > } XiveEND;
> >
> > +/* Notification Virtual Target (NVT) */
> > +typedef struct XiveNVT {
> > +uint32_tw0;
> > +#define NVT_W0_VALID PPC_BIT32(0)
> > +uint32_tw1;
> > +uint32_tw2;
> > +uint32_tw3;
> > +uint32_tw4;
> > +uint32_tw5;
> > +uint32_tw6;
> > +uint32_tw7;
> > +uint32_tw8;
> > +#define NVT_W8_GRP_VALID PPC_BIT32(0)
> > +uint32_tw9;
> > +
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
> The last sub-engine of the XIVE architecture is the Interrupt
> Virtualization Presentation Engine (IVPE). On HW, they share elements,
> the Power Bus interface (CQ), the routing table descriptors, and they
> can be combined in the same HW logic. We do the same in QEMU and
> combine both engines in the XiveRouter for simplicity.
Ok, I'm not entirely convinced combining the IVPE and IVRE into a
single object is a good idea, but we can probably discuss that once
I've read further.
> When the IVRE has completed its job of matching an event source with a
> Notification Virtual Target (NVT) to notify, it forwards the event
> notification to the IVPE sub-engine. The IVPE scans the thread
> interrupt contexts of the Notification Virtual Targets (NVT)
> dispatched on the HW processor threads and if a match is found, it
> signals the thread. If not, the IVPE escalates the notification to
> some other targets and records the notification in a backlog queue.
>
> The IVPE maintains the thread interrupt context state for each of its
> NVTs not dispatched on HW processor threads in the Notification
> Virtual Target table (NVTT).
>
> The model currently only supports single NVT notifications.
>
> Signed-off-by: Cédric Le Goater
> ---
> include/hw/ppc/xive.h | 13 +++
> include/hw/ppc/xive_regs.h | 22
> hw/intc/xive.c | 223 +
> 3 files changed, 258 insertions(+)
>
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 5987f26ddb98..e715a6c6923d 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -197,6 +197,10 @@ typedef struct XiveRouterClass {
> XiveEND *end);
> int (*set_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> XiveEND *end);
> +int (*get_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> + XiveNVT *nvt);
> +int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> + XiveNVT *nvt);
As with the ENDs, I don't think get/set is a good interface for a
bigger-than-word-size object.
> } XiveRouterClass;
>
> void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
> @@ -207,6 +211,10 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t
> end_blk, uint32_t end_idx,
> XiveEND *end);
> int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> XiveEND *end);
> +int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> +XiveNVT *nvt);
> +int xive_router_set_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> +XiveNVT *nvt);
>
> /*
> * XIVE END ESBs
> @@ -274,4 +282,9 @@ extern const MemoryRegionOps xive_tm_ops;
>
> void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
>
> +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
> +{
> +return (nvt_blk << 19) | nvt_idx;
I'm guessing this formula is the standard way of combining the NVT
block and index into a single word? If so, I think we should
standardize on passing a single word "nvt_id" around and only
splitting it when we need to use the block separately. Same goes for
the end_id, assuming there's a standard way of putting that into a
single word. That will address the point I raised earlier about lisn
being passed around as a single word, but these later stage ids being
split.
We'll probably want some inlines or macros to build an
nvt/end/lisn/whatever id from block and index as well.
> +}
> +
> #endif /* PPC_XIVE_H */
> diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
> index 2e3d6cb507da..05cb992d2815 100644
> --- a/include/hw/ppc/xive_regs.h
> +++ b/include/hw/ppc/xive_regs.h
> @@ -158,4 +158,26 @@ typedef struct XiveEND {
> #define END_W7_F1_LOG_SERVER_ID PPC_BITMASK32(1, 31)
> } XiveEND;
>
> +/* Notification Virtual Target (NVT) */
> +typedef struct XiveNVT {
> +uint32_tw0;
> +#define NVT_W0_VALID PPC_BIT32(0)
> +uint32_tw1;
> +uint32_tw2;
> +uint32_tw3;
> +uint32_tw4;
> +uint32_tw5;
> +uint32_tw6;
> +uint32_tw7;
> +uint32_tw8;
> +#define NVT_W8_GRP_VALID PPC_BIT32(0)
> +uint32_tw9;
> +uint32_twa;
> +uint32_twb;
> +uint32_twc;
> +uint32_twd;
> +uint32_twe;
> +uint32_twf;
> +} XiveNVT;
> +
> #endif /* PPC_XIVE_REGS_H */
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 4c6cb5d52975..5ba3b06e6e25 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -373,6 +373,32 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor
> *mon)
> }
> }
>
> +/* The
[Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
The last sub-engine of the XIVE architecture is the Interrupt
Virtualization Presentation Engine (IVPE). On HW, they share elements,
the Power Bus interface (CQ), the routing table descriptors, and they
can be combined in the same HW logic. We do the same in QEMU and
combine both engines in the XiveRouter for simplicity.
When the IVRE has completed its job of matching an event source with a
Notification Virtual Target (NVT) to notify, it forwards the event
notification to the IVPE sub-engine. The IVPE scans the thread
interrupt contexts of the Notification Virtual Targets (NVT)
dispatched on the HW processor threads and if a match is found, it
signals the thread. If not, the IVPE escalates the notification to
some other targets and records the notification in a backlog queue.
The IVPE maintains the thread interrupt context state for each of its
NVTs not dispatched on HW processor threads in the Notification
Virtual Target table (NVTT).
The model currently only supports single NVT notifications.
Signed-off-by: Cédric Le Goater
---
include/hw/ppc/xive.h | 13 +++
include/hw/ppc/xive_regs.h | 22
hw/intc/xive.c | 223 +
3 files changed, 258 insertions(+)
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index 5987f26ddb98..e715a6c6923d 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -197,6 +197,10 @@ typedef struct XiveRouterClass {
XiveEND *end);
int (*set_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
XiveEND *end);
+int (*get_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
+ XiveNVT *nvt);
+int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
+ XiveNVT *nvt);
} XiveRouterClass;
void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
@@ -207,6 +211,10 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk,
uint32_t end_idx,
XiveEND *end);
int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
XiveEND *end);
+int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
+XiveNVT *nvt);
+int xive_router_set_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
+XiveNVT *nvt);
/*
* XIVE END ESBs
@@ -274,4 +282,9 @@ extern const MemoryRegionOps xive_tm_ops;
void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
+static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
+{
+return (nvt_blk << 19) | nvt_idx;
+}
+
#endif /* PPC_XIVE_H */
diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
index 2e3d6cb507da..05cb992d2815 100644
--- a/include/hw/ppc/xive_regs.h
+++ b/include/hw/ppc/xive_regs.h
@@ -158,4 +158,26 @@ typedef struct XiveEND {
#define END_W7_F1_LOG_SERVER_ID PPC_BITMASK32(1, 31)
} XiveEND;
+/* Notification Virtual Target (NVT) */
+typedef struct XiveNVT {
+uint32_tw0;
+#define NVT_W0_VALID PPC_BIT32(0)
+uint32_tw1;
+uint32_tw2;
+uint32_tw3;
+uint32_tw4;
+uint32_tw5;
+uint32_tw6;
+uint32_tw7;
+uint32_tw8;
+#define NVT_W8_GRP_VALID PPC_BIT32(0)
+uint32_tw9;
+uint32_twa;
+uint32_twb;
+uint32_twc;
+uint32_twd;
+uint32_twe;
+uint32_twf;
+} XiveNVT;
+
#endif /* PPC_XIVE_REGS_H */
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 4c6cb5d52975..5ba3b06e6e25 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -373,6 +373,32 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
}
}
+/* The HW CAM (23bits) is hardwired to :
+ *
+ * 0x000||0b1||4Bit chip number||7Bit Thread number.
+ *
+ * and when the block grouping extension is enabled :
+ *
+ * 4Bit chip number||0x001||7Bit Thread number.
+ */
+static uint32_t tctx_hw_cam_line(bool block_group, uint8_t chip_id, uint8_t
tid)
+{
+if (block_group) {
+return 1 << 11 | (chip_id & 0xf) << 7 | (tid & 0x7f);
+} else {
+return (chip_id & 0xf) << 11 | 1 << 7 | (tid & 0x7f);
+}
+}
+
+static uint32_t xive_tctx_hw_cam_line(XiveTCTX *tctx, bool block_group)
+{
+PowerPCCPU *cpu = POWERPC_CPU(tctx->cs);
+CPUPPCState *env = >env;
+uint32_t pir = env->spr_cb[SPR_PIR].default_value;
+
+return tctx_hw_cam_line(block_group, (pir >> 8) & 0xf, pir & 0x7f);
+}
+
static void xive_tctx_reset(void *dev)
{
XiveTCTX *tctx = XIVE_TCTX(dev);
@@ -1013,6 +1039,195 @@ int xive_router_set_end(XiveRouter *xrtr, uint8_t
end_blk, uint32_t end_idx,
return xrc->set_end(xrtr, end_blk, end_idx, end);
}
+int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
+XiveNVT
