| Message ID | 1418760534-18163-8-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Jan, On 17/12/2014 10:16, Jan Beulich wrote: >>>> On 16.12.14 at 21:08, <julien.grall@linaro.org> wrote: >> --- a/xen/common/Makefile >> +++ b/xen/common/Makefile >> @@ -2,6 +2,7 @@ obj-y += bitmap.o >> obj-y += core_parking.o >> obj-y += cpu.o >> obj-y += cpupool.o >> +obj-y += device.o > > Shouldn't this instead be two lines, one using HAS_PCI and the second > HAS_DEVICE_TREE? When ARM will gain PCI will, it will fail to compile because device.o is included twice. > >> @@ -75,8 +76,19 @@ struct pci_dev { >> #define PT_FAULT_THRESHOLD 10 >> } fault; >> u64 vf_rlen[6]; >> + >> + struct device dev; >> }; > > I'm not convinced yet that growing this structure (of which we have > quite many instances on some systems) is really worth it, in particular > on x86 where we (so far) only have one device type anyway. Actually this will growing by only sizeof (enum type) on x86. Having a generic way to describe device will really help ARM code (see IOMMU). If we don't have a such thing, we may need to duplicate quite a lots of code. Which will make hard to maintain. >> +#define pci_to_dev(pcidev) (&(pcidev)->dev) >> + >> +static inline struct pci_dev *dev_to_pci(struct device *dev) >> +{ >> + ASSERT(dev->type == DEV_PCI); >> + >> + return container_of(dev, struct pci_dev, dev); >> +} > > While the former is const-correct, I dislike the inability of passing > pointers to const into helper functions like the latter. I can't think > of a good solution other than introducing a second const variant > of it, but I suppose we should try to find alternatives before > adding such a construct that moves us in a direction opposite to > getting our code more const-correct. Oh right. I didn't though about that case. I will turn this inline function into a macro. Regards,
Hi Jan, On 17/12/14 10:46, Jan Beulich wrote: >>>> On 17.12.14 at 11:30, <julien.grall@linaro.org> wrote: >> On 17/12/2014 10:16, Jan Beulich wrote: >>>>>> On 16.12.14 at 21:08, <julien.grall@linaro.org> wrote: >>>> --- a/xen/common/Makefile >>>> +++ b/xen/common/Makefile >>>> @@ -2,6 +2,7 @@ obj-y += bitmap.o >>>> obj-y += core_parking.o >>>> obj-y += cpu.o >>>> obj-y += cpupool.o >>>> +obj-y += device.o >>> >>> Shouldn't this instead be two lines, one using HAS_PCI and the second >>> HAS_DEVICE_TREE? >> >> When ARM will gain PCI will, it will fail to compile because device.o is >> included twice. > > Not necessarily: If we don't do this already, we should eliminate > duplicates from $(obj-y) just like Linux does. I will give a look. >>>> @@ -75,8 +76,19 @@ struct pci_dev { >>>> #define PT_FAULT_THRESHOLD 10 >>>> } fault; >>>> u64 vf_rlen[6]; >>>> + >>>> + struct device dev; >>>> }; >>> >>> I'm not convinced yet that growing this structure (of which we have >>> quite many instances on some systems) is really worth it, in particular >>> on x86 where we (so far) only have one device type anyway. >> >> Actually this will growing by only sizeof (enum type) on x86. > > No, by 8 bytes (due to padding). >> Having a generic way to describe device will really help ARM code (see >> IOMMU). >> >> If we don't have a such thing, we may need to duplicate quite a lots of >> code. Which will make hard to maintain. > > Not really, if e.g. "device" was simply an alias of "pci_dev" on x86. How many pci_dev instance you could have on a platform? 1000? Though it might be a high value but that mean we use 2k more of RAM. It doesn't seem to bad for the benefit to have a clear code. >>>> +#define pci_to_dev(pcidev) (&(pcidev)->dev) >>>> + >>>> +static inline struct pci_dev *dev_to_pci(struct device *dev) >>>> +{ >>>> + ASSERT(dev->type == DEV_PCI); >>>> + >>>> + return container_of(dev, struct pci_dev, dev); >>>> +} >>> >>> While the former is const-correct, I dislike the inability of passing >>> pointers to const into helper functions like the latter. I can't think >>> of a good solution other than introducing a second const variant >>> of it, but I suppose we should try to find alternatives before >>> adding such a construct that moves us in a direction opposite to >>> getting our code more const-correct. >> >> Oh right. I didn't though about that case. I will turn this inline >> function into a macro. > > I'm afraid that won't help, as you still need to specify a type as > 2nd argument to container_of(), and that type can't be both > const and non-const at the same time, i.e. you can't easily > inherit the const-ness of the passed in pointer. I agree that we will drop the const-ness. But is it really an issue? We won't have many place where we don't want to modify the pci_dev. Regards,
Hello Zhang, Please respect the netiquette and avoid lines over 80 characters. On 18/12/2014 01:12, Zhang, Yang Z wrote: > I'd suggest splitting the changes to common code to a separate patch and also CC the VT-d/AMD maintainers. This patch is already common code. Though, there was some changes in arch/arm because of interdependency. Splitting more won't make more sense. > Because I didn't find those definitions when reviewing the 8th patch and I need to search the whole patch set to find them. it's not a new problem. A patch may have a dependency on another patch who has dependency on another one... This would end up to be CCed on every patch and spam your inbox. I usually provide a git repo with my patch series in the cover letter. So if you miss anything you can look to the code and found the relevant patch. Anyway... I will CC next time Sincerely yours,
Hi Jan, On 17/12/2014 17:17, Jan Beulich wrote: >>>> Julien Grall <julien.grall@linaro.org> 12/17/14 2:04 PM >>> >> On 17/12/14 10:46, Jan Beulich wrote: >>>>>> On 17.12.14 at 11:30, <julien.grall@linaro.org> wrote: >>>> Having a generic way to describe device will really help ARM code (see >>>> IOMMU). >>>> >>>> If we don't have a such thing, we may need to duplicate quite a lots of >>>> code. Which will make hard to maintain. >>> >>> Not really, if e.g. "device" was simply an alias of "pci_dev" on x86. >> >> How many pci_dev instance you could have on a platform? 1000? Though it >> might be a high value but that mean we use 2k more of RAM. > > Sure the total amount isn't big. But these days everyone thinks that way, and > data size gets grown without much consideration. And you shouldn't just think > about RAM cache utilization. I will go ahead with the aliasing. >> It doesn't seem to bad for the benefit to have a clear code. > > Aliasing device and pci_dev for x86 would yield similar clarity afaict. To be sure, by aliasing you mean creating a typedef? For x86: typedef struct pci_dev device_t; And for ARM: typedef struct device device_t; > >>>>>> +#define pci_to_dev(pcidev) (&(pcidev)->dev) >>>>>> + >>>>>> +static inline struct pci_dev *dev_to_pci(struct device *dev) >>>>>> +{ >>>>>> + ASSERT(dev->type == DEV_PCI); >>>>>> + >>>>>> + return container_of(dev, struct pci_dev, dev); >>>>>> +} >>>>> >>>>> While the former is const-correct, I dislike the inability of passing >>>>> pointers to const into helper functions like the latter. I can't think >>>>> of a good solution other than introducing a second const variant >>>>> of it, but I suppose we should try to find alternatives before >>>>> adding such a construct that moves us in a direction opposite to >>>>> getting our code more const-correct. >>>> >>>> Oh right. I didn't though about that case. I will turn this inline >>>> function into a macro. >>> >>> I'm afraid that won't help, as you still need to specify a type as >>> 2nd argument to container_of(), and that type can't be both >>> const and non-const at the same time, i.e. you can't easily >>> inherit the const-ness of the passed in pointer. >> >> I agree that we will drop the const-ness. But is it really an issue? >> >> We won't have many place where we don't want to modify the pci_dev. > > Did you check (including places where const could be added)? But at least > you didn't have to drop and const-s, so I'm not heavily objecting the change > you propose. The only usage will be in the IOMMU code where most of the time we require a non const version. At least on the SMMU driver, we have to store data per-device. This is to know what is the SMMU master for this device.
On 18/12/2014 16:02, Jan Beulich wrote: >>>> On 18.12.14 at 16:56, <julien.grall@linaro.org> wrote: >> On 17/12/2014 17:17, Jan Beulich wrote: >>> Aliasing device and pci_dev for x86 would yield similar clarity afaict. >> >> To be sure, by aliasing you mean creating a typedef? >> >> For x86: >> typedef struct pci_dev device_t; >> >> And for ARM: >> typedef struct device device_t; > > Yes, I think that's the only reasonable thing. Using a #define would > seem ugly no matter which direction you did it. Right. We have some place where device and pci_device are used as variable. It would have introduced some strange compilation error. I will go ahead with this solution. Regards,
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c index 693b9af..de702ff 100644 --- a/xen/arch/arm/device.c +++ b/xen/arch/arm/device.c @@ -17,7 +17,7 @@ * GNU General Public License for more details. */ -#include <asm/device.h> +#include <xen/device.h> #include <xen/errno.h> #include <xen/lib.h> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index de180d8..b701a2f 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -9,10 +9,10 @@ #include <asm/regs.h> #include <xen/errno.h> #include <xen/device_tree.h> +#include <xen/device.h> #include <xen/libfdt/libfdt.h> #include <xen/guest_access.h> #include <xen/iocap.h> -#include <asm/device.h> #include <asm/setup.h> #include <asm/platform.h> #include <asm/psci.h> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index f149e09..048350b 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -27,12 +27,11 @@ #include <xen/softirq.h> #include <xen/list.h> #include <xen/device_tree.h> +#include <xen/device.h> #include <xen/libfdt/libfdt.h> #include <asm/p2m.h> #include <asm/domain.h> #include <asm/platform.h> -#include <asm/device.h> - #include <asm/io.h> #include <asm/gic.h> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 076aa62..c6d1876 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -31,12 +31,12 @@ #include <xen/errno.h> #include <xen/delay.h> #include <xen/device_tree.h> +#include <xen/device.h> #include <xen/sizes.h> #include <xen/libfdt/libfdt.h> #include <asm/p2m.h> #include <asm/domain.h> #include <asm/io.h> -#include <asm/device.h> #include <asm/gic.h> #include <asm/gic_v3_defs.h> #include <asm/cpufeature.h> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index e7a1af5..d1ab6b5 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -27,10 +27,10 @@ #include <xen/softirq.h> #include <xen/list.h> #include <xen/device_tree.h> +#include <xen/device.h> #include <asm/p2m.h> #include <asm/domain.h> #include <asm/platform.h> -#include <asm/device.h> #include <asm/io.h> #include <asm/gic.h> #include <asm/vgic.h> diff --git a/xen/common/Makefile b/xen/common/Makefile index 8391246..03ed719 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -2,6 +2,7 @@ obj-y += bitmap.o obj-y += core_parking.o obj-y += cpu.o obj-y += cpupool.o +obj-y += device.o obj-$(HAS_DEVICE_TREE) += device_tree.o obj-y += domctl.o obj-y += domain.o diff --git a/xen/common/device.c b/xen/common/device.c new file mode 100644 index 0000000..3450f20 --- /dev/null +++ b/xen/common/device.c @@ -0,0 +1,21 @@ +#include <xen/types.h> +#include <xen/device.h> + +void device_initialize(struct device *dev, enum device_type type) +{ + dev->type = type; + +#ifdef HAS_DEVICE_TREE + if ( type == DEV_DT ) + dev->of_node = dev_to_dt(dev); +#endif +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 34a1b9e..f471008 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -1450,6 +1450,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt, prev_pp = &pp->next; #endif np->name = pp->value; + device_initialize(&np->dev, DEV_DT); memcpy(pp->value, ps, sz - 1); ((char *)pp->value)[sz - 1] = 0; dt_dprintk("fixed up name for %s -> %s\n", pathp, diff --git a/xen/drivers/char/dt-uart.c b/xen/drivers/char/dt-uart.c index fa92b5c..01eced1 100644 --- a/xen/drivers/char/dt-uart.c +++ b/xen/drivers/char/dt-uart.c @@ -17,11 +17,11 @@ * GNU General Public License for more details. */ -#include <asm/device.h> -#include <asm/types.h> +#include <xen/device.h> #include <xen/console.h> #include <xen/device_tree.h> #include <xen/serial.h> +#include <asm/types.h> /* * Configure UART port with a string: diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c index cba8729..75246e1 100644 --- a/xen/drivers/char/exynos4210-uart.c +++ b/xen/drivers/char/exynos4210-uart.c @@ -24,7 +24,7 @@ #include <xen/init.h> #include <xen/irq.h> #include <xen/mm.h> -#include <asm/device.h> +#include <xen/device.h> #include <asm/exynos4210-uart.h> #include <asm/io.h> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index 161b251..6df3f95 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -25,7 +25,7 @@ #include <xen/vmap.h> #include <asm/io.h> #ifdef HAS_DEVICE_TREE -#include <asm/device.h> +#include <xen/device.h> #endif #ifdef CONFIG_X86 #include <asm/fixmap.h> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c index 16d1454..c4cd442 100644 --- a/xen/drivers/char/omap-uart.c +++ b/xen/drivers/char/omap-uart.c @@ -16,7 +16,7 @@ #include <xen/init.h> #include <xen/irq.h> #include <xen/device_tree.h> -#include <asm/device.h> +#include <xen/device.h> #include <xen/errno.h> #include <xen/mm.h> #include <xen/vmap.h> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index dd19ce8..cc91224 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -23,8 +23,8 @@ #include <xen/init.h> #include <xen/irq.h> #include <xen/device_tree.h> +#include <xen/device.h> #include <xen/errno.h> -#include <asm/device.h> #include <xen/mm.h> #include <xen/vmap.h> #include <asm/pl011-uart.h> diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c index 3007b99..3e9303a 100644 --- a/xen/drivers/passthrough/arm/iommu.c +++ b/xen/drivers/passthrough/arm/iommu.c @@ -18,7 +18,7 @@ #include <xen/lib.h> #include <xen/iommu.h> #include <xen/device_tree.h> -#include <asm/device.h> +#include <xen/device.h> static const struct iommu_ops *iommu_ops; diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 78c6977..9fbd2a2 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -278,6 +278,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) if ( !pdev ) return NULL; + device_initialize(&pdev->dev, DEV_PCI); + *(u16*) &pdev->seg = pseg->nr; *((u8*) &pdev->bus) = bus; *((u8*) &pdev->devfn) = devfn; diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h index 72a9028..fdcd097 100644 --- a/xen/include/asm-arm/device.h +++ b/xen/include/asm-arm/device.h @@ -2,7 +2,8 @@ #define __ASM_ARM_DEVICE_H #include <xen/init.h> -#include <xen/device_tree.h> + +struct dt_device_node; enum device_match { diff --git a/xen/include/asm-x86/device.h b/xen/include/asm-x86/device.h new file mode 100644 index 0000000..9d4c352 --- /dev/null +++ b/xen/include/asm-x86/device.h @@ -0,0 +1,17 @@ +#ifndef __ASM_X86_DEVICE_H +#define __ASM_X86_DEVICE_H + +struct dev_archdata { + /* No device specific arch data */ +}; + +#endif /* __ASM_X86_DEVICE_H */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/xen/device.h b/xen/include/xen/device.h new file mode 100644 index 0000000..a9e20ad --- /dev/null +++ b/xen/include/xen/device.h @@ -0,0 +1,40 @@ +#ifndef __XEN_DEVICE_H__ +#define __XEN_DEVICE_H__ + +#include <asm/device.h> + +enum device_type +{ + DEV_PCI, + DEV_DT, +}; + +/* struct device - The basic device structure */ +struct device +{ + enum device_type type; +#ifdef HAS_DEVICE_TREE + struct dt_device_node *of_node; /* Used by drivers imported from Linux */ +#endif + struct dev_archdata archdata; +}; + +#define dev_is_pci(dev) ((dev)->type == DEV_PCI) +#define dev_is_dt(dev) ((dev->type == DEV_DT) + +#ifdef HAS_DEVICE_TREE +#include <xen/device_tree.h> +#endif + +void device_initialize(struct device *dev, enum device_type type); + +#endif /* __XEN_DEVICE_H__ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index 6502369..890d356 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -12,6 +12,8 @@ #include <asm/byteorder.h> #include <public/xen.h> +#include <xen/device.h> +#include <xen/kernel.h> #include <xen/init.h> #include <xen/string.h> #include <xen/types.h> @@ -80,8 +82,19 @@ struct dt_device_node { /* IOMMU specific fields */ bool is_protected; struct list_head domain_list; + + struct device dev; }; +#define dt_to_dev(dt_node) (&(dt_node)->dev) + +static inline struct dt_device_node *dev_to_dt(struct device *dev) +{ + ASSERT(dev->type == DEV_DT); + + return container_of(dev, struct dt_device_node, dev); +} + #define MAX_PHANDLE_ARGS 16 struct dt_phandle_args { struct dt_device_node *np; diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 5f295f3..6ace79d 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -13,6 +13,7 @@ #include <xen/irq.h> #include <xen/pci_regs.h> #include <xen/pfn.h> +#include <xen/device.h> #include <asm/pci.h> /* @@ -75,8 +76,19 @@ struct pci_dev { #define PT_FAULT_THRESHOLD 10 } fault; u64 vf_rlen[6]; + + struct device dev; }; +#define pci_to_dev(pcidev) (&(pcidev)->dev) + +static inline struct pci_dev *dev_to_pci(struct device *dev) +{ + ASSERT(dev->type == DEV_PCI); + + return container_of(dev, struct pci_dev, dev); +} + #define for_each_pdev(domain, pdev) \ list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
Currently, Xen is supporting PCI and Platform device (based on Device Tree). While we don't support both at the same time: platform device for ARM and PCI for x86, ARM will gain support on PCI soon. Some drivers, such as IOMMU drivers, may handle PCI and platform device in the same way. Only few lines of code differs. Rather than requesting to provide 2 set of functions (one for PCI and one for platform device), introduce a generic structure "device" which is embedded in each specialized device. At the same time replace any use of asm/device.h by xen/device.h. This is required to be able to compile ARM correctly. Signed-off-by: Julien Grall <julien.grall@linaro.org> CC: Jan Beulich <jbeulich@suse.com> CC: Keir Fraser <keir@xen.org> --- xen/arch/arm/device.c | 2 +- xen/arch/arm/domain_build.c | 2 +- xen/arch/arm/gic-v2.c | 3 +-- xen/arch/arm/gic-v3.c | 2 +- xen/arch/arm/gic.c | 2 +- xen/common/Makefile | 1 + xen/common/device.c | 21 +++++++++++++++++++ xen/common/device_tree.c | 1 + xen/drivers/char/dt-uart.c | 4 ++-- xen/drivers/char/exynos4210-uart.c | 2 +- xen/drivers/char/ns16550.c | 2 +- xen/drivers/char/omap-uart.c | 2 +- xen/drivers/char/pl011.c | 2 +- xen/drivers/passthrough/arm/iommu.c | 2 +- xen/drivers/passthrough/pci.c | 2 ++ xen/include/asm-arm/device.h | 3 ++- xen/include/asm-x86/device.h | 17 ++++++++++++++++ xen/include/xen/device.h | 40 +++++++++++++++++++++++++++++++++++++ xen/include/xen/device_tree.h | 13 ++++++++++++ xen/include/xen/pci.h | 12 +++++++++++ 20 files changed, 121 insertions(+), 14 deletions(-) create mode 100644 xen/common/device.c create mode 100644 xen/include/asm-x86/device.h create mode 100644 xen/include/xen/device.h