Re: [PATCH v6 9/9] xen/asm-generic: introduce generic device.h

Fri, 22 Dec 2023 06:16:34 -0800 

Hi Oleksii,

On 22/12/2023 13:16, Oleksii wrote:

On Thu, 2023-12-21 at 19:38 +0000, Julien Grall wrote:

Hi,

On 20/12/2023 14:08, Oleksii Kurochko wrote:

Arm, PPC and RISC-V use the same device.h thereby device.h
was moved to asm-generic. Arm's device.h was taken as a base with
the following changes:
   - #ifdef PCI related things.
   - #ifdef ACPI related things.
   - Rename #ifdef guards.
   - Add SPDX tag.
   - #ifdef CONFIG_HAS_DEVICE_TREE related things.
   - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.

Also Arm and PPC are switched to asm-generic version of device.h

Signed-off-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
---

       Jan wrote the following:
         Overall I think there are too many changes done all in one
go here.
         But it's mostly Arm which is affected, so I'll leave
judging about that
         to the Arm maintainers.
      Arm maintainers, will it be fine for you to not split the 
patch?


So in general I agree with Jan, patches should be kept small so they
are
easy to review.

Given the discussion has been on-going for a while (we are at v6),  I
will give an attempt to review the patch as-is. But in the future,
please try to split. The smaller it is, the easier to review. For
code
movement and refactoring, I tend to first have a few refactoring
patches
and then move the code in a separate patch. So it is easier to spot
the
differences.

Thanks, I'll separate the patch.



---
Changes in V6:
   - Rebase only.
---
Changes in V5:
    - Removed generated file: xen/include/headers++.chk.new
    - Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for
PPC as
      CONFIG_HAS_DEVICE_TREE will be always used for PPC.
---
Changes in V4:
   - Updated the commit message
   - Switched Arm and PPC to asm-generic version of device.h
   - Replaced HAS_PCI with CONFIG_HAS_PCI
   - ifdef-ing iommu filed of dev_archdata struct with
CONFIG_HAS_PASSTHROUGH
   - ifdef-ing iommu_fwspec of device struct with
CONFIG_HAS_PASSTHROUGH
   - ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
   - Updated the commit message ( remove a note with question about
     if device.h should be in asm-generic or not )
   - Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
   - Rationalized usage of CONFIG_HAS_* in device.h
   - Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
---
Changes in V3:
   - ifdef device tree related things.
   - update the commit message
---
Changes in V2:
        - take ( as common ) device.h from Arm as PPC and RISC-V
use it as a base.
        - #ifdef PCI related things.
        - #ifdef ACPI related things.
        - rename DEVICE_GIC to DEVIC_IC.
        - rename #ifdef guards.
        - switch Arm and PPC to generic device.h
        - add SPDX tag
        - update the commit message

---
   xen/arch/arm/device.c                         |  15 ++-
   xen/arch/arm/domain_build.c                   |   2 +-
   xen/arch/arm/gic-v2.c                         |   4 +-
   xen/arch/arm/gic-v3.c                         |   6 +-
   xen/arch/arm/gic.c                            |   4 +-
   xen/arch/arm/include/asm/Makefile             |   1 +
   xen/arch/ppc/include/asm/Makefile             |   1 +
   xen/arch/ppc/include/asm/device.h             |  53 --------
   .../asm => include/asm-generic}/device.h      | 125 +++++++++++--
-----
   9 files changed, 102 insertions(+), 109 deletions(-)
   delete mode 100644 xen/arch/ppc/include/asm/device.h
   rename xen/{arch/arm/include/asm => include/asm-generic}/device.h
(79%)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 1f631d3274..affbe79f9a 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -16,7 +16,10 @@
   #include <xen/lib.h>
  extern const struct device_desc _sdevice[], _edevice[]; 
+
+#ifdef CONFIG_ACPI
   extern const struct acpi_device_desc _asdevice[], _aedevice[];
+#endif
  int __init device_init(struct dt_device_node *dev, enum 
device_class class,
                          const void *data)
@@ -45,6 +48,7 @@ int __init device_init(struct dt_device_node
*dev, enum device_class class,
       return -EBADF;
   }
+#ifdef CONFIG_ACPI 
   int __init acpi_device_init(enum device_class class, const void
*data, int class_type)
   {
       const struct acpi_device_desc *desc;
@@ -61,6 +65,7 @@ int __init acpi_device_init(enum device_class
class, const void *data, int class
      return -EBADF; 
   }
+#endif
  enum device_class device_get_class(const struct dt_device_node 
*dev)
   {
@@ -329,9 +334,13 @@ int handle_device(struct domain *d, struct
dt_device_node *dev, p2m_type_t p2mt,
       struct map_range_data mr_data = {
           .d = d,
           .p2mt = p2mt,
-        .skip_mapping = !own_device ||
-                        (is_pci_passthrough_enabled() &&
-                        (device_get_class(dev) ==
DEVICE_PCI_HOSTBRIDGE)),
+        .skip_mapping =
+                        !own_device
+#ifdef CONFIG_HAS_PCI
+                        || (is_pci_passthrough_enabled() &&
+                        (device_get_class(dev) ==
DEVICE_PCI_HOSTBRIDGE))
+#endif


So the #ifdef is only here because DEVICE_PCI_HOSTBRIDGE is not
defined.
It is not clear what's the actual problem of keeping
DEVICE_PCI_HOSTBRIDGE available for every build.

The only reason for that is that it seems to be used only in thecase when 
CONFIG_HAS_PCI is enabled ( probably not all archs will
have PCI support ).
Possibly yes. But you will always need some compat layer unless you manage to get rid of any use of PCI (or any feature you don't want) within "common" code.
Here, it is not clear to me what you are effectively trying to protect against. IOW, what could go wrong if PCI_HOSTBRIDGE is available for everyone?
I know it means we would never return DEVICE_PCI_HOSTBRIDGE even if the device is actually a hostbridge. But the same is true if Xen doesn't have a driver for the hostbridge (not everyone are using ECAM).
So at the end of the day this is a trade-off between keep the code readable (from my POV) and reducing the amount of symbols/defines exposed.
This is also matching my view on Jan's proposal to protect the static keyword for first_valid_mfn with #ifdef. There is a limit in the amount of #ifdef I will tolerate. 

I'd like to hear the opinion from the others.


Generally,
I think it's okay not to use #ifdef DEVICE_PCI_HOSTBRIDGE to keep
the Arm code cleaner.

Does it make sense?
I don't quite understand your last sentence. Are you saying you would be ok to remove #ifdef CONFIG_HAS_PCI around DEVICE_PCI_HOSTBRIDGE? 

Cheers,

--
Julien Grall

Reply via email to