[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 02/27] ARM: GICv3: allocate LPI pending and property table
On 23/03/17 14:40, Andre Przywara wrote:
Hi,
Hi Andre,
On 21/03/17 21:23, Julien Grall wrote:
Hi Andre,
On 03/16/2017 11:20 AM, Andre Przywara wrote:
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index bf64c61..86f7b53 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -49,6 +49,21 @@ config HAS_ITS
bool "GICv3 ITS MSI controller support"
depends on HAS_GICV3
+config MAX_PHYS_LPI_BITS
+ depends on HAS_ITS
+ int "Maximum bits for GICv3 host LPIs (14-32)"
+ range 14 32
+ default "20"
+ help
+ Specifies the maximum number of LPIs (in bits) Xen should take
+ care of. The host ITS may provide support for a very large
number
+ of supported LPIs, for all of which we may not want to
allocate
+ memory, so this number here allows to limit this.
+ Xen itself does not know how many LPIs domains will ever need
+ beforehand.
+ This can be overridden on the command line with the
max_lpi_bits
+ parameter.
I continue to disagree on this Kconfig option. There is no sensible
default value and I don't see how a distribution will be able to pick-up
one value here.
If the number of LPIs have to be restricted, this should be done via the
command line or a per-platform option.
So are you opting for taking the hardware provided value, unless there
is a command line option or a per-platform limit?
Please keep in mind that the situation here is different from the device
table:
- There is no indirect table option for the property and pending table.
Any redistributor supporting 32 bits worth of LPIs would lead to a
4GB property table and 512MB pending table allocation.
- An OS like Linux can make sensible assumptions about the number of
LPIs needed and only allocate as much LPIs as required. Linux for
instance uses at most 65536. Xen cannot easily make this decision.
So we would need to go as high as possible, but not too high to not
waste memory. I see different tradeoffs here between a distribution
targeting servers or another one aiming at some embedded devices, for
instance. So I would expect exactly this decision to be covered by a
Kconfig option.
Hence why a parameter on the command line is the better place for that.
The decision is left to the user.
I have already made my point on previous e-mails so I am not going to
argue more.
So as I mentioned before, I am happy to loose the Kconfig option, but
then we need some sensible default value. In our case we could be cheeky
here for now and just use the Linux value, because a Linux Dom0 would be
the only user. But that doesn't sound very future proof, though this may
not matter for 4.9.
I don't think we need a sensible default value and IHMO there is none. I
would left the user to decide the exact number.
+/* Global state */
+static struct {
+ /* The global LPI property table, shared by all redistributors. */
+ uint8_t *lpi_property;
+ /*
+ * Number of physical LPIs the host supports. This is a property of
+ * the GIC hardware. We depart from the habit of naming these things
+ * "physical" in Xen, as the GICv3/4 spec uses the term "physical
LPI"
+ * in a different context to differentiate them from "virtual LPIs".
+ */
+ unsigned long int nr_host_lpis;
Why unsigned long?
Because someone requested that I store the actual number of LPIs, not
the number of bits required to encode them. So I need to cover the case
of exactly 32 bits worth of LPIs, which u32 cannot hold.
Fair enough.
[...]
+int gicv3_lpi_init_rdist(void __iomem * rdist_base)
+{
+ uint32_t reg;
+ uint64_t table_reg;
+ int ret;
+
+ /* We don't support LPIs without an ITS. */
+ if ( !gicv3_its_host_has_its() )
+ return -ENODEV;
+
+ /* Make sure LPIs are disabled before setting up the tables. */
+ reg = readl_relaxed(rdist_base + GICR_CTLR);
+ if ( reg & GICR_CTLR_ENABLE_LPIS )
+ return -EBUSY;
+
+ ret = gicv3_lpi_allocate_pendtable(&table_reg);
+ if (ret)
+ return ret;
+ writeq_relaxed(table_reg, rdist_base + GICR_PENDBASER);
Same question as on the previous version. The cacheability and
shareability may not stick. This was a bug fix in Linux (see commit
241a386) and I am struggling to understand why Xen would not be affected?
Oh, OK, we need to remove cacheability if shareability is denied, though
that sounds like a hardware bug to me if this is not observed by the
redistributor.
The reason I didn't care about it here was that software doesn't use the
pending table, so there would be nothing on the code side to be done in
case the redistributor shoots down one of our requests.
The mentioned Linux patch covers all ITS tables and the property table
as well.
The patch was sent by Marc and I trust him to do the right things.
Looking at the commit message, it is to avoid cacheable traffic. Unless
you give me a good reason for not doing that, please do the change.
I don't want to end-up digging into weird bug later on because we
decided that "the hardware is not supposed to do that".
[...]
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 836a103..12bd155 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -220,6 +220,8 @@ enum gic_version {
GIC_V3,
};
+#define LPI_OFFSET 8192
My comments on the previous version about LPI_OFFSET are not addressed.
And one question was left unanswered.
Do you mean this?
It would make much sense to have this definition moved in irq.h
close to NR_IRQS.
Admittedly I missed that over the next question...
I think I wasn't convinced in the first place to put this GIC specific
detail into the generic irq.h, but I see that it makes sense since we
have the do_LPI() prototype in there as well.
Also, I am a bit surprised that NR_IRQS & co has not been modified.
Is there any reason for that?
But I answered on this one.
Is there anything else you want to have answered?
Yes, the question in <dae3afdb-d64d-a81b-7df3-ed8a61e00ad1@xxxxxxx>.
"Can we have a comment on top of NR_IRQS explaining why LPIs are not
taken into account?"
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
