Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] drivers:soc:fsl:qbman:qman.c: Sleep instead of stuck hacking jiffies.
From: Karim Eshapa @ 2017-05-04  4:58 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1493508775.25397.26.camel@buserror.net>

Avoid stuck and hacking jiffies for a long time and using msleep()
for certatin numeber of cylces without the factor of safety
but using the the long delay considering the factor of safety
with the while loop such that after msleep for a short period
of time we check the  msg if it's OK, breaking the big loop delay.

Signed-off-by: Karim Eshapa <karim.eshapa@gmail.com>
---
 drivers/soc/fsl/qbman/qman.c | 47 ++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index 6f509f6..4f99298 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -1067,32 +1067,33 @@ static irqreturn_t portal_isr(int irq, void *ptr)
 static int drain_mr_fqrni(struct qm_portal *p)
 {
 	const union qm_mr_entry *msg;
+	unsigned long stop;
+	unsigned int timeout = jiffies_to_msecs(1000);
 loop:
 	msg = qm_mr_current(p);
-	if (!msg) {
-		/*
-		 * if MR was full and h/w had other FQRNI entries to produce, we
-		 * need to allow it time to produce those entries once the
-		 * existing entries are consumed. A worst-case situation
-		 * (fully-loaded system) means h/w sequencers may have to do 3-4
-		 * other things before servicing the portal's MR pump, each of
-		 * which (if slow) may take ~50 qman cycles (which is ~200
-		 * processor cycles). So rounding up and then multiplying this
-		 * worst-case estimate by a factor of 10, just to be
-		 * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
-		 * one entry at a time, so h/w has an opportunity to produce new
-		 * entries well before the ring has been fully consumed, so
-		 * we're being *really* paranoid here.
-		 */
-		u64 now, then = jiffies;
-
-		do {
-			now = jiffies;
-		} while ((then + 10000) > now);
+	stop = jiffies + 10000;
+	/*
+	 * if MR was full and h/w had other FQRNI entries to produce, we
+	 * need to allow it time to produce those entries once the
+	 * existing entries are consumed. A worst-case situation
+	 * (fully-loaded system) means h/w sequencers may have to do 3-4
+	 * other things before servicing the portal's MR pump, each of
+	 * which (if slow) may take ~50 qman cycles (which is ~200
+	 * processor cycles). So rounding up and then multiplying this
+	 * worst-case estimate by a factor of 10, just to be
+	 * ultra-paranoid, goes as high as 10,000 cycles. NB, we consume
+	 * one entry at a time, so h/w has an opportunity to produce new
+	 * entries well before the ring has been fully consumed, so
+	 * we're being *really* paranoid here.
+	 */
+	do {
+		if (msg)
+			break;
+		msleep(timeout);
 		msg = qm_mr_current(p);
-		if (!msg)
-			return 0;
-	}
+	} while (time_before(jiffies, stop));
+	if (!msg)
+		return 0;
 	if ((msg->verb & QM_MR_VERB_TYPE_MASK) != QM_MR_VERB_FQRNI) {
 		/* We aren't draining anything but FQRNIs */
 		pr_err("Found verb 0x%x in MR\n", msg->verb);
-- 
2.7.4

^ permalink raw reply related

* [PATCH] arm64: Add translation functions for /dev/mem read/write
From: Ard Biesheuvel @ 2017-05-04  6:40 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <345a59fb-085f-a776-2d7e-4889a0f86d7f@codeaurora.org>

On 3 May 2017 at 22:47, Goel, Sameer <sgoel@codeaurora.org> wrote:
>
>
> On 5/3/2017 2:18 PM, Leif Lindholm wrote:
>> On Wed, May 03, 2017 at 11:07:45AM -0600, Goel, Sameer wrote:
>>> On 5/3/2017 5:26 AM, Will Deacon wrote:
>>>> [adding some /dev/mem fans to cc]
>>>>
>>>> On Tue, May 02, 2017 at 02:28:05PM -0600, Sameer Goel wrote:
>>>>> Port architecture specific xlate and unxlate functions for /dev/mem
>>>>> read/write. This sets up the mapping for a valid physical address if a
>>>>> kernel direct mapping is not already present.
>>>>>
>>>>> This is a generic issue as a user space app should not be allowed to crash
>>>>> the kernel.
>>>>
>>>>> This issue was observed when systemd tried to access performance
>>>>> pointer record from the FPDT table.
>>>>
>>>> Why is it doing that? Is there not a way to get this via /sys?
>>>
>>> There is no ACPI FPDT implementation in the kernel, so the userspace
>>> systemd code is getting the FPDT table contents from /sys
>>> and parsing the entries. The performance pointer record is a
>>> reserved address populated by UEFI and the userspace code tries to
>>> access it using /dev/mem. The physical address is valid, so cannot
>>> push back on the user space code.
>>
>> OK, so then we need to add support for parsing this table in the
>> kernel and exposing the referred-to regions in a controllable fashion.
>> Maybe something that belongs under /sys/firmware/efi (adding Matt), or
>> maybe something that deserves its own driver.
>>
>> The only two use-cases for /dev/mem on arm64 are:
>> - Implementing interfaces in the kernel takes up-front effort.
>> - Being able to accidentally panic the kernel from userland.
>>
> We will see this issue with any access using /dev/mem to a MEMBLOCK_NOMAP marked
> memblock. The kernel crash issue has to be fixed irrespective of ACPI FPDT support.
>

I reported the same issue a couple of weeks ago [0]. So while we all
agree that such accesses shouldn't oops the kernel, I think we may
disagree on whether such accesses should be allowed in the first
place, especially when using read/write on /dev/mem (as opposed to
mmap())

One the UEFI/EDK2 side, there are two fundamental issues here, which
we should resolve:
- The use of EfiRuntimeServicesData for such regions: these tables
have no significance to the firmware itself (not after
ExitBootServices()) anyway, and so the only reason for choosing this
memory type is that they are guaranteed to be left untouched by the
OS. Also, using this type rather than something like 'ACPI Reclaim'
results in the memory to be occupied regardless of whether you
understand cq. are interested in its contents, which is something we
usually try to avoid in UEFI.
- The unconditional use of the EFI_MEMORY_RUNTIME attribute for
EfiRuntimeServicesData regions, which requests the OS to create a
runtime mapping for it in the OS page tables. We should be able to
take this attribute as a cue that the firmware has no interest in its
contents (given that it never requested a mapping for it) making it
safe for the OS to map it with any attributes it likes. However, EDK2
currently does not provide this facility, i.e., the EFI_MEMORY_RUNTIME
bit is always set.

So modulo the feedback on my patch, I think that approach is
preferred, and we should really look into cleaning this up further on
the firmware side. For now, the userland fix is to use mmap() rather
than read() (which is already documented in the code as something that
should not be relied upon to remain available indefinitely).





[0] http://marc.info/?l=linux-arm-kernel&m=149198561609050

^ permalink raw reply

* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Auger Eric @ 2017-05-04  7:00 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170427144516.GL50776@lvm>

Hi Christoffer,

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).
> 
> 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).  I suggest you do the pending check by adding a
> function called something like vgic_v3_lpi_is_pending() which scans the
> bit in memory, clears the memory bit, and returns the value.  Clearing
> the pending bit in memory when moving it to the struct irq is nice,
> because you then don't have to clear out the entire pending table later
> and we don't keep 'consumed' data lying around.  This change should be
> implemented in its_sync_lpi_pending_table() as well, but note that you
> need never call that function in the normal restore path using this
> design.
> 
> I hope this makes sense.

I am dubious about the above changes at the moment.
its_sync_lpi_pending_table() gets called on GITS_CTLR setting which is
documented to be the last step of the restoration. I wonder why the
above changes cannot be part of another series later on.

Consuming the RAM bit status means we record it in irq->pending_latch so
I guess we should have the irq->pending_latch setting in the same
function as the one that retrieves the bit status in guest RAM. So I
would rename vgic_v3_lpi_is_pending into something like
int vgic_v3_sync_lpi_pending_status(struct kvm *kvm, u32 intid)
Since this covers a single LPI, the removes the byte access optimization
found in its_sync_lpi_pending_table

Also if I understand it correctly this means the sync will be done on
both add_lpi and GITS_CTLR setting

What do you think?

Thanks

Eric
> 
> 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 19/22] KVM: arm64: vgic-its: ITT save and restore
From: Christoffer Dall @ 2017-05-04  7:31 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <3af9ae62-1e20-e0f4-a2a9-db0a7a1b10ef@redhat.com>

On Wed, May 03, 2017 at 11:55:34PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 03/05/2017 18:37, Christoffer Dall wrote:
> > On Wed, May 03, 2017 at 06:08:58PM +0200, Auger Eric wrote:
> >> Hi Christoffer,
> >>
> >> On 30/04/2017 22:14, Christoffer Dall wrote:
> >>> On Fri, Apr 14, 2017 at 12:15:31PM +0200, Eric Auger wrote:
> >>>> Introduce routines to save and restore device ITT and their
> >>>> interrupt table entries (ITE).
> >>>>
> >>>> The routines will be called on device table save and
> >>>> restore. They will become static in subsequent patches.
> >>>
> >>> Why this bottom-up approach?  Couldn't you start by having the patch
> >>> that restores the device table and define the static functions that
> >>> return an error there
> >> done
> >> , and then fill them in with subsequent patches
> >>> (liek this one)?
> >>>
> >>> That would have the added benefit of being able to tell how things are
> >>> designed to be called.
> >>>
> >>>>
> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>>>
> >>>> ---
> >>>> v4 -> v5:
> >>>> - ITE are now sorted by eventid on the flush
> >>>> - rename *flush* into *save*
> >>>> - use macros for shits and masks
> >>>> - pass ite_esz to vgic_its_save_ite
> >>>>
> >>>> v3 -> v4:
> >>>> - lookup_table and compute_next_eventid_offset become static in this
> >>>>   patch
> >>>> - remove static along with vgic_its_flush/restore_itt to avoid
> >>>>   compilation warnings
> >>>> - next field only computed with a shift (mask removed)
> >>>> - handle the case where the last element has not been found
> >>>>
> >>>> v2 -> v3:
> >>>> - add return 0 in vgic_its_restore_ite (was in subsequent patch)
> >>>>
> >>>> v2: creation
> >>>> ---
> >>>>  virt/kvm/arm/vgic/vgic-its.c | 128 ++++++++++++++++++++++++++++++++++++++++++-
> >>>>  virt/kvm/arm/vgic/vgic.h     |   4 ++
> >>>>  2 files changed, 129 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >>>> index 35b2ca1..b02fc3f 100644
> >>>> --- a/virt/kvm/arm/vgic/vgic-its.c
> >>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >>>> @@ -23,6 +23,7 @@
> >>>>  #include <linux/interrupt.h>
> >>>>  #include <linux/list.h>
> >>>>  #include <linux/uaccess.h>
> >>>> +#include <linux/list_sort.h>
> >>>>  
> >>>>  #include <linux/irqchip/arm-gic-v3.h>
> >>>>  
> >>>> @@ -1695,7 +1696,7 @@ u32 compute_next_devid_offset(struct list_head *h, struct its_device *dev)
> >>>>  	return min_t(u32, next_offset, VITS_DTE_MAX_DEVID_OFFSET);
> >>>>  }
> >>>>  
> >>>> -u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
> >>>> +static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
> >>>>  {
> >>>>  	struct list_head *e = &ite->ite_list;
> >>>>  	struct its_ite *next;
> >>>> @@ -1737,8 +1738,8 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
> >>>>   *
> >>>>   * Return: < 0 on error, 1 if last element identified, 0 otherwise
> >>>>   */
> >>>> -int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
> >>>> -		 int start_id, entry_fn_t fn, void *opaque)
> >>>> +static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
> >>>> +			int start_id, entry_fn_t fn, void *opaque)
> >>>>  {
> >>>>  	void *entry = kzalloc(esz, GFP_KERNEL);
> >>>>  	struct kvm *kvm = its->dev->kvm;
> >>>> @@ -1773,6 +1774,127 @@ int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
> >>>>  }
> >>>>  
> >>>>  /**
> >>>> + * vgic_its_save_ite - Save an interrupt translation entry at @gpa
> >>>> + */
> >>>> +static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
> >>>> +			      struct its_ite *ite, gpa_t gpa, int ite_esz)
> >>>> +{
> >>>> +	struct kvm *kvm = its->dev->kvm;
> >>>> +	u32 next_offset;
> >>>> +	u64 val;
> >>>> +
> >>>> +	next_offset = compute_next_eventid_offset(&dev->itt_head, ite);
> >>>> +	val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
> >>>> +	       ((u64)ite->lpi << KVM_ITS_ITE_PINTID_SHIFT) |
> >>>> +		ite->collection->collection_id;
> >>>> +	val = cpu_to_le64(val);
> >>>> +	return kvm_write_guest(kvm, gpa, &val, ite_esz);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * vgic_its_restore_ite - restore an interrupt translation entry
> >>>> + * @event_id: id used for indexing
> >>>> + * @ptr: pointer to the ITE entry
> >>>> + * @opaque: pointer to the its_device
> >>>> + * @next: id offset to the next entry
> >>>> + */
> >>>> +static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
> >>>> +				void *ptr, void *opaque, u32 *next)
> >>>> +{
> >>>> +	struct its_device *dev = (struct its_device *)opaque;
> >>>> +	struct its_collection *collection;
> >>>> +	struct kvm *kvm = its->dev->kvm;
> >>>> +	u64 val, *p = (u64 *)ptr;
> >>>
> >>> nit: initializations on separate line (and possible do that just above
> >>> assigning val).
> >> done
> >>>
> >>>> +	struct vgic_irq *irq;
> >>>> +	u32 coll_id, lpi_id;
> >>>> +	struct its_ite *ite;
> >>>> +	int ret;
> >>>> +
> >>>> +	val = *p;
> >>>> +	*next = 1;
> >>>> +
> >>>> +	val = le64_to_cpu(val);
> >>>> +
> >>>> +	coll_id = val & KVM_ITS_ITE_ICID_MASK;
> >>>> +	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
> >>>> +
> >>>> +	if (!lpi_id)
> >>>> +		return 0;
> >>>
> >>> are all non-zero LPI IDs valid?  Don't we have a wrapper that tests if
> >>> the ID is valid?
> >> no, lpi_id must be >= GIC_MIN_LPI=8192; added that check.
> >> ABI Doc says lpi_id==0 is interpreted as invalid. Other values <
> >> GIC_MIN_LPI cause an -EINVAL error
> >>>
> >>> (looks like it's possible to add LPIs with the INTID range of SPIs, SGIs
> >>> and PPIs here)
> >>
> >>>
> >>>> +
> >>>> +	*next = val >> KVM_ITS_ITE_NEXT_SHIFT;
> >>>
> >>> Don't we need to validate this somehow since it will presumably be used
> >>> to forward a pointer somehow by the caller?
> >> checked against max number of eventids supported by the device
> >>>
> >>>> +
> >>>> +	collection = find_collection(its, coll_id);
> >>>> +	if (!collection)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	ret = vgic_its_alloc_ite(dev, &ite, collection,
> >>>> +				  lpi_id, event_id);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	irq = vgic_add_lpi(kvm, lpi_id);
> >>>> +	if (IS_ERR(irq))
> >>>> +		return PTR_ERR(irq);
> >>>> +	ite->irq = irq;
> >>>> +
> >>>> +	/* restore the configuration of the LPI */
> >>>> +	ret = update_lpi_config(kvm, irq, NULL);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	update_affinity_ite(kvm, ite);
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +static int vgic_its_ite_cmp(void *priv, struct list_head *a,
> >>>> +			    struct list_head *b)
> >>>> +{
> >>>> +	struct its_ite *itea = container_of(a, struct its_ite, ite_list);
> >>>> +	struct its_ite *iteb = container_of(b, struct its_ite, ite_list);
> >>>> +
> >>>> +	if (itea->event_id < iteb->event_id)
> >>>> +		return -1;
> >>>> +	else
> >>>> +		return 1;
> >>>> +}
> >>>> +
> >>>> +int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
> >>>> +{
> >>>> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> >>>> +	gpa_t base = device->itt_addr;
> >>>> +	struct its_ite *ite;
> >>>> +	int ret, ite_esz = abi->ite_esz;
> >>>
> >>> nit: initializations on separate line
> >> OK
> >>>
> >>>> +
> >>>> +	list_sort(NULL, &device->itt_head, vgic_its_ite_cmp);
> >>>> +
> >>>> +	list_for_each_entry(ite, &device->itt_head, ite_list) {
> >>>> +		gpa_t gpa = base + ite->event_id * ite_esz;
> >>>> +
> >>>> +		ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>> +int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
> >>>> +{
> >>>> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> >>>> +	gpa_t base = dev->itt_addr;
> >>>> +	int ret, ite_esz = abi->ite_esz;
> >>>> +	size_t max_size = BIT_ULL(dev->nb_eventid_bits) * ite_esz;
> >>>
> >>> nit: initializations on separate line
> >> OK
> >>>
> >>>> +
> >>>> +	ret =  lookup_table(its, base, max_size, ite_esz, 0,
> >>>> +			    vgic_its_restore_ite, dev);
> >>>
> >>> nit: extra white space
> >>>
> >>>> +
> >>>> +	if (ret < 0)
> >>>> +		return ret;
> >>>> +
> >>>> +	/* if the last element has not been found we are in trouble */
> >>>> +	return ret ? 0 : -EINVAL;
> >>>
> >>> hmm, these are values potentially created by the guest in guest RAM,
> >>> right?  So do we really abort migration and return an error to userspace
> >>> in this case?
> >> So we discussed with Peter/dave we shouldn't abort() in qemu in case of
> >> such error. The restore table IOCTL will return an error. Up to qemu to
> >> print the error. Destination guest will not be functional though.
> >>
> > 
> > ok, I'm just wondering if userspace can make a qualified decision based
> > on this error code.  EINVAL typically means that userspace provided
> > something incorrect, which I suppose in a sense is true, but this should
> > be the only case where we return EINVAL here.
>   Userspace must be able to
> > tell the cases apart where the guest programmed bogus into memory before
> > migration started, in which case we should ignore-and-resume, and where
> > QEMU errornously provide some bogus value where the machine state
> > becomes unreliable and must be powered down.
> guest does not feed much besides few registers the ITS table restore
> depends on. In case we want a more subtle error management at userspace
> level all the error codes need to be revisited I am afraid. My plan was
> to be more rough at the beginning and ignore & resume if ITS table
> restore fails.
> 

Do we require that the VM is quiesced the entire time between saving the
ITS state to memory and copying all memory over the wire and capturing
all register state?  If so, then an error to restore would be because of
userspace doing something wrong and handling that accordingly is fine.

However, if there is any situation where the guest can by accident
write some incorrect value into RAM where the ITS data structures happen
to be, and the VM is migrated afterwards with the potential result of
just killing the VM, then that's unacceptable, because it's a gross
deviation from how the hardware works, and the migration should be
transparent to the VM.

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH v5 21/22] KVM: arm64: vgic-its: Fix pending table sync
From: Christoffer Dall @ 2017-05-04  7:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <03432fc8-5e39-2f50-1e36-1c4b2f9be773@redhat.com>

On Thu, May 04, 2017 at 12:20:13AM +0200, Auger Eric wrote:
> Hi,
> 
> On 30/04/2017 23:10, Christoffer Dall wrote:
> > On Fri, Apr 14, 2017 at 12:15:33PM +0200, Eric Auger wrote:
> >> In its_sync_lpi_pending_table() we currently ignore the
> >> target_vcpu of the LPIs. We sync the pending bit found in
> >> the vcpu pending table even if the LPI is not targeting it.
> >>
> >> Also in vgic_its_cmd_handle_invall() we are supposed to
> >> read the config table data for the LPIs associated to the
> >> collection ID. At the moment we refresh all LPI config
> >> information.
> >>
> >> This patch passes a vpcu to vgic_copy_lpi_list() so that
> >> this latter returns a snapshot of the LPIs targeting this
> >> CPU and only those.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> ---
> >>  virt/kvm/arm/vgic/vgic-its.c | 24 ++++++++++++------------
> >>  1 file changed, 12 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >> index 86dfc6c..be848be 100644
> >> --- a/virt/kvm/arm/vgic/vgic-its.c
> >> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >> @@ -252,13 +252,13 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
> >>  }
> >>  
> >>  /*
> >> - * Create a snapshot of the current LPI list, so that we can enumerate all
> >> - * LPIs without holding any lock.
> >> - * Returns the array length and puts the kmalloc'ed array into intid_ptr.
> >> + * Create a snapshot of the current LPIs targeting @vcpu, so that we can
> >> + * enumerate those LPIs without holding any lock.
> >> + * Returns their number and puts the kmalloc'ed array into intid_ptr.
> >>   */
> >> -static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr)
> >> +static int vgic_copy_lpi_list(struct kvm_vcpu *vcpu, u32 **intid_ptr)
> >>  {
> >> -	struct vgic_dist *dist = &kvm->arch.vgic;
> >> +	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> >>  	struct vgic_irq *irq;
> >>  	u32 *intids;
> >>  	int irq_count = dist->lpi_list_count, i = 0;
> >> @@ -277,14 +277,14 @@ static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr)
> >>  	spin_lock(&dist->lpi_list_lock);
> >>  	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
> >>  		/* We don't need to "get" the IRQ, as we hold the list lock. */
> >> -		intids[i] = irq->intid;
> >> -		if (++i == irq_count)
> >> -			break;
> >> +		if (irq->target_vcpu != vcpu)
> >> +			continue;
> >> +		intids[i++] = irq->intid;
> > 
> > were we checking the ++i == irq_count condition for no good reason
> > before since we can just drop it now?
> I didn't get why we had that check.
> 

ok, Andre is cc'ed so unless he complains let's just get rid of it.

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH v4 0/8] Add support for DCMI camera interface of STMicroelectronics STM32 SoC series
From: Alexandre Torgue @ 2017-05-04  7:35 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1492704445-22186-1-git-send-email-hugues.fruchet@st.com>

Hi Hugues,

On 04/20/2017 06:07 PM, Hugues Fruchet wrote:
> This patchset introduces a basic support for Digital Camera Memory Interface
> (DCMI) of STMicroelectronics STM32 SoC series.
>
> This first basic support implements RGB565 & YUV frame grabbing.
> Cropping and JPEG support will be added later on.
>
> This has been tested on STM324x9I-EVAL evaluation board embedding
> an OV2640 camera sensor.
>
> This driver depends on:
>   - [PATCHv6 00/14] atmel-isi/ov7670/ov2640: convert to standalone drivers http://www.spinics.net/lists/linux-media/msg113480.html

For stm32 machine part (DT+config):
Acked-by: Alexandre TORGUE <alexandre.torgue@st.com>

I will add it in future pull request.

Regards
Alex

>
> ===========
> = history =
> ===========
> version 4:
>   - "v4l2-compliance -s -f" report
>   - fix behaviour in case of start_streaming failure (DMA memory shortage for ex.)
>   - dt-bindings: Fix remarks from Rob Herring:
>     http://www.mail-archive.com/linux-media at vger.kernel.org/msg111340.html
>     Add "Acked-by: Rob Herring <robh@kernel.org>"
>
> version 3:
>   - stm32-dcmi: Add "Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>"
>   - dt-bindings: Fix remarks from Rob Herring:
>     http://www.mail-archive.com/linux-media at vger.kernel.org/msg110956.html
>
> version 2:
>   - Fix a Kbuild warning in probe:
>     http://www.mail-archive.com/linux-media at vger.kernel.org/msg110678.html
>   - Fix a warning in dcmi_queue_setup()
>   - dt-bindings: warn on sensor signals level inversion in board example
>   - Typos fixing
>
> version 1:
>   - Initial submission
>
> ===================
> = v4l2-compliance =
> ===================
> Below is the v4l2-compliance report for this current version of the DCMI camera interface.
> v4l2-compliance has been built from v4l-utils-1.12.3.
>
> ~ # v4l2-compliance -s -f -d /dev/video0
> v4l2-compliance SHA   : f5f45e17ee98a0ebad7836ade2b34ceec909d751
>
> Driver Info:
>         Driver name   : stm32-dcmi
>         Card type     : STM32 Digital Camera Memory Int
>         Bus info      : platform:dcmi
>         Driver version: 4.11.0
>         Capabilities  : 0x85200001
>                 Video Capture
>                 Read/Write
>                 Streaming
>                 Extended Pix Format
>                 Device Capabilities
>         Device Caps   : 0x05200001
>                 Video Capture
>                 Read/Write
>                 Streaming
>                 Extended Pix Format
>
> Compliance test for device /dev/video0 (not using libv4l2):
>
> Required ioctls:
>         test VIDIOC_QUERYCAP: OK
>
> Allow for multiple opens:
>         test second video open: OK
>         test VIDIOC_QUERYCAP: OK
>         test VIDIOC_G/S_PRIORITY: OK
>         test for unlimited opens: OK
>
> Debug ioctls:
>         test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
>         test VIDIOC_LOG_STATUS: OK
>
> Input ioctls:
>         test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
>         test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>         test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
>         test VIDIOC_ENUMAUDIO: OK (Not Supported)
>         test VIDIOC_G/S/ENUMINPUT: OK
>         test VIDIOC_G/S_AUDIO: OK (Not Supported)
>         Inputs: 1 Audio Inputs: 0 Tuners: 0
>
> Output ioctls:
>         test VIDIOC_G/S_MODULATOR: OK (Not Supported)
>         test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>         test VIDIOC_ENUMAUDOUT: OK (Not Supported)
>         test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
>         test VIDIOC_G/S_AUDOUT: OK (Not Supported)
>         Outputs: 0 Audio Outputs: 0 Modulators: 0
>
> Input/Output configuration ioctls:
>         test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>         test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
>         test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
>         test VIDIOC_G/S_EDID: OK (Not Supported)
>
> Test input 0:
>
>         Control ioctls:
>                 test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
>                 test VIDIOC_QUERYCTRL: OK
>                 test VIDIOC_G/S_CTRL: OK
>                 test VIDIOC_G/S/TRY_EXT_CTRLS: OK
>                 test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
>                 test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
>                 Standard Controls: 3 Private Controls: 0
>
>         Format ioctls:
>                 test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>                 test VIDIOC_G/S_PARM: OK (Not Supported)
>                 test VIDIOC_G_FBUF: OK (Not Supported)
>                 test VIDIOC_G_FMT: OK
>                 test VIDIOC_TRY_FMT: OK
>                 test VIDIOC_S_FMT: OK
>                 test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
>                 test Cropping: OK (Not Supported)
>                 test Composing: OK (Not Supported)
>                 test Scaling: OK
>
>         Codec ioctls:
>                 test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
>                 test VIDIOC_G_ENC_INDEX: OK (Not Supported)
>                 test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>
>         Buffer ioctls:
>                 test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>                 test VIDIOC_EXPBUF: OK
>
> Test input 0:
>
> Streaming ioctls:
>         test read/write: OK
>         test MMAP: OK
>         test USERPTR: OK (Not Supported)
>         test DMABUF: Cannot test, specify --expbuf-device
>
> Stream using all formats:
>         test MMAP for Format YUYV, Frame Size 176x144:
>                 Stride 352, Field None: OK
>         test MMAP for Format YUYV, Frame Size 320x240:
>                 Stride 640, Field None: OK
>         test MMAP for Format UYVY, Frame Size 176x144:
>                 Stride 352, Field None: OK
>         test MMAP for Format UYVY, Frame Size 320x240:
>                 Stride 640, Field None: OK
>         test MMAP for Format RGBP, Frame Size 176x144:
>                 Stride 352, Field None: OK
>         test MMAP for Format RGBP, Frame Size 320x240:
>                 Stride 640, Field None: OK
>
> Total: 52, Succeeded: 52, Failed: 0, Warnings: 0
>
> Hugues Fruchet (8):
>   dt-bindings: Document STM32 DCMI bindings
>   [media] stm32-dcmi: STM32 DCMI camera interface driver
>   ARM: dts: stm32: Enable DCMI support on STM32F429 MCU
>   ARM: dts: stm32: Enable DCMI camera interface on STM32F429-EVAL board
>   ARM: dts: stm32: Enable STMPE1600 gpio expander of STM32F429-EVAL
>     board
>   ARM: dts: stm32: Enable OV2640 camera support of STM32F429-EVAL board
>   ARM: configs: stm32: STMPE1600 GPIO expander
>   ARM: configs: stm32: DCMI + OV2640 camera support
>
>  .../devicetree/bindings/media/st,stm32-dcmi.txt    |   46 +
>  arch/arm/boot/dts/stm32429i-eval.dts               |   56 +
>  arch/arm/boot/dts/stm32f429.dtsi                   |   37 +
>  arch/arm/configs/stm32_defconfig                   |    9 +
>  drivers/media/platform/Kconfig                     |   12 +
>  drivers/media/platform/Makefile                    |    2 +
>  drivers/media/platform/stm32/Makefile              |    1 +
>  drivers/media/platform/stm32/stm32-dcmi.c          | 1419 ++++++++++++++++++++
>  8 files changed, 1582 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/st,stm32-dcmi.txt
>  create mode 100644 drivers/media/platform/stm32/Makefile
>  create mode 100644 drivers/media/platform/stm32/stm32-dcmi.c
>

^ permalink raw reply

* [RFC PATCH] mtd: spi-nor: handle signal case as failure
From: Ludovic BARRE @ 2017-05-04  7:40 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <b3de06f6-9fcd-4b58-60f2-14c55d83898d@wedev4u.fr>

On 05/03/2017 11:23 PM, Cyrille Pitchen wrote:
> Hi all,
>
> Le 03/05/2017 ? 11:53, Nicholas Mc Guire a ?crit :
>>   The problem is that stm32_qspi_wait_cmd() will indicate success in case of
>>   being interrupted. The if condition is incomplete here as
>>   wait_for_copletion_interruptible_timeout can return -ERESTARTSYS but this
>>   is not handled by if(!wait_for_completion_interruptible_timeout()).
>>   While somewhat unlikely it probably would be hard to figure out what went
>>   wrong if the signal case does occur.
>>
>> Fixes: commit 0d43d7ab277a ("mtd: spi-nor: add driver for STM32 quad spi flash controller")
>> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
>> ---
>> Problem found by experimental coccinelle script
>>
>> Its not clear if its sufficient to simply treat this case as failure or
>> if it might need some debug output to allow differentiation of signal
>> and timeout case.
>>
>> Patch was compile tested with: stm32_defconfig +CONFIG_MTD=y, CONFIG_MTD_SPI_NOR=y,
>> CONFIG_SPI_STM32_QUADSPI=m
>>
>> Patch is against v4.11-rc8 (localversion-next is next-20170503)
>>
>>   drivers/mtd/spi-nor/stm32-quadspi.c  | 10 ++++++++--
>>   1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c b/drivers/mtd/spi-nor/stm32-quadspi.c
>> index 1056e74..27147ad 100644
>> --- a/drivers/mtd/spi-nor/stm32-quadspi.c
>> +++ b/drivers/mtd/spi-nor/stm32-quadspi.c
>> @@ -159,6 +159,7 @@ static int stm32_qspi_wait_cmd(struct stm32_qspi *qspi)
>>   {
>>   	u32 cr;
>>   	int err = 0;
>> +	long timeout = 0;
>>   
>>   	if (readl_relaxed(qspi->io_base + QUADSPI_SR) & SR_TCF)
>>   		return 0;
>> @@ -167,8 +168,13 @@ static int stm32_qspi_wait_cmd(struct stm32_qspi *qspi)
>>   	cr = readl_relaxed(qspi->io_base + QUADSPI_CR);
>>   	writel_relaxed(cr | CR_TCIE, qspi->io_base + QUADSPI_CR);
>>   
>> -	if (!wait_for_completion_interruptible_timeout(&qspi->cmd_completion,
>> -						       msecs_to_jiffies(1000)))
> Ludovic, can we just use wait_for_completion_timeout() instead?
exactly, I would use _interruptible extension to allow "user" to
abort a long transfer (without wait the end of command transfert).
But it's not crucial for stm32_qspi, this could be replace by
wait_for_completion_timeout.
sorry to disturb the "mtd pull" :-(

Best regards,
Ludo
> I think so but you surely know better than me :)
> Do you plan to receive signal? maybe to abort the current SPI flash command?
>
> Best regards,
>
> Cyrille
>
>> +	timeout = wait_for_completion_interruptible_timeout(
>> +				&qspi->cmd_completion, msecs_to_jiffies(1000));
>> +
>> +	/* since the calling side only cares about success of failure
>> +	 * returning -ETIMEDOUT even when interrupted should be ok here
>> +	 */
>> +	if (timeout == 0 || timeout == -ERESTARTSYS)
>>   		err = -ETIMEDOUT;
>>   
>>   	writel_relaxed(cr, qspi->io_base + QUADSPI_CR);
>>

^ permalink raw reply

* [PATCH v5 19/22] KVM: arm64: vgic-its: ITT save and restore
From: Auger Eric @ 2017-05-04  7:40 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170504073110.GA5923@cbox>

Hi Christoffer,

On 04/05/2017 09:31, Christoffer Dall wrote:
> On Wed, May 03, 2017 at 11:55:34PM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 03/05/2017 18:37, Christoffer Dall wrote:
>>> On Wed, May 03, 2017 at 06:08:58PM +0200, Auger Eric wrote:
>>>> Hi Christoffer,
>>>>
>>>> On 30/04/2017 22:14, Christoffer Dall wrote:
>>>>> On Fri, Apr 14, 2017 at 12:15:31PM +0200, Eric Auger wrote:
>>>>>> Introduce routines to save and restore device ITT and their
>>>>>> interrupt table entries (ITE).
>>>>>>
>>>>>> The routines will be called on device table save and
>>>>>> restore. They will become static in subsequent patches.
>>>>>
>>>>> Why this bottom-up approach?  Couldn't you start by having the patch
>>>>> that restores the device table and define the static functions that
>>>>> return an error there
>>>> done
>>>> , and then fill them in with subsequent patches
>>>>> (liek this one)?
>>>>>
>>>>> That would have the added benefit of being able to tell how things are
>>>>> designed to be called.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>>
>>>>>> ---
>>>>>> v4 -> v5:
>>>>>> - ITE are now sorted by eventid on the flush
>>>>>> - rename *flush* into *save*
>>>>>> - use macros for shits and masks
>>>>>> - pass ite_esz to vgic_its_save_ite
>>>>>>
>>>>>> v3 -> v4:
>>>>>> - lookup_table and compute_next_eventid_offset become static in this
>>>>>>   patch
>>>>>> - remove static along with vgic_its_flush/restore_itt to avoid
>>>>>>   compilation warnings
>>>>>> - next field only computed with a shift (mask removed)
>>>>>> - handle the case where the last element has not been found
>>>>>>
>>>>>> v2 -> v3:
>>>>>> - add return 0 in vgic_its_restore_ite (was in subsequent patch)
>>>>>>
>>>>>> v2: creation
>>>>>> ---
>>>>>>  virt/kvm/arm/vgic/vgic-its.c | 128 ++++++++++++++++++++++++++++++++++++++++++-
>>>>>>  virt/kvm/arm/vgic/vgic.h     |   4 ++
>>>>>>  2 files changed, 129 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>>>>> index 35b2ca1..b02fc3f 100644
>>>>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>>>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>>  #include <linux/interrupt.h>
>>>>>>  #include <linux/list.h>
>>>>>>  #include <linux/uaccess.h>
>>>>>> +#include <linux/list_sort.h>
>>>>>>  
>>>>>>  #include <linux/irqchip/arm-gic-v3.h>
>>>>>>  
>>>>>> @@ -1695,7 +1696,7 @@ u32 compute_next_devid_offset(struct list_head *h, struct its_device *dev)
>>>>>>  	return min_t(u32, next_offset, VITS_DTE_MAX_DEVID_OFFSET);
>>>>>>  }
>>>>>>  
>>>>>> -u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>>>>>> +static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>>>>>>  {
>>>>>>  	struct list_head *e = &ite->ite_list;
>>>>>>  	struct its_ite *next;
>>>>>> @@ -1737,8 +1738,8 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>>>>>>   *
>>>>>>   * Return: < 0 on error, 1 if last element identified, 0 otherwise
>>>>>>   */
>>>>>> -int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>>>>> -		 int start_id, entry_fn_t fn, void *opaque)
>>>>>> +static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>>>>> +			int start_id, entry_fn_t fn, void *opaque)
>>>>>>  {
>>>>>>  	void *entry = kzalloc(esz, GFP_KERNEL);
>>>>>>  	struct kvm *kvm = its->dev->kvm;
>>>>>> @@ -1773,6 +1774,127 @@ int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>>>>>  }
>>>>>>  
>>>>>>  /**
>>>>>> + * vgic_its_save_ite - Save an interrupt translation entry at @gpa
>>>>>> + */
>>>>>> +static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
>>>>>> +			      struct its_ite *ite, gpa_t gpa, int ite_esz)
>>>>>> +{
>>>>>> +	struct kvm *kvm = its->dev->kvm;
>>>>>> +	u32 next_offset;
>>>>>> +	u64 val;
>>>>>> +
>>>>>> +	next_offset = compute_next_eventid_offset(&dev->itt_head, ite);
>>>>>> +	val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
>>>>>> +	       ((u64)ite->lpi << KVM_ITS_ITE_PINTID_SHIFT) |
>>>>>> +		ite->collection->collection_id;
>>>>>> +	val = cpu_to_le64(val);
>>>>>> +	return kvm_write_guest(kvm, gpa, &val, ite_esz);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * vgic_its_restore_ite - restore an interrupt translation entry
>>>>>> + * @event_id: id used for indexing
>>>>>> + * @ptr: pointer to the ITE entry
>>>>>> + * @opaque: pointer to the its_device
>>>>>> + * @next: id offset to the next entry
>>>>>> + */
>>>>>> +static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>>>>>> +				void *ptr, void *opaque, u32 *next)
>>>>>> +{
>>>>>> +	struct its_device *dev = (struct its_device *)opaque;
>>>>>> +	struct its_collection *collection;
>>>>>> +	struct kvm *kvm = its->dev->kvm;
>>>>>> +	u64 val, *p = (u64 *)ptr;
>>>>>
>>>>> nit: initializations on separate line (and possible do that just above
>>>>> assigning val).
>>>> done
>>>>>
>>>>>> +	struct vgic_irq *irq;
>>>>>> +	u32 coll_id, lpi_id;
>>>>>> +	struct its_ite *ite;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	val = *p;
>>>>>> +	*next = 1;
>>>>>> +
>>>>>> +	val = le64_to_cpu(val);
>>>>>> +
>>>>>> +	coll_id = val & KVM_ITS_ITE_ICID_MASK;
>>>>>> +	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
>>>>>> +
>>>>>> +	if (!lpi_id)
>>>>>> +		return 0;
>>>>>
>>>>> are all non-zero LPI IDs valid?  Don't we have a wrapper that tests if
>>>>> the ID is valid?
>>>> no, lpi_id must be >= GIC_MIN_LPI=8192; added that check.
>>>> ABI Doc says lpi_id==0 is interpreted as invalid. Other values <
>>>> GIC_MIN_LPI cause an -EINVAL error
>>>>>
>>>>> (looks like it's possible to add LPIs with the INTID range of SPIs, SGIs
>>>>> and PPIs here)
>>>>
>>>>>
>>>>>> +
>>>>>> +	*next = val >> KVM_ITS_ITE_NEXT_SHIFT;
>>>>>
>>>>> Don't we need to validate this somehow since it will presumably be used
>>>>> to forward a pointer somehow by the caller?
>>>> checked against max number of eventids supported by the device
>>>>>
>>>>>> +
>>>>>> +	collection = find_collection(its, coll_id);
>>>>>> +	if (!collection)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	ret = vgic_its_alloc_ite(dev, &ite, collection,
>>>>>> +				  lpi_id, event_id);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	irq = vgic_add_lpi(kvm, lpi_id);
>>>>>> +	if (IS_ERR(irq))
>>>>>> +		return PTR_ERR(irq);
>>>>>> +	ite->irq = irq;
>>>>>> +
>>>>>> +	/* restore the configuration of the LPI */
>>>>>> +	ret = update_lpi_config(kvm, irq, NULL);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	update_affinity_ite(kvm, ite);
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgic_its_ite_cmp(void *priv, struct list_head *a,
>>>>>> +			    struct list_head *b)
>>>>>> +{
>>>>>> +	struct its_ite *itea = container_of(a, struct its_ite, ite_list);
>>>>>> +	struct its_ite *iteb = container_of(b, struct its_ite, ite_list);
>>>>>> +
>>>>>> +	if (itea->event_id < iteb->event_id)
>>>>>> +		return -1;
>>>>>> +	else
>>>>>> +		return 1;
>>>>>> +}
>>>>>> +
>>>>>> +int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
>>>>>> +{
>>>>>> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>>>>>> +	gpa_t base = device->itt_addr;
>>>>>> +	struct its_ite *ite;
>>>>>> +	int ret, ite_esz = abi->ite_esz;
>>>>>
>>>>> nit: initializations on separate line
>>>> OK
>>>>>
>>>>>> +
>>>>>> +	list_sort(NULL, &device->itt_head, vgic_its_ite_cmp);
>>>>>> +
>>>>>> +	list_for_each_entry(ite, &device->itt_head, ite_list) {
>>>>>> +		gpa_t gpa = base + ite->event_id * ite_esz;
>>>>>> +
>>>>>> +		ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz);
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
>>>>>> +{
>>>>>> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>>>>>> +	gpa_t base = dev->itt_addr;
>>>>>> +	int ret, ite_esz = abi->ite_esz;
>>>>>> +	size_t max_size = BIT_ULL(dev->nb_eventid_bits) * ite_esz;
>>>>>
>>>>> nit: initializations on separate line
>>>> OK
>>>>>
>>>>>> +
>>>>>> +	ret =  lookup_table(its, base, max_size, ite_esz, 0,
>>>>>> +			    vgic_its_restore_ite, dev);
>>>>>
>>>>> nit: extra white space
>>>>>
>>>>>> +
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	/* if the last element has not been found we are in trouble */
>>>>>> +	return ret ? 0 : -EINVAL;
>>>>>
>>>>> hmm, these are values potentially created by the guest in guest RAM,
>>>>> right?  So do we really abort migration and return an error to userspace
>>>>> in this case?
>>>> So we discussed with Peter/dave we shouldn't abort() in qemu in case of
>>>> such error. The restore table IOCTL will return an error. Up to qemu to
>>>> print the error. Destination guest will not be functional though.
>>>>
>>>
>>> ok, I'm just wondering if userspace can make a qualified decision based
>>> on this error code.  EINVAL typically means that userspace provided
>>> something incorrect, which I suppose in a sense is true, but this should
>>> be the only case where we return EINVAL here.
>>   Userspace must be able to
>>> tell the cases apart where the guest programmed bogus into memory before
>>> migration started, in which case we should ignore-and-resume, and where
>>> QEMU errornously provide some bogus value where the machine state
>>> becomes unreliable and must be powered down.
>> guest does not feed much besides few registers the ITS table restore
>> depends on. In case we want a more subtle error management at userspace
>> level all the error codes need to be revisited I am afraid. My plan was
>> to be more rough at the beginning and ignore & resume if ITS table
>> restore fails.
>>
> 
> Do we require that the VM is quiesced the entire time between saving the
> ITS state to memory and copying all memory over the wire and capturing
> all register state?  If so, then an error to restore would be because of
> userspace doing something wrong and handling that accordingly is fine.

yes the ITS table save into RAM starts when we have a guarantee that all
the VCPUS are stopped (we take all locks). The restore happens before
the VM gets resumed. At least this is the QEMU integration as of today.

Thanks

Eric
> 
> However, if there is any situation where the guest can by accident
> write some incorrect value into RAM where the ITS data structures happen
> to be, and the VM is migrated afterwards with the potential result of
> just killing the VM, then that's unacceptable, because it's a gross
> deviation from how the hardware works, and the migration should be
> transparent to the VM.
> 
> Thanks,
> -Christoffer
> 

^ permalink raw reply

* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Marc Zyngier @ 2017-05-04  7:40 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <3c16c95a-6bbc-a25c-7dc2-b4fd13685327@redhat.com>

On 04/05/17 08:00, Auger Eric wrote:
> Hi Christoffer,
> 
> 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).
>>
>> 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).  I suggest you do the pending check by adding a
>> function called something like vgic_v3_lpi_is_pending() which scans the
>> bit in memory, clears the memory bit, and returns the value.  Clearing
>> the pending bit in memory when moving it to the struct irq is nice,
>> because you then don't have to clear out the entire pending table later
>> and we don't keep 'consumed' data lying around.  This change should be
>> implemented in its_sync_lpi_pending_table() as well, but note that you
>> need never call that function in the normal restore path using this
>> design.
>>
>> I hope this makes sense.
> 
> I am dubious about the above changes at the moment.
> its_sync_lpi_pending_table() gets called on GITS_CTLR setting which is
> documented to be the last step of the restoration. I wonder why the
> above changes cannot be part of another series later on.

I think that's one of the issues. See below.

> Consuming the RAM bit status means we record it in irq->pending_latch so
> I guess we should have the irq->pending_latch setting in the same
> function as the one that retrieves the bit status in guest RAM. So I
> would rename vgic_v3_lpi_is_pending into something like
> int vgic_v3_sync_lpi_pending_status(struct kvm *kvm, u32 intid)
> Since this covers a single LPI, the removes the byte access optimization
> found in its_sync_lpi_pending_table

Well, never mind the optimization. How many LPIs are we restoring in a
typical VM? 10? 1000? That's just one byte access per LPI. Of course,
I'd rather have fewer guest memory accesses, but a restore is an
incredibly rare event, so I'm not too bothered about the extra usec! ;-)

> 
> Also if I understand it correctly this means the sync will be done on
> both add_lpi and GITS_CTLR setting

Why GITS_CTLR? The Enable bit only controls the effect of
GITS_TRANSLATER... I believe that vgic_add_lpi() is the only point where
we should snapshot the pending state.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply

* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Christoffer Dall @ 2017-05-04  7:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <3c16c95a-6bbc-a25c-7dc2-b4fd13685327@redhat.com>

On Thu, May 04, 2017 at 09:00:29AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> 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).
> > 
> > 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).  I suggest you do the pending check by adding a
> > function called something like vgic_v3_lpi_is_pending() which scans the
> > bit in memory, clears the memory bit, and returns the value.  Clearing
> > the pending bit in memory when moving it to the struct irq is nice,
> > because you then don't have to clear out the entire pending table later
> > and we don't keep 'consumed' data lying around.  This change should be
> > implemented in its_sync_lpi_pending_table() as well, but note that you
> > need never call that function in the normal restore path using this
> > design.
> > 
> > I hope this makes sense.
> 
> I am dubious about the above changes at the moment.
> its_sync_lpi_pending_table() gets called on GITS_CTLR setting which is
> documented to be the last step of the restoration. I wonder why the
> above changes cannot be part of another series later on.

I suppose they could be.  I just really dislike introducing wording in
the ABI which implies that state that belongs to the redistributor gets
restored when you restore a certain ITS register; this gives the wrong
impression of how the hardware works and it is going to bite us when we
introduce LPI support without the ITS.

Implementation wise, I don't care strongly if you wish to scan the table
multiple times or just lookup the pending bit when you have to.  I find
it much cleaner to emulate what the hardware would be doing which is the
ITS asking the redistributor for the pending state, and the
redistributor is in charge of maintaining consistency between memory an
cached state.

I don't think it's weird to introduce a dependency between the
redistributor and the ITS in the restore sequence, because the ITS is
inherently built to be able to interface with the redistributor, and I
don't see any way around it, given you need to know the address of the
pending table anyway.

> 
> Consuming the RAM bit status means we record it in irq->pending_latch so
> I guess we should have the irq->pending_latch setting in the same
> function as the one that retrieves the bit status in guest RAM.

Yes.

> So I
> would rename vgic_v3_lpi_is_pending into something like
> int vgic_v3_sync_lpi_pending_status(struct kvm *kvm, u32 intid)

no objection

> Since this covers a single LPI, the removes the byte access optimization
> found in its_sync_lpi_pending_table

sure

> 
> Also if I understand it correctly this means the sync will be done on
> both add_lpi and GITS_CTLR setting
> 
No, it would only be done when needing the information for a specific
LPI; there should be no real need to scan the entire table until we have
support for LPIs without the ITS in which case the table must be scanned
when restoring an enabled redistributor.

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Auger Eric @ 2017-05-04  7:54 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <73c4e7c5-14f0-8d47-cd30-7fd072a8f365@arm.com>

Hi Marc,
On 04/05/2017 09:40, Marc Zyngier wrote:
> On 04/05/17 08:00, Auger Eric wrote:
>> Hi Christoffer,
>>
>> 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).
>>>
>>> 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).  I suggest you do the pending check by adding a
>>> function called something like vgic_v3_lpi_is_pending() which scans the
>>> bit in memory, clears the memory bit, and returns the value.  Clearing
>>> the pending bit in memory when moving it to the struct irq is nice,
>>> because you then don't have to clear out the entire pending table later
>>> and we don't keep 'consumed' data lying around.  This change should be
>>> implemented in its_sync_lpi_pending_table() as well, but note that you
>>> need never call that function in the normal restore path using this
>>> design.
>>>
>>> I hope this makes sense.
>>
>> I am dubious about the above changes at the moment.
>> its_sync_lpi_pending_table() gets called on GITS_CTLR setting which is
>> documented to be the last step of the restoration. I wonder why the
>> above changes cannot be part of another series later on.
> 
> I think that's one of the issues. See below.
> 
>> Consuming the RAM bit status means we record it in irq->pending_latch so
>> I guess we should have the irq->pending_latch setting in the same
>> function as the one that retrieves the bit status in guest RAM. So I
>> would rename vgic_v3_lpi_is_pending into something like
>> int vgic_v3_sync_lpi_pending_status(struct kvm *kvm, u32 intid)
>> Since this covers a single LPI, the removes the byte access optimization
>> found in its_sync_lpi_pending_table
> 
> Well, never mind the optimization. How many LPIs are we restoring in a
> typical VM? 10? 1000? That's just one byte access per LPI. Of course,
> I'd rather have fewer guest memory accesses, but a restore is an
> incredibly rare event, so I'm not too bothered about the extra usec! ;-)
> 
>>
>> Also if I understand it correctly this means the sync will be done on
>> both add_lpi and GITS_CTLR setting
> 
> Why GITS_CTLR? The Enable bit only controls the effect of
> GITS_TRANSLATER...

Hum sorry I mixed up. the sync is currently done on GIC*R*_CTLR
vgic_mmio_write_v3r_ctlr/vgic_enable_lpis/its_sync_lpi_pending_table

As the redistributors are restored *before* the ITS this sync is void as
no LPI exist at that time.

That's why I did the sync (again) on ITS table restore. Sorry for the
noise.

OK let's go with the sync in vgic_add_lpi() ...

Thanks

Eric

 I believe that vgic_add_lpi() is the only point where
> we should snapshot the pending state.
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply

* [linux-sunxi] [PATCH v2 18/20] drm/sun4i: Add HDMI support
From: Chen-Yu Tsai @ 2017-05-04  7:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <c8db196b5084b317c3a43a0caea1363951e6e5bf.1493812478.git-series.maxime.ripard@free-electrons.com>

On Wed, May 3, 2017 at 7:59 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The earlier Allwinner SoCs (A10, A10s, A20, A31) have an embedded HDMI
> controller.
>
> That HDMI controller is able to do audio and CEC, but those have been left
> out for now.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/sun4i/Kconfig               |   9 +-
>  drivers/gpu/drm/sun4i/Makefile              |   6 +-
>  drivers/gpu/drm/sun4i/sun4i_hdmi.h          | 157 +++++++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c  | 127 +++++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c      | 493 +++++++++++++++++++++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 225 ++++++++++-
>  6 files changed, 1017 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi.h
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
>  create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
>
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index a4b357db8856..35299c4e2594 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -12,3 +12,12 @@ config DRM_SUN4I
>           Choose this option if you have an Allwinner SoC with a
>           Display Engine. If M is selected the module will be called
>           sun4i-drm.
> +
> +if DRM_SUN4I
> +config DRM_SUN4I_HDMI
> +       tristate "Allwinner A10 HDMI Controller Support"
> +       help
> +         Choose this option if you have an Allwinner SoC with an HDMI
> +         controller.
> +
> +endif
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 59b757350a1f..c09bf8093710 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -11,3 +11,9 @@ obj-$(CONFIG_DRM_SUN4I)               += sun4i-drm.o sun4i-tcon.o
>  obj-$(CONFIG_DRM_SUN4I)                += sun4i_backend.o
>  obj-$(CONFIG_DRM_SUN4I)                += sun6i_drc.o
>  obj-$(CONFIG_DRM_SUN4I)                += sun4i_tv.o
> +
> +sun4i-drm-hdmi-y += sun4i_hdmi_enc.o
> +sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o
> +sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o
> +
> +obj-$(CONFIG_DRM_SUN4I_HDMI)   += sun4i-drm-hdmi.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> new file mode 100644
> index 000000000000..40d57b195b48
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h
> @@ -0,0 +1,157 @@
> +/*
> + * Copyright (C) 2016 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#ifndef _SUN4I_HDMI_H_
> +#define _SUN4I_HDMI_H_
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_encoder.h>
> +
> +#define SUN4I_HDMI_CTRL_REG            0x004
> +#define SUN4I_HDMI_CTRL_ENABLE                 BIT(31)
> +
> +#define SUN4I_HDMI_IRQ_REG             0x008
> +#define SUN4I_HDMI_IRQ_STA_MASK                        0x73
> +#define SUN4I_HDMI_IRQ_STA_FIFO_OF             BIT(1)
> +#define SUN4I_HDMI_IRQ_STA_FIFO_UF             BIT(0)
> +
> +#define SUN4I_HDMI_HPD_REG             0x00c
> +#define SUN4I_HDMI_HPD_HIGH                    BIT(0)
> +
> +#define SUN4I_HDMI_VID_CTRL_REG                0x010
> +#define SUN4I_HDMI_VID_CTRL_ENABLE             BIT(31)
> +#define SUN4I_HDMI_VID_CTRL_HDMI_MODE          BIT(30)
> +
> +#define SUN4I_HDMI_VID_TIMING_ACT_REG  0x014
> +#define SUN4I_HDMI_VID_TIMING_BP_REG   0x018
> +#define SUN4I_HDMI_VID_TIMING_FP_REG   0x01c
> +#define SUN4I_HDMI_VID_TIMING_SPW_REG  0x020
> +
> +#define SUN4I_HDMI_VID_TIMING_X(x)             ((((x) - 1) & GENMASK(11, 0)))
> +#define SUN4I_HDMI_VID_TIMING_Y(y)             ((((y) - 1) & GENMASK(11, 0)) << 16)
> +
> +#define SUN4I_HDMI_VID_TIMING_POL_REG  0x024
> +#define SUN4I_HDMI_VID_TIMING_POL_TX_CLK        (0x3e0 << 16)
> +#define SUN4I_HDMI_VID_TIMING_POL_VSYNC                BIT(1)
> +#define SUN4I_HDMI_VID_TIMING_POL_HSYNC                BIT(0)
> +
> +#define SUN4I_HDMI_AVI_INFOFRAME_REG(n)        (0x080 + (n))
> +
> +#define SUN4I_HDMI_PAD_CTRL0_REG       0x200
> +#define SUN4I_HDMI_PAD_CTRL0_BIASEN            BIT(31)
> +#define SUN4I_HDMI_PAD_CTRL0_LDOCEN            BIT(30)
> +#define SUN4I_HDMI_PAD_CTRL0_LDODEN            BIT(29)
> +#define SUN4I_HDMI_PAD_CTRL0_PWENC             BIT(28)
> +#define SUN4I_HDMI_PAD_CTRL0_PWEND             BIT(27)
> +#define SUN4I_HDMI_PAD_CTRL0_PWENG             BIT(26)
> +#define SUN4I_HDMI_PAD_CTRL0_CKEN              BIT(25)
> +#define SUN4I_HDMI_PAD_CTRL0_TXEN              BIT(23)
> +
> +#define SUN4I_HDMI_PAD_CTRL1_REG       0x204
> +#define SUN4I_HDMI_PAD_CTRL1_AMP_OPT           BIT(23)
> +#define SUN4I_HDMI_PAD_CTRL1_AMPCK_OPT         BIT(22)
> +#define SUN4I_HDMI_PAD_CTRL1_EMP_OPT           BIT(20)
> +#define SUN4I_HDMI_PAD_CTRL1_EMPCK_OPT         BIT(19)
> +#define SUN4I_HDMI_PAD_CTRL1_REG_DEN           BIT(15)
> +#define SUN4I_HDMI_PAD_CTRL1_REG_DENCK         BIT(14)
> +#define SUN4I_HDMI_PAD_CTRL1_REG_EMP(n)                (((n) & 7) << 10)
> +#define SUN4I_HDMI_PAD_CTRL1_HALVE_CLK         BIT(6)
> +#define SUN4I_HDMI_PAD_CTRL1_REG_AMP(n)                (((n) & 7) << 3)
> +
> +#define SUN4I_HDMI_PLL_CTRL_REG                0x208
> +#define SUN4I_HDMI_PLL_CTRL_PLL_EN             BIT(31)
> +#define SUN4I_HDMI_PLL_CTRL_BWS                        BIT(30)
> +#define SUN4I_HDMI_PLL_CTRL_HV_IS_33           BIT(29)
> +#define SUN4I_HDMI_PLL_CTRL_LDO1_EN            BIT(28)
> +#define SUN4I_HDMI_PLL_CTRL_LDO2_EN            BIT(27)
> +#define SUN4I_HDMI_PLL_CTRL_SDIV2              BIT(25)
> +#define SUN4I_HDMI_PLL_CTRL_VCO_GAIN(n)                (((n) & 7) << 20)
> +#define SUN4I_HDMI_PLL_CTRL_S(n)               (((n) & 7) << 16)
> +#define SUN4I_HDMI_PLL_CTRL_CP_S(n)            (((n) & 0xf) << 12)
> +#define SUN4I_HDMI_PLL_CTRL_CS(n)              (((n) & 0xf) << 8)
> +#define SUN4I_HDMI_PLL_CTRL_DIV(n)             (((n) & 0xf) << 4)
> +#define SUN4I_HDMI_PLL_CTRL_DIV_MASK           GENMASK(7, 4)
> +#define SUN4I_HDMI_PLL_CTRL_VCO_S(n)           ((n) & 0xf)
> +
> +#define SUN4I_HDMI_PLL_DBG0_REG                0x20c
> +#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT(n)     (((n) & 1) << 21)
> +#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK   BIT(21)
> +#define SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_SHIFT  21
> +
> +#define SUN4I_HDMI_PKT_CTRL_REG(n)     (0x2f0 + (4 * (n)))
> +#define SUN4I_HDMI_PKT_CTRL_TYPE(n, t)         ((t) << (((n) % 4) * 4))
> +
> +#define SUN4I_HDMI_UNKNOWN_REG         0x300
> +#define SUN4I_HDMI_UNKNOWN_INPUT_SYNC          BIT(27)
> +
> +#define SUN4I_HDMI_DDC_CTRL_REG                0x500
> +#define SUN4I_HDMI_DDC_CTRL_ENABLE             BIT(31)
> +#define SUN4I_HDMI_DDC_CTRL_START_CMD          BIT(30)
> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK      BIT(8)
> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ      (0 << 8)
> +#define SUN4I_HDMI_DDC_CTRL_RESET              BIT(0)
> +
> +#define SUN4I_HDMI_DDC_ADDR_REG                0x504
> +#define SUN4I_HDMI_DDC_ADDR_SEGMENT(seg)       (((seg) & 0xff) << 24)
> +#define SUN4I_HDMI_DDC_ADDR_EDDC(addr)         (((addr) & 0xff) << 16)
> +#define SUN4I_HDMI_DDC_ADDR_OFFSET(off)                (((off) & 0xff) << 8)
> +#define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)                ((addr) & 0xff)
> +
> +#define SUN4I_HDMI_DDC_FIFO_CTRL_REG   0x510
> +#define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR         BIT(31)
> +
> +#define SUN4I_HDMI_DDC_FIFO_DATA_REG   0x518
> +#define SUN4I_HDMI_DDC_BYTE_COUNT_REG  0x51c
> +
> +#define SUN4I_HDMI_DDC_CMD_REG         0x520
> +#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ  6
> +
> +#define SUN4I_HDMI_DDC_CLK_REG         0x528
> +#define SUN4I_HDMI_DDC_CLK_M(m)                        (((m) & 0x7) << 3)
> +#define SUN4I_HDMI_DDC_CLK_N(n)                        ((n) & 0x7)
> +
> +#define SUN4I_HDMI_DDC_LINE_CTRL_REG   0x540
> +#define SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE    BIT(9)
> +#define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE    BIT(8)
> +
> +#define SUN4I_HDMI_DDC_FIFO_SIZE       16
> +
> +enum sun4i_hdmi_pkt_type {
> +       SUN4I_HDMI_PKT_AVI = 2,
> +       SUN4I_HDMI_PKT_END = 15,
> +};
> +
> +struct sun4i_hdmi {
> +       struct drm_connector    connector;
> +       struct drm_encoder      encoder;
> +       struct device           *dev;
> +
> +       void __iomem            *base;
> +
> +       /* Parent clocks */
> +       struct clk              *bus_clk;
> +       struct clk              *mod_clk;
> +       struct clk              *pll0_clk;
> +       struct clk              *pll1_clk;
> +
> +       /* And the clocks we create */
> +       struct clk              *ddc_clk;
> +       struct clk              *tmds_clk;
> +
> +       struct sun4i_drv        *drv;
> +
> +       bool                    hdmi_monitor;
> +};
> +
> +int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk);
> +int sun4i_tmds_create(struct sun4i_hdmi *hdmi);
> +
> +#endif /* _SUN4I_HDMI_H_ */
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
> new file mode 100644
> index 000000000000..4692e8c345ed
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c
> @@ -0,0 +1,127 @@
> +/*
> + * Copyright (C) 2016 Free Electrons
> + * Copyright (C) 2016 NextThing Co
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/clk-provider.h>
> +
> +#include "sun4i_tcon.h"
> +#include "sun4i_hdmi.h"
> +
> +struct sun4i_ddc {
> +       struct clk_hw           hw;
> +       struct sun4i_hdmi       *hdmi;
> +};
> +
> +static inline struct sun4i_ddc *hw_to_ddc(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct sun4i_ddc, hw);
> +}
> +
> +static unsigned long sun4i_ddc_calc_divider(unsigned long rate,
> +                                           unsigned long parent_rate,
> +                                           u8 *m, u8 *n)
> +{
> +       unsigned long best_rate = 0;
> +       u8 best_m = 0, best_n = 0, _m, _n;
> +
> +       for (_m = 0; _m < 8; _m++) {
> +               for (_n = 0; _n < 8; _n++) {
> +                       unsigned long tmp_rate;
> +
> +                       tmp_rate = (((parent_rate / 2) / 10) >> _n) / (_m + 1);
> +
> +                       if (tmp_rate > rate)
> +                               continue;
> +
> +                       if (abs(rate - tmp_rate) < abs(rate - best_rate)) {
> +                               best_rate = tmp_rate;
> +                               best_m = _m;
> +                               best_n = _n;
> +                       }
> +               }
> +       }
> +
> +       if (m && n) {
> +               *m = best_m;
> +               *n = best_n;
> +       }
> +
> +       return best_rate;
> +}
> +
> +static long sun4i_ddc_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                unsigned long *prate)
> +{
> +       return sun4i_ddc_calc_divider(rate, *prate, NULL, NULL);
> +}
> +
> +static unsigned long sun4i_ddc_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       struct sun4i_ddc *ddc = hw_to_ddc(hw);
> +       u32 reg;
> +       u8 m, n;
> +
> +       reg = readl(ddc->hdmi->base + SUN4I_HDMI_DDC_CLK_REG);
> +       m = (reg >> 3) & 0x7;
> +       n = reg & 0x7;
> +
> +       return (((parent_rate / 2) / 10) >> n) / (m + 1);
> +}
> +
> +static int sun4i_ddc_set_rate(struct clk_hw *hw, unsigned long rate,
> +                             unsigned long parent_rate)
> +{
> +       struct sun4i_ddc *ddc = hw_to_ddc(hw);
> +       u8 div_m, div_n;
> +
> +       sun4i_ddc_calc_divider(rate, parent_rate, &div_m, &div_n);
> +
> +       writel(SUN4I_HDMI_DDC_CLK_M(div_m) | SUN4I_HDMI_DDC_CLK_N(div_n),
> +              ddc->hdmi->base + SUN4I_HDMI_DDC_CLK_REG);
> +
> +       return 0;
> +}
> +
> +static const struct clk_ops sun4i_ddc_ops = {
> +       .recalc_rate    = sun4i_ddc_recalc_rate,
> +       .round_rate     = sun4i_ddc_round_rate,
> +       .set_rate       = sun4i_ddc_set_rate,
> +};
> +
> +int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *parent)
> +{
> +       struct clk_init_data init;
> +       struct sun4i_ddc *ddc;
> +       const char *parent_name;
> +
> +       parent_name = __clk_get_name(parent);
> +       if (!parent_name)
> +               return -ENODEV;
> +
> +       ddc = devm_kzalloc(hdmi->dev, sizeof(*ddc), GFP_KERNEL);
> +       if (!ddc)
> +               return -ENOMEM;
> +
> +       init.name = "hdmi-ddc";
> +       init.ops = &sun4i_ddc_ops;
> +       init.parent_names = &parent_name;
> +       init.num_parents = 1;
> +
> +       ddc->hdmi = hdmi;
> +       ddc->hw.init = &init;
> +
> +       hdmi->ddc_clk = devm_clk_register(hdmi->dev, &ddc->hw);
> +       if (IS_ERR(hdmi->ddc_clk))
> +               return PTR_ERR(hdmi->ddc_clk);
> +
> +       return 0;
> +}
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> new file mode 100644
> index 000000000000..9bec23164f53
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c
> @@ -0,0 +1,493 @@
> +/*
> + * Copyright (C) 2016 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/iopoll.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "sun4i_backend.h"
> +#include "sun4i_drv.h"
> +#include "sun4i_hdmi.h"
> +#include "sun4i_tcon.h"
> +
> +#define DDC_SEGMENT_ADDR       0x30
> +
> +static inline struct sun4i_hdmi *
> +drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder)
> +{
> +       return container_of(encoder, struct sun4i_hdmi,
> +                           encoder);
> +}
> +
> +static inline struct sun4i_hdmi *
> +drm_connector_to_sun4i_hdmi(struct drm_connector *connector)
> +{
> +       return container_of(connector, struct sun4i_hdmi,
> +                           connector);
> +}
> +
> +static int sun4i_hdmi_setup_avi_infoframes(struct sun4i_hdmi *hdmi,
> +                                          struct drm_display_mode *mode)
> +{
> +       struct hdmi_avi_infoframe frame;
> +       u8 buffer[17];
> +       int i, ret;
> +
> +       ret = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode);
> +       if (ret < 0) {
> +               DRM_ERROR("Failed to get infoframes from mode\n");
> +               return ret;
> +       }
> +
> +       ret = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer));
> +       if (ret < 0) {
> +               DRM_ERROR("Failed to pack infoframes\n");
> +               return ret;
> +       }
> +
> +       for (i = 0; i < sizeof(buffer); i++)
> +               writeb(buffer[i], hdmi->base + SUN4I_HDMI_AVI_INFOFRAME_REG(i));
> +
> +       return 0;
> +}
> +
> +static int sun4i_hdmi_atomic_check(struct drm_encoder *encoder,
> +                                  struct drm_crtc_state *crtc_state,
> +                                  struct drm_connector_state *conn_state)
> +{
> +       struct drm_display_mode *mode = &crtc_state->mode;
> +
> +       if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> +               return -EINVAL;
> +
> +       return 0;
> +}
> +
> +static void sun4i_hdmi_disable(struct drm_encoder *encoder)
> +{
> +       struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
> +       struct sun4i_drv *drv = hdmi->drv;
> +       struct sun4i_tcon *tcon = drv->tcon;

I assume you didn't take into consideration the latest patches I sent
that remove this field.

Instead you'll want to do drm_crtc_to_sun4i_crtc(encoder->crtc)->tcon.

> +       u32 val;
> +
> +       DRM_DEBUG_DRIVER("Disabling the HDMI Output\n");
> +
> +       val = readl(hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +       val &= ~SUN4I_HDMI_VID_CTRL_ENABLE;
> +       writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +
> +       sun4i_tcon_channel_disable(tcon, 1);
> +}
> +
> +static void sun4i_hdmi_enable(struct drm_encoder *encoder)
> +{
> +       struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
> +       struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
> +       struct sun4i_drv *drv = hdmi->drv;
> +       struct sun4i_tcon *tcon = drv->tcon;
> +       u32 val = 0;
> +
> +       DRM_DEBUG_DRIVER("Enabling the HDMI Output\n");
> +
> +       sun4i_tcon_channel_enable(tcon, 1);
> +
> +       sun4i_hdmi_setup_avi_infoframes(hdmi, mode);
> +       val |= SUN4I_HDMI_PKT_CTRL_TYPE(0, SUN4I_HDMI_PKT_AVI);
> +       val |= SUN4I_HDMI_PKT_CTRL_TYPE(1, SUN4I_HDMI_PKT_END);
> +       writel(val, hdmi->base + SUN4I_HDMI_PKT_CTRL_REG(0));
> +
> +       val = SUN4I_HDMI_VID_CTRL_ENABLE;
> +       if (hdmi->hdmi_monitor)
> +               val |= SUN4I_HDMI_VID_CTRL_HDMI_MODE;
> +
> +       writel(val, hdmi->base + SUN4I_HDMI_VID_CTRL_REG);
> +}
> +
> +static void sun4i_hdmi_mode_set(struct drm_encoder *encoder,
> +                               struct drm_display_mode *mode,
> +                               struct drm_display_mode *adjusted_mode)
> +{
> +       struct sun4i_hdmi *hdmi = drm_encoder_to_sun4i_hdmi(encoder);
> +       struct sun4i_drv *drv = hdmi->drv;
> +       struct sun4i_tcon *tcon = drv->tcon;
> +       unsigned int x, y;
> +       u32 val;
> +
> +       sun4i_tcon1_mode_set(tcon, mode);
> +       sun4i_tcon_set_mux(tcon, 1, encoder);
> +
> +       clk_set_rate(tcon->sclk1, mode->crtc_clock * 1000);
> +       clk_set_rate(hdmi->mod_clk, mode->crtc_clock * 1000);
> +       clk_set_rate(hdmi->tmds_clk, mode->crtc_clock * 1000);
> +
> +       /* Set input sync enable */
> +       writel(SUN4I_HDMI_UNKNOWN_INPUT_SYNC,
> +              hdmi->base + SUN4I_HDMI_UNKNOWN_REG);
> +
> +       /* Setup timing registers */
> +       writel(SUN4I_HDMI_VID_TIMING_X(mode->hdisplay) |
> +              SUN4I_HDMI_VID_TIMING_Y(mode->vdisplay),

I can't figure out whether we should use mode->crtc_* or not...

> +              hdmi->base + SUN4I_HDMI_VID_TIMING_ACT_REG);
> +
> +       x = mode->htotal - mode->hsync_start;
> +       y = mode->vtotal - mode->vsync_start;
> +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> +              hdmi->base + SUN4I_HDMI_VID_TIMING_BP_REG);
> +
> +       x = mode->hsync_start - mode->hdisplay;
> +       y = mode->vsync_start - mode->vdisplay;
> +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> +              hdmi->base + SUN4I_HDMI_VID_TIMING_FP_REG);
> +
> +       x = mode->hsync_end - mode->hsync_start;
> +       y = mode->vsync_end - mode->vsync_start;
> +       writel(SUN4I_HDMI_VID_TIMING_X(x) | SUN4I_HDMI_VID_TIMING_Y(y),
> +              hdmi->base + SUN4I_HDMI_VID_TIMING_SPW_REG);
> +
> +       val = SUN4I_HDMI_VID_TIMING_POL_TX_CLK;
> +       if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> +               val |= SUN4I_HDMI_VID_TIMING_POL_HSYNC;
> +
> +       if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> +               val |= SUN4I_HDMI_VID_TIMING_POL_VSYNC;
> +
> +       writel(val, hdmi->base + SUN4I_HDMI_VID_TIMING_POL_REG);
> +}
> +
> +static const struct drm_encoder_helper_funcs sun4i_hdmi_helper_funcs = {
> +       .atomic_check   = sun4i_hdmi_atomic_check,
> +       .disable        = sun4i_hdmi_disable,
> +       .enable         = sun4i_hdmi_enable,
> +       .mode_set       = sun4i_hdmi_mode_set,
> +};
> +
> +static const struct drm_encoder_funcs sun4i_hdmi_funcs = {
> +       .destroy        = drm_encoder_cleanup,
> +};
> +
> +static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi,
> +                                    unsigned int blk, unsigned int offset,
> +                                    u8 *buf, unsigned int count)
> +{
> +       unsigned long reg;
> +       int i;
> +
> +       reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> +       reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK;
> +       writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ,
> +              hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> +
> +       writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) |
> +              SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) |
> +              SUN4I_HDMI_DDC_ADDR_OFFSET(offset) |
> +              SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR),
> +              hdmi->base + SUN4I_HDMI_DDC_ADDR_REG);
> +
> +       reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
> +       writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR,
> +              hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG);
> +
> +       writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG);
> +       writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ,
> +              hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
> +
> +       reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> +       writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
> +              hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> +
> +       if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
> +                              !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
> +                              100, 100000))
> +               return -EIO;
> +
> +       for (i = 0; i < count; i++)
> +               buf[i] = readb(hdmi->base + SUN4I_HDMI_DDC_FIFO_DATA_REG);
> +
> +       return 0;
> +}
> +
> +static int sun4i_hdmi_read_edid_block(void *data, u8 *buf, unsigned int blk,
> +                                     size_t length)
> +{
> +       struct sun4i_hdmi *hdmi = data;
> +       int retry = 2, i;
> +
> +       do {
> +               for (i = 0; i < length; i += SUN4I_HDMI_DDC_FIFO_SIZE) {
> +                       unsigned char offset = blk * EDID_LENGTH + i;
> +                       unsigned int count = min((unsigned int)SUN4I_HDMI_DDC_FIFO_SIZE,
> +                                                length - i);
> +                       int ret;
> +
> +                       ret = sun4i_hdmi_read_sub_block(hdmi, blk, offset,
> +                                                       buf + i, count);
> +                       if (ret)
> +                               return ret;
> +               }
> +       } while (!drm_edid_block_valid(buf, blk, true, NULL) && (retry--));
> +
> +       return 0;
> +}
> +
> +static int sun4i_hdmi_get_modes(struct drm_connector *connector)
> +{
> +       struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
> +       unsigned long reg;
> +       struct edid *edid;
> +       int ret;
> +
> +       /* Reset i2c controller */
> +       writel(SUN4I_HDMI_DDC_CTRL_ENABLE | SUN4I_HDMI_DDC_CTRL_RESET,
> +              hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> +       if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg,
> +                              !(reg & SUN4I_HDMI_DDC_CTRL_RESET),
> +                              100, 2000))
> +               return -EIO;
> +
> +       writel(SUN4I_HDMI_DDC_LINE_CTRL_SDA_ENABLE |
> +              SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLE,
> +              hdmi->base + SUN4I_HDMI_DDC_LINE_CTRL_REG);
> +
> +       clk_set_rate(hdmi->ddc_clk, 100000);

You might want to explicitly enable the DDC clk around the following call.

> +
> +       edid = drm_do_get_edid(connector, sun4i_hdmi_read_edid_block, hdmi);
> +       if (!edid)
> +               return 0;
> +
> +       hdmi->hdmi_monitor = drm_detect_hdmi_monitor(edid);
> +       DRM_DEBUG_DRIVER("Monitor is %s monitor\n",
> +                        hdmi->hdmi_monitor ? "an HDMI" : "a DVI");
> +
> +       drm_mode_connector_update_edid_property(connector, edid);
> +       ret = drm_add_edid_modes(connector, edid);
> +       kfree(edid);
> +
> +       return ret;
> +}
> +
> +static const struct drm_connector_helper_funcs sun4i_hdmi_connector_helper_funcs = {
> +       .get_modes      = sun4i_hdmi_get_modes,
> +};
> +
> +static enum drm_connector_status
> +sun4i_hdmi_connector_detect(struct drm_connector *connector, bool force)
> +{
> +       struct sun4i_hdmi *hdmi = drm_connector_to_sun4i_hdmi(connector);
> +       unsigned long reg;
> +
> +       if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_HPD_REG, reg,
> +                              reg & SUN4I_HDMI_HPD_HIGH,
> +                              0, 500000))
> +               return connector_status_disconnected;
> +
> +       return connector_status_connected;
> +}
> +
> +static const struct drm_connector_funcs sun4i_hdmi_connector_funcs = {
> +       .dpms                   = drm_atomic_helper_connector_dpms,
> +       .detect                 = sun4i_hdmi_connector_detect,
> +       .fill_modes             = drm_helper_probe_single_connector_modes,
> +       .destroy                = drm_connector_cleanup,
> +       .reset                  = drm_atomic_helper_connector_reset,
> +       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +       .atomic_destroy_state   = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static int sun4i_hdmi_bind(struct device *dev, struct device *master,
> +                          void *data)
> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct drm_device *drm = data;
> +       struct sun4i_drv *drv = drm->dev_private;
> +       struct sun4i_hdmi *hdmi;
> +       struct resource *res;
> +       u32 reg;
> +       int ret;
> +
> +       hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> +       if (!hdmi)
> +               return -ENOMEM;
> +       dev_set_drvdata(dev, hdmi);
> +       hdmi->dev = dev;
> +       hdmi->drv = drv;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       hdmi->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(hdmi->base)) {
> +               dev_err(dev, "Couldn't map the HDMI encoder registers\n");
> +               return PTR_ERR(hdmi->base);
> +       }
> +
> +       hdmi->bus_clk = devm_clk_get(dev, "ahb");
> +       if (IS_ERR(hdmi->bus_clk)) {
> +               dev_err(dev, "Couldn't get the HDMI bus clock\n");
> +               return PTR_ERR(hdmi->bus_clk);
> +       }
> +       clk_prepare_enable(hdmi->bus_clk);
> +
> +       hdmi->mod_clk = devm_clk_get(dev, "mod");
> +       if (IS_ERR(hdmi->mod_clk)) {
> +               dev_err(dev, "Couldn't get the HDMI mod clock\n");
> +               return PTR_ERR(hdmi->mod_clk);
> +       }
> +       clk_prepare_enable(hdmi->mod_clk);
> +
> +       hdmi->pll0_clk = devm_clk_get(dev, "pll-0");
> +       if (IS_ERR(hdmi->pll0_clk)) {
> +               dev_err(dev, "Couldn't get the HDMI PLL 0 clock\n");
> +               return PTR_ERR(hdmi->pll0_clk);
> +       }
> +
> +       hdmi->pll1_clk = devm_clk_get(dev, "pll-1");
> +       if (IS_ERR(hdmi->pll1_clk)) {
> +               dev_err(dev, "Couldn't get the HDMI PLL 1 clock\n");
> +               return PTR_ERR(hdmi->pll1_clk);
> +       }
> +
> +       ret = sun4i_tmds_create(hdmi);
> +       if (ret) {
> +               dev_err(dev, "Couldn't create the TMDS clock\n");
> +               return ret;
> +       }
> +
> +       writel(SUN4I_HDMI_CTRL_ENABLE, hdmi->base + SUN4I_HDMI_CTRL_REG);
> +
> +       writel(SUN4I_HDMI_PAD_CTRL0_TXEN | SUN4I_HDMI_PAD_CTRL0_CKEN |
> +              SUN4I_HDMI_PAD_CTRL0_PWENG | SUN4I_HDMI_PAD_CTRL0_PWEND |
> +              SUN4I_HDMI_PAD_CTRL0_PWENC | SUN4I_HDMI_PAD_CTRL0_LDODEN |
> +              SUN4I_HDMI_PAD_CTRL0_LDOCEN | SUN4I_HDMI_PAD_CTRL0_BIASEN,
> +              hdmi->base + SUN4I_HDMI_PAD_CTRL0_REG);
> +
> +       /*
> +        * We can't just initialize the register there, we need to
> +        * protect the clock bits that have already been read out and
> +        * cached by the clock framework.
> +        */
> +       reg = readl(hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +       reg = reg & SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;

Nit: just use the &= short hand.

> +       reg |= SUN4I_HDMI_PAD_CTRL1_REG_AMP(6) |
> +               SUN4I_HDMI_PAD_CTRL1_REG_EMP(2) |
> +               SUN4I_HDMI_PAD_CTRL1_REG_DENCK |
> +               SUN4I_HDMI_PAD_CTRL1_REG_DEN |
> +               SUN4I_HDMI_PAD_CTRL1_EMPCK_OPT |
> +               SUN4I_HDMI_PAD_CTRL1_EMP_OPT |
> +               SUN4I_HDMI_PAD_CTRL1_AMPCK_OPT |
> +               SUN4I_HDMI_PAD_CTRL1_AMP_OPT;
> +       writel(reg, hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +
> +       reg = readl(hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
> +       reg = reg & SUN4I_HDMI_PLL_CTRL_DIV_MASK;

Same here.

> +       reg |= SUN4I_HDMI_PLL_CTRL_VCO_S(8) | SUN4I_HDMI_PLL_CTRL_CS(7) |
> +               SUN4I_HDMI_PLL_CTRL_CP_S(239) | SUN4I_HDMI_PLL_CTRL_S(7) |
> +               SUN4I_HDMI_PLL_CTRL_VCO_GAIN(4) | SUN4I_HDMI_PLL_CTRL_SDIV2 |
> +               SUN4I_HDMI_PLL_CTRL_LDO2_EN | SUN4I_HDMI_PLL_CTRL_LDO1_EN |
> +               SUN4I_HDMI_PLL_CTRL_HV_IS_33 | SUN4I_HDMI_PLL_CTRL_BWS |
> +               SUN4I_HDMI_PLL_CTRL_PLL_EN;
> +       writel(reg, hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
> +
> +       ret = sun4i_ddc_create(hdmi, hdmi->tmds_clk);
> +       if (ret) {
> +               dev_err(dev, "Couldn't create the DDC clock\n");
> +               return ret;
> +       }
> +
> +       drm_encoder_helper_add(&hdmi->encoder,
> +                              &sun4i_hdmi_helper_funcs);
> +       ret = drm_encoder_init(drm,
> +                              &hdmi->encoder,
> +                              &sun4i_hdmi_funcs,
> +                              DRM_MODE_ENCODER_TMDS,
> +                              NULL);
> +       if (ret) {
> +               dev_err(dev, "Couldn't initialise the HDMI encoder\n");
> +               return ret;
> +       }
> +
> +       hdmi->encoder.possible_crtcs = drm_of_find_possible_crtcs(drm,
> +                                                                 dev->of_node);
> +       if (!hdmi->encoder.possible_crtcs)
> +               return ret;

You need a valid error return value here. Maybe -EPROBE_DEFER?

> +
> +       drm_connector_helper_add(&hdmi->connector,
> +                                &sun4i_hdmi_connector_helper_funcs);

/* There is no HPD interrupt, so we need to poll the controller */
hdmi->connector.polled = DRM_CONNECTOR_POLL_CONNECT |
                         DRM_CONNECTOR_POLL_DISCONNECT;

> +       ret = drm_connector_init(drm, &hdmi->connector,
> +                                &sun4i_hdmi_connector_funcs,
> +                                DRM_MODE_CONNECTOR_HDMIA);
> +       if (ret) {
> +               dev_err(dev,
> +                       "Couldn't initialise the HDMI connector\n");
> +               goto err_cleanup_connector;
> +       }
> +
> +       drm_mode_connector_attach_encoder(&hdmi->connector, &hdmi->encoder);
> +
> +       return 0;
> +
> +err_cleanup_connector:
> +       drm_encoder_cleanup(&hdmi->encoder);
> +       return ret;
> +}
> +
> +static void sun4i_hdmi_unbind(struct device *dev, struct device *master,
> +                           void *data)
> +{
> +       struct sun4i_hdmi *hdmi = dev_get_drvdata(dev);
> +
> +       drm_connector_cleanup(&hdmi->connector);
> +       drm_encoder_cleanup(&hdmi->encoder);
> +}
> +
> +static const struct component_ops sun4i_hdmi_ops = {
> +       .bind   = sun4i_hdmi_bind,
> +       .unbind = sun4i_hdmi_unbind,
> +};
> +
> +static int sun4i_hdmi_probe(struct platform_device *pdev)
> +{
> +       return component_add(&pdev->dev, &sun4i_hdmi_ops);
> +}
> +
> +static int sun4i_hdmi_remove(struct platform_device *pdev)
> +{
> +       component_del(&pdev->dev, &sun4i_hdmi_ops);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id sun4i_hdmi_of_table[] = {
> +       { .compatible = "allwinner,sun5i-a10s-hdmi" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_hdmi_of_table);
> +
> +static struct platform_driver sun4i_hdmi_driver = {
> +       .probe          = sun4i_hdmi_probe,
> +       .remove         = sun4i_hdmi_remove,
> +       .driver         = {
> +               .name           = "sun4i-hdmi",
> +               .of_match_table = sun4i_hdmi_of_table,
> +       },
> +};
> +module_platform_driver(sun4i_hdmi_driver);
> +
> +MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
> +MODULE_DESCRIPTION("Allwinner A10 HDMI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
> new file mode 100644
> index 000000000000..5cf2527bffc8
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
> @@ -0,0 +1,225 @@
> +/*
> + * Copyright (C) 2016 Free Electrons
> + * Copyright (C) 2016 NextThing Co
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + */
> +
> +#include <linux/clk-provider.h>
> +
> +#include "sun4i_tcon.h"
> +#include "sun4i_hdmi.h"
> +
> +struct sun4i_tmds {
> +       struct clk_hw           hw;
> +       struct sun4i_hdmi       *hdmi;
> +};
> +
> +static inline struct sun4i_tmds *hw_to_tmds(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct sun4i_tmds, hw);
> +}
> +
> +
> +static unsigned long sun4i_tmds_calc_divider(unsigned long rate,
> +                                            unsigned long parent_rate,
> +                                            u8 *div,
> +                                            bool *half)
> +{
> +       unsigned long best_rate = 0;
> +       u8 best_m = 0, m;
> +       bool is_double;
> +
> +       for (m = 1; m < 16; m++) {
> +               u8 d;
> +
> +               for (d = 1; d < 3; d++) {
> +                       unsigned long tmp_rate;
> +
> +                       tmp_rate = parent_rate / m / d;
> +
> +                       if (tmp_rate > rate)
> +                               continue;
> +
> +                       if (!best_rate ||
> +                           (rate - tmp_rate) < (rate - best_rate)) {
> +                               best_rate = tmp_rate;
> +                               best_m = m;
> +                               is_double = d;
> +                       }
> +               }
> +       }
> +
> +       if (div && half) {
> +               *div = best_m;
> +               *half = is_double;
> +       }
> +
> +       return best_rate;
> +}
> +
> +
> +static int sun4i_tmds_determine_rate(struct clk_hw *hw,
> +                                    struct clk_rate_request *req)
> +{
> +       struct clk_hw *parent;
> +       unsigned long best_parent = 0;
> +       unsigned long rate = req->rate;
> +       int best_div = 1, best_half = 1;
> +       int i, j;
> +
> +       /*
> +        * We only consider PLL3, since the TCON is very likely to be
> +        * clocked from it, and to have the same rate than our HDMI
> +        * clock, so we should not need to do anything.
> +        */
> +
> +       parent = clk_hw_get_parent_by_index(hw, 0);
> +       if (!parent)
> +               return -EINVAL;
> +
> +       for (i = 1; i < 3; i++) {
> +               for (j = 1; j < 16; j++) {
> +                       unsigned long ideal = rate * i * j;
> +                       unsigned long rounded;
> +
> +                       rounded = clk_hw_round_rate(parent, ideal);
> +
> +                       if (rounded == ideal) {
> +                               best_parent = rounded;
> +                               best_half = i;
> +                               best_div = j;
> +                               goto out;
> +                       }
> +
> +                       if (abs(rate - rounded / i) <
> +                           abs(rate - best_parent / best_div)) {
> +                               best_parent = rounded;
> +                               best_div = i;
> +                       }
> +               }
> +       }
> +
> +out:
> +       req->rate = best_parent / best_half / best_div;
> +       req->best_parent_rate = best_parent;
> +       req->best_parent_hw = parent;
> +
> +       return 0;
> +}
> +
> +static unsigned long sun4i_tmds_recalc_rate(struct clk_hw *hw,
> +                                           unsigned long parent_rate)
> +{
> +       struct sun4i_tmds *tmds = hw_to_tmds(hw);
> +       u32 reg;
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +       if (reg & SUN4I_HDMI_PAD_CTRL1_HALVE_CLK)
> +               parent_rate /= 2;
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
> +       reg = (reg >> 4) & 0xf;
> +       if (!reg)
> +               reg = 1;
> +
> +       return parent_rate / reg;
> +}
> +
> +static int sun4i_tmds_set_rate(struct clk_hw *hw, unsigned long rate,
> +                              unsigned long parent_rate)
> +{
> +       struct sun4i_tmds *tmds = hw_to_tmds(hw);
> +       bool half;
> +       u32 reg;
> +       u8 div;
> +
> +       sun4i_tmds_calc_divider(rate, parent_rate, &div, &half);
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +       reg &= ~SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;
> +       if (half)
> +               reg |= SUN4I_HDMI_PAD_CTRL1_HALVE_CLK;
> +       writel(reg, tmds->hdmi->base + SUN4I_HDMI_PAD_CTRL1_REG);
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
> +       reg &= ~SUN4I_HDMI_PLL_CTRL_DIV_MASK;
> +       writel(reg | SUN4I_HDMI_PLL_CTRL_DIV(div),
> +              tmds->hdmi->base + SUN4I_HDMI_PLL_CTRL_REG);
> +
> +       return 0;
> +}
> +
> +static u8 sun4i_tmds_get_parent(struct clk_hw *hw)
> +{
> +       struct sun4i_tmds *tmds = hw_to_tmds(hw);
> +       u32 reg;
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
> +       return ((reg & SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK) >>
> +               SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_SHIFT);
> +}
> +
> +static int sun4i_tmds_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct sun4i_tmds *tmds = hw_to_tmds(hw);
> +       u32 reg;
> +
> +       if (index > 1)
> +               return -EINVAL;
> +
> +       reg = readl(tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
> +       reg &= ~SUN4I_HDMI_PLL_DBG0_TMDS_PARENT_MASK;
> +       writel(reg | SUN4I_HDMI_PLL_DBG0_TMDS_PARENT(index),
> +              tmds->hdmi->base + SUN4I_HDMI_PLL_DBG0_REG);
> +
> +       return 0;
> +}
> +
> +static const struct clk_ops sun4i_tmds_ops = {
> +       .determine_rate = sun4i_tmds_determine_rate,
> +       .recalc_rate    = sun4i_tmds_recalc_rate,
> +       .set_rate       = sun4i_tmds_set_rate,
> +
> +       .get_parent     = sun4i_tmds_get_parent,
> +       .set_parent     = sun4i_tmds_set_parent,
> +};
> +
> +int sun4i_tmds_create(struct sun4i_hdmi *hdmi)
> +{
> +       struct clk_init_data init;
> +       struct sun4i_tmds *tmds;
> +       const char *parents[2];
> +
> +       parents[0] = __clk_get_name(hdmi->pll0_clk);
> +       if (!parents[0])
> +               return -ENODEV;
> +
> +       parents[1] = __clk_get_name(hdmi->pll1_clk);
> +       if (!parents[1])
> +               return -ENODEV;
> +
> +       tmds = devm_kzalloc(hdmi->dev, sizeof(*tmds), GFP_KERNEL);
> +       if (!tmds)
> +               return -ENOMEM;
> +
> +       init.name = "hdmi-tmds";
> +       init.ops = &sun4i_tmds_ops;
> +       init.parent_names = parents;
> +       init.num_parents = 2;
> +       init.flags = CLK_SET_RATE_PARENT;
> +
> +       tmds->hdmi = hdmi;
> +       tmds->hw.init = &init;
> +
> +       hdmi->tmds_clk = devm_clk_register(hdmi->dev, &tmds->hw);
> +       if (IS_ERR(hdmi->tmds_clk))
> +               return PTR_ERR(hdmi->tmds_clk);
> +
> +       return 0;
> +}
> --

The rest looks good. This is coming together nicely.

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

^ permalink raw reply

* [PATCH 2/3] asm-generic: don't #include <linux/atomic.h> in qspinlock_types.h
From: Arnd Bergmann @ 2017-05-04  8:01 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170503145141.4966-3-ynorov@caviumnetworks.com>

On Wed, May 3, 2017 at 4:51 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> The "qspinlock_types.h" doesn't need linux/atomic.h directly. So
> because of this, and because including of it requires the protection
> against recursive inclusion, it looks reasonable to move the
> inclusion exactly where it is needed. This change affects the x86_64
> arch, as the only user of qspinlocks at now. I have build-tested the
> change on x86_64 with CONFIG_PARAVIRT enabled and disabled.
>
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Please keep this together with the other patches as they get merged through
the arm64 tree.

       Arnd

^ permalink raw reply

* [PATCH v2 19/20] ARM: sun5i: a10s: Add the HDMI controller node
From: Chen-Yu Tsai @ 2017-05-04  8:04 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1e3ddf81459adbab455b2211fe6b78bd6cf1bbec.1493812478.git-series.maxime.ripard@free-electrons.com>

On Wed, May 3, 2017 at 7:59 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The A10s has an HDMI controller connected to the second TCON channel. Add
> it to our DT.
>
> Since the TV Encoder was the only channel 1 user so far, also add the
> property now that we have several users.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/boot/dts/sun5i-a10s.dtsi | 50 ++++++++++++++++++++++++++++++++-
>  arch/arm/boot/dts/sun5i.dtsi      |  1 +-
>  2 files changed, 51 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
> index 1e38ff80366c..c9d4ee12599d 100644
> --- a/arch/arm/boot/dts/sun5i-a10s.dtsi
> +++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
> @@ -71,7 +71,49 @@
>                 };
>         };
>
> +       display-engine {
> +               compatible = "allwinner,sun5i-a10s-display-engine",
> +                            "allwinner,sun5i-a13-display-engine";

I assume this indicates a difference between A10s and A13, namely the former
has an HDMI controller. The compatible string should be documented.

> +               allwinner,pipelines = <&fe0>;
> +       };
> +
>         soc at 01c00000 {
> +               hdmi: hdmi at 01c16000 {
> +                       compatible = "allwinner,sun5i-a10s-hdmi";
> +                       reg = <0x01c16000 0x1000>;
> +                       interrupts = <58>;
> +                       clocks = <&ccu CLK_AHB_HDMI>, <&ccu CLK_HDMI>,
> +                                <&ccu CLK_PLL_VIDEO0_2X>,
> +                                <&ccu CLK_PLL_VIDEO1_2X>;
> +                       clock-names = "ahb", "mod", "pll-0", "pll-1";
> +                       dmas = <&dma SUN4I_DMA_NORMAL 16>,
> +                              <&dma SUN4I_DMA_NORMAL 16>,
> +                              <&dma SUN4I_DMA_DEDICATED 24>;
> +                       dma-names = "ddc-tx", "ddc-rx", "audio-tx";
> +                       status = "disabled";
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               hdmi_in: port at 0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <0>;
> +
> +                                       hdmi_in_tcon0: endpoint {

Missing reg property here, since you have #address-cells = 1.

Otherwise,

Acked-by: Chen-Yu Tsai <wens@csie.org>

> +                                               remote-endpoint = <&tcon0_out_hdmi>;
> +                                       };
> +                               };
> +
> +                               hdmi_out: port at 1 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
> +                                       reg = <1>;
> +                               };
> +                       };
> +               };
> +
>                 pwm: pwm at 01c20e00 {
>                         compatible = "allwinner,sun5i-a10s-pwm";
>                         reg = <0x01c20e00 0xc>;
> @@ -128,3 +170,11 @@
>
>  &sram_a {
>  };
> +
> +&tcon0_out {
> +       tcon0_out_hdmi: endpoint at 2 {
> +               reg = <2>;
> +               remote-endpoint = <&hdmi_in_tcon0>;
> +               allwinner,tcon-channel = <1>;
> +       };
> +};
> diff --git a/arch/arm/boot/dts/sun5i.dtsi b/arch/arm/boot/dts/sun5i.dtsi
> index 5175f9cc9bed..0e29f1d98a9e 100644
> --- a/arch/arm/boot/dts/sun5i.dtsi
> +++ b/arch/arm/boot/dts/sun5i.dtsi
> @@ -272,6 +272,7 @@
>                                         tcon0_out_tve0: endpoint at 1 {
>                                                 reg = <1>;
>                                                 remote-endpoint = <&tve0_in_tcon0>;
> +                                               allwinner,tcon-channel = <1>;
>                                         };
>                                 };
>                         };
> --
> git-series 0.8.11

^ permalink raw reply

* [PATCH] Documentation: earlycon: fix Marvell Armada 3700 UART name
From: Gregory CLEMENT @ 2017-05-04  8:04 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1493855376-4341-1-git-send-email-andre.przywara@arm.com>

Hi Andre,
 
 On jeu., mai 04 2017, Andre Przywara <andre.przywara@arm.com> wrote:

> The Marvell Armada 3700 UART uses "ar3700_uart" for its earlycon name.
> Adjust documentation to match the code.

Actually I think it was the code which was wrong. But as it is already
part of the kernel it's too late to modify it.

>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index facc20a..7ee86c2 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -973,7 +973,7 @@
>  			A valid base address must be provided, and the serial
>  			port must already be setup and configured.
>  
> -		armada3700_uart,<addr>
> +		ar3700_uart,<addr>
>  			Start an early, polled-mode console on the
>  			Armada 3700 serial port at the specified
>  			address. The serial port must already be setup
> -- 
> 2.8.2
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

^ permalink raw reply

* [linux-sunxi] [PATCH v2 20/20] ARM: sun5i: a10s-olinuxino: Enable HDMI
From: Chen-Yu Tsai @ 2017-05-04  8:05 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <c7ac6faa0568e6418291255af537c4b070bdea63.1493812478.git-series.maxime.ripard@free-electrons.com>

On Wed, May 3, 2017 at 7:59 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> The A10s Olinuxino has an HDMI connector. Make sure we can use it.
>
> Acked-by: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts | 29 +++++++++++++++++-
>  1 file changed, 29 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts b/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
> index 894f874a5beb..1d13b6407222 100644
> --- a/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
> +++ b/arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts
> @@ -63,6 +63,17 @@
>                 stdout-path = "serial0:115200n8";
>         };
>
> +       connector {
> +               compatible = "hdmi-connector";
> +               type = "a";

Just curious, should we take this into consideration when creating
drm_connector?

ChenYu

> +
> +               port {
> +                       hdmi_con_in: endpoint {
> +                               remote-endpoint = <&hdmi_out_con>;
> +                       };
> +               };
> +       };
> +
>         leds {
>                 compatible = "gpio-leds";
>                 pinctrl-names = "default";
> @@ -76,6 +87,10 @@
>         };
>  };
>
> +&be0 {
> +       status = "okay";
> +};
> +
>  &ehci0 {
>         status = "okay";
>  };
> @@ -91,6 +106,16 @@
>         status = "okay";
>  };
>
> +&hdmi {
> +       status = "okay";
> +};
> +
> +&hdmi_out {
> +       hdmi_out_con: endpoint {
> +               remote-endpoint = <&hdmi_con_in>;
> +       };
> +};
> +
>  &i2c0 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&i2c0_pins_a>;
> @@ -248,6 +273,10 @@
>         status = "okay";
>  };
>
> +&tcon0 {
> +       status = "okay";
> +};
> +
>  &uart0 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&uart0_pins_a>;
> --
> git-series 0.8.11
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply

* [PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC
From: Marc Zyngier @ 2017-05-04  8:09 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170503183300.4618-2-cdall@linaro.org>

On 03/05/17 19:32, Christoffer Dall wrote:
> Since we got support for devices in userspace which allows reporting the
> PMU overflow output status to userspace, we should actually allow
> creating the PMU on systems without an in-kernel irqchip, which in turn
> requires us to slightly clarify error codes for the ABI and move things
> around for the initialization phase.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
>  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> index 02f5068..352af6e 100644
> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
>  Returns: -EBUSY: The PMU overflow interrupt is already set
>           -ENXIO: The overflow interrupt not set when attempting to get it
>           -ENODEV: PMUv3 not supported
> -         -EINVAL: Invalid PMU overflow interrupt number supplied
> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
> +                  trying to set the IRQ number without using an in-kernel
> +                  irqchip.
>  
>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
>  
>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
>  Parameters: no additional parameter in kvm_device_attr.addr
> -Returns: -ENODEV: PMUv3 not supported
> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
> -                 attribute
> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
> +                 conigured as required prior to calling this attribute
>           -EBUSY: PMUv3 already initialized
>  
> -Request the initialization of the PMUv3.  This must be done after creating the
> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
> -supported.
> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> +virtual GIC implementation, this must be done after initializing the in-kernel
> +irqchip.
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index 4b43e7f..f046b08 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  	if (!kvm_arm_support_pmu_v3())
>  		return -ENODEV;
>  
> -	/*
> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
> -	 * because we do not support forwarding PMU overflow interrupts to
> -	 * userspace yet.
> -	 */
> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
> -		return -ENODEV;
> -
> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
> -	    !kvm_arm_pmu_irq_initialized(vcpu))
> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>  		return -ENXIO;
>  
>  	if (kvm_arm_pmu_v3_ready(vcpu))
>  		return -EBUSY;
>  
> +	if (irqchip_in_kernel(vcpu->kvm)) {
> +		/*
> +		 * If using the PMU with an in-kernel virtual GIC
> +		 * implementation, we require the GIC to be already
> +		 * initialized when initializing the PMU.
> +		 */
> +		if (!vgic_initialized(vcpu->kvm))
> +			return -ENODEV;
> +
> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
> +			return -ENXIO;
> +	}
> +
>  	kvm_pmu_vcpu_reset(vcpu);
>  	vcpu->arch.pmu.ready = true;
>  
> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		int __user *uaddr = (int __user *)(long)attr->addr;
>  		int irq;
>  
> +		if (!irqchip_in_kernel(vcpu->kvm))
> +			return -EINVAL;
> +

Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
generate a -ENXIO, and has_attr is going to lie about the availability
of KVM_ARM_VCPU_PMU_V3_IRQ...

>  		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>  			return -ENODEV;
>  
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply

* [PATCH 3/5] KVM: arm/arm64: Move irq_is_ppi() to header file
From: Marc Zyngier @ 2017-05-04  8:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170503183300.4618-4-cdall@linaro.org>

On 03/05/17 19:32, Christoffer Dall wrote:
> We are about to need this define in the arch timer code as well so move
> it to a common location.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  include/kvm/arm_vgic.h | 2 ++
>  virt/kvm/arm/pmu.c     | 2 --
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 26ed4fb..1541f5d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -38,6 +38,8 @@
>  #define VGIC_MIN_LPI		8192
>  #define KVM_IRQCHIP_NUM_PINS	(1020 - 32)
>  
> +#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
> +
>  enum vgic_type {
>  	VGIC_V2,		/* Good ol' GICv2 */
>  	VGIC_V3,		/* New fancy GICv3 */
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index f046b08..9b3e3ea 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -481,8 +481,6 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> -#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
> -
>  /*
>   * For one VM the interrupt type must be same for each vcpu.
>   * As a PPI, the interrupt number is the same for all vcpus,
> 

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply

* [PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC
From: Christoffer Dall @ 2017-05-04  8:13 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <dcaefb19-6dab-a990-2e10-497d02b14242@arm.com>

On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
> On 03/05/17 19:32, Christoffer Dall wrote:
> > Since we got support for devices in userspace which allows reporting the
> > PMU overflow output status to userspace, we should actually allow
> > creating the PMU on systems without an in-kernel irqchip, which in turn
> > requires us to slightly clarify error codes for the ABI and move things
> > around for the initialization phase.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
> >  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
> >  2 files changed, 26 insertions(+), 17 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> > index 02f5068..352af6e 100644
> > --- a/Documentation/virtual/kvm/devices/vcpu.txt
> > +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> > @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
> >  Returns: -EBUSY: The PMU overflow interrupt is already set
> >           -ENXIO: The overflow interrupt not set when attempting to get it
> >           -ENODEV: PMUv3 not supported
> > -         -EINVAL: Invalid PMU overflow interrupt number supplied
> > +         -EINVAL: Invalid PMU overflow interrupt number supplied or
> > +                  trying to set the IRQ number without using an in-kernel
> > +                  irqchip.
> >  
> >  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
> >  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> > @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
> >  
> >  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
> >  Parameters: no additional parameter in kvm_device_attr.addr
> > -Returns: -ENODEV: PMUv3 not supported
> > -         -ENXIO: PMUv3 not properly configured as required prior to calling this
> > -                 attribute
> > +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> > +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
> > +                 conigured as required prior to calling this attribute
> >           -EBUSY: PMUv3 already initialized
> >  
> > -Request the initialization of the PMUv3.  This must be done after creating the
> > -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
> > -supported.
> > +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> > +virtual GIC implementation, this must be done after initializing the in-kernel
> > +irqchip.
> > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> > index 4b43e7f..f046b08 100644
> > --- a/virt/kvm/arm/pmu.c
> > +++ b/virt/kvm/arm/pmu.c
> > @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
> >  	if (!kvm_arm_support_pmu_v3())
> >  		return -ENODEV;
> >  
> > -	/*
> > -	 * We currently require an in-kernel VGIC to use the PMU emulation,
> > -	 * because we do not support forwarding PMU overflow interrupts to
> > -	 * userspace yet.
> > -	 */
> > -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
> > -		return -ENODEV;
> > -
> > -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
> > -	    !kvm_arm_pmu_irq_initialized(vcpu))
> > +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> >  		return -ENXIO;
> >  
> >  	if (kvm_arm_pmu_v3_ready(vcpu))
> >  		return -EBUSY;
> >  
> > +	if (irqchip_in_kernel(vcpu->kvm)) {
> > +		/*
> > +		 * If using the PMU with an in-kernel virtual GIC
> > +		 * implementation, we require the GIC to be already
> > +		 * initialized when initializing the PMU.
> > +		 */
> > +		if (!vgic_initialized(vcpu->kvm))
> > +			return -ENODEV;
> > +
> > +		if (!kvm_arm_pmu_irq_initialized(vcpu))
> > +			return -ENXIO;
> > +	}
> > +
> >  	kvm_pmu_vcpu_reset(vcpu);
> >  	vcpu->arch.pmu.ready = true;
> >  
> > @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >  		int __user *uaddr = (int __user *)(long)attr->addr;
> >  		int irq;
> >  
> > +		if (!irqchip_in_kernel(vcpu->kvm))
> > +			return -EINVAL;
> > +
> 
> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
> generate a -ENXIO, and has_attr is going to lie about the availability
> of KVM_ARM_VCPU_PMU_V3_IRQ...
> 

Here's the text from api.txt:

  Tests whether a device supports a particular attribute.  A successful
  return indicates the attribute is implemented.  It does not necessarily
  indicate that the attribute can be read or written in the device's
  current state.  "addr" is ignored.

My interpretation therefore is that QEMU can use this ioctl to figure
out if the feature is supported (sort of like a capability), but that
doesn't mean that the configuration of the VM is such that the attribute
can be get or set at that moment.

For example, there will also alway be situations where you can get an
attr, but not set an attr, what should the has_attr return then?

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH] ARM: V7M: Set cacheid iff DminLine or IminLine is nonzero
From: Vladimir Murzin @ 2017-05-04  8:13 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170503191437.GB22219@n2100.armlinux.org.uk>

On 03/05/17 20:14, Russell King - ARM Linux wrote:
> On Tue, May 02, 2017 at 09:31:24AM +0100, Vladimir Murzin wrote:
>> On 26/04/17 11:04, Vladimir Murzin wrote:
>>> Cache support is optional feature in M-class cores, thus DminLine or
>>> IminLine of Cache Type Register is zero if caches are not implemented,
>>> but we check the whole CTR which has other features encoded there.
>>> Let's be more precise and check for DminLine and IminLine of CTR
>>> before we set cacheid.
>>>
>>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>>> ---
>>>  arch/arm/kernel/setup.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
>>> index f4e5450..231a1d83 100644
>>> --- a/arch/arm/kernel/setup.c
>>> +++ b/arch/arm/kernel/setup.c
>>> @@ -315,7 +315,7 @@ static void __init cacheid_init(void)
>>>  	if (arch >= CPU_ARCH_ARMv6) {
>>>  		unsigned int cachetype = read_cpuid_cachetype();
>>>  
>>> -		if ((arch == CPU_ARCH_ARMv7M) && !cachetype) {
>>> +		if ((arch == CPU_ARCH_ARMv7M) && !(cachetype & 0xf000f)) {
>>>  			cacheid = 0;
>>>  		} else if ((cachetype & (7 << 29)) == 4 << 29) {
>>>  			/* ARMv7 register format */
>>>
>>
>> Ok for patch tracker?
> 
> Not yet, I've been away and I've no time right now to evaluate this
> change.  I'm hopefully going to catch up with some email in the coming
> days.
> 

Noted.

Cheers
Vladimir

^ permalink raw reply

* [PATCH v3 1/3] arm64: kvm: support kvmtool to detect RAS extension feature
From: James Morse @ 2017-05-04  8:19 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <8d443da4-d17f-2487-10d5-b6c9639dc5a4@redhat.com>

Hi Paolo,

On 02/05/17 16:48, Paolo Bonzini wrote:
> On 02/05/2017 09:56, Christoffer Dall wrote:
>> The subject and description of these patches are also misleading.
>> Hopefully this is in no way tied to kvmtool, but to userspace
>> generically, for example also to be used by QEMU?
> 
> Yes, QEMU already has a similar capability on x86.
> 
> Does ARM support background scrubbing of memory to detect errors? 

As part of RAS support, yes. A way for firmware to notify the OS about these
events was recently added to the ACPI specification.
We are aiming to turn on ARCH_SUPPORTS_MEMORY_FAILURE which does the Linux end
of things. Punit has a series here:
https://www.spinics.net/lists/arm-kernel/msg575944.html


> If
> so, are there any plans to support action-optional SIGBUS on ARM?

It looks like ARCH_SUPPORTS_MEMORY_FAILURE will bring that in, so yes.



Thanks,

James

^ permalink raw reply

* [PATCH v5 19/22] KVM: arm64: vgic-its: ITT save and restore
From: Christoffer Dall @ 2017-05-04  8:23 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <3d00df6f-3360-e9b3-7618-e3ec528ae36a@redhat.com>

On Thu, May 04, 2017 at 09:40:35AM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 04/05/2017 09:31, Christoffer Dall wrote:
> > On Wed, May 03, 2017 at 11:55:34PM +0200, Auger Eric wrote:
> >> Hi Christoffer,
> >>
> >> On 03/05/2017 18:37, Christoffer Dall wrote:
> >>> On Wed, May 03, 2017 at 06:08:58PM +0200, Auger Eric wrote:
> >>>> Hi Christoffer,
> >>>>
> >>>> On 30/04/2017 22:14, Christoffer Dall wrote:
> >>>>> On Fri, Apr 14, 2017 at 12:15:31PM +0200, Eric Auger wrote:
> >>>>>> Introduce routines to save and restore device ITT and their
> >>>>>> interrupt table entries (ITE).
> >>>>>>
> >>>>>> The routines will be called on device table save and
> >>>>>> restore. They will become static in subsequent patches.
> >>>>>
> >>>>> Why this bottom-up approach?  Couldn't you start by having the patch
> >>>>> that restores the device table and define the static functions that
> >>>>> return an error there
> >>>> done
> >>>> , and then fill them in with subsequent patches
> >>>>> (liek this one)?
> >>>>>
> >>>>> That would have the added benefit of being able to tell how things are
> >>>>> designed to be called.
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>>>>>
> >>>>>> ---
> >>>>>> v4 -> v5:
> >>>>>> - ITE are now sorted by eventid on the flush
> >>>>>> - rename *flush* into *save*
> >>>>>> - use macros for shits and masks
> >>>>>> - pass ite_esz to vgic_its_save_ite
> >>>>>>
> >>>>>> v3 -> v4:
> >>>>>> - lookup_table and compute_next_eventid_offset become static in this
> >>>>>>   patch
> >>>>>> - remove static along with vgic_its_flush/restore_itt to avoid
> >>>>>>   compilation warnings
> >>>>>> - next field only computed with a shift (mask removed)
> >>>>>> - handle the case where the last element has not been found
> >>>>>>
> >>>>>> v2 -> v3:
> >>>>>> - add return 0 in vgic_its_restore_ite (was in subsequent patch)
> >>>>>>
> >>>>>> v2: creation
> >>>>>> ---
> >>>>>>  virt/kvm/arm/vgic/vgic-its.c | 128 ++++++++++++++++++++++++++++++++++++++++++-
> >>>>>>  virt/kvm/arm/vgic/vgic.h     |   4 ++
> >>>>>>  2 files changed, 129 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> >>>>>> index 35b2ca1..b02fc3f 100644
> >>>>>> --- a/virt/kvm/arm/vgic/vgic-its.c
> >>>>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
> >>>>>> @@ -23,6 +23,7 @@
> >>>>>>  #include <linux/interrupt.h>
> >>>>>>  #include <linux/list.h>
> >>>>>>  #include <linux/uaccess.h>
> >>>>>> +#include <linux/list_sort.h>
> >>>>>>  
> >>>>>>  #include <linux/irqchip/arm-gic-v3.h>
> >>>>>>  
> >>>>>> @@ -1695,7 +1696,7 @@ u32 compute_next_devid_offset(struct list_head *h, struct its_device *dev)
> >>>>>>  	return min_t(u32, next_offset, VITS_DTE_MAX_DEVID_OFFSET);
> >>>>>>  }
> >>>>>>  
> >>>>>> -u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
> >>>>>> +static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
> >>>>>>  {
> >>>>>>  	struct list_head *e = &ite->ite_list;
> >>>>>>  	struct its_ite *next;
> >>>>>> @@ -1737,8 +1738,8 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
> >>>>>>   *
> >>>>>>   * Return: < 0 on error, 1 if last element identified, 0 otherwise
> >>>>>>   */
> >>>>>> -int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
> >>>>>> -		 int start_id, entry_fn_t fn, void *opaque)
> >>>>>> +static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
> >>>>>> +			int start_id, entry_fn_t fn, void *opaque)
> >>>>>>  {
> >>>>>>  	void *entry = kzalloc(esz, GFP_KERNEL);
> >>>>>>  	struct kvm *kvm = its->dev->kvm;
> >>>>>> @@ -1773,6 +1774,127 @@ int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
> >>>>>>  }
> >>>>>>  
> >>>>>>  /**
> >>>>>> + * vgic_its_save_ite - Save an interrupt translation entry at @gpa
> >>>>>> + */
> >>>>>> +static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
> >>>>>> +			      struct its_ite *ite, gpa_t gpa, int ite_esz)
> >>>>>> +{
> >>>>>> +	struct kvm *kvm = its->dev->kvm;
> >>>>>> +	u32 next_offset;
> >>>>>> +	u64 val;
> >>>>>> +
> >>>>>> +	next_offset = compute_next_eventid_offset(&dev->itt_head, ite);
> >>>>>> +	val = ((u64)next_offset << KVM_ITS_ITE_NEXT_SHIFT) |
> >>>>>> +	       ((u64)ite->lpi << KVM_ITS_ITE_PINTID_SHIFT) |
> >>>>>> +		ite->collection->collection_id;
> >>>>>> +	val = cpu_to_le64(val);
> >>>>>> +	return kvm_write_guest(kvm, gpa, &val, ite_esz);
> >>>>>> +}
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * vgic_its_restore_ite - restore an interrupt translation entry
> >>>>>> + * @event_id: id used for indexing
> >>>>>> + * @ptr: pointer to the ITE entry
> >>>>>> + * @opaque: pointer to the its_device
> >>>>>> + * @next: id offset to the next entry
> >>>>>> + */
> >>>>>> +static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
> >>>>>> +				void *ptr, void *opaque, u32 *next)
> >>>>>> +{
> >>>>>> +	struct its_device *dev = (struct its_device *)opaque;
> >>>>>> +	struct its_collection *collection;
> >>>>>> +	struct kvm *kvm = its->dev->kvm;
> >>>>>> +	u64 val, *p = (u64 *)ptr;
> >>>>>
> >>>>> nit: initializations on separate line (and possible do that just above
> >>>>> assigning val).
> >>>> done
> >>>>>
> >>>>>> +	struct vgic_irq *irq;
> >>>>>> +	u32 coll_id, lpi_id;
> >>>>>> +	struct its_ite *ite;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	val = *p;
> >>>>>> +	*next = 1;
> >>>>>> +
> >>>>>> +	val = le64_to_cpu(val);
> >>>>>> +
> >>>>>> +	coll_id = val & KVM_ITS_ITE_ICID_MASK;
> >>>>>> +	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
> >>>>>> +
> >>>>>> +	if (!lpi_id)
> >>>>>> +		return 0;
> >>>>>
> >>>>> are all non-zero LPI IDs valid?  Don't we have a wrapper that tests if
> >>>>> the ID is valid?
> >>>> no, lpi_id must be >= GIC_MIN_LPI=8192; added that check.
> >>>> ABI Doc says lpi_id==0 is interpreted as invalid. Other values <
> >>>> GIC_MIN_LPI cause an -EINVAL error
> >>>>>
> >>>>> (looks like it's possible to add LPIs with the INTID range of SPIs, SGIs
> >>>>> and PPIs here)
> >>>>
> >>>>>
> >>>>>> +
> >>>>>> +	*next = val >> KVM_ITS_ITE_NEXT_SHIFT;
> >>>>>
> >>>>> Don't we need to validate this somehow since it will presumably be used
> >>>>> to forward a pointer somehow by the caller?
> >>>> checked against max number of eventids supported by the device
> >>>>>
> >>>>>> +
> >>>>>> +	collection = find_collection(its, coll_id);
> >>>>>> +	if (!collection)
> >>>>>> +		return -EINVAL;
> >>>>>> +
> >>>>>> +	ret = vgic_its_alloc_ite(dev, &ite, collection,
> >>>>>> +				  lpi_id, event_id);
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>> +	irq = vgic_add_lpi(kvm, lpi_id);
> >>>>>> +	if (IS_ERR(irq))
> >>>>>> +		return PTR_ERR(irq);
> >>>>>> +	ite->irq = irq;
> >>>>>> +
> >>>>>> +	/* restore the configuration of the LPI */
> >>>>>> +	ret = update_lpi_config(kvm, irq, NULL);
> >>>>>> +	if (ret)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>> +	update_affinity_ite(kvm, ite);
> >>>>>> +	return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int vgic_its_ite_cmp(void *priv, struct list_head *a,
> >>>>>> +			    struct list_head *b)
> >>>>>> +{
> >>>>>> +	struct its_ite *itea = container_of(a, struct its_ite, ite_list);
> >>>>>> +	struct its_ite *iteb = container_of(b, struct its_ite, ite_list);
> >>>>>> +
> >>>>>> +	if (itea->event_id < iteb->event_id)
> >>>>>> +		return -1;
> >>>>>> +	else
> >>>>>> +		return 1;
> >>>>>> +}
> >>>>>> +
> >>>>>> +int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
> >>>>>> +{
> >>>>>> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> >>>>>> +	gpa_t base = device->itt_addr;
> >>>>>> +	struct its_ite *ite;
> >>>>>> +	int ret, ite_esz = abi->ite_esz;
> >>>>>
> >>>>> nit: initializations on separate line
> >>>> OK
> >>>>>
> >>>>>> +
> >>>>>> +	list_sort(NULL, &device->itt_head, vgic_its_ite_cmp);
> >>>>>> +
> >>>>>> +	list_for_each_entry(ite, &device->itt_head, ite_list) {
> >>>>>> +		gpa_t gpa = base + ite->event_id * ite_esz;
> >>>>>> +
> >>>>>> +		ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz);
> >>>>>> +		if (ret)
> >>>>>> +			return ret;
> >>>>>> +	}
> >>>>>> +	return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
> >>>>>> +{
> >>>>>> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> >>>>>> +	gpa_t base = dev->itt_addr;
> >>>>>> +	int ret, ite_esz = abi->ite_esz;
> >>>>>> +	size_t max_size = BIT_ULL(dev->nb_eventid_bits) * ite_esz;
> >>>>>
> >>>>> nit: initializations on separate line
> >>>> OK
> >>>>>
> >>>>>> +
> >>>>>> +	ret =  lookup_table(its, base, max_size, ite_esz, 0,
> >>>>>> +			    vgic_its_restore_ite, dev);
> >>>>>
> >>>>> nit: extra white space
> >>>>>
> >>>>>> +
> >>>>>> +	if (ret < 0)
> >>>>>> +		return ret;
> >>>>>> +
> >>>>>> +	/* if the last element has not been found we are in trouble */
> >>>>>> +	return ret ? 0 : -EINVAL;
> >>>>>
> >>>>> hmm, these are values potentially created by the guest in guest RAM,
> >>>>> right?  So do we really abort migration and return an error to userspace
> >>>>> in this case?
> >>>> So we discussed with Peter/dave we shouldn't abort() in qemu in case of
> >>>> such error. The restore table IOCTL will return an error. Up to qemu to
> >>>> print the error. Destination guest will not be functional though.
> >>>>
> >>>
> >>> ok, I'm just wondering if userspace can make a qualified decision based
> >>> on this error code.  EINVAL typically means that userspace provided
> >>> something incorrect, which I suppose in a sense is true, but this should
> >>> be the only case where we return EINVAL here.
> >>   Userspace must be able to
> >>> tell the cases apart where the guest programmed bogus into memory before
> >>> migration started, in which case we should ignore-and-resume, and where
> >>> QEMU errornously provide some bogus value where the machine state
> >>> becomes unreliable and must be powered down.
> >> guest does not feed much besides few registers the ITS table restore
> >> depends on. In case we want a more subtle error management at userspace
> >> level all the error codes need to be revisited I am afraid. My plan was
> >> to be more rough at the beginning and ignore & resume if ITS table
> >> restore fails.
> >>
> > 
> > Do we require that the VM is quiesced the entire time between saving the
> > ITS state to memory and copying all memory over the wire and capturing
> > all register state?  If so, then an error to restore would be because of
> > userspace doing something wrong and handling that accordingly is fine.
> 
> yes the ITS table save into RAM starts when we have a guarantee that all
> the VCPUS are stopped (we take all locks). 

The important bit is whether or not userspace is allowed to start any
VCPUs again before copying over all RAM etc.  I suppose not.

> The restore happens before
> the VM gets resumed. At least this is the QEMU integration as of today.
> 

Does our ABI mandate this behavior (document it somewhere) ?

Thanks,
-Christoffer

^ permalink raw reply

* [PATCH] ARM: Fix __show_regs output timestamps
From: Vladimir Murzin @ 2017-05-04  8:27 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1493857488.22125.31.camel@perches.com>

On 04/05/17 01:24, Joe Perches wrote:
> On Wed, 2017-05-03 at 22:30 +0100, Russell King - ARM Linux wrote:
>> On Wed, May 03, 2017 at 12:44:11PM -0700, Joe Perches wrote:
>>> On Wed, 2017-05-03 at 20:23 +0100, Russell King - ARM Linux wrote:
>>>> On Wed, Apr 26, 2017 at 10:39:49AM -0700, Joe Perches wrote:
>>>>> Multiple line formats are not preferred as the second and
>>>>> subsequent lines may not have timestamps.
>>>>>
>>>>> Lacking timestamps makes reading the output a bit difficult.
>>>>> This also makes arm/arm64 output more similar.
>>>>>
>>>>> Previous:
>>>>>
>>>>> [ 1514.093231] pc : [<bf79c304>]    lr : [<bf79ced8>]    psr: a00f0013
>>>>> sp : ecdd7e20  ip : 00000000  fp : ffffffff
>>>>>
>>>>> New:
>>>>>
>>>>> [ 1514.093231] pc : [<bf79c304>]    lr : [<bf79ced8>]    psr: a00f0013
>>>>> [ 1514.105316] sp : ecdd7e20  ip : 00000000  fp : ffffffff
>>>>>
>>>>> Signed-off-by: Joe Perches <joe@perches.com>
>>>>
>>>> Hi Joe,
>>>>
>>>> Could you put this in my patch system please, I'm unlikely to remember to
>>>> apply it otherwise if not already there (massive email backlog.)
>>>>
>>>> Thanks.
>>>
>>> Your patch system bounced my perfectly formatted patch
>>> because your system wants totally unnecessary additional
>>> information specific to your workflow.
>>>
>>> No thanks, I don't need the additional work just to
>>> please your system and neither should anyone else.
>>
>> Don't expect me to remember to apply your patch then.  I've got days of
>> catch up, and I'm just not going to remember.  Sorry.
> 
> <shrug>
> 
> If your systems require special handling on the
> part of patch submitters, you should document it
> in the kernel tree.
> 
> Better, someone else should find the time to apply
> properly formatted patches.
> 

Joe, I find this patch handy, so I've uploaded it into Russell's patch system
on your behalf and it has been accepted as patch 8673/1.

Cheers
Vladimir

> 
> _______________________________________________
> 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 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC
From: Marc Zyngier @ 2017-05-04  8:28 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170504081337.GF5923@cbox>

On 04/05/17 09:13, Christoffer Dall wrote:
> On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
>> On 03/05/17 19:32, Christoffer Dall wrote:
>>> Since we got support for devices in userspace which allows reporting the
>>> PMU overflow output status to userspace, we should actually allow
>>> creating the PMU on systems without an in-kernel irqchip, which in turn
>>> requires us to slightly clarify error codes for the ABI and move things
>>> around for the initialization phase.
>>>
>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>> ---
>>>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
>>>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
>>>  2 files changed, 26 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
>>> index 02f5068..352af6e 100644
>>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
>>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
>>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
>>>  Returns: -EBUSY: The PMU overflow interrupt is already set
>>>           -ENXIO: The overflow interrupt not set when attempting to get it
>>>           -ENODEV: PMUv3 not supported
>>> -         -EINVAL: Invalid PMU overflow interrupt number supplied
>>> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
>>> +                  trying to set the IRQ number without using an in-kernel
>>> +                  irqchip.
>>>  
>>>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
>>>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
>>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
>>>  
>>>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
>>>  Parameters: no additional parameter in kvm_device_attr.addr
>>> -Returns: -ENODEV: PMUv3 not supported
>>> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
>>> -                 attribute
>>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
>>> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
>>> +                 conigured as required prior to calling this attribute
>>>           -EBUSY: PMUv3 already initialized
>>>  
>>> -Request the initialization of the PMUv3.  This must be done after creating the
>>> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
>>> -supported.
>>> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>>> +virtual GIC implementation, this must be done after initializing the in-kernel
>>> +irqchip.
>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>> index 4b43e7f..f046b08 100644
>>> --- a/virt/kvm/arm/pmu.c
>>> +++ b/virt/kvm/arm/pmu.c
>>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>  	if (!kvm_arm_support_pmu_v3())
>>>  		return -ENODEV;
>>>  
>>> -	/*
>>> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
>>> -	 * because we do not support forwarding PMU overflow interrupts to
>>> -	 * userspace yet.
>>> -	 */
>>> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
>>> -		return -ENODEV;
>>> -
>>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
>>> -	    !kvm_arm_pmu_irq_initialized(vcpu))
>>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>  		return -ENXIO;
>>>  
>>>  	if (kvm_arm_pmu_v3_ready(vcpu))
>>>  		return -EBUSY;
>>>  
>>> +	if (irqchip_in_kernel(vcpu->kvm)) {
>>> +		/*
>>> +		 * If using the PMU with an in-kernel virtual GIC
>>> +		 * implementation, we require the GIC to be already
>>> +		 * initialized when initializing the PMU.
>>> +		 */
>>> +		if (!vgic_initialized(vcpu->kvm))
>>> +			return -ENODEV;
>>> +
>>> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
>>> +			return -ENXIO;
>>> +	}
>>> +
>>>  	kvm_pmu_vcpu_reset(vcpu);
>>>  	vcpu->arch.pmu.ready = true;
>>>  
>>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>>>  		int __user *uaddr = (int __user *)(long)attr->addr;
>>>  		int irq;
>>>  
>>> +		if (!irqchip_in_kernel(vcpu->kvm))
>>> +			return -EINVAL;
>>> +
>>
>> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
>> generate a -ENXIO, and has_attr is going to lie about the availability
>> of KVM_ARM_VCPU_PMU_V3_IRQ...
>>
> 
> Here's the text from api.txt:
> 
>   Tests whether a device supports a particular attribute.  A successful
>   return indicates the attribute is implemented.  It does not necessarily
>   indicate that the attribute can be read or written in the device's
>   current state.  "addr" is ignored.
> 
> My interpretation therefore is that QEMU can use this ioctl to figure
> out if the feature is supported (sort of like a capability), but that
> doesn't mean that the configuration of the VM is such that the attribute
> can be get or set at that moment.
> 
> For example, there will also alway be situations where you can get an
> attr, but not set an attr, what should the has_attr return then?

My issue here is that whether we can get/set the interrupt or not is not
a function of the device itself, but of the way it is "wired". No matter
what "the device's current state" is, we'll never be able to get/set the
interrupt.

I'd tend to err on the side of caution and return something that is
unambiguous, be maybe I have too strict an interpretation of the API.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply

* [PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC
From: Christoffer Dall @ 2017-05-04  8:38 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <db5a1fb7-e77e-b9f8-9be4-161e94279d10@arm.com>

On Thu, May 04, 2017 at 09:28:50AM +0100, Marc Zyngier wrote:
> On 04/05/17 09:13, Christoffer Dall wrote:
> > On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
> >> On 03/05/17 19:32, Christoffer Dall wrote:
> >>> Since we got support for devices in userspace which allows reporting the
> >>> PMU overflow output status to userspace, we should actually allow
> >>> creating the PMU on systems without an in-kernel irqchip, which in turn
> >>> requires us to slightly clarify error codes for the ABI and move things
> >>> around for the initialization phase.
> >>>
> >>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> >>> ---
> >>>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
> >>>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
> >>>  2 files changed, 26 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> >>> index 02f5068..352af6e 100644
> >>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> >>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> >>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
> >>>  Returns: -EBUSY: The PMU overflow interrupt is already set
> >>>           -ENXIO: The overflow interrupt not set when attempting to get it
> >>>           -ENODEV: PMUv3 not supported
> >>> -         -EINVAL: Invalid PMU overflow interrupt number supplied
> >>> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
> >>> +                  trying to set the IRQ number without using an in-kernel
> >>> +                  irqchip.
> >>>  
> >>>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
> >>>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> >>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
> >>>  
> >>>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
> >>>  Parameters: no additional parameter in kvm_device_attr.addr
> >>> -Returns: -ENODEV: PMUv3 not supported
> >>> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
> >>> -                 attribute
> >>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> >>> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
> >>> +                 conigured as required prior to calling this attribute
> >>>           -EBUSY: PMUv3 already initialized
> >>>  
> >>> -Request the initialization of the PMUv3.  This must be done after creating the
> >>> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
> >>> -supported.
> >>> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> >>> +virtual GIC implementation, this must be done after initializing the in-kernel
> >>> +irqchip.
> >>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> >>> index 4b43e7f..f046b08 100644
> >>> --- a/virt/kvm/arm/pmu.c
> >>> +++ b/virt/kvm/arm/pmu.c
> >>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
> >>>  	if (!kvm_arm_support_pmu_v3())
> >>>  		return -ENODEV;
> >>>  
> >>> -	/*
> >>> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
> >>> -	 * because we do not support forwarding PMU overflow interrupts to
> >>> -	 * userspace yet.
> >>> -	 */
> >>> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
> >>> -		return -ENODEV;
> >>> -
> >>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
> >>> -	    !kvm_arm_pmu_irq_initialized(vcpu))
> >>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> >>>  		return -ENXIO;
> >>>  
> >>>  	if (kvm_arm_pmu_v3_ready(vcpu))
> >>>  		return -EBUSY;
> >>>  
> >>> +	if (irqchip_in_kernel(vcpu->kvm)) {
> >>> +		/*
> >>> +		 * If using the PMU with an in-kernel virtual GIC
> >>> +		 * implementation, we require the GIC to be already
> >>> +		 * initialized when initializing the PMU.
> >>> +		 */
> >>> +		if (!vgic_initialized(vcpu->kvm))
> >>> +			return -ENODEV;
> >>> +
> >>> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
> >>> +			return -ENXIO;
> >>> +	}
> >>> +
> >>>  	kvm_pmu_vcpu_reset(vcpu);
> >>>  	vcpu->arch.pmu.ready = true;
> >>>  
> >>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >>>  		int __user *uaddr = (int __user *)(long)attr->addr;
> >>>  		int irq;
> >>>  
> >>> +		if (!irqchip_in_kernel(vcpu->kvm))
> >>> +			return -EINVAL;
> >>> +
> >>
> >> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
> >> generate a -ENXIO, and has_attr is going to lie about the availability
> >> of KVM_ARM_VCPU_PMU_V3_IRQ...
> >>
> > 
> > Here's the text from api.txt:
> > 
> >   Tests whether a device supports a particular attribute.  A successful
> >   return indicates the attribute is implemented.  It does not necessarily
> >   indicate that the attribute can be read or written in the device's
> >   current state.  "addr" is ignored.
> > 
> > My interpretation therefore is that QEMU can use this ioctl to figure
> > out if the feature is supported (sort of like a capability), but that
> > doesn't mean that the configuration of the VM is such that the attribute
> > can be get or set at that moment.
> > 
> > For example, there will also alway be situations where you can get an
> > attr, but not set an attr, what should the has_attr return then?
> 
> My issue here is that whether we can get/set the interrupt or not is not
> a function of the device itself, but of the way it is "wired". No matter
> what "the device's current state" is, we'll never be able to get/set the
> interrupt.
> 
> I'd tend to err on the side of caution and return something that is
> unambiguous, be maybe I have too strict an interpretation of the API.
> 

Hmm, I see the has_attr as a method for userspace to discover "does this
kernel have this feature".  If we make has_attr return a value depending
on the VM having an in-kernel gic or not, we implicitly require
userspace to create a VM with an in-kernel GIC to discover if this
kernel has that feature, and therefore also impose an ordering
requirement of figuring out the capabilities of the kernel (i.e. create
the GIC before checking this API).

I think QEMU should be able to do:

  if (has_attr()) {
     kvm_supports_set_timer_irq = true;
     vtimer_irq = foo;
  } else {
     kvm_supports_set_timer_irq = false;
     vtimer_irq = 27; /* default, we're stuck with it */
  }

  create_board_definition();
  create_dt();
  create_acpi();

  /* do whatever */

  if (kvm_supports_set_timer_irq && kvm_irqchip_in_kernel()) {
  	kvm_arm_timer_set_irq(...);
  }

And all this should not be coupled to when we create the irqchip device.

But I may be failing to see the case where the current implementation
creates a problem for userspace, in which case we should document the
ordering requirement.

Note: that I don't think it's expected that has_attr implies get_attr
and set_attr succeed.  For example, has_attr is currently typically
called with a null pointer to the addr field.

Thanks,
-Christoffer

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox