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.ja...@linaro.org wrote:
From: Manish Jaggi <manish.ja...@linaro.org>
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.ja...@linaro.org>
---
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.ja...@linaro.org>
+ * 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.
https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02899.html
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * 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.
ok
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.
ok.
+{
+ 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.
ok.
+
+int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
Ditto.
Ditto for? Sorry didnt catch your point here.
__init.
+ 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.
ok
+ 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,
s/u*/uint_/
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.
ok.
+ 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".
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
