| Message ID | 3cc9ecc3abcd21c5ed7276b01bf5963e6a5fd5e0.1694702259.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce stub headers necessary for full Xen build | expand |
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? > +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. > + DEVICE_PCI_HOSTBRIDGE, And this one's PCI-specific. 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. Jan
Hi, 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. >> +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. > > 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(). Cheers,
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. >>> +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. Jan
Hi Jan, 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. Cheers,
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". Avoiding duplication could e.g. be achieved by making RISC-V symlink Arm's header. Jan
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,
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. Jan
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,
On 19.10.2023 13:27, Julien Grall wrote: > 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 ... which are sufficiently arch-agnostic. > that doesn't involve one arch to symlink headers from another arch. Whether to use symlinks or #include "../../arch/..." or yet something else is a matter of mechanics. Jan
Hi, On 19/10/2023 12:41, Jan Beulich wrote: > On 19.10.2023 13:27, Julien Grall wrote: >> that doesn't involve one arch to symlink headers from another arch. > > Whether to use symlinks or #include "../../arch/..." or yet something else is > a matter of mechanics. #include "../../arch/../" is pretty much in the same category. This is simply hiding the fact they could be in asm-generic. Anyway, I have shared my view. Let see what the others thinks. Cheers,
On Thu, 2023-10-19 at 11:14 +0200, 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? > > > +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. Yes, but the definition of GIC sounds common, so I decided to leave it. But it can be changed. > > > + DEVICE_PCI_HOSTBRIDGE, > > And this one's PCI-specific. > > 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. At least right now, I am using the same header device.h as in ARM, and there wasn't a need for a customized version of the header. ~ Oleksii
On Thu, 2023-10-19 at 13:12 +0100, Julien Grall wrote: > Hi, > > On 19/10/2023 12:41, Jan Beulich wrote: > > On 19.10.2023 13:27, Julien Grall wrote: > > > that doesn't involve one arch to symlink headers from another > > > arch. > > > > Whether to use symlinks or #include "../../arch/..." or yet > > something else is > > a matter of mechanics. > > #include "../../arch/../" is pretty much in the same category. This > is > simply hiding the fact they could be in asm-generic. > > Anyway, I have shared my view. Let see what the others thinks. I have the same point: if something is shared at least between two arch, it should go to ASM-generic. And that is the reason why I pushed device.h header to asm-generic. It is needed to rename some stuff (e.g... GIC ) in it or add some ifdefs. ~ Oleksii
On 23.10.2023 12:17, Oleksii wrote: > On Thu, 2023-10-19 at 13:12 +0100, Julien Grall wrote: >> Hi, >> >> On 19/10/2023 12:41, Jan Beulich wrote: >>> On 19.10.2023 13:27, Julien Grall wrote: >>>> that doesn't involve one arch to symlink headers from another >>>> arch. >>> >>> Whether to use symlinks or #include "../../arch/..." or yet >>> something else is >>> a matter of mechanics. >> >> #include "../../arch/../" is pretty much in the same category. This >> is >> simply hiding the fact they could be in asm-generic. >> >> Anyway, I have shared my view. Let see what the others thinks. > I have the same point: if something is shared at least between two > arch, it should go to ASM-generic. I continue to disagree: What if one pair of arch-es shares one set of things, and another shares another set? You can't fit both pairs then with a single fallback header (unless of course you make it a big #if / #else / #endif, which I'm inclined to say isn't the goal with headers put in asm-generic/). Jan > And that is the reason why I pushed device.h header to asm-generic. > It is needed to rename some stuff (e.g... GIC ) in it or add some > ifdefs. > > ~ Oleksii
On 23.10.2023 12:12, Oleksii wrote: > On Thu, 2023-10-19 at 11:14 +0200, 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? >> >>> +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. > Yes, but the definition of GIC sounds common, so I decided to leave it. > But it can be changed. > >> >>> + DEVICE_PCI_HOSTBRIDGE, >> >> And this one's PCI-specific. >> >> 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. > At least right now, I am using the same header device.h as in ARM, Are you? I just double checked, and I can't see yours matching theirs. First example of a difference is them having struct dev_archdata. Jan > and > there wasn't a need for a customized version of the header. > > ~ Oleksii >
Hi Jan, On 23/10/2023 11:33, Jan Beulich wrote: > On 23.10.2023 12:17, Oleksii wrote: >> On Thu, 2023-10-19 at 13:12 +0100, Julien Grall wrote: >>> Hi, >>> >>> On 19/10/2023 12:41, Jan Beulich wrote: >>>> On 19.10.2023 13:27, Julien Grall wrote: >>>>> that doesn't involve one arch to symlink headers from another >>>>> arch. >>>> >>>> Whether to use symlinks or #include "../../arch/..." or yet >>>> something else is >>>> a matter of mechanics. >>> >>> #include "../../arch/../" is pretty much in the same category. This >>> is >>> simply hiding the fact they could be in asm-generic. >>> >>> Anyway, I have shared my view. Let see what the others thinks. >> I have the same point: if something is shared at least between two >> arch, it should go to ASM-generic. > > I continue to disagree: What if one pair of arch-es shares one set > of things, and another shares another set? You can't fit both pairs > then with a single fallback header (unless of course you make it a > big #if / #else / #endif, which I'm inclined to say isn't the goal > with headers put in asm-generic/). TBH, I would expect that if RISC-V and Arm are using the same headers, then PPC would likely use it as well. So this would qualify to be in asm-generic/. Now, I don't think we have to resolve the case where we have have two arch using one set of headers and the other another sets. We can cross that line once we have an example. Cheers,
On Mon, 2023-10-23 at 12:35 +0200, Jan Beulich wrote: > On 23.10.2023 12:12, Oleksii wrote: > > On Thu, 2023-10-19 at 11:14 +0200, 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? > > > > > > > +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. > > Yes, but the definition of GIC sounds common, so I decided to leave > > it. > > But it can be changed. > > > > > > > > > + DEVICE_PCI_HOSTBRIDGE, > > > > > > And this one's PCI-specific. > > > > > > 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. > > At least right now, I am using the same header device.h as in ARM, > > Are you? I just double checked, and I can't see yours matching > theirs. > First example of a difference is them having struct dev_archdata. I just tried to commit minimum for now. It is how device.h is looked ( but still I have to align with ARM's version, I used older version of it for some reason I don't remember ) now: I just tried to commit to the minimum for now. It is how device.h looks ( but still, I have to align it with ARM's version. I used an older version of it for some reason I don't remember ) now: https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/include/asm/device.h?ref_type=heads > ~ Oleksii
diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h new file mode 100644 index 0000000000..66e69ecd78 --- /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, +}; + +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, + DEVICE_PCI_HOSTBRIDGE, + /* Use for error */ + DEVICE_UNKNOWN, +}; + +struct device_desc { + /* Device name */ + const char *name; + /* Device class */ + enum device_class class; + /* List of devices supported by this driver */ + const struct dt_device_match *dt_match; + /* + * Device initialization. + * + * -EAGAIN is used to indicate that device probing is deferred. + */ + int (*init)(struct dt_device_node *dev, const void *data); +}; + +typedef struct device device_t; + +#define DT_DEVICE_START(_name, _namestr, _class) \ +static const struct device_desc __dev_desc_##_name __used \ +__section(".dev.info") = { \ + .name = _namestr, \ + .class = _class, \ + +#define DT_DEVICE_END \ +}; + +#endif /* __ASM_GENERIC_DEVICE_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
The patch introduces stub header needed for full Xen build. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/include/asm-generic/device.h | 65 ++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 xen/include/asm-generic/device.h