| Message ID | 1499274819-15607-9-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 05, 2017 at 07:13:21PM +0200, Cédric Le Goater wrote: > These flags define some characteristics of the source : > > - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a > specific hcall H_INT_ESB What's the other option? > - XIVE_SRC_LSI LSI or MSI source Hrm. This definitely duplicates info that is in the XICS per irq state which you're re-using (and which you're using in the xive code at this point). > - XIVE_SRC_TRIGGER the full function page supports trigger > - XIVE_SRC_STORE_EOI EOI can with a store. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/intc/xive.c | 1 + > include/hw/ppc/xive.h | 9 +++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 816031b8ac81..8f8bb8b787bd 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -345,6 +345,7 @@ static Property xive_ics_properties[] = { > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > DEFINE_PROP_UINT32("irq-base", ICSState, offset, 0), > DEFINE_PROP_UINT32("shift", XiveICSState, esb_shift, 0), > + DEFINE_PROP_UINT64("flags", XiveICSState, flags, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > index 5303d96f5f59..1178300c9df3 100644 > --- a/include/hw/ppc/xive.h > +++ b/include/hw/ppc/xive.h > @@ -30,9 +30,18 @@ typedef struct XiveICSState XiveICSState; > #define TYPE_ICS_XIVE "xive-source" > #define ICS_XIVE(obj) OBJECT_CHECK(XiveICSState, (obj), TYPE_ICS_XIVE) > > +/* > + * XIVE Interrupt source flags > + */ > +#define XIVE_SRC_H_INT_ESB (1ull << (63 - 60)) > +#define XIVE_SRC_LSI (1ull << (63 - 61)) > +#define XIVE_SRC_TRIGGER (1ull << (63 - 62)) > +#define XIVE_SRC_STORE_EOI (1ull << (63 - 63)) > + > struct XiveICSState { > ICSState parent_obj; > > + uint64_t flags; > uint32_t esb_shift; > MemoryRegion esb_iomem; >
On Mon, 2017-07-24 at 14:36 +1000, David Gibson wrote: > On Wed, Jul 05, 2017 at 07:13:21PM +0200, Cédric Le Goater wrote: > > These flags define some characteristics of the source : > > > > - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a > > specific hcall H_INT_ESB > > What's the other option? Direct MMIO access. Normally all interrupts use normal MMIOs, each interrupts has an associated MMIO page with special MMIOs to control the source state (PQ bits). This is something I added to the PAPR spec (and the OPAL <-> Linux interface) to allow firmware to work around broken HW (which happens on some P9 versions). > > - XIVE_SRC_LSI LSI or MSI source > > Hrm. This definitely duplicates info that is in the XICS per irq > state which you're re-using (and which you're using in the xive code > at this point). I think all those flags correspond to the flags passed via the PAPR API, so it makes sense to have them there. > > - XIVE_SRC_TRIGGER the full function page supports trigger > > - XIVE_SRC_STORE_EOI EOI can with a store. > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > --- > > hw/intc/xive.c | 1 + > > include/hw/ppc/xive.h | 9 +++++++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > > index 816031b8ac81..8f8bb8b787bd 100644 > > --- a/hw/intc/xive.c > > +++ b/hw/intc/xive.c > > @@ -345,6 +345,7 @@ static Property xive_ics_properties[] = { > > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > > DEFINE_PROP_UINT32("irq-base", ICSState, offset, 0), > > DEFINE_PROP_UINT32("shift", XiveICSState, esb_shift, 0), > > + DEFINE_PROP_UINT64("flags", XiveICSState, flags, 0), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > > index 5303d96f5f59..1178300c9df3 100644 > > --- a/include/hw/ppc/xive.h > > +++ b/include/hw/ppc/xive.h > > @@ -30,9 +30,18 @@ typedef struct XiveICSState XiveICSState; > > #define TYPE_ICS_XIVE "xive-source" > > #define ICS_XIVE(obj) OBJECT_CHECK(XiveICSState, (obj), TYPE_ICS_XIVE) > > > > +/* > > + * XIVE Interrupt source flags > > + */ > > +#define XIVE_SRC_H_INT_ESB (1ull << (63 - 60)) > > +#define XIVE_SRC_LSI (1ull << (63 - 61)) > > +#define XIVE_SRC_TRIGGER (1ull << (63 - 62)) > > +#define XIVE_SRC_STORE_EOI (1ull << (63 - 63)) > > + > > struct XiveICSState { > > ICSState parent_obj; > > > > + uint64_t flags; > > uint32_t esb_shift; > > MemoryRegion esb_iomem; > > > >
On Mon, Jul 24, 2017 at 05:00:57PM +1000, Benjamin Herrenschmidt wrote: > On Mon, 2017-07-24 at 14:36 +1000, David Gibson wrote: > > On Wed, Jul 05, 2017 at 07:13:21PM +0200, Cédric Le Goater wrote: > > > These flags define some characteristics of the source : > > > > > > - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a > > > specific hcall H_INT_ESB > > > > What's the other option? > > Direct MMIO access. Normally all interrupts use normal MMIOs, > each interrupts has an associated MMIO page with special MMIOs > to control the source state (PQ bits). This is something I added > to the PAPR spec (and the OPAL <-> Linux interface) to allow firmware > to work around broken HW (which happens on some P9 versions). Ok.. and that's something that can be decided at runtime?
On Mon, 2017-07-24 at 19:50 +1000, David Gibson wrote: > On Mon, Jul 24, 2017 at 05:00:57PM +1000, Benjamin Herrenschmidt wrote: > > On Mon, 2017-07-24 at 14:36 +1000, David Gibson wrote: > > > On Wed, Jul 05, 2017 at 07:13:21PM +0200, Cédric Le Goater wrote: > > > > These flags define some characteristics of the source : > > > > > > > > - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a > > > > specific hcall H_INT_ESB > > > > > > What's the other option? > > > > Direct MMIO access. Normally all interrupts use normal MMIOs, > > each interrupts has an associated MMIO page with special MMIOs > > to control the source state (PQ bits). This is something I added > > to the PAPR spec (and the OPAL <-> Linux interface) to allow firmware > > to work around broken HW (which happens on some P9 versions). > > Ok.. and that's something that can be decided at runtime? Well, at this point I think nothing will set that flag.... It's there for workaround around HW bugs on some chips. At least in full emu it shouldn't happen unless we try to emulate those bugs. Hopefully direct MMIO will just work. Ben.
On 07/24/2017 01:07 PM, Benjamin Herrenschmidt wrote: > On Mon, 2017-07-24 at 19:50 +1000, David Gibson wrote: >> On Mon, Jul 24, 2017 at 05:00:57PM +1000, Benjamin Herrenschmidt wrote: >>> On Mon, 2017-07-24 at 14:36 +1000, David Gibson wrote: >>>> On Wed, Jul 05, 2017 at 07:13:21PM +0200, Cédric Le Goater wrote: >>>>> These flags define some characteristics of the source : >>>>> >>>>> - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a >>>>> specific hcall H_INT_ESB >>>> >>>> What's the other option? >>> >>> Direct MMIO access. Normally all interrupts use normal MMIOs, >>> each interrupts has an associated MMIO page with special MMIOs >>> to control the source state (PQ bits). This is something I added >>> to the PAPR spec (and the OPAL <-> Linux interface) to allow firmware >>> to work around broken HW (which happens on some P9 versions). >> >> Ok.. and that's something that can be decided at runtime? > > Well, at this point I think nothing will set that flag.... It's there > for workaround around HW bugs on some chips. At least in full emu it > shouldn't happen unless we try to emulate those bugs. Hopefully direct > MMIO will just work. Nevertheless I have added support for the hcall in Linux and QEMU. To use, I think we could create a specific source. C.
On Mon, Jul 24, 2017 at 09:07:19PM +1000, Benjamin Herrenschmidt wrote: > On Mon, 2017-07-24 at 19:50 +1000, David Gibson wrote: > > On Mon, Jul 24, 2017 at 05:00:57PM +1000, Benjamin Herrenschmidt wrote: > > > On Mon, 2017-07-24 at 14:36 +1000, David Gibson wrote: > > > > On Wed, Jul 05, 2017 at 07:13:21PM +0200, Cédric Le Goater wrote: > > > > > These flags define some characteristics of the source : > > > > > > > > > > - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a > > > > > specific hcall H_INT_ESB > > > > > > > > What's the other option? > > > > > > Direct MMIO access. Normally all interrupts use normal MMIOs, > > > each interrupts has an associated MMIO page with special MMIOs > > > to control the source state (PQ bits). This is something I added > > > to the PAPR spec (and the OPAL <-> Linux interface) to allow firmware > > > to work around broken HW (which happens on some P9 versions). > > > > Ok.. and that's something that can be decided at runtime? > > Well, at this point I think nothing will set that flag.... It's there > for workaround around HW bugs on some chips. At least in full emu it > shouldn't happen unless we try to emulate those bugs. Hopefully direct > MMIO will just work. Hm. That doesn't seem like a good match for a per-irq state structure.
On Mon, Jul 24, 2017 at 01:47:28PM +0200, Cédric Le Goater wrote: > On 07/24/2017 01:07 PM, Benjamin Herrenschmidt wrote: > > On Mon, 2017-07-24 at 19:50 +1000, David Gibson wrote: > >> On Mon, Jul 24, 2017 at 05:00:57PM +1000, Benjamin Herrenschmidt wrote: > >>> On Mon, 2017-07-24 at 14:36 +1000, David Gibson wrote: > >>>> On Wed, Jul 05, 2017 at 07:13:21PM +0200, Cédric Le Goater wrote: > >>>>> These flags define some characteristics of the source : > >>>>> > >>>>> - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a > >>>>> specific hcall H_INT_ESB > >>>> > >>>> What's the other option? > >>> > >>> Direct MMIO access. Normally all interrupts use normal MMIOs, > >>> each interrupts has an associated MMIO page with special MMIOs > >>> to control the source state (PQ bits). This is something I added > >>> to the PAPR spec (and the OPAL <-> Linux interface) to allow firmware > >>> to work around broken HW (which happens on some P9 versions). > >> > >> Ok.. and that's something that can be decided at runtime? > > > > Well, at this point I think nothing will set that flag.... It's there > > for workaround around HW bugs on some chips. At least in full emu it > > shouldn't happen unless we try to emulate those bugs. Hopefully direct > > MMIO will just work. > > Nevertheless I have added support for the hcall in Linux and QEMU. > To use, I think we could create a specific source. So, IIUC, it's host constraints that would make this one way or the other. So what happens when a guest migrates from a host which has it one way to one which has it the other way?
On Tue, 2017-07-25 at 14:18 +1000, David Gibson wrote: > > Well, at this point I think nothing will set that flag.... It's there > > for workaround around HW bugs on some chips. At least in full emu it > > shouldn't happen unless we try to emulate those bugs. Hopefully direct > > MMIO will just work. > > Hm. That doesn't seem like a good match for a per-irq state > structure. The flag is returned to the guest on a per-IRQ basis, so are the LSI etc... flags, but at the HW level, indeed, they correspond to attributes of blocks of interrupts. It might be easier in qemu to keep that in the per-source flags though. Especially when we start having actual HW interrupts under the hood with KVM. it's easier to keep the state self contained for each of them. Ben.
On Tue, 2017-07-25 at 14:19 +1000, David Gibson wrote: > > Nevertheless I have added support for the hcall in Linux and QEMU. > > To use, I think we could create a specific source. > > So, IIUC, it's host constraints that would make this one way or the > other. So what happens when a guest migrates from a host which has it > one way to one which has it the other way? It's probably ok to always call the hcall for the awy set -> unset, the other way around is a problem, but it will end up depending on the kind of interrupts we pass through. Ben.
On 07/24/2017 11:50 AM, David Gibson wrote: > On Mon, Jul 24, 2017 at 05:00:57PM +1000, Benjamin Herrenschmidt wrote: >> On Mon, 2017-07-24 at 14:36 +1000, David Gibson wrote: >>> On Wed, Jul 05, 2017 at 07:13:21PM +0200, Cédric Le Goater wrote: >>>> These flags define some characteristics of the source : >>>> >>>> - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a >>>> specific hcall H_INT_ESB >>> >>> What's the other option? >> >> Direct MMIO access. Normally all interrupts use normal MMIOs, >> each interrupts has an associated MMIO page with special MMIOs >> to control the source state (PQ bits). This is something I added >> to the PAPR spec (and the OPAL <-> Linux interface) to allow firmware >> to work around broken HW (which happens on some P9 versions). > > Ok.. and that's something that can be decided at runtime? > This is a characteristic of an Interrupt Source and the associated object should be created with such a flag. But I don't think will ever use it in QEMU, maybe with KVM. C.
On 07/25/2017 07:47 AM, Benjamin Herrenschmidt wrote: > On Tue, 2017-07-25 at 14:18 +1000, David Gibson wrote: >>> Well, at this point I think nothing will set that flag.... It's there >>> for workaround around HW bugs on some chips. At least in full emu it >>> shouldn't happen unless we try to emulate those bugs. Hopefully direct >>> MMIO will just work. >> >> Hm. That doesn't seem like a good match for a per-irq state >> structure. > > The flag is returned to the guest on a per-IRQ basis, so are the LSI > etc... flags, but at the HW level, indeed, they correspond to > attributes of blocks of interrupts. > > It might be easier in qemu to keep that in the per-source flags > though. Theses flags are at the source level : XIVE_SRC_H_INT_ESB XIVE_SRC_TRIGGER XIVE_SRC_STORE_EOI The XIVE_SRC_LSI flag is only returned in the GET_SOURCE_INFO hcall but, internally, we use the ICSIRQState flags XICS_FLAGS_IRQ_LSI to store the information per-irq. Thanks, C. > Especially when we start having actual HW interrupts under the > hood with KVM. it's easier to keep the state self contained > for each of them. > > Ben. >
On Tue, Jul 25, 2017 at 03:47:37PM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2017-07-25 at 14:18 +1000, David Gibson wrote: > > > Well, at this point I think nothing will set that flag.... It's there > > > for workaround around HW bugs on some chips. At least in full emu it > > > shouldn't happen unless we try to emulate those bugs. Hopefully direct > > > MMIO will just work. > > > > Hm. That doesn't seem like a good match for a per-irq state > > structure. > > The flag is returned to the guest on a per-IRQ basis, so are the LSI > etc... flags, but at the HW level, indeed, they correspond to > attributes of blocks of interrupts. > > It might be easier in qemu to keep that in the per-source flags > though. Yeah, I think so. > Especially when we start having actual HW interrupts under the > hood with KVM. it's easier to keep the state self contained > for each of them. > > Ben. >
diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 816031b8ac81..8f8bb8b787bd 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -345,6 +345,7 @@ static Property xive_ics_properties[] = { DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), DEFINE_PROP_UINT32("irq-base", ICSState, offset, 0), DEFINE_PROP_UINT32("shift", XiveICSState, esb_shift, 0), + DEFINE_PROP_UINT64("flags", XiveICSState, flags, 0), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h index 5303d96f5f59..1178300c9df3 100644 --- a/include/hw/ppc/xive.h +++ b/include/hw/ppc/xive.h @@ -30,9 +30,18 @@ typedef struct XiveICSState XiveICSState; #define TYPE_ICS_XIVE "xive-source" #define ICS_XIVE(obj) OBJECT_CHECK(XiveICSState, (obj), TYPE_ICS_XIVE) +/* + * XIVE Interrupt source flags + */ +#define XIVE_SRC_H_INT_ESB (1ull << (63 - 60)) +#define XIVE_SRC_LSI (1ull << (63 - 61)) +#define XIVE_SRC_TRIGGER (1ull << (63 - 62)) +#define XIVE_SRC_STORE_EOI (1ull << (63 - 63)) + struct XiveICSState { ICSState parent_obj; + uint64_t flags; uint32_t esb_shift; MemoryRegion esb_iomem;
These flags define some characteristics of the source : - XIVE_SRC_H_INT_ESB the Event State Buffer are controlled with a specific hcall H_INT_ESB - XIVE_SRC_LSI LSI or MSI source - XIVE_SRC_TRIGGER the full function page supports trigger - XIVE_SRC_STORE_EOI EOI can with a store. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/xive.c | 1 + include/hw/ppc/xive.h | 9 +++++++++ 2 files changed, 10 insertions(+)