Re: [Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
On 22/09/15 11:44, Ian Campbell wrote: > On Tue, 2015-09-22 at 11:29 +0100, Julien Grall wrote: >> >> On 22/09/2015 11:05, Ian Campbell wrote: >>> On Tue, 2015-09-22 at 10:45 +0100, Julien Grall wrote: On 22/09/2015 10:17, Vijay Kilari wrote: >> Why do you need a command line option to enable/disable the >> physical >> ITS? > > I have added this to remove ITS support?. > Did I misunderstood it. May be I have to avoid generating its dt > node > to disable its for dom0? My question is why do you want to let the user disabling the ITS using the command line? What's the use case? >>> >>> It's always useful to be able to nobble this sort of thing, either for >>> debugging/development purposes or to allow users to workaround issues. >> >> This is can be done via the firmware table (see the property "status" in >> the DT). > > This is not always trivial to override (think DTB provided by the UEFI > firmware). Hmmm, right. I always have in mind the U-boot case and forgot the UEFI one. >> I see no advantage to use the command line for a such purpose, >> mainly for the debugging/development point. >> >> Also what kind of workaround do you expect to be fixed by disabling ITS? > > Any bug relating to running on an ITS which a user trips over in the field. TBH, I asked this question because it has been introduced in this version without any explanation. Maybe I'm too picky but I think it's important to know what the usage of a new option. Aside that, just reading the code, I can tell that this parameter can't even work on Thunder-X. With its_enable=false, the platform code (see patch #28) will fail to add the PCI and therefore return an error which will make Xen to fail building DOM0. > This isn't really any different from the myriad of options which already > exist to disable particular hardware features lists in > http://xenbits.xen.org/docs/unstable/misc/xen-command-line.html (many of > which are x86 specific, including I notice now msi=) >> I know that it will disable all the PCI MSI in general. But should not >> it be done with a PCI related parameter? > > Maybe there should be a PCI option as well, but disabling PCI MSI should > inherit from the disabling of the ITS. Just as it should be disabled if the > ITS isn't present at all. While today we support MSI only with ITS, we will have at some point GICv2m and maybe new interrupt controllers. I would much prefer having a common parameter (such a "msi=...") to disable MSI on the platform rather adding a new parameter for each new interrupt controller using MSI. It much easier to tell the user "please disable msi" rather than "please check what interrupt controller you are using" and after another round of mail "please use this option". > >>> Assuming the switch is simple an reasonably self contained (and it really >>> should be, since it should just be checked at start of day and then arrange >>> to behave like no ITS is present) then I can't see why not to have it. >> >> I think that disabling the ITS is more complex than not using in Xen. >> This will be tied with PCI passthrough very soon and we will have to >> spread this decision everywhere. >> >> We would also have to remove everything related to ITS (i.e >> msi-parent...) to the DT to avoid to pass an half-valid DT to DOM0. >> Otherwise we may just not be able to boot or using PCI (even without >> MSI). > > So this might fall into the "assuming the switch is simple" caveat which I > mentioned, although I'm not convinced we need to go this far. We could > equally well just mark the ITS node disabled and leave everything else > alone. Today the ITS driver in Linux doesn't check if the ITS node is disabled or not. If we don't take into account this fact, the ITS node are creating from scratch for DOM0. If the vITS is not present there will no region to create it. So adding fake node maybe more complex than just having a DT partially valid and hoping the guest doing the right thing. The behavior chosen should be documented in the docs/misc/xen-command-line.markdown to make clear what is expected with this option turned on. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
On Tue, 2015-09-22 at 11:29 +0100, Julien Grall wrote: > > On 22/09/2015 11:05, Ian Campbell wrote: > > On Tue, 2015-09-22 at 10:45 +0100, Julien Grall wrote: > > > > > > On 22/09/2015 10:17, Vijay Kilari wrote: > > > > > Why do you need a command line option to enable/disable the > > > > > physical > > > > > ITS? > > > > > > > > I have added this to remove ITS support?. > > > > Did I misunderstood it. May be I have to avoid generating its dt > > > > node > > > > to disable its for dom0? > > > > > > My question is why do you want to let the user disabling the ITS > > > using > > > the command line? What's the use case? > > > > It's always useful to be able to nobble this sort of thing, either for > > debugging/development purposes or to allow users to workaround issues. > > This is can be done via the firmware table (see the property "status" in > the DT). This is not always trivial to override (think DTB provided by the UEFI firmware). > I see no advantage to use the command line for a such purpose, > mainly for the debugging/development point. > > Also what kind of workaround do you expect to be fixed by disabling ITS? Any bug relating to running on an ITS which a user trips over in the field. This isn't really any different from the myriad of options which already exist to disable particular hardware features lists in http://xenbits.xen.org/docs/unstable/misc/xen-command-line.html (many of which are x86 specific, including I notice now msi=) > I know that it will disable all the PCI MSI in general. But should not > it be done with a PCI related parameter? Maybe there should be a PCI option as well, but disabling PCI MSI should inherit from the disabling of the ITS. Just as it should be disabled if the ITS isn't present at all. > > Assuming the switch is simple an reasonably self contained (and it really > > should be, since it should just be checked at start of day and then arrange > > to behave like no ITS is present) then I can't see why not to have it. > > I think that disabling the ITS is more complex than not using in Xen. > This will be tied with PCI passthrough very soon and we will have to > spread this decision everywhere. > > We would also have to remove everything related to ITS (i.e > msi-parent...) to the DT to avoid to pass an half-valid DT to DOM0. > Otherwise we may just not be able to boot or using PCI (even without > MSI). So this might fall into the "assuming the switch is simple" caveat which I mentioned, although I'm not convinced we need to go this far. We could equally well just mark the ITS node disabled and leave everything else alone. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
On 22/09/2015 11:05, Ian Campbell wrote: On Tue, 2015-09-22 at 10:45 +0100, Julien Grall wrote: On 22/09/2015 10:17, Vijay Kilari wrote: Why do you need a command line option to enable/disable the physical ITS? I have added this to remove ITS support?. Did I misunderstood it. May be I have to avoid generating its dt node to disable its for dom0? My question is why do you want to let the user disabling the ITS using the command line? What's the use case? It's always useful to be able to nobble this sort of thing, either for debugging/development purposes or to allow users to workaround issues. This is can be done via the firmware table (see the property "status" in the DT). I see no advantage to use the command line for a such purpose, mainly for the debugging/development point. Also what kind of workaround do you expect to be fixed by disabling ITS? I know that it will disable all the PCI MSI in general. But should not it be done with a PCI related parameter? Assuming the switch is simple an reasonably self contained (and it really should be, since it should just be checked at start of day and then arrange to behave like no ITS is present) then I can't see why not to have it. I think that disabling the ITS is more complex than not using in Xen. This will be tied with PCI passthrough very soon and we will have to spread this decision everywhere. We would also have to remove everything related to ITS (i.e msi-parent...) to the DT to avoid to pass an half-valid DT to DOM0. Otherwise we may just not be able to boot or using PCI (even without MSI). Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
On Tue, 2015-09-22 at 10:45 +0100, Julien Grall wrote: > > On 22/09/2015 10:17, Vijay Kilari wrote: > > > Why do you need a command line option to enable/disable the physical > > > ITS? > > > > I have added this to remove ITS support?. > > Did I misunderstood it. May be I have to avoid generating its dt node > > to disable its for dom0? > > My question is why do you want to let the user disabling the ITS using > the command line? What's the use case? It's always useful to be able to nobble this sort of thing, either for debugging/development purposes or to allow users to workaround issues. Assuming the switch is simple an reasonably self contained (and it really should be, since it should just be checked at start of day and then arrange to behave like no ITS is present) then I can't see why not to have it. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
On 22/09/2015 10:17, Vijay Kilari wrote: Why do you need a command line option to enable/disable the physical ITS? I have added this to remove ITS support?. Did I misunderstood it. May be I have to avoid generating its dt node to disable its for dom0? My question is why do you want to let the user disabling the ITS using the command line? What's the use case? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
On Mon, Sep 21, 2015 at 8:50 PM, Julien Grall wrote:
> Hi Vijay,
>
> On 18/09/15 14:09, vijay.kil...@gmail.com wrote:
>> From: Vijaya Kumar K
>>
>> Initialize physical ITS if HW supports LPIs.
>>
>> Signed-off-by: Vijaya Kumar K
>> ---
>> v7: - Export lpi support information to vgic-v3 driver from gic-v3.
>> - Drop gic_lpi_supported() helper function
>> - Add boot param to enable or disable physical ITS
>> v6: - Updated lpi_supported gic_info member for GICv2 and GICv3
>> - Introduced helper gic_lpi_supported() and exported
>> v5: - Made check of its dt node availability before
>> setting lpi_supported flag
>> ---
>> xen/arch/arm/gic-v3.c | 38
>> ++---
>> xen/arch/arm/vgic-v3.c|5 -
>> xen/include/asm-arm/gic_v3_defs.h |4 +++-
>> xen/include/asm-arm/vgic.h|2 +-
>> 4 files changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index c4c77a7..ac8a0ea 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -55,6 +55,18 @@ static struct {
>> } gicv3;
>>
>> static struct gic_info gicv3_info;
>> +/* Enable/Disable ITS support */
>> +static bool_t its_enable = 1;
>> +/* Availability of ITS support after successful ITS initialization */
>> +static bool_t its_enabled = 0;
>> +
>> +static void __init parse_its_param(char *s)
>> +{
>> +if ( !parse_bool(s) )
>> +its_enable = 0;
>> +}
>> +
>> +custom_param("its", parse_its_param);
>
> Why do you need a command line option to enable/disable the physical ITS?
I have added this to remove ITS support?.
Did I misunderstood it. May be I have to avoid generating its dt node
to disable its for dom0?
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
Hi Vijay,
On 18/09/15 14:09, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K
>
> Initialize physical ITS if HW supports LPIs.
>
> Signed-off-by: Vijaya Kumar K
> ---
> v7: - Export lpi support information to vgic-v3 driver from gic-v3.
> - Drop gic_lpi_supported() helper function
> - Add boot param to enable or disable physical ITS
> v6: - Updated lpi_supported gic_info member for GICv2 and GICv3
> - Introduced helper gic_lpi_supported() and exported
> v5: - Made check of its dt node availability before
> setting lpi_supported flag
> ---
> xen/arch/arm/gic-v3.c | 38
> ++---
> xen/arch/arm/vgic-v3.c|5 -
> xen/include/asm-arm/gic_v3_defs.h |4 +++-
> xen/include/asm-arm/vgic.h|2 +-
> 4 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c4c77a7..ac8a0ea 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -55,6 +55,18 @@ static struct {
> } gicv3;
>
> static struct gic_info gicv3_info;
> +/* Enable/Disable ITS support */
> +static bool_t its_enable = 1;
> +/* Availability of ITS support after successful ITS initialization */
> +static bool_t its_enabled = 0;
> +
> +static void __init parse_its_param(char *s)
> +{
> +if ( !parse_bool(s) )
> +its_enable = 0;
> +}
> +
> +custom_param("its", parse_its_param);
Why do you need a command line option to enable/disable the physical ITS?
>
> /* per-cpu re-distributor base */
> DEFINE_PER_CPU(struct rdist, rdist);
> @@ -590,7 +602,7 @@ static void __init gicv3_dist_init(void)
> * Here we override HW supported number of LPIs and
> * limit to to LPIs specified in nr_lpis.
> */
> -if ( gicv3_dist_supports_lpis() )
> +if ( its_enabled && gicv3_dist_supports_lpis() )
> gicv3_info.nr_irq_ids = nr_lpis + FIRST_GIC_LPI;
> else
> {
> @@ -714,6 +726,10 @@ static int __cpuinit gicv3_cpu_init(void)
> if ( gicv3_enable_redist() )
> return -ENODEV;
>
> +/* Give LPIs a spin */
> +if ( its_enabled && gicv3_dist_supports_lpis() )
If the ITS is not enabled, the list of ITS nodes will be empty and
therefore its_cpu_init will return directly.
So its_enabled is not necessary.
> +its_cpu_init();
> +
> /* Set priority on PPI and SGI interrupts */
> priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
> GIC_PRI_IPI);
> @@ -1303,14 +1319,30 @@ static int __init gicv3_init(void)
> i, r->base, r->base + r->size);
> }
>
> -vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
> - gicv3.rdist_stride);
> +reg = readl_relaxed(GICD + GICD_TYPER);
> +
> +gicv3.rdist_data.id_bits = ((reg >> GICD_TYPE_ID_BITS_SHIFT) &
> +GICD_TYPE_ID_BITS_MASK) + 1;
> +
> gicv3_init_v2(node, dbase);
>
> spin_lock_init(&gicv3.lock);
>
> spin_lock(&gicv3.lock);
>
> +if ( its_enable && gicv3_dist_supports_lpis() )
> +{
> +/*
> + * LPI support is enabled only if HW supports it and
> + * ITS dt node is available
> + */
> +if ( !its_init(&gicv3.rdist_data) )
> +its_enabled = 1;
> +}
> +
> +vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
> + gicv3.rdist_stride, its_enabled);
> +
Why do you need to execute all this new code with the gicv3.lock taken?
AFAICT there is no use of GICv3 registers inside the ITS initialization.
> gicv3_dist_init();
> res = gicv3_cpu_init();
> gicv3_hyp_init();
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 12c5d87..52d4277 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -51,6 +51,8 @@
>
> static struct {
> bool_t enabled;
> +/* Check if its supported */
> +bool_t lpi_support;
While ITS means LPIs is supported, the invert is not true.
Can you please rename this variable to its_enabled.
> /* Distributor interface address */
> paddr_t dbase;
> /* Re-distributor regions */
> @@ -62,9 +64,10 @@ static struct {
> void vgic_v3_setup_hw(paddr_t dbase,
>unsigned int nr_rdist_regions,
>const struct rdist_region *regions,
> - uint32_t rdist_stride)
> + uint32_t rdist_stride, bool_t lpi_support)
Ditto.
> {
> vgic_v3_hw.enabled = 1;
> +vgic_v3_hw.lpi_support = lpi_support;
Ditto.
> vgic_v3_hw.dbase = dbase;
> vgic_v3_hw.nr_rdist_regions = nr_rdist_regions;
> vgic_v3_hw.regions = regions;
> diff --git a/xen/include/asm-arm/gic_v3_defs.h
> b/xen/include/asm-arm/gic_v3_defs.h
> index 1bc88f6..f819589 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -46,7 +46,9 @@
> #define GICC_SRE_EL2_ENEL1
[Xen-devel] [PATCH v7 14/28] xen/arm: ITS: Initialize physical ITS and export lpi support
From: Vijaya Kumar K
Initialize physical ITS if HW supports LPIs.
Signed-off-by: Vijaya Kumar K
---
v7: - Export lpi support information to vgic-v3 driver from gic-v3.
- Drop gic_lpi_supported() helper function
- Add boot param to enable or disable physical ITS
v6: - Updated lpi_supported gic_info member for GICv2 and GICv3
- Introduced helper gic_lpi_supported() and exported
v5: - Made check of its dt node availability before
setting lpi_supported flag
---
xen/arch/arm/gic-v3.c | 38 ++---
xen/arch/arm/vgic-v3.c|5 -
xen/include/asm-arm/gic_v3_defs.h |4 +++-
xen/include/asm-arm/vgic.h|2 +-
4 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c4c77a7..ac8a0ea 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -55,6 +55,18 @@ static struct {
} gicv3;
static struct gic_info gicv3_info;
+/* Enable/Disable ITS support */
+static bool_t its_enable = 1;
+/* Availability of ITS support after successful ITS initialization */
+static bool_t its_enabled = 0;
+
+static void __init parse_its_param(char *s)
+{
+if ( !parse_bool(s) )
+its_enable = 0;
+}
+
+custom_param("its", parse_its_param);
/* per-cpu re-distributor base */
DEFINE_PER_CPU(struct rdist, rdist);
@@ -590,7 +602,7 @@ static void __init gicv3_dist_init(void)
* Here we override HW supported number of LPIs and
* limit to to LPIs specified in nr_lpis.
*/
-if ( gicv3_dist_supports_lpis() )
+if ( its_enabled && gicv3_dist_supports_lpis() )
gicv3_info.nr_irq_ids = nr_lpis + FIRST_GIC_LPI;
else
{
@@ -714,6 +726,10 @@ static int __cpuinit gicv3_cpu_init(void)
if ( gicv3_enable_redist() )
return -ENODEV;
+/* Give LPIs a spin */
+if ( its_enabled && gicv3_dist_supports_lpis() )
+its_cpu_init();
+
/* Set priority on PPI and SGI interrupts */
priority = (GIC_PRI_IPI << 24 | GIC_PRI_IPI << 16 | GIC_PRI_IPI << 8 |
GIC_PRI_IPI);
@@ -1303,14 +1319,30 @@ static int __init gicv3_init(void)
i, r->base, r->base + r->size);
}
-vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
- gicv3.rdist_stride);
+reg = readl_relaxed(GICD + GICD_TYPER);
+
+gicv3.rdist_data.id_bits = ((reg >> GICD_TYPE_ID_BITS_SHIFT) &
+GICD_TYPE_ID_BITS_MASK) + 1;
+
gicv3_init_v2(node, dbase);
spin_lock_init(&gicv3.lock);
spin_lock(&gicv3.lock);
+if ( its_enable && gicv3_dist_supports_lpis() )
+{
+/*
+ * LPI support is enabled only if HW supports it and
+ * ITS dt node is available
+ */
+if ( !its_init(&gicv3.rdist_data) )
+its_enabled = 1;
+}
+
+vgic_v3_setup_hw(dbase, gicv3.rdist_count, gicv3.rdist_regions,
+ gicv3.rdist_stride, its_enabled);
+
gicv3_dist_init();
res = gicv3_cpu_init();
gicv3_hyp_init();
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 12c5d87..52d4277 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -51,6 +51,8 @@
static struct {
bool_t enabled;
+/* Check if its supported */
+bool_t lpi_support;
/* Distributor interface address */
paddr_t dbase;
/* Re-distributor regions */
@@ -62,9 +64,10 @@ static struct {
void vgic_v3_setup_hw(paddr_t dbase,
unsigned int nr_rdist_regions,
const struct rdist_region *regions,
- uint32_t rdist_stride)
+ uint32_t rdist_stride, bool_t lpi_support)
{
vgic_v3_hw.enabled = 1;
+vgic_v3_hw.lpi_support = lpi_support;
vgic_v3_hw.dbase = dbase;
vgic_v3_hw.nr_rdist_regions = nr_rdist_regions;
vgic_v3_hw.regions = regions;
diff --git a/xen/include/asm-arm/gic_v3_defs.h
b/xen/include/asm-arm/gic_v3_defs.h
index 1bc88f6..f819589 100644
--- a/xen/include/asm-arm/gic_v3_defs.h
+++ b/xen/include/asm-arm/gic_v3_defs.h
@@ -46,7 +46,9 @@
#define GICC_SRE_EL2_ENEL1 (1UL << 3)
/* Additional bits in GICD_TYPER defined by GICv3 */
-#define GICD_TYPE_ID_BITS_SHIFT 19
+#define GICD_TYPE_ID_BITS_SHIFT (19)
+#define GICD_TYPE_ID_BITS_MASK (0x1f)
+#define GICD_TYPE_LPIS (0x1UL << 17)
#define GICD_TYPER_LPIS_SUPPORTED(1U << 17)
#define GICD_CTLR_RWP(1UL << 31)
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index cf5a790..2bf061f 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -362,7 +362,7 @@ struct rdist_region;
void vgic_v3_setup_hw(paddr_t dbase,
unsigned int nr_rdist_regions,
const struct rdist_region *regions,
- uint32_t rdist_stride);
+ uint32_t rdist_stride, bool_t lpi_suppo
