Re: [PATCH v2 02/27] ARM: GICv3: allocate LPI pending and property table

[Julien Grall <julien.grall@arm.com>]

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@arm.com>.

"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@lists.xen.org
https://lists.xen.org/xen-devel