[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC 01/11] acpi: arm: Public API for populating and query based on requesterid

On 19/01/18 06:05, Manish Jaggi wrote:

On 01/17/2018 12:01 AM, Julien Grall wrote:
Hi Manish,

Hi Julien,
Thanks for reviewing the patch.

I sent the previous e-mail too soon.

On 02/01/18 09:27, manish.jaggi@xxxxxxxxxx wrote:
From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>

  Public API to populate and query map between requester id and
  streamId/DeviceID. IORT is parsed one time (outside this patch)
  and two lists are created one for mapping between reuesterId and streamid
  and another between requesterID and deviceID.

  These lists eliminate the need to reparse IORT for querying streamid
  or deviceid using requesterid.

  Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
  xen/drivers/acpi/Makefile     |   1 +
  xen/drivers/acpi/arm/Makefile |   1 +

We have a directory arch/arm/acpi/. So please move all your code there.
The current files in arch/arm/acpi hold only boot time/low level code.
IMHO creating drivers/acpi/arm makes more sense.
Linux also has iort code in drivers/acpi/arm.

drivers/acpi mostly contain generic ACPI code. ridmap.c and iort.c is AFAICT Arm specific. So arch/arm/acpi is a better place.


+ *
+ * Manish Jaggi <manish.jaggi@xxxxxxxxxx>
+ * Copyright (c) 2018 Linaro.
+ *
+ * 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.

Xen is GPLv2 only and hence the copyright wrong. You want to use:

This program is free software; you can redistribute it and/or modify it
under the terms and conditions of the GNU General Public License,
version 2, as published by the Free Software Foundation.

I picked this copyright from xen/arch/arm/traps.c.

  * xen/arch/arm/traps.c
  * ARM Trap handlers
  * Copyright (c) 2011 Citrix Systems.
  * 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 Licese, or
  * (at your option) any later version.

So IIUYC, traps.c copyright is also wrong.
How do we plan to fix all other files in xen code which use the same copyright.


+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * GNU General Public License for more details.
+ */
+#include <acpi/ridmap.h>
+#include <xen/iommu.h>
+#include <xen/kernel.h>
+#include <xen/list.h>
+#include <xen/pci.h>
+struct list_head rid_streamid_map_list;
+struct list_head rid_deviceid_map_list;

Please drop _list. This is pointless to know that when you can discover it.

I think it is not pointless.
There is a point here. :)
  _list is added to show that it is a list to make it more verbose.
Without _list the name could mean a single mapping as well.

When I read, rid_streamid_map. I understand it is a map of rid/streamid. Not a single mapping. But ...

If you care to see
xen/common/rangeset.c:27:    struct list_head range_list;

I hope you can appreciate the point.

... look at the name length here, 10 characters. Yours is 22 characters. This is 1/4 of a line. That's just stupid.

Also, can you explain the rationale of using an unsorted list over another structure?
Since rid - streamId mapping also requires pcirc_node so it would require two level of sorting.
First sort based on pcirc_node and next on basis of rid.
Does it makes sense to have all that complex code here ?
  as this API will be used only once per pci device
Along that please give an idea how often and where the query API will be used.
BTW, this is called from pci_for_each_dma_alias code flow.

The document the rationale.

+void init_ridmaps(void)

This likely need to be __init.

+    INIT_LIST_HEAD(&rid_deviceid_map_list);
+    INIT_LIST_HEAD(&rid_streamid_map_list);

This function is not necessary. Declaring LIST_HEAD(rid_streamid_map_list) will do the trick.
+int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,

Ditto for? Sorry didnt catch your point here.


+                         struct acpi_iort_node *smmu_node,
+                         u32 input_base, u32 output_base, u32 id_count)

u32 & co should not be used in new code (unless imported from Linux). Please use uint32_t & co.
I couldn't find this in xen coding style document.
Could you please point to the section which says u32 should not be used.

It is not in the coding style but Xen is phasing out from u*. What's the problem?

+    struct rid_streamid_map *rid_map;

Newline here as it should be between after declarations.


+    rid_map = xzalloc(struct rid_streamid_map);
+    if (!rid_map)

This should be ( ... ).

+        return -ENOMEM;

You either return -ENOMEM or 0 in this function. It sounds like to me that bool would be the best.
I think ENOMEM should be used here. The error code is designed specifically for this purpose.

Fair enough.

+    return 0;
+void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 *streamid,


But how come the rid is 16-bit here when Linux is using 32-bit?
IIUC rid is 16bit only. Dont know why linux is using 32bit.

rid = bus - 8bits , devfn 8bits.

 From PCI Express specification
The Requester ID is a 16-bit value that is unique for every PCI Express Function within a Hierarchy..

If you think it is a 32bit value please let me know how to use upper 16 bits.

Well AFAICT, the IORT stores 32-bit. So it is probably best to stick with it.

Also, I am a bit puzzled how the caller is expected to use it.
I thought it would be self explanatory query streamid based on rid.
But if it is not verbose enough for you, I will add this explicitly.

Not at all. More than this function is returning void. A query function is usually return a bool/int.

From the name I would expect the function to return whether a translation was found. But it returns void.

IHMO, this is a pretty bad idea and make more expectation on the value for the caller.

Lastly, I would appreciate documentation on at least the function exported.

+                    struct acpi_iort_node **smmu_node)
+    struct rid_streamid_map *rmap;
+    list_for_each_entry(rmap, &rid_streamid_map_list, entry)
+    {
+        if (rmap->pcirc_node == pcirc_node)

Coding style.
Can we have a checkpatch.pl for xen.
This would help in cases when code has a mix of files with linux coding style and xen coding style.

This is been worked on.


I am not a big fan of exporting those 2 maps. But I will see how you use it before commenting.
Lot of xen code using the same way. Is that all wrong?

Second sentence: "I will see how you use it before commenting".


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.