* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Auger Eric @ 2017-04-27 17:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427163852.GO50776@lvm>
Hi Christoffer,
On 27/04/2017 18:38, Christoffer Dall wrote:
> On Thu, Apr 27, 2017 at 05:29:35PM +0200, Auger Eric wrote:
>>
>>
>> On 27/04/2017 16:45, Christoffer Dall wrote:
>>> Hi Eric,
>>>
>>> On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote:
>>>> On 27/04/2017 13:02, Christoffer Dall wrote:
>>>>> On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote:
>>>>>> On 27/04/2017 10:57, Christoffer Dall wrote:
>>>>>>> On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote:
>>>>>>>> On 26/04/2017 14:31, Christoffer Dall wrote:
>>>>>>>>> On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote:
>>>>>>>>>> Add description for how to access ITS registers and how to save/restore
>>>>>>>>>> ITS tables into/from memory.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> v4 -> v5:
>>>>>>>>>> - take into account Christoffer's comments
>>>>>>>>>> - pending table save on GICV3 side now
>>>>>>>>>>
>>>>>>>>>> v3 -> v4:
>>>>>>>>>> - take into account Peter's comments:
>>>>>>>>>> - typos
>>>>>>>>>> - KVM_DEV_ARM_VGIC_GRP_ITS_TABLES kvm_device_attr = 0
>>>>>>>>>> - add a validity bit in DTE
>>>>>>>>>> - document all fields in CTE and ITE
>>>>>>>>>> - document ABI revision
>>>>>>>>>> - take into account Andre's comments:
>>>>>>>>>> - document restrictions about GITS_CREADR writing and GITS_IIDR
>>>>>>>>>> - document -EBUSY error if one or more VCPUS are runnning
>>>>>>>>>> - document 64b registers only can be accessed with 64b access
>>>>>>>>>> - itt_addr field matches bits [51:8] of the itt_addr
>>>>>>>>>>
>>>>>>>>>> v1 -> v2:
>>>>>>>>>> - DTE and ITE now are 8 bytes
>>>>>>>>>> - DTE and ITE now indexed by deviceid/eventid
>>>>>>>>>> - use ITE name instead of ITTE
>>>>>>>>>> - mentions ITT_addr matches bits [51:8] of the actual address
>>>>>>>>>> - mentions LE layout
>>>>>>>>>> ---
>>>>>>>>>> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 99 ++++++++++++++++++++++
>>>>>>>>>> 1 file changed, 99 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>>> index 6081a5b..b5f010d 100644
>>>>>>>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>>> @@ -32,7 +32,106 @@ Groups:
>>>>>>>>>> KVM_DEV_ARM_VGIC_CTRL_INIT
>>>>>>>>>> request the initialization of the ITS, no additional parameter in
>>>>>>>>>> kvm_device_attr.addr.
>>>>>>>>>> +
>>>>>>>>>> + KVM_DEV_ARM_ITS_SAVE_TABLES
>>>>>>>>>> + save the ITS table data into guest RAM, at the location provisioned
>>>>>>>>>> + by the guest in corresponding registers/table entries.
>>>>>>>>>> +
>>>>>>>>>> + The layout of the tables in guest memory defines an ABI. The entries
>>>>>>>>>> + are laid out in little endian format as described in the last paragraph.
>>>>>>>>>> +
>>>>>>>>>> + KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>>>>>> + restore the ITS tables from guest RAM to ITS internal structures.
>>>>>>>>>> +
>>>>>>>>>> + The GICV3 must be restored before the ITS and all ITS registers but
>>>>>>>>>> + the GITS_CTLR must be restored before restoring the ITS tables.
>>>>>>>>>> +
>>>>>>>>>> + The GITS_IIDR read-only register must also be restored before
>>>>>>>>>> + the table restore as the IIDR revision field encodes the ABI revision.
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> what is the expected sequence of operations. For example, to restore
>>>>>>>>> the ITS, do I call KVM_DEV_ARM_VGIC_CTRL_INIT first, then restore all
>>>>>>>>> the memory and registers, and finally call KVM_DEV_ARM_ITS_RESTORE_TABLES?
>>>>>>>> Yes KVM_DEV_ARM_VGIC_CTRL_INIT comes first, then restore all registers
>>>>>>>> except GITS_CTLR, then table restore, then GITS_CTLR
>>>>>>>>>
>>>>>>>>> Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>>>>> and restore GITS_CTLR (which enables the ITS)?
>>>>>>>>
>>>>>>>> Yep, when GITS_CTLR is set, LPIs may be enabled and this on that event
>>>>>>>> that the pending table is read. But the whole pending table is not read
>>>>>>>> as we only iterate on registered LPIs. So the ITT must have been
>>>>>>>> restored previously.
>>>>>>>>
>>>>>>>> I became aware that the pending table sync is done twice, once in the
>>>>>>>> pending table restore, and once in the GITS_CTLR restore. So if we
>>>>>>>> leave this order specification, I should be able to remove the sync on
>>>>>>>> table restore. This was the original reason why GITS_CTLR restore has
>>>>>>>> been done at the very end.
>>>>>>>
>>>>>>> I'm sorry, I'm a bit confused. Do we not need
>>>>>>> KVM_DEV_ARM_ITS_RESTORE_TABLES at all then?
>>>>>>
>>>>>> Yes you do. I was talking about the RDIST pending table sync. The save
>>>>>> is explicit using GICV3 device KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.
>>>>>> However the sync is implicit on GITS_CTLR restore if LPIs are enabled.
>>>>>> and today I do it also on ITS device KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>> which is not requested I think since GITS_CTLR restore does it already.
>>>>>
>>>>> Shouldn't restoring the pending tables happen when restoring some
>>>>> redeistributor state and not anything related to the ITS?
>>>>
>>>> Marc wrote:
>>>> "
>>>> I don't think you necessarily need a coarse map. When restoring the ITS
>>>> tables, you can always read the pending bit when creating the LPI
>>>> structure (it has been written to RAM at save time). Note that we
>>>> already do something like this in vgic_enable_lpis().
>>>> "
>>>>
>>>> This is currently what is implemented I think. the pending tables are
>>>> currently sync'ed on GITS_CTLR set (if LPI are enabled) + erroneously
>>>> also on on ITS table restore
>>>>
>>>> The problematic is: Either you know in advance which LPI INTIDare used
>>>> or you need to parse the whole pending table (possibly using the 1st kB
>>>> as coarse mapping).
>>>>
>>>> If you don't know the LPI INTIDs in advance it is only possible to
>>>> restore the pending bit of pending LPIs. At that time you would
>>>> re-allocate those pending LPI (vgic_add_lpi) and when you restore the
>>>> ITS ITT you would do the same for those which were not pending. Looks
>>>> really heavy to me: coarse mapping + dual vgic_add_lpi path.
>>>>
>>>> Otherwise we would need to add another dependency between RDIST pending
>>>> table restore and ITS table restore but this looks even more weird, no?
>>>>
>>>>
>>> So I just sat down with Andre and Marc and we tried to work through this
>>> and came up with the best scheme. I apologize in advance for the
>>> one-way nature of this e-mail, and I am of course open to discussing the
>>> following proposal again if you do not agree.
>>>
>>> What I think this document should say, is that the following ordering
>>> must be followed when restoring the GIC and the ITS:
>>>
>>> First, restore all guest memory
>>>
>>> Second, restore ALL redistributors
>>>
>>> Third, restore the ITS, in the following order:
>>> 1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT)
>>> 2. Restore GITS_CBASER
>>> 3. Restore all other GITS_ registers, except GITS_CTLR!
>>> 4. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES)
>>> 5. Restore GITS_CTLR
>>>
>>> The rationale is that we really want the redistributor and the ITS
>>> restore to be independent and follow the architecture. This means that
>>> our ABI for the redistributor should still work without restoring an ITS
>>> (if we ever decide to support LPIs for KVM without the ITS).
>>
>> OK. Note I already mentioned that GICv3 must be restored before the ITS.
>> To me this comprised the RDIST.
>
> Possibly, but I think it's good to write out the whole thing so we
> clearly understand the flow. That could better be achieved by
> correcting my proposed text above to say something like "Second, restore
> ALL redistributors to ensure the pending and configuration tables can be
> read."
>
>>
>> I understand the above description of the ordering comes in addition to
>> the existing text, right?
>
> Yes
>
>> in other words I keep the GITS_READR,
>> GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES
>> section.
>>
>
> Yes. But you don't need to do any reading of the pending table on any
> of the restore operations.
well you told me to do it on vgic_add_lpi(). This is obviously called on
ITS table restore. /me confused. Obviously this is implicit and should
not be documented. Is that what you meant? btw this is not documented
atm I think.
>
> Btw., I think it's GITS_CREADR, not GITS_READR.
right.
Thanks
Eric
>
>>>
>>> In terms of our current implementation this means that vgic_add_lpi()
>>> should ask the redistributor what the state of the LPI is (priority,
>>> enabled, pending).
>> this practically means I move update_lpi_config call from
>> vgic_its_restore_ite to vgic_add_lpi(). OK
>
> Pretty much, yes.
>
> Thanks,
> -Christoffer
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* [PATCH v5 11/22] KVM: arm64: vgic-its: Interpret MAPD Size field and check related errors
From: Christoffer Dall @ 2017-04-27 17:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <bc81470d-b5b3-0b81-e845-7f3527ef0f6f@redhat.com>
On Thu, Apr 27, 2017 at 07:15:27PM +0200, Auger Eric wrote:
> Hi,
> On 27/04/2017 18:25, Christoffer Dall wrote:
> > On Fri, Apr 14, 2017 at 12:15:23PM +0200, Eric Auger wrote:
> >> Up to now the MAPD's ITT size field has been ignored. It encodes
> >> the number of eventid bit minus 1. It should be used to check
> >> the eventid when a MAPTI command is issued on a device. Let's
> >> store the number of eventid bits in the its_device and do the
> >> check on MAPTI. Also make sure the ITT size field does
> >> not exceed the GITS_TYPER IDBITS field.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>
> >> ---
> >> v4 -> v5:
> >> - its_cmd_get_size macro now returns the actual number of eventid bits
> >> - use GIC_ENCODE_SZ macro to encode ID_bits
> >>
> >> v3 -> v4:
> >> - VITS_TYPER_IDBITS set to 16 to be homogeneous with VITS_ESZ definition
> >> and correct users
> >> - nb_eventid_bits correspond to the actual number of eventid bits
> >> ---
> >> include/linux/irqchip/arm-gic-v3.h | 2 ++
> >> virt/kvm/arm/vgic/vgic-its.c | 15 ++++++++++++++-
> >> 2 files changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> >> index 54c20bd..0c6798c 100644
> >> --- a/include/linux/irqchip/arm-gic-v3.h
> >> +++ b/include/linux/irqchip/arm-gic-v3.h
> >> @@ -345,9 +345,11 @@
> >> #define E_ITS_INT_UNMAPPED_INTERRUPT 0x010307
> >> #define E_ITS_CLEAR_UNMAPPED_INTERRUPT 0x010507
> >> #define E_ITS_MAPD_DEVICE_OOR 0x010801
> >> +#define E_ITS_MAPD_ITTSIZE_OOR 0x010802
> >> #define E_ITS_MAPC_PROCNUM_OOR 0x010902
> >> #define E_ITS_MAPC_COLLECTION_OOR 0x010903
> >> #define E_ITS_MAPTI_UNMAPPED_DEVICE 0x010a04
> >> +#define E_ITS_MAPTI_ID_OOR 0x010a05
> >> #define E_ITS_MAPTI_PHYSICALID_OOR 0x010a06
> >> #define E_ITS_INV_UNMAPPED_INTERRUPT 0x010c07
> >> #define E_ITS_INVALL_UNMAPPED_COLLECTION 0x010d09
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index 1b5797e..0f3c8f3 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -105,6 +105,7 @@ struct its_device {
> >>
> >> /* the head for the list of ITTEs */
> >> struct list_head itt_head;
> >> + u32 nb_eventid_bits;
> >
> > what does nb_ mean?
> number ;-)
>
Ah, I think the plural form of bits here implied that, but if you
insist, then can you call it num_eventid_bits?
I was like, "notifier block", "nota bene", I'm lost...
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH v5 15/22] KVM: arm64: vgic-its: vgic_its_alloc_ite/device
From: Christoffer Dall @ 2017-04-27 17:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1492164934-988-16-git-send-email-eric.auger@redhat.com>
On Fri, Apr 14, 2017 at 12:15:27PM +0200, Eric Auger wrote:
> Add two new helpers to allocate an its ite and an its device.
> This will avoid duplication on restore path.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>
> ---
>
> v4 -> v5:
> - add Marc's A-b
>
> v3 -> v4:
> - fix allocation
> - add comment about its_lock mutex hold
>
> v1 -> v2:
> - report itt_size fix and remove ITE_SIZE
> - s/itte/ite/g
> ---
> virt/kvm/arm/vgic/vgic-its.c | 73 ++++++++++++++++++++++++++++++--------------
> 1 file changed, 50 insertions(+), 23 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 55267ab..56c5123 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -742,6 +742,27 @@ static void vgic_its_free_collection(struct vgic_its *its, u32 coll_id)
> kfree(collection);
> }
>
> +/* Must be called with its_lock mutex held */
> +static int vgic_its_alloc_ite(struct its_device *device,
> + struct its_ite **itep,
> + struct its_collection *collection,
> + u32 lpi_id, u32 event_id)
> +{
> + struct its_ite *ite;
> +
> + ite = kzalloc(sizeof(*ite), GFP_KERNEL);
> + if (!ite)
> + return -ENOMEM;
> +
> + ite->event_id = event_id;
> + ite->collection = collection;
> + ite->lpi = lpi_id;
> +
> + list_add_tail(&ite->ite_list, &device->itt_head);
> + *itep = ite;
> + return 0;
> +}
This static function should just return a 'struct its_ite *itep' and on
error you can return ERR_PTR(-ENOMEM).
> +
> /*
> * The MAPTI and MAPI commands map LPIs to ITTEs.
> * Must be called with its_lock mutex held.
> @@ -755,7 +776,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> struct its_ite *ite;
> struct its_device *device;
> struct its_collection *collection, *new_coll = NULL;
> - int lpi_nr;
> + int lpi_nr, ret;
> struct vgic_irq *irq;
>
> device = find_its_device(its, device_id);
> @@ -785,19 +806,13 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> new_coll = collection;
> }
>
> - ite = kzalloc(sizeof(struct its_ite), GFP_KERNEL);
> - if (!ite) {
> + ret = vgic_its_alloc_ite(device, &ite, collection, lpi_nr, event_id);
> + if (ret) {
> if (new_coll)
> vgic_its_free_collection(its, coll_id);
> - return -ENOMEM;
> + return ret;
> }
>
> - ite->event_id = event_id;
> - list_add_tail(&ite->ite_list, &device->itt_head);
> -
> - ite->collection = collection;
> - ite->lpi = lpi_nr;
> -
> irq = vgic_add_lpi(kvm, lpi_nr);
> if (IS_ERR(irq)) {
> if (new_coll)
> @@ -836,6 +851,29 @@ static void vgic_its_unmap_device(struct kvm *kvm, struct its_device *device)
> kfree(device);
> }
>
> +/* Must be called with its_lock mutex held */
> +static int vgic_its_alloc_device(struct vgic_its *its,
> + struct its_device **devp,
> + u32 device_id, gpa_t itt_addr,
> + u8 nb_eventid_bits)
> +{
> + struct its_device *device;
> +
> + device = kzalloc(sizeof(*device), GFP_KERNEL);
> + if (!device)
> + return -ENOMEM;
> +
> + device->device_id = device_id;
> + device->itt_addr = itt_addr;
> + device->nb_eventid_bits = nb_eventid_bits;
> + INIT_LIST_HEAD(&device->itt_head);
> +
> + list_add_tail(&device->dev_list, &its->device_list);
> + *devp = device;
> +
> + return 0;
> +}
also here, return ERR_PTR
> +
> /*
> * MAPD maps or unmaps a device ID to Interrupt Translation Tables (ITTs).
> * Must be called with the its_lock mutex held.
> @@ -872,19 +910,8 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
> if (!valid)
> return 0;
>
> - device = kzalloc(sizeof(struct its_device), GFP_KERNEL);
> - if (!device)
> - return -ENOMEM;
> -
> - device->device_id = device_id;
> - device->nb_eventid_bits = nb_eventid_bits;
> - device->itt_addr = itt_addr;
> -
> - INIT_LIST_HEAD(&device->itt_head);
> -
> - list_add_tail(&device->dev_list, &its->device_list);
> -
> - return 0;
> + return vgic_its_alloc_device(its, &device, device_id,
> + itt_addr, nb_eventid_bits);
> }
>
> /*
> --
> 2.5.5
>
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH 2/2] arm64: pmu: Wire-up L2 cache events for ARMv8 PMUv3
From: Will Deacon @ 2017-04-27 17:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <28edde78-b965-8136-1c95-11b9182de405@gmail.com>
On Tue, Apr 25, 2017 at 10:13:51AM -0700, Florian Fainelli wrote:
> On 04/25/2017 05:44 AM, Will Deacon wrote:
> > Hi Florian,
> >
> > On Thu, Apr 20, 2017 at 12:05:46PM -0700, Florian Fainelli wrote:
> >> The ARMv8 PMUv3 cache map did not include the L2 cache events, add
> >> them.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >> arch/arm64/kernel/perf_event.c | 5 +++++
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> >> index 4f011cdd756d..a664c575f3fd 100644
> >> --- a/arch/arm64/kernel/perf_event.c
> >> +++ b/arch/arm64/kernel/perf_event.c
> >> @@ -264,6 +264,11 @@ static const unsigned armv8_pmuv3_perf_cache_map[PERF_COUNT_HW_CACHE_MAX]
> >> [C(L1I)][C(OP_READ)][C(RESULT_ACCESS)] = ARMV8_PMUV3_PERFCTR_L1I_CACHE,
> >> [C(L1I)][C(OP_READ)][C(RESULT_MISS)] = ARMV8_PMUV3_PERFCTR_L1I_CACHE_REFILL,
> >>
> >> + [C(LL)][C(OP_READ)][C(RESULT_ACCESS)] = ARMV8_PMUV3_PERFCTR_L2D_CACHE,
> >> + [C(LL)][C(OP_READ)][C(RESULT_MISS)] = ARMV8_PMUV3_PERFCTR_L2D_CACHE_REFILL,
> >> + [C(LL)][C(OP_WRITE)][C(RESULT_ACCESS)] = ARMV8_PMUV3_PERFCTR_L2D_CACHE,
> >> + [C(LL)][C(OP_WRITE)][C(RESULT_MISS)] = ARMV8_PMUV3_PERFCTR_L2D_CACHE_REFILL,
> >
> > I don't think this is correct in general. 'LL' stands for "last-level",
> > which may be L3 or even a system cache in the interconnect. Tying that to L2
> > is the wrong thing to do from perf's generic event perspective.
> >
> > I'm ok with what you're proposing for A53 (where the PMU can only count
> > events out to the L2), but I'm reluctant to make this change for the generic
> > PMUv3 events.
>
> That makes sense, shall I resubmit the first patch by itself or can you
> or Catalin take it as-is?
I'll talk to Catalin tomorrow and try to get the A53 bit queued.
Will
^ permalink raw reply
* [PATCH v2] arm64: perf: Use only exclude_kernel attribute when kernel is running in HYP
From: Will Deacon @ 2017-04-27 17:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170426134141.GA6417@localhost>
On Wed, Apr 26, 2017 at 01:41:42PM +0000, Jayachandran C wrote:
> On Wed, Apr 26, 2017 at 11:10:21AM +0100, Will Deacon wrote:
> > On Wed, Apr 26, 2017 at 07:22:46AM +0000, Pinski, Andrew wrote:
> > > On 4/25/2017 11:53 PM, Jayachandran C. wrote:
> > > > On Tue, Apr 25, 2017 at 10:23 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > >> On Tue, Apr 25, 2017 at 09:13:40AM +0530, Ganapatrao Kulkarni wrote:
> > > >>> On Mon, Apr 24, 2017 at 9:15 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > >>>> On Thu, Apr 20, 2017 at 02:56:50PM +0530, Ganapatrao Kulkarni wrote:
> > > >>>>> OK, if you are ok with sysfs part, i can send next version with that
> > > >>>>> change only?.
> > > >>>> I think the sysfs part is still a little dodgy, since you still expose the
> > > >>>> "exclude_hv" file with a value of 0 when not running at EL2, which would
> > > >>>> imply that exclude_hv is forced to zero. I don't think that's correct.
> > > >>> okay, i can make exclude_hv visible only when kernel booted in EL2.
> > > >>> is it ok to have empty directory "attr" when kernel booted to EL1?
> > > >>> attr can be place holder for any other miscellaneous attributes, that
> > > >>> can be added in future.
> > > >> Sounds good to me, although I'll seek comment from the other perf folks
> > > >> before merging anything with ABI implications.
> > > > Do you really think this is the solution given:
> > > > - this is an arm64 specific sysfs interface that is tied to the perf API
> >
> > That's why I want feedback from others. The intention would be that this can
> > be used by other PMUs as well, since it's not uncommon that parts of the
> > sizeable perf_event_attr structure are not used by a given PMU.
> >
> > > > - the perf API documentation has to be updated for this
> >
> > So? If having to update documentation means we shouldn't change the kernel,
> > then we may as well all find new jobs.
> >
> > > > - All the applications that use the perf API have to be modified to
> > > > check this sysfs interface
> > > > - If the application fails to do so, a very narrow corner case
> > > > (exclude_hv != exclude_kernel and VHE enabled) fails.
> >
> > See below, but apparently people care about it.
> >
> > > > Any application that really cares can already do see if exclude_hv !=
> > > > exclude_kernel case works by calling perf_open_event() with those
> > > > options and checking the return value.
> >
> > That's a good point: there is *something* userspace can do, although that
> > would be arm64-specific and doesn't really help with the state-space
> > explosion you get with combinations of invalid/unused perf_event_attr
> > fields.
> >
> > > An example of an application which needs to changed is HHVM. Currently
> > > it sets exclude_hv to true but exclude_kernel to false as it does not
> > > care about the hypervisor associated perf events associated with the
> > > code, only the kernel and userspace associated evnts.
> > > Yes we could submit a patch to use the sysfs interface to check but it
> > > would look funny and the facebook folks might reject the patch as it is
> > > ARM64 specific in generic code. Note this is how all of this discussion
> > > started was HHVM's call to perf_open_event was failing.
> >
> > Hmm, if you're saying that HHVM won't be changed to use the sysfs stuff,
> > then why are we bothering?
> >
> > Not sure where this leaves us.
>
> If my understanding is correct, the sysfs suggestion above is going to
> add API complexity without solving the issue. Ignoring the exclude_hv if
> it cannot be honored would be a better solution.
Better for HHVM, sure, but I don't think it's better in general. It means
that we silently do the opposite of what the user has requested in some
configurations.
Will
^ permalink raw reply
* [PATCH 0/3 v2] ARM/ARM64: silence large module first time allocation
From: Florian Fainelli @ 2017-04-27 17:38 UTC (permalink / raw)
To: linux-arm-kernel
With kernels built with CONFIG_ARM{,64}_MODULES_PLTS=y, the first allocation
done from module space will fail, produce a general OOM allocation and also a
vmap warning. The second allocation from vmalloc space may or may not be
successful, but is actually the one we are interested about in these cases.
This patch series passed __GFP_NOWARN to silence such allocations from the
ARM/ARM64 module loader's first time allocation when the MODULES_PLT option is
enabled, and also makes alloc_vmap_area() react to the caller setting
__GFP_NOWARN to silence "vmap allocation for size..." messages.
Changes in v2:
- check __GFP_NOWARN out of the printk_ratelimited() check (Michal)
Here is an example of what we would get without these two patches, pretty
scary huh?
# insmod /mnt/nfs/huge.ko
[ 22.114143] random: nonblocking pool is initialized
[ 22.183575] vmap allocation for size 15736832 failed: use vmalloc=<size> to increase size.
[ 22.191873] vmalloc: allocation failure: 15729534 bytes
[ 22.197112] insmod: page allocation failure: order:0, mode:0xd0
[ 22.203048] CPU: 2 PID: 1506 Comm: insmod Tainted: G O 4.1.20-1.9pre-01082-gbbbff07bc3ce #9
[ 22.212536] Hardware name: Broadcom STB (Flattened Device Tree)
[ 22.218480] [<c0017eec>] (unwind_backtrace) from [<c00135c8>] (show_stack+0x10/0x14)
[ 22.226238] [<c00135c8>] (show_stack) from [<c0638684>] (dump_stack+0x90/0xa4)
[ 22.233473] [<c0638684>] (dump_stack) from [<c00aae1c>] (warn_alloc_failed+0x104/0x144)
[ 22.241490] [<c00aae1c>] (warn_alloc_failed) from [<c00d72e0>] (__vmalloc_node_range+0x170/0x218)
[ 22.250375] [<c00d72e0>] (__vmalloc_node_range) from [<c00147d0>] (module_alloc+0x50/0xac)
[ 22.258651] [<c00147d0>] (module_alloc) from [<c008ae2c>] (module_alloc_update_bounds+0xc/0x6c)
[ 22.267360] [<c008ae2c>] (module_alloc_update_bounds) from [<c008b778>] (load_module+0x8ec/0x2058)
[ 22.276329] [<c008b778>] (load_module) from [<c008cfd4>] (SyS_init_module+0xf0/0x174)
[ 22.284170] [<c008cfd4>] (SyS_init_module) from [<c0010140>] (ret_fast_syscall+0x0/0x3c)
[ 22.292277] Mem-Info:
[ 22.294567] active_anon:5236 inactive_anon:1773 isolated_anon:0
[ 22.294567] active_file:1 inactive_file:3822 isolated_file:0
[ 22.294567] unevictable:0 dirty:0 writeback:0 unstable:0
[ 22.294567] slab_reclaimable:238 slab_unreclaimable:1594
[ 22.294567] mapped:855 shmem:2950 pagetables:36 bounce:0
[ 22.294567] free:39031 free_pcp:198 free_cma:3928
[ 22.327196] DMA free:156124kB min:1880kB low:2348kB high:2820kB active_anon:20944kB inactive_anon:7092kB active_file:4kB inactive_file:15288kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:262144kB managed:227676kB mlocked:0kB dirty:0kB writeback:0kB mapped:3420kB shmem:11800kB slab_reclaimable:952kB slab_unreclaimable:6376kB kernel_stack:560kB pagetables:144kB unstable:0kB bounce:0kB free_pcp:792kB local_pcp:68kB free_cma:15712kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[ 22.371631] lowmem_reserve[]: 0 0 0 0
[ 22.375372] HighMem free:0kB min:128kB low:128kB high:128kB active_anon:0kB inactive_anon:0kB active_file:0kB inactive_file:0kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:2883584kB managed:0kB mlocked:0kB dirty:0kB writeback:0kB mapped:0kB shmem:0kB slab_reclaimable:0kB slab_unreclaimable:0kB kernel_stack:0kB pagetables:0kB unstable:0kB bounce:0kB free_pcp:0kB local_pcp:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? yes
[ 22.416249] lowmem_reserve[]: 0 0 0 0
[ 22.419986] DMA: 3*4kB (UEM) 4*8kB (UE) 1*16kB (M) 4*32kB (UEMC) 3*64kB (EMC) 1*128kB (E) 4*256kB (UEMC) 2*512kB (UE) 2*1024kB (MC) 4*2048kB (UEMC) 35*4096kB (MRC) = 156156kB
[ 22.435922] HighMem: 0*4kB 0*8kB 0*16kB 0*32kB 0*64kB 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB 0*4096kB = 0kB
[ 22.446130] 6789 total pagecache pages
[ 22.449889] 0 pages in swap cache
[ 22.453212] Swap cache stats: add 0, delete 0, find 0/0
[ 22.458447] Free swap = 0kB
[ 22.461334] Total swap = 0kB
[ 22.464222] 786432 pages RAM
[ 22.467110] 720896 pages HighMem/MovableOnly
[ 22.471388] 725417 pages reserved
[ 22.474711] 4096 pages cma reserved
[ 22.511310] big_init: I am a big module using 3932160 bytes of data!
Florian Fainelli (3):
mm: Silence vmap() allocation failures based on caller gfp_flags
ARM: Silence first allocation with CONFIG_ARM_MODULE_PLTS=y
arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
arch/arm/kernel/module.c | 11 +++++++++--
arch/arm64/kernel/module.c | 7 ++++++-
mm/vmalloc.c | 2 +-
3 files changed, 16 insertions(+), 4 deletions(-)
--
2.9.3
^ permalink raw reply
* [PATCH v2 1/3] mm: Silence vmap() allocation failures based on caller gfp_flags
From: Florian Fainelli @ 2017-04-27 17:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427173900.2538-1-f.fainelli@gmail.com>
If the caller has set __GFP_NOWARN don't print the following message:
vmap allocation for size 15736832 failed: use vmalloc=<size> to increase
size.
This can happen with the ARM/Linux or ARM64/Linux module loader built
with CONFIG_ARM{,64}_MODULE_PLTS=y which does a first attempt at loading
a large module from module space, then falls back to vmalloc space.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
mm/vmalloc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 0b057628a7ba..d8a851634674 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -521,9 +521,13 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
}
}
+ if (gfp_mask & __GFP_NOWARN)
+ goto out;
+
if (printk_ratelimit())
pr_warn("vmap allocation for size %lu failed: use vmalloc=<size> to increase size\n",
size);
+out:
kfree(va);
return ERR_PTR(-EBUSY);
}
--
2.9.3
^ permalink raw reply related
* [PATCH v2 2/3] ARM: Silence first allocation with CONFIG_ARM_MODULE_PLTS=y
From: Florian Fainelli @ 2017-04-27 17:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427173900.2538-1-f.fainelli@gmail.com>
When CONFIG_ARM_MODULE_PLTS is enabled, the first allocation using the
module space fails, because the module is too big, and then the module
allocation is attempted from vmalloc space. Silence the first allocation
failure in that case by setting __GFP_NOWARN.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
arch/arm/kernel/module.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 80254b47dc34..503d1a39464a 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -40,8 +40,15 @@
#ifdef CONFIG_MMU
void *module_alloc(unsigned long size)
{
- void *p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
- GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
+ gfp_t gfp_mask = GFP_KERNEL;
+ void *p;
+
+#if IS_ENABLED(CONFIG_ARM_MODULE_PLTS)
+ /* Silence the initial allocation */
+ gfp_mask |= __GFP_NOWARN;
+#endif
+ p = __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
+ gfp_mask, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
__builtin_return_address(0));
if (!IS_ENABLED(CONFIG_ARM_MODULE_PLTS) || p)
return p;
--
2.9.3
^ permalink raw reply related
* [PATCH v2 3/3] arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
From: Florian Fainelli @ 2017-04-27 17:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427173900.2538-1-f.fainelli@gmail.com>
When CONFIG_ARM64_MODULE_PLTS is enabled, the first allocation using the
module space fails, because the module is too big, and then the module
allocation is attempted from vmalloc space. Silence the first allocation
failure in that case by setting __GFP_NOWARN.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
arch/arm64/kernel/module.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 7f316982ce00..58bd5cfdd544 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -32,11 +32,16 @@
void *module_alloc(unsigned long size)
{
+ gfp_t gfp_mask = GFP_KERNEL;
void *p;
+#if IS_ENABLED(CONFIG_ARM64_MODULE_PLTS)
+ /* Silence the initial allocation */
+ gfp_mask |= __GFP_NOWARN;
+#endif
p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
module_alloc_base + MODULES_VSIZE,
- GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
+ gfp_mask, PAGE_KERNEL_EXEC, 0,
NUMA_NO_NODE, __builtin_return_address(0));
if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
--
2.9.3
^ permalink raw reply related
* [PATCH 1/2] thermal: broadcom: Allow for NSP to use ns-thermal driver
From: Jon Mason @ 2017-04-27 17:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427163658.GC18276@localhost.localdomain>
On Thu, Apr 27, 2017 at 12:37 PM, Eduardo Valentin <edubezval@gmail.com> wrote:
> Hey Jason,
It's Jon :)
>
> On Tue, Apr 25, 2017 at 04:49:10PM -0400, Jon Mason wrote:
>> Change the iProc Kconfig to select THERMAL and THERMAL_OF, which allows
>> the ns-thermal driver to be selected via menuconfig. Also, change the
>> ns-thermal driver to work on any iProc based SoC. Finally, tweak the
>> Kconfig description to mention support for NSP and make the default on
>> for iProc based platforms.
>
>
> Thanks for the patch, but..
>>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>> ---
>> arch/arm/mach-bcm/Kconfig | 2 ++
>> drivers/thermal/broadcom/Kconfig | 9 +++++----
>> 2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
>> index a0e66d8..da2bfeb 100644
>> --- a/arch/arm/mach-bcm/Kconfig
>> +++ b/arch/arm/mach-bcm/Kconfig
>> @@ -19,6 +19,8 @@ config ARCH_BCM_IPROC
>> select GPIOLIB
>> select ARM_AMBA
>> select PINCTRL
>> + select THERMAL
>> + select THERMAL_OF
>> help
>> This enables support for systems based on Broadcom IPROC architected SoCs.
>> The IPROC complex contains one or more ARM CPUs along with common
>
> It would be better if this is split and sent through your arch tree, to
> avoid conflicts. I could also pick it if you get an ack from one of your
> maintainers. Still, first option is preferable.
Sure, I'll be happy to split this off. I should've thought to split
it up before sending. Thanks for the suggestion.
>
>> diff --git a/drivers/thermal/broadcom/Kconfig b/drivers/thermal/broadcom/Kconfig
>> index f0dea8a..26d706c 100644
>> --- a/drivers/thermal/broadcom/Kconfig
>> +++ b/drivers/thermal/broadcom/Kconfig
>> @@ -1,8 +1,9 @@
>> config BCM_NS_THERMAL
>> tristate "Northstar thermal driver"
>> depends on ARCH_BCM_IPROC || COMPILE_TEST
>> + default ARCH_BCM_IPROC
>
> Not sure if this is really what you wanted. Based on your commit log
> message, you meant the following, perhaps?
>
> + default y if ARCH_BCM_IPROC
IIUC, my original default works, as we have used it frequently in
other places in the kernel.
grep -rI "default ARCH_BCM_IPROC" * | wc -l
15
However, if the above is preferred (or the other 15 massively broken),
I'll be happy to do it that way.
>> help
>> - Northstar is a family of SoCs that includes e.g. BCM4708, BCM47081,
>> - BCM4709 and BCM47094. It contains DMU (Device Management Unit) block
>> - with a thermal sensor that allows checking CPU temperature. This
>> - driver provides support for it.
>> + Support for the Northstar and Northstar Plus family of SoCs (e.g.
>> + BCM4708, BCM4709, BCM5301x, BCM95852X, etc). It contains DMU (Device
>
> Did we look BCM47094 somehow on this patch?
Naa, just trying to be more concise, while adding the NSP products to
the list.. BCM47094 is a type of BCM4709. So, it is still there :)
>
>> + Management Unit) block with a thermal sensor that allows checking CPU
>> + temperature.
>> --
>> 2.7.4
>>
^ permalink raw reply
* [PATCH v5 12/22] KVM: arm64: vgic-its: Interpret MAPD ITT_addr field
From: Auger Eric @ 2017-04-27 17:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427164356.GP50776@lvm>
Hi,
On 27/04/2017 18:43, Christoffer Dall wrote:
> On Fri, Apr 14, 2017 at 12:15:24PM +0200, Eric Auger wrote:
>> Up to now the MAPD ITT_addr had been ignored. We will need it
>> for save/restore. Let's record it in the its_device struct.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>> v4 -> v5:
>> - its_cmd_get_ittaddr macro now returns the actual ITT GPA
>>
>> v3 -> v4:
>> - in vgic_its_cmd_handle_mapd, itt_addr directly is shifted
>> - correct ittaddr bitmask to support 48bit GPA
>> ---
>> virt/kvm/arm/vgic/vgic-its.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 0f3c8f3..757598d 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -106,6 +106,7 @@ struct its_device {
>> /* the head for the list of ITTEs */
>> struct list_head itt_head;
>> u32 nb_eventid_bits;
>> + gpa_t itt_addr;
>> u32 device_id;
>> };
>>
>> @@ -569,6 +570,7 @@ static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
>> #define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1, 0, 32)
>> #define its_cmd_get_physical_id(cmd) its_cmd_mask_field(cmd, 1, 32, 32)
>> #define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2, 0, 16)
>> +#define its_cmd_get_ittaddr(cmd) (its_cmd_mask_field(cmd, 2, 8, 47) << 8)
>
> Why 47? The ITT_addr field is bits [51:8] unless I'm looking at
> something wrong?
Initially I limited to 48 bits since I found in the code:
/*
* We only implement 48 bits of PA at the moment, although the ITS
* supports more. Let's be restrictive here.
*/
#define BASER_ADDRESS(x) ((x) & GENMASK_ULL(47, 16))
#define CBASER_ADDRESS(x) ((x) & GENMASK_ULL(47, 12))
But practically as I encode the ITT addr field on 52 bits now in the DTE
there is no reason anymore.
Thanks
Eric
>
>> #define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2, 16, 32)
>> #define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2, 63, 1)
>>
>> @@ -840,6 +842,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>> u32 device_id = its_cmd_get_deviceid(its_cmd);
>> bool valid = its_cmd_get_validbit(its_cmd);
>> u8 nb_eventid_bits = its_cmd_get_size(its_cmd);
>> + gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
>> struct its_device *device;
>>
>> if (!vgic_its_check_id(its, its->baser_device_table, device_id))
>> @@ -871,6 +874,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
>>
>> device->device_id = device_id;
>> device->nb_eventid_bits = nb_eventid_bits;
>> + device->itt_addr = itt_addr;
>>
>> INIT_LIST_HEAD(&device->itt_head);
>>
>> --
>> 2.5.5
>>
>
> Thanks,
> -Christoffer
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply
* [PATCHv2 0/2] arm64: fix hotplug rwsem boot fallout
From: Mark Rutland @ 2017-04-27 17:44 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
These patches address a boot failure on arm64 observed when booting a kernel
built with the hotplug rwsem changes currently queued in the tip smp/hotplug
branch.
I've given these a spin on Juno R1, which now boots happily.
FWIW, I also did a test-merge with the arm64 for-next/core branch, which git
was able to figure it out automatically. The result builds cleanly and boots
without issue on Juno R1.
Catalin/Will, assuming there are no further gotchas, are you happy for the
arm64 patch to go via the tip tree?
Thanks,
Mark.
Since v1 [1]:
* Fix update_cpu_capabilities() by splitting boot/secondary
[1] https://lkml.kernel.org/r/20170427154806.GA6646 at leverpostej
Mark Rutland (1):
arm64: cpufeature: use static_branch_enable_cpuslocked()
Sebastian Andrzej Siewior (1):
jump_label: Provide static_key_[enable|/slow_inc]_cpuslocked()
arch/arm64/include/asm/cpufeature.h | 5 +++--
arch/arm64/kernel/cpu_errata.c | 13 ++++++++++++-
arch/arm64/kernel/cpufeature.c | 5 ++++-
arch/arm64/kernel/smp.c | 7 +++----
include/linux/jump_label.h | 7 +++++++
kernel/jump_label.c | 10 ++++++++++
6 files changed, 39 insertions(+), 8 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCHv2 1/2] jump_label: Provide static_key_[enable|/slow_inc]_cpuslocked()
From: Mark Rutland @ 2017-04-27 17:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1493315077-19496-1-git-send-email-mark.rutland@arm.com>
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Provide static_key_[enable|slow_inc]_cpuslocked() variant that
don't take cpu_hotplug_lock().
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: jbaron at akamai.com
---
include/linux/jump_label.h | 7 +++++++
kernel/jump_label.c | 10 ++++++++++
2 files changed, 17 insertions(+)
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index d7b17d1..c80d8b1 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -164,6 +164,7 @@ extern void arch_jump_label_transform_static(struct jump_entry *entry,
extern void jump_label_apply_nops(struct module *mod);
extern int static_key_count(struct static_key *key);
extern void static_key_enable(struct static_key *key);
+extern void static_key_enable_cpuslocked(struct static_key *key);
extern void static_key_disable(struct static_key *key);
extern void static_key_disable_cpuslocked(struct static_key *key);
@@ -252,6 +253,11 @@ static inline void static_key_enable(struct static_key *key)
static_key_slow_inc(key);
}
+static inline void static_key_enable_cpuslocked(struct static_key *key)
+{
+ static_key_enable(key);
+}
+
static inline void static_key_disable(struct static_key *key)
{
int count = static_key_count(key);
@@ -429,6 +435,7 @@ struct static_key_false {
*/
#define static_branch_enable(x) static_key_enable(&(x)->key)
+#define static_branch_enable_cpuslocked(x) static_key_enable_cpuslocked(&(x)->key)
#define static_branch_disable(x) static_key_disable(&(x)->key)
#define static_branch_disable_cpuslocked(x) static_key_disable_cpuslocked(&(x)->key)
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index d71124e..6343f4c 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -90,6 +90,16 @@ void static_key_enable(struct static_key *key)
}
EXPORT_SYMBOL_GPL(static_key_enable);
+void static_key_enable_cpuslocked(struct static_key *key)
+{
+ int count = static_key_count(key);
+
+ WARN_ON_ONCE(count < 0 || count > 1);
+
+ if (!count)
+ static_key_slow_inc_cpuslocked(key);
+}
+
void static_key_disable(struct static_key *key)
{
int count = static_key_count(key);
--
1.9.1
^ permalink raw reply related
* [PATCHv2 2/2] arm64: cpufeature: use static_branch_enable_cpuslocked()
From: Mark Rutland @ 2017-04-27 17:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1493315077-19496-1-git-send-email-mark.rutland@arm.com>
Recently, the hotplug locking was conveted to use a percpu rwsem. Unlike
the existing {get,put}_online_cpus() logic, this can't nest.
Unfortunately, in arm64's secondary boot path we can end up nesting via
static_branch_enable() in cpus_set_cap() when we detect an erratum.
This leads to a stream of messages as below, where the secondary
attempts to schedule before it has been fully onlined. As the CPU
orchestrating the onlining holds the rswem, this hangs the system.
[ 0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
[ 0.250337] Modules linked in:
[ 0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
[ 0.250349] Hardware name: ARM Juno development board (r1) (DT)
[ 0.250353] Call trace:
[ 0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
[ 0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
[ 0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
[ 0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
[ 0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
[ 0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
[ 0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
[ 0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
[ 0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
[ 0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
[ 0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
[ 0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
[ 0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
[ 0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
[ 0.250444] [<000000008093d1b4>] 0x8093d1b4
We call cpus_set_cap() from update_cpu_capabilities(), which is called
from the secondary boot path (where the CPU orchestrating the onlining
holds the hotplug rwsem), and in the primary boot path, where this is
not held.
This patch makes cpus_set_cap() use static_branch_enable_cpuslocked(),
and updates all the callers of update_cpu_capabilities() consistent with
the change.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
[Mark: minor fixups]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 5 +++--
arch/arm64/kernel/cpu_errata.c | 13 ++++++++++++-
arch/arm64/kernel/cpufeature.c | 5 ++++-
arch/arm64/kernel/smp.c | 7 +++----
4 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index f31c48d..c96353a 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
num, ARM64_NCAPS);
} else {
__set_bit(num, cpu_hwcaps);
- static_branch_enable(&cpu_hwcap_keys[num]);
+ static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
}
}
@@ -222,7 +222,8 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
void enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps);
void check_local_cpu_capabilities(void);
-void update_cpu_errata_workarounds(void);
+void update_secondary_cpu_errata_workarounds(void);
+void update_boot_cpu_errata_workarounds(void);
void __init enable_errata_workarounds(void);
void verify_local_cpu_errata_workarounds(void);
diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
index f6cc67e..379ad8d 100644
--- a/arch/arm64/kernel/cpu_errata.c
+++ b/arch/arm64/kernel/cpu_errata.c
@@ -175,9 +175,20 @@ void verify_local_cpu_errata_workarounds(void)
}
}
-void update_cpu_errata_workarounds(void)
+/*
+ * Secondary CPUs are booted with the waker holding the
+ * CPU hotplug lock, hence we don't need to lock it here again.
+ */
+void update_secondary_cpu_errata_workarounds(void)
+{
+ update_cpu_capabilities(arm64_errata, "enabling workaround for");
+}
+
+void update_boot_cpu_errata_workarounds(void)
{
+ get_online_cpus();
update_cpu_capabilities(arm64_errata, "enabling workaround for");
+ put_online_cpus();
}
void __init enable_errata_workarounds(void)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index abda8e8..62d3a12 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -956,6 +956,7 @@ static void __init setup_elf_hwcaps(const struct arm64_cpu_capabilities *hwcaps)
cap_set_elf_hwcap(hwcaps);
}
+/* Should be called with CPU hotplug lock held */
void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
const char *info)
{
@@ -1075,14 +1076,16 @@ void check_local_cpu_capabilities(void)
* advertised capabilities.
*/
if (!sys_caps_initialised)
- update_cpu_errata_workarounds();
+ update_secondary_cpu_errata_workarounds();
else
verify_local_cpu_capabilities();
}
static void __init setup_feature_capabilities(void)
{
+ get_online_cpus();
update_cpu_capabilities(arm64_features, "detected feature:");
+ put_online_cpus();
enable_cpu_capabilities(arm64_features);
}
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 9b10365..d9ddd5b 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -447,11 +447,10 @@ void __init smp_prepare_boot_cpu(void)
cpuinfo_store_boot_cpu();
save_boot_cpu_run_el();
/*
- * Run the errata work around checks on the boot CPU, once we have
- * initialised the cpu feature infrastructure from
- * cpuinfo_store_boot_cpu() above.
+ * Run the errata work around checks on the boot CPU, now that
+ * cpuinfo_store_boot_cpu() has set things up.
*/
- update_cpu_errata_workarounds();
+ update_boot_cpu_errata_workarounds();
}
static u64 __init of_get_cpu_mpidr(struct device_node *dn)
--
1.9.1
^ permalink raw reply related
* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Christoffer Dall @ 2017-04-27 17:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f7b51e1e-1e66-2076-9b94-2ca0416531a9@redhat.com>
On Thu, Apr 27, 2017 at 07:27:22PM +0200, Auger Eric wrote:
> Hi Christoffer,
>
> On 27/04/2017 18:38, Christoffer Dall wrote:
> > On Thu, Apr 27, 2017 at 05:29:35PM +0200, Auger Eric wrote:
> >>
> >>
> >> On 27/04/2017 16:45, Christoffer Dall wrote:
> >>> Hi Eric,
> >>>
> >>> On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote:
> >>>> On 27/04/2017 13:02, Christoffer Dall wrote:
> >>>>> On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote:
> >>>>>> On 27/04/2017 10:57, Christoffer Dall wrote:
> >>>>>>> On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote:
> >>>>>>>> On 26/04/2017 14:31, Christoffer Dall wrote:
> >>>>>>>>> On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote:
> >>>>>>>>>> Add description for how to access ITS registers and how to save/restore
> >>>>>>>>>> ITS tables into/from memory.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>>>>>>>>>
> >>>>>>>>>> ---
> >>>>>>>>>> v4 -> v5:
> >>>>>>>>>> - take into account Christoffer's comments
> >>>>>>>>>> - pending table save on GICV3 side now
> >>>>>>>>>>
> >>>>>>>>>> v3 -> v4:
> >>>>>>>>>> - take into account Peter's comments:
> >>>>>>>>>> - typos
> >>>>>>>>>> - KVM_DEV_ARM_VGIC_GRP_ITS_TABLES kvm_device_attr = 0
> >>>>>>>>>> - add a validity bit in DTE
> >>>>>>>>>> - document all fields in CTE and ITE
> >>>>>>>>>> - document ABI revision
> >>>>>>>>>> - take into account Andre's comments:
> >>>>>>>>>> - document restrictions about GITS_CREADR writing and GITS_IIDR
> >>>>>>>>>> - document -EBUSY error if one or more VCPUS are runnning
> >>>>>>>>>> - document 64b registers only can be accessed with 64b access
> >>>>>>>>>> - itt_addr field matches bits [51:8] of the itt_addr
> >>>>>>>>>>
> >>>>>>>>>> v1 -> v2:
> >>>>>>>>>> - DTE and ITE now are 8 bytes
> >>>>>>>>>> - DTE and ITE now indexed by deviceid/eventid
> >>>>>>>>>> - use ITE name instead of ITTE
> >>>>>>>>>> - mentions ITT_addr matches bits [51:8] of the actual address
> >>>>>>>>>> - mentions LE layout
> >>>>>>>>>> ---
> >>>>>>>>>> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 99 ++++++++++++++++++++++
> >>>>>>>>>> 1 file changed, 99 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >>>>>>>>>> index 6081a5b..b5f010d 100644
> >>>>>>>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >>>>>>>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >>>>>>>>>> @@ -32,7 +32,106 @@ Groups:
> >>>>>>>>>> KVM_DEV_ARM_VGIC_CTRL_INIT
> >>>>>>>>>> request the initialization of the ITS, no additional parameter in
> >>>>>>>>>> kvm_device_attr.addr.
> >>>>>>>>>> +
> >>>>>>>>>> + KVM_DEV_ARM_ITS_SAVE_TABLES
> >>>>>>>>>> + save the ITS table data into guest RAM, at the location provisioned
> >>>>>>>>>> + by the guest in corresponding registers/table entries.
> >>>>>>>>>> +
> >>>>>>>>>> + The layout of the tables in guest memory defines an ABI. The entries
> >>>>>>>>>> + are laid out in little endian format as described in the last paragraph.
> >>>>>>>>>> +
> >>>>>>>>>> + KVM_DEV_ARM_ITS_RESTORE_TABLES
> >>>>>>>>>> + restore the ITS tables from guest RAM to ITS internal structures.
> >>>>>>>>>> +
> >>>>>>>>>> + The GICV3 must be restored before the ITS and all ITS registers but
> >>>>>>>>>> + the GITS_CTLR must be restored before restoring the ITS tables.
> >>>>>>>>>> +
> >>>>>>>>>> + The GITS_IIDR read-only register must also be restored before
> >>>>>>>>>> + the table restore as the IIDR revision field encodes the ABI revision.
> >>>>>>>>>> +
> >>>>>>>>>
> >>>>>>>>> what is the expected sequence of operations. For example, to restore
> >>>>>>>>> the ITS, do I call KVM_DEV_ARM_VGIC_CTRL_INIT first, then restore all
> >>>>>>>>> the memory and registers, and finally call KVM_DEV_ARM_ITS_RESTORE_TABLES?
> >>>>>>>> Yes KVM_DEV_ARM_VGIC_CTRL_INIT comes first, then restore all registers
> >>>>>>>> except GITS_CTLR, then table restore, then GITS_CTLR
> >>>>>>>>>
> >>>>>>>>> Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES
> >>>>>>>>> and restore GITS_CTLR (which enables the ITS)?
> >>>>>>>>
> >>>>>>>> Yep, when GITS_CTLR is set, LPIs may be enabled and this on that event
> >>>>>>>> that the pending table is read. But the whole pending table is not read
> >>>>>>>> as we only iterate on registered LPIs. So the ITT must have been
> >>>>>>>> restored previously.
> >>>>>>>>
> >>>>>>>> I became aware that the pending table sync is done twice, once in the
> >>>>>>>> pending table restore, and once in the GITS_CTLR restore. So if we
> >>>>>>>> leave this order specification, I should be able to remove the sync on
> >>>>>>>> table restore. This was the original reason why GITS_CTLR restore has
> >>>>>>>> been done at the very end.
> >>>>>>>
> >>>>>>> I'm sorry, I'm a bit confused. Do we not need
> >>>>>>> KVM_DEV_ARM_ITS_RESTORE_TABLES at all then?
> >>>>>>
> >>>>>> Yes you do. I was talking about the RDIST pending table sync. The save
> >>>>>> is explicit using GICV3 device KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.
> >>>>>> However the sync is implicit on GITS_CTLR restore if LPIs are enabled.
> >>>>>> and today I do it also on ITS device KVM_DEV_ARM_ITS_RESTORE_TABLES
> >>>>>> which is not requested I think since GITS_CTLR restore does it already.
> >>>>>
> >>>>> Shouldn't restoring the pending tables happen when restoring some
> >>>>> redeistributor state and not anything related to the ITS?
> >>>>
> >>>> Marc wrote:
> >>>> "
> >>>> I don't think you necessarily need a coarse map. When restoring the ITS
> >>>> tables, you can always read the pending bit when creating the LPI
> >>>> structure (it has been written to RAM at save time). Note that we
> >>>> already do something like this in vgic_enable_lpis().
> >>>> "
> >>>>
> >>>> This is currently what is implemented I think. the pending tables are
> >>>> currently sync'ed on GITS_CTLR set (if LPI are enabled) + erroneously
> >>>> also on on ITS table restore
> >>>>
> >>>> The problematic is: Either you know in advance which LPI INTIDare used
> >>>> or you need to parse the whole pending table (possibly using the 1st kB
> >>>> as coarse mapping).
> >>>>
> >>>> If you don't know the LPI INTIDs in advance it is only possible to
> >>>> restore the pending bit of pending LPIs. At that time you would
> >>>> re-allocate those pending LPI (vgic_add_lpi) and when you restore the
> >>>> ITS ITT you would do the same for those which were not pending. Looks
> >>>> really heavy to me: coarse mapping + dual vgic_add_lpi path.
> >>>>
> >>>> Otherwise we would need to add another dependency between RDIST pending
> >>>> table restore and ITS table restore but this looks even more weird, no?
> >>>>
> >>>>
> >>> So I just sat down with Andre and Marc and we tried to work through this
> >>> and came up with the best scheme. I apologize in advance for the
> >>> one-way nature of this e-mail, and I am of course open to discussing the
> >>> following proposal again if you do not agree.
> >>>
> >>> What I think this document should say, is that the following ordering
> >>> must be followed when restoring the GIC and the ITS:
> >>>
> >>> First, restore all guest memory
> >>>
> >>> Second, restore ALL redistributors
> >>>
> >>> Third, restore the ITS, in the following order:
> >>> 1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT)
> >>> 2. Restore GITS_CBASER
> >>> 3. Restore all other GITS_ registers, except GITS_CTLR!
> >>> 4. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES)
> >>> 5. Restore GITS_CTLR
> >>>
> >>> The rationale is that we really want the redistributor and the ITS
> >>> restore to be independent and follow the architecture. This means that
> >>> our ABI for the redistributor should still work without restoring an ITS
> >>> (if we ever decide to support LPIs for KVM without the ITS).
> >>
> >> OK. Note I already mentioned that GICv3 must be restored before the ITS.
> >> To me this comprised the RDIST.
> >
> > Possibly, but I think it's good to write out the whole thing so we
> > clearly understand the flow. That could better be achieved by
> > correcting my proposed text above to say something like "Second, restore
> > ALL redistributors to ensure the pending and configuration tables can be
> > read."
> >
> >>
> >> I understand the above description of the ordering comes in addition to
> >> the existing text, right?
> >
> > Yes
> >
> >> in other words I keep the GITS_READR,
> >> GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES
> >> section.
> >>
> >
> > Yes. But you don't need to do any reading of the pending table on any
> > of the restore operations.
> well you told me to do it on vgic_add_lpi(). This is obviously called on
> ITS table restore. /me confused.
Sorry, I meant you do not need to scan the entire table independently
from restoring other state that requires building the data structures.
> Obviously this is implicit and should
> not be documented. Is that what you meant? btw this is not documented
> atm I think.
What I care about is that the ABI is clear and represents what the
architecture does. So in terms of documentation in the ABI, we don't
need to mention anything about when this is done, but we also do not
need to specify any interaction between the pending tables and the ITS,
beyond that the redestributors and memory must be restored before the
ITS.
Hope this clarifies.
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH v2 1/3] mm: Silence vmap() allocation failures based on caller gfp_flags
From: Michal Hocko @ 2017-04-27 17:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427173900.2538-2-f.fainelli@gmail.com>
On Thu 27-04-17 10:38:58, Florian Fainelli wrote:
> If the caller has set __GFP_NOWARN don't print the following message:
> vmap allocation for size 15736832 failed: use vmalloc=<size> to increase
> size.
>
> This can happen with the ARM/Linux or ARM64/Linux module loader built
> with CONFIG_ARM{,64}_MODULE_PLTS=y which does a first attempt at loading
> a large module from module space, then falls back to vmalloc space.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
just a nit
> ---
> mm/vmalloc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 0b057628a7ba..d8a851634674 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -521,9 +521,13 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> }
> }
>
> + if (gfp_mask & __GFP_NOWARN)
> + goto out;
> +
> if (printk_ratelimit())
if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit())
> pr_warn("vmap allocation for size %lu failed: use vmalloc=<size> to increase size\n",
> size);
would be shorter and you wouldn't need the goto and a label.
> +out:
> kfree(va);
> return ERR_PTR(-EBUSY);
> }
> --
> 2.9.3
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [PATCHv2 2/2] arm64: cpufeature: use static_branch_enable_cpuslocked()
From: Will Deacon @ 2017-04-27 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1493315077-19496-3-git-send-email-mark.rutland@arm.com>
On Thu, Apr 27, 2017 at 06:44:37PM +0100, Mark Rutland wrote:
> Recently, the hotplug locking was conveted to use a percpu rwsem. Unlike
> the existing {get,put}_online_cpus() logic, this can't nest.
> Unfortunately, in arm64's secondary boot path we can end up nesting via
> static_branch_enable() in cpus_set_cap() when we detect an erratum.
>
> This leads to a stream of messages as below, where the secondary
> attempts to schedule before it has been fully onlined. As the CPU
> orchestrating the onlining holds the rswem, this hangs the system.
>
> [ 0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
> [ 0.250337] Modules linked in:
> [ 0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
> [ 0.250349] Hardware name: ARM Juno development board (r1) (DT)
> [ 0.250353] Call trace:
> [ 0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
> [ 0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
> [ 0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
> [ 0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
> [ 0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
> [ 0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
> [ 0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
> [ 0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
> [ 0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
> [ 0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
> [ 0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
> [ 0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
> [ 0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
> [ 0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
> [ 0.250444] [<000000008093d1b4>] 0x8093d1b4
>
> We call cpus_set_cap() from update_cpu_capabilities(), which is called
> from the secondary boot path (where the CPU orchestrating the onlining
> holds the hotplug rwsem), and in the primary boot path, where this is
> not held.
>
> This patch makes cpus_set_cap() use static_branch_enable_cpuslocked(),
> and updates all the callers of update_cpu_capabilities() consistent with
> the change.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Reported-by: Catalin Marinas <catalin.marinas@arm.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> [Mark: minor fixups]
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> arch/arm64/include/asm/cpufeature.h | 5 +++--
> arch/arm64/kernel/cpu_errata.c | 13 ++++++++++++-
> arch/arm64/kernel/cpufeature.c | 5 ++++-
> arch/arm64/kernel/smp.c | 7 +++----
> 4 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index f31c48d..c96353a 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
> num, ARM64_NCAPS);
> } else {
> __set_bit(num, cpu_hwcaps);
> - static_branch_enable(&cpu_hwcap_keys[num]);
> + static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
> }
> }
>
> @@ -222,7 +222,8 @@ void update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
> void enable_cpu_capabilities(const struct arm64_cpu_capabilities *caps);
> void check_local_cpu_capabilities(void);
>
> -void update_cpu_errata_workarounds(void);
> +void update_secondary_cpu_errata_workarounds(void);
> +void update_boot_cpu_errata_workarounds(void);
> void __init enable_errata_workarounds(void);
> void verify_local_cpu_errata_workarounds(void);
>
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index f6cc67e..379ad8d 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -175,9 +175,20 @@ void verify_local_cpu_errata_workarounds(void)
> }
> }
>
> -void update_cpu_errata_workarounds(void)
> +/*
> + * Secondary CPUs are booted with the waker holding the
> + * CPU hotplug lock, hence we don't need to lock it here again.
> + */
> +void update_secondary_cpu_errata_workarounds(void)
> +{
> + update_cpu_capabilities(arm64_errata, "enabling workaround for");
> +}
> +
> +void update_boot_cpu_errata_workarounds(void)
> {
> + get_online_cpus();
> update_cpu_capabilities(arm64_errata, "enabling workaround for");
> + put_online_cpus();
> }
These functions seem to have unhelpful names, especially when compared to
the naming scheme used by the core code. I'd prefer to have:
update_cpu_errata_workarounds: just calls update_cpu_capabilities
update_cpu_errata_workarounds_cpuslocked: does get_online_cpus(), then calls
update_cpu_errata_workarounds, then does put_online_cpus();
With that change:
Acked-by: Will Deacon <will.deacon@arm.com>
for -tip.
Will
^ permalink raw reply
* [PATCH v2 1/3] mm: Silence vmap() allocation failures based on caller gfp_flags
From: Florian Fainelli @ 2017-04-27 18:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427175653.GB30672@dhcp22.suse.cz>
On 04/27/2017 10:56 AM, Michal Hocko wrote:
> On Thu 27-04-17 10:38:58, Florian Fainelli wrote:
>> If the caller has set __GFP_NOWARN don't print the following message:
>> vmap allocation for size 15736832 failed: use vmalloc=<size> to increase
>> size.
>>
>> This can happen with the ARM/Linux or ARM64/Linux module loader built
>> with CONFIG_ARM{,64}_MODULE_PLTS=y which does a first attempt at loading
>> a large module from module space, then falls back to vmalloc space.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> just a nit
>
>> ---
>> mm/vmalloc.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index 0b057628a7ba..d8a851634674 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -521,9 +521,13 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
>> }
>> }
>>
>> + if (gfp_mask & __GFP_NOWARN)
>> + goto out;
>> +
>> if (printk_ratelimit())
>
> if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit())
>> pr_warn("vmap allocation for size %lu failed: use vmalloc=<size> to increase size\n",
>> size);
>
> would be shorter and you wouldn't need the goto and a label.
Do you want me to resubmit with that change included?
--
Florian
^ permalink raw reply
* [PATCH v5 16/22] KVM: arm64: vgic-its: Add infrastructure for table lookup
From: Christoffer Dall @ 2017-04-27 18:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1492164934-988-17-git-send-email-eric.auger@redhat.com>
On Fri, Apr 14, 2017 at 12:15:28PM +0200, Eric Auger wrote:
> Add a generic lookup_table() helper whose role consists in
> scanning a contiguous table located in guest RAM and applying
> a callback on each entry. Entries can be handled as linked lists
> since the callback may return an offset to the next entry and
> also tell that an entry is the last one.
>
> Helper functions also are added to compute the device/event ID
> offset to the next DTE/ITE.
>
> compute_next_devid_offset, compute_next_eventid_offset and
> lookup_table will become static in subsequent patches
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
> v4 -> v5:
> - use kvm_read_guest
>
> v3 -> v4:
> - remove static to avoid compilation warning
> - correct size computation in looup_table()
> - defines now encode the number of bits used for devid and eventid offsets
> - use BIT() - 1 to encode the max offets
> ---
> virt/kvm/arm/vgic/vgic-its.c | 93 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 56c5123..c22b35d 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -195,6 +195,8 @@ static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
>
> #define VITS_TYPER_IDBITS 16
> #define VITS_TYPER_DEVBITS 16
> +#define VITS_DTE_MAX_DEVID_OFFSET (BIT(14) - 1)
> +#define VITS_ITE_MAX_EVENTID_OFFSET (BIT(16) - 1)
>
> /*
> * Finds and returns a collection in the ITS collection table.
> @@ -1674,6 +1676,97 @@ int vgic_its_attr_regs_access(struct kvm_device *dev,
> return ret;
> }
>
> +u32 compute_next_devid_offset(struct list_head *h, struct its_device *dev)
> +{
> + struct list_head *e = &dev->dev_list;
> + struct its_device *next;
> + u32 next_offset;
> +
> + if (e->next == h)
> + return 0;
> + next = list_entry(e->next, struct its_device, dev_list);
> + next_offset = next->device_id - dev->device_id;
> +
> + return min_t(u32, next_offset, VITS_DTE_MAX_DEVID_OFFSET);
> +}
> +
> +u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
> +{
> + struct list_head *e = &ite->ite_list;
> + struct its_ite *next;
> + u32 next_offset;
> +
> + if (e->next == h)
> + return 0;
> + next = list_entry(e->next, struct its_ite, ite_list);
> + next_offset = next->event_id - ite->event_id;
> +
> + return min_t(u32, next_offset, VITS_ITE_MAX_EVENTID_OFFSET);
> +}
> +
> +/**
> + * entry_fn_t - Callback called on a table entry restore path
> + * @its: its handle
> + * @id: id of the entry
> + * @entry: pointer to the entry
> + * @opaque: pointer to an opaque data
> + * @next_offset: minimal ID offset to the next entry. 0 if this
> + * entry is the last one, 1 if the entry is invalid, >= 1 if an
> + * entry's next_offset field was truly decoded
> + *
> + * Return: < 0 on error, 0 otherwise
> + */
> +typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
> + void *opaque, u32 *next_offset);
> +
> +/**
> + * lookup_table - scan a contiguous table in guest RAM and applies a function
> + * to each entry
> + *
> + * @its: its handle
> + * @base: base gpa of the table
> + * @size: size of the table in bytes
> + * @esz: entry size in bytes
> + * @start_id: first entry's ID
I'm a little confused. Why is this not 0?
> + * @fn: function to apply on each entry
> + *
> + * Return: < 0 on error, 1 if last element identified, 0 otherwise
How can you scan the entire table and not find the last element?
> + */
> +int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
> + int start_id, entry_fn_t fn, void *opaque)
Is this really a lookup? I think this should be called
'scan_its_table'.
> +{
> + void *entry = kzalloc(esz, GFP_KERNEL);
> + struct kvm *kvm = its->dev->kvm;
> + unsigned long len = size;
> + u32 id = start_id;
why are ids u32 and not just an unsigned long?
> + gpa_t gpa = base;
> + int ret;
> +
> + while (len > 0) {
> + u32 next_offset;
> + size_t byte_offset;
> +
> + ret = kvm_read_guest(kvm, gpa, entry, esz);
> + if (ret)
> + goto out;
> +
> + ret = fn(its, id, entry, opaque, &next_offset);
> + if (ret < 0 || (!ret && !next_offset))
> + goto out;
> +
> + byte_offset = next_offset * esz;
> + id += next_offset;
> + gpa += byte_offset;
> + len -= byte_offset;
> + }
> + kfree(entry);
> + return 0;
> +
here you can just set 'ret = 0;'
> +out:
> + kfree(entry);
> + return (ret < 0 ? ret : 1);
and then here you can do 'return (ret <= 0 ? ret : 1)'
but I actually think it's nicer to just write:
if (ret <= 0)
return ret;
return 1;
> +}
> +
> /**
> * vgic_its_save_device_tables - Save the device table and all ITT
> * into guest RAM
> --
> 2.5.5
>
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH] arm64: KVM: Fix decoding of Rt/Rt2 when trapping AArch32 CP accesses
From: Marc Zyngier @ 2017-04-27 18:06 UTC (permalink / raw)
To: linux-arm-kernel
Our 32bit CP14/15 handling inherited some of the ARMv7 code for handling
the trapped system registers, completely missing the fact that the
fields for Rt and Rt2 are now 5 bit wide, and not 4...
Let's fix it, and provide an accessor for the most common Rt case.
Cc: stable at vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
arch/arm64/include/asm/kvm_emulate.h | 6 ++++++
arch/arm64/kvm/sys_regs.c | 8 ++++----
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index f5ea0ba70f07..fe39e6841326 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -240,6 +240,12 @@ static inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vcpu)
return kvm_vcpu_get_hsr(vcpu) & ESR_ELx_FSC_TYPE;
}
+static inline int kvm_vcpu_sys_get_rt(struct kvm_vcpu *vcpu)
+{
+ u32 esr = kvm_vcpu_get_hsr(vcpu);
+ return (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
+}
+
static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
{
return vcpu_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index efbe9e8e7a78..0fe27024a2e1 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1529,8 +1529,8 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
{
struct sys_reg_params params;
u32 hsr = kvm_vcpu_get_hsr(vcpu);
- int Rt = (hsr >> 5) & 0xf;
- int Rt2 = (hsr >> 10) & 0xf;
+ int Rt = kvm_vcpu_sys_get_rt(vcpu);
+ int Rt2 = (hsr >> 10) & 0x1f;
params.is_aarch32 = true;
params.is_32bit = false;
@@ -1586,7 +1586,7 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
{
struct sys_reg_params params;
u32 hsr = kvm_vcpu_get_hsr(vcpu);
- int Rt = (hsr >> 5) & 0xf;
+ int Rt = kvm_vcpu_sys_get_rt(vcpu);
params.is_aarch32 = true;
params.is_32bit = true;
@@ -1688,7 +1688,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
{
struct sys_reg_params params;
unsigned long esr = kvm_vcpu_get_hsr(vcpu);
- int Rt = (esr >> 5) & 0x1f;
+ int Rt = kvm_vcpu_sys_get_rt(vcpu);
int ret;
trace_kvm_handle_sys_reg(esr);
--
2.11.0
^ permalink raw reply related
* [PATCH v2 3/3] arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
From: Ard Biesheuvel @ 2017-04-27 18:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427173900.2538-4-f.fainelli@gmail.com>
> On 27 Apr 2017, at 18:39, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> When CONFIG_ARM64_MODULE_PLTS is enabled, the first allocation using the
> module space fails, because the module is too big, and then the module
> allocation is attempted from vmalloc space. Silence the first allocation
> failure in that case by setting __GFP_NOWARN.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> arch/arm64/kernel/module.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 7f316982ce00..58bd5cfdd544 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -32,11 +32,16 @@
>
> void *module_alloc(unsigned long size)
> {
> + gfp_t gfp_mask = GFP_KERNEL;
> void *p;
>
> +#if IS_ENABLED(CONFIG_ARM64_MODULE_PLTS)
> + /* Silence the initial allocation */
> + gfp_mask |= __GFP_NOWARN;
> +#endif
Please use IS_ENABLED() instead here
> p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> module_alloc_base + MODULES_VSIZE,
> - GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> + gfp_mask, PAGE_KERNEL_EXEC, 0,
> NUMA_NO_NODE, __builtin_return_address(0));
>
> if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
> --
> 2.9.3
>
Other than that, and with Michal's nit addressed:
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
^ permalink raw reply
* [PATCH v5 12/22] KVM: arm64: vgic-its: Interpret MAPD ITT_addr field
From: Christoffer Dall @ 2017-04-27 18:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5f187dbf-6167-7bd3-4a8e-37545941d05c@redhat.com>
On Thu, Apr 27, 2017 at 07:44:25PM +0200, Auger Eric wrote:
> Hi,
>
> On 27/04/2017 18:43, Christoffer Dall wrote:
> > On Fri, Apr 14, 2017 at 12:15:24PM +0200, Eric Auger wrote:
> >> Up to now the MAPD ITT_addr had been ignored. We will need it
> >> for save/restore. Let's record it in the its_device struct.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>
> >> ---
> >> v4 -> v5:
> >> - its_cmd_get_ittaddr macro now returns the actual ITT GPA
> >>
> >> v3 -> v4:
> >> - in vgic_its_cmd_handle_mapd, itt_addr directly is shifted
> >> - correct ittaddr bitmask to support 48bit GPA
> >> ---
> >> virt/kvm/arm/vgic/vgic-its.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index 0f3c8f3..757598d 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -106,6 +106,7 @@ struct its_device {
> >> /* the head for the list of ITTEs */
> >> struct list_head itt_head;
> >> u32 nb_eventid_bits;
> >> + gpa_t itt_addr;
> >> u32 device_id;
> >> };
> >>
> >> @@ -569,6 +570,7 @@ static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
> >> #define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1, 0, 32)
> >> #define its_cmd_get_physical_id(cmd) its_cmd_mask_field(cmd, 1, 32, 32)
> >> #define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2, 0, 16)
> >> +#define its_cmd_get_ittaddr(cmd) (its_cmd_mask_field(cmd, 2, 8, 47) << 8)
> >
> > Why 47? The ITT_addr field is bits [51:8] unless I'm looking at
> > something wrong?
>
> Initially I limited to 48 bits since I found in the code:
>
> /*
> * We only implement 48 bits of PA at the moment, although the ITS
> * supports more. Let's be restrictive here.
> */
> #define BASER_ADDRESS(x) ((x) & GENMASK_ULL(47, 16))
> #define CBASER_ADDRESS(x) ((x) & GENMASK_ULL(47, 12))
>
> But practically as I encode the ITT addr field on 52 bits now in the DTE
> there is no reason anymore.
Well, regardless, shouldn't you be bassing 44 to its_cmd_mask_field(),
because you're shifting left 8 bits afterwards?
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH v2 3/3] arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
From: Florian Fainelli @ 2017-04-27 18:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <C103C078-3462-43D9-AEF5-5DEC3A74CA7E@linaro.org>
On 04/27/2017 11:07 AM, Ard Biesheuvel wrote:
>
>> On 27 Apr 2017, at 18:39, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> When CONFIG_ARM64_MODULE_PLTS is enabled, the first allocation using the
>> module space fails, because the module is too big, and then the module
>> allocation is attempted from vmalloc space. Silence the first allocation
>> failure in that case by setting __GFP_NOWARN.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> arch/arm64/kernel/module.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
>> index 7f316982ce00..58bd5cfdd544 100644
>> --- a/arch/arm64/kernel/module.c
>> +++ b/arch/arm64/kernel/module.c
>> @@ -32,11 +32,16 @@
>>
>> void *module_alloc(unsigned long size)
>> {
>> + gfp_t gfp_mask = GFP_KERNEL;
>> void *p;
>>
>> +#if IS_ENABLED(CONFIG_ARM64_MODULE_PLTS)
>> + /* Silence the initial allocation */
>> + gfp_mask |= __GFP_NOWARN;
>> +#endif
>
> Please use IS_ENABLED() instead here
How do you mean?
if (IS_ENABLED()) vs. #if IS_ENABLED()?
>
>> p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
>> module_alloc_base + MODULES_VSIZE,
>> - GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
>> + gfp_mask, PAGE_KERNEL_EXEC, 0,
>> NUMA_NO_NODE, __builtin_return_address(0));
>>
>> if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
>> --
>> 2.9.3
>>
>
> Other than that, and with Michal's nit addressed:
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
--
Florian
^ permalink raw reply
* [PATCH v2 3/3] arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
From: Ard Biesheuvel @ 2017-04-27 18:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53d960d0-e44c-3a8d-17fd-a3895ecee858@gmail.com>
> On 27 Apr 2017, at 19:09, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>> On 04/27/2017 11:07 AM, Ard Biesheuvel wrote:
>>
>>> On 27 Apr 2017, at 18:39, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>
>>> When CONFIG_ARM64_MODULE_PLTS is enabled, the first allocation using the
>>> module space fails, because the module is too big, and then the module
>>> allocation is attempted from vmalloc space. Silence the first allocation
>>> failure in that case by setting __GFP_NOWARN.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>> arch/arm64/kernel/module.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
>>> index 7f316982ce00..58bd5cfdd544 100644
>>> --- a/arch/arm64/kernel/module.c
>>> +++ b/arch/arm64/kernel/module.c
>>> @@ -32,11 +32,16 @@
>>>
>>> void *module_alloc(unsigned long size)
>>> {
>>> + gfp_t gfp_mask = GFP_KERNEL;
>>> void *p;
>>>
>>> +#if IS_ENABLED(CONFIG_ARM64_MODULE_PLTS)
>>> + /* Silence the initial allocation */
>>> + gfp_mask |= __GFP_NOWARN;
>>> +#endif
>>
>> Please use IS_ENABLED() instead here
>
> How do you mean?
>
> if (IS_ENABLED()) vs. #if IS_ENABLED()?
>
Apologies, I didn't read carefully.
Use the C if not the preprocessor if
>>
>>> p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
>>> module_alloc_base + MODULES_VSIZE,
>>> - GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
>>> + gfp_mask, PAGE_KERNEL_EXEC, 0,
>>> NUMA_NO_NODE, __builtin_return_address(0));
>>>
>>> if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
>>> --
>>> 2.9.3
>>>
>>
>> Other than that, and with Michal's nit addressed:
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>
>
> --
> Florian
^ permalink raw reply
* [PATCH v5 01/10] arm64: allwinner: a64: enable RSB on A64
From: icenowy at aosc.io @ 2017-04-27 18:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427132805.izv6xfktlhfc4yty@lukather>
? 2017-04-27 21:28?Maxime Ripard ???
> On Wed, Apr 26, 2017 at 11:20:14PM +0800, Icenowy Zheng wrote:
>> Allwinner A64 have a RSB controller like the one on A23/A33 SoCs.
>>
>> Add it and its pinmux.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> Acked-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>> Changes in v2:
>> - Removed bonus properties in pio node.
>> - Added Chen-Yu's ACK.
>>
>> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 19
>> +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> index c7f669f5884f..05ec9fc5e81f 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> @@ -422,6 +422,25 @@
>> #gpio-cells = <3>;
>> interrupt-controller;
>> #interrupt-cells = <3>;
>> +
>> + r_rsb_pins: rsb at 0 {
>> + pins = "PL0", "PL1";
>> + function = "s_rsb";
>> + };
>> + };
>> +
>> + r_rsb: rsb at 1f03400 {
>> + compatible = "allwinner,sun8i-a23-rsb";
>> + reg = <0x01f03400 0x400>;
>> + interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> + clocks = <&r_ccu 6>;
>
> Please use the defines here..
Linux-4.12 doesn't yet enter rc1, and the defines are still not in
Linus's tree.
Please note that I have already mentioned that this patch is necessary
to be merged into 4.12, otherwise poweroff won't work properly at 4.12 .
So I think it shouldn't still use defines.
I will fix here after 4.12-rc1 is out, along with other r_ccu
usages.
>
>> + clock-frequency = <3000000>;
>> + resets = <&r_ccu 2>;
>
> And here.
>
> Thanks!
> Maxime
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox