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

[Julien Grall <julien@xxxxxxx>]

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.
> 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.
Cheers,

--
Julien Grall