[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT
Hi,
On 03/23/2018 04:17 AM, Manish Jaggi wrote:
On 03/23/2018 06:58 AM, Julien Grall wrote:
On 03/13/2018 03:20 PM, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
+ *
+ * fwits_node - ITS Node pointer in Firmware IORT
+ * offset - offset of the equivalent its node to be stored in
+ * hwdom's IORT
+ */
+static int is_uniq_fwits_node(struct acpi_iort_node *fwits_node,
The name is a bit odd given that you add the ITS node. On the previous
version, I requested to document that behavior...
I think the name is quite appropriate. Also in this patch I have added
description of the flow so this should be fairly intuitive.
Could you please let me know the specific point you dont understand, I
can explain that.
The fact that a function calling is_* will add the element to the list.
An is_* function should only check the element is in the list.
So yes, it is not intuitive for me.
But you likely want to rename the function to add_fwits_node(...) or
something similar.
I think name is quite appropriate.
See above.
+ unsigned int offset)
+{
+ struct fwits_hwits_map *map;
+
+ list_for_each_entry(map, &fwits_hwits_list, entry)
+ {
+ if ( map->fwits_node == fwits_node )
+ return 0;
+ }
+
+ map = xzalloc(struct fwits_hwits_map);
Where this memory is going to be freed?
Since this list can be used multiple times even after creation of IORT
for dom0, say thinking ahead for domUs.
IORT for DomU will not rely on the host firmware and will be created by
the toolstack. It does not make sense to keep that around.
+ if ( !map )
+ return -ENOMEM;
+
+ map->fwits_node = fwits_node;
+ map->hwitsnode_offset = offset;
+ list_add_tail(&map->entry, &fwits_hwits_list);
+
+ return 1;
+}
+
+/*
+ * Returns the offset of corresponding its node to fwits_node
+ * written in hwdom's IORT.
+ *
+ * This function would be used when write hwdoms pcirc nodes' idmap
+ * entries.
+ */
+static
+unsigned int hwitsnode_offset_from_map(struct acpi_iort_node
*fwits_node)
+{
+ struct fwits_hwits_map *map;
+
+ list_for_each_entry(map, &fwits_hwits_list, entry)
+ {
+ if ( map->fwits_node == fwits_node )
+ return map->hwitsnode_offset;
+ }
+
+ return 0;
0 could never be a valid offset, right?
Yes
See a bug_on when it is used.
I don't much care about the BUG_ON()... I was only checking what the
spec says here.
But... you document the return really well on the previous function and
here it is seems to be forgotten.
+}
+
+static void write_hwits_nodes(u8 *iort, unsigned int *offset,
Please avoid using u* and use uint_* instead. I expect that you fix
all of for the next version.
Here, I think you want to use void *.
+ unsigned int *num_nodes)
+{
+ struct rid_devid_map *rmap;
+ unsigned int of = *offset;
Please name it off. This is clearer that it is an offset. But as I
said on the previous version, why can't you just re-use offset?
I will change it to off, will not break anything.
It does not answer my question.
+ int n = 0;
+
+ /*
+ * rid_devid_list is iterated to get unique its group nodes
+ * Each unique ITS group node is written in hardware domains IORT
+ * by using some values from the firmware ITS group node.
+ */
+ list_for_each_entry(rmap, &rid_devid_list, entry)
+ {
+ struct acpi_iort_node *node;
+ struct acpi_iort_its_group *grp;
+ struct acpi_iort_its_group *fw_grp;
+
+ /* save its_node_offset_map in a list uniquely */
+ if ( is_uniq_fwits_node(rmap->its_node, of) == 1 )
If the function is returning -ENOMEM, then you will ignore the node
without a warning. That's going to be a real pain to find out a ITS
node is not present if that happen.
Here, you should propagate error if something wrong is going.
ok.
+ {
+ node = (struct acpi_iort_node *) &iort[of];
+ grp = (struct acpi_iort_its_group *)(&node->node_data);
+
+ node->type = ACPI_IORT_NODE_ITS_GROUP;
+ node->length = sizeof(struct acpi_iort_node) +
+ sizeof(struct acpi_iort_its_group) -
+ sizeof(node->node_data);
While the substraction is good, this is odd enough to warrant a
comment. But likely But likely you want to provide macros for defining
the length. This will clean a lot the code.
+
+ node->revision = rmap->its_node->revision;
I am not sure this is right. You rewrite the IORT based on a given
revision. Imagine the host IORT get updated to a newer spec but not Xen.
Not sure if I follow your comment here.
Xen gets host IORT from firmware, how will it get updated?
The host IORT will be built on top of a revision X. For the hwdom IORT,
at the moment, you are always building on top of revision 0.
While today X == 0, this may change in the future and will prevent
proper booting.
+
+ of += node->length;
+ n++;
+ }
+ }
+ *offset = of;
+ *num_nodes = n;
+}
+
+static void write_hwpcirc_nodes(u8 *iort, unsigned int *pos,
+ unsigned int *num_nodes)
+{
+ struct acpi_iort_node *opcirc_node, *pcirc_node;
+ struct acpi_iort_node *hwdom_pcirc_node = NULL;
+ struct rid_devid_map *rmap;
+ struct acpi_iort_id_mapping *idmap;
+ int num_idmap = 0, n = 0;
+ unsigned int old_pos = *pos;
+
+ opcirc_node = NULL;
+ /* Iterate rid_map_devid list */
+ list_for_each_entry(rmap, &rid_devid_list, entry)
+ {
+ struct acpi_iort_root_complex *rc;
+ struct acpi_iort_root_complex *rc_fw;
+ int add_node = 0;
This should be bool.
I am going to stop the review here because I feel I am just repeating
all my comments from the first version and new ones. So please go
through *all* my e-mails from the previous version, answer to my
question, and respin it.
Could you please have a look at other patches and if there is
functionality related change that I need to make, i can add that in my
next rev.
I am afraid I am not going to have time reviewing for the next couple of
weeks.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
