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

Re: [PATCH v1 04/29] xen/asm-generic: introduce stub header device.h



Hi Jan,

On 19/10/2023 12:14, Jan Beulich wrote:
On 19.10.2023 13:07, Julien Grall wrote:


On 19/10/2023 12:01, Jan Beulich wrote:
On 19.10.2023 12:57, Julien Grall wrote:
On 19/10/2023 11:53, Jan Beulich wrote:
On 19.10.2023 12:42, Julien Grall wrote:
On 19/10/2023 10:14, Jan Beulich wrote:
On 14.09.2023 16:56, Oleksii Kurochko wrote:
--- /dev/null
+++ b/xen/include/asm-generic/device.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_DEVICE_H__
+#define __ASM_GENERIC_DEVICE_H__
+
+struct dt_device_node;
+
+enum device_type
+{
+    DEV_DT,
+    DEV_PCI,
+};

Are both of these really generic?

I think can be re-used for RISC-V to have an abstract view a device.
This is for instance used in the IOMMU code where both PCI and platform
(here called DT) can be assigned to a domain. The driver will need to
know the difference, but the common layer doesn't need to.

Question to me is whether DT and PCI can be considered "common", which
is a prereq for being used here.

I think it can. See more below.


+struct device {
+    enum device_type type;
+#ifdef CONFIG_HAS_DEVICE_TREE
+    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
+#endif
+};
+
+enum device_class
+{
+    DEVICE_SERIAL,
+    DEVICE_IOMMU,
+    DEVICE_GIC,

This one certainly is Arm-specific.

This could be renamed to DEVICE_IC (or INTERRUPT_CONTROLLER)


+    DEVICE_PCI_HOSTBRIDGE,

And this one's PCI-specific.

Are you suggesting to #ifdef it? If so, I don't exactly see the value here.

What to do with it is secondary to me. I was questioning its presence here.

Overall same question as before: Are you expecting that RISC-V is going to
get away without a customized header? I wouldn't think so.

I think it can be useful. Most likely you will have multiple drivers for
a class and you may want to initialize certain device class early than
others. See how it is used in device_init().

I'm afraid I don't see how your reply relates to the question of such a
fallback header being sensible to have, or whether instead RISC-V will
need its own private header anyway.

My point is that RISC-V will most likely duplicate what Arm did (they
are already copying the dom0less code). So the header would end up to be
duplicated. This is not ideal and therefore we want to share the header.

I don't particularly care whether it lives in asm-generic or somewhere.
I just want to avoid the duplication.

Avoiding duplication is one goal, which I certainly appreciate. The header
as presented here is, however, only a subset of Arm's if I'm not mistaken.
If moving all of Arm's code here, I then wonder whether that really can
count as "generic".

  From previous discussion, I recalled that we seemed to agree that if
applies for most the architecture, then it should be considered common.

Hmm, not my recollection - a certain amount of "does this make sense from
an abstract perspective" should also be applied.

Avoiding duplication could e.g. be achieved by making RISC-V symlink Arm's
header.

Ewwwwww. Removing the fact I dislike it, I can see some issues with this
approach in term of review. Who is responsible to review for any changes
here? Surely, we don't only want to the Arm folks to review.

That could be achieved by an F: entry in the RISC-V section of ./MAINTAINERS.

This works for one arch. But if PPC needs the same, then this is another symbolic link.

At which point, how would this be different from asm-generic? We need to have a way to share common headers that doesn't involve one arch to symlink headers from another arch.

Cheers,

--
Julien Grall



 


Rackspace

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