| Message ID | 20220218075727.2737623-2-davidgow@google.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | kunit,um: Fix kunit.py build --alltests | expand |
Am 2022-02-18 um 02:57 schrieb David Gow: > From: Randy Dunlap <rdunlap@infradead.org> > > cpuinfo_x86 and its associated macros are not available under ARCH=um, > even though CONFIG_X86_64 is defined. > > This patch (and discussion) were originally posted here: > https://lkml.org/lkml/2022/1/24/1547 > > This produces the following build errors: > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1556:9: note: in expansion of macro ‘cpu_data’ > return cpu_data(first_cpu_of_numa_node).apicid; > ^~~~~~~~ > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1560:1: error: control reaches end of non-void function [-Werror=return-type] > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c: In function ‘kfd_fill_iolink_info_for_cpu’: > ../arch/um/include/asm/processor-generic.h:103:19: error: called object is not a function or function pointer > #define cpu_data (&boot_cpu_data) > ~^~~~~~~~~~~~~~~ > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1688:27: note: in expansion of macro ‘cpu_data’ > struct cpuinfo_x86 *c = &cpu_data(0); > ^~~~~~~~ > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1691:7: error: dereferencing pointer to incomplete type ‘struct cpuinfo_x86’ > if (c->x86_vendor == X86_VENDOR_AMD) > ^~ > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1691:23: error: ‘X86_VENDOR_AMD’ undeclared (first use in this function); did you mean ‘X86_VENDOR_ANY’? > if (c->x86_vendor == X86_VENDOR_AMD) > ^~~~~~~~~~~~~~ > X86_VENDOR_ANY > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c: In function ‘kfd_create_vcrat_image_cpu’: > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1742:11: warning: unused variable ‘entries’ [-Wunused-variable] > uint32_t entries = 0; > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: David Gow <davidgow@google.com> > --- > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 6 +++--- > drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > index 9624bbe8b501..b1e2d117be3d 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > @@ -1682,7 +1682,7 @@ static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size, > return 0; > } > > -#ifdef CONFIG_X86_64 > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) I don't think it makes sense to compile a hardware device driver in a UML config. Instead of scattering UML #ifdefs through our code, I would recommend adding this to Kconfig: --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -254,7 +254,7 @@ source "drivers/gpu/drm/radeon/Kconfig" config DRM_AMDGPU tristate "AMD GPU" - depends on DRM && PCI && MMU + depends on DRM && PCI && MMU && !UML select FW_LOADER select DRM_KMS_HELPER select DRM_SCHED That would address patch 2 of this series as well. Regards, Felix > static int kfd_fill_iolink_info_for_cpu(int numa_node_id, int *avail_size, > uint32_t *num_entries, > struct crat_subtype_iolink *sub_type_hdr) > @@ -1741,7 +1741,7 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) > struct crat_subtype_generic *sub_type_hdr; > int avail_size = *size; > int numa_node_id; > -#ifdef CONFIG_X86_64 > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) > uint32_t entries = 0; > #endif > int ret = 0; > @@ -1806,7 +1806,7 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) > sub_type_hdr->length); > > /* Fill in Subtype: IO Link */ > -#ifdef CONFIG_X86_64 > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) > ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size, > &entries, > (struct crat_subtype_iolink *)sub_type_hdr); > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c > index 948fbb39336e..b38fc530ffe2 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c > @@ -1552,7 +1552,7 @@ static int kfd_cpumask_to_apic_id(const struct cpumask *cpumask) > first_cpu_of_numa_node = cpumask_first(cpumask); > if (first_cpu_of_numa_node >= nr_cpu_ids) > return -1; > -#ifdef CONFIG_X86_64 > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) > return cpu_data(first_cpu_of_numa_node).apicid; > #else > return first_cpu_of_numa_node;
On Fri, Feb 18, 2022 at 11:39 AM Felix Kuehling <felix.kuehling@amd.com> wrote: > > > Am 2022-02-18 um 02:57 schrieb David Gow: > > From: Randy Dunlap <rdunlap@infradead.org> > > > > cpuinfo_x86 and its associated macros are not available under ARCH=um, > > even though CONFIG_X86_64 is defined. > > > > This patch (and discussion) were originally posted here: > > https://lkml.org/lkml/2022/1/24/1547 > > > > This produces the following build errors: > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1556:9: note: in expansion of macro ‘cpu_data’ > > return cpu_data(first_cpu_of_numa_node).apicid; > > ^~~~~~~~ > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1560:1: error: control reaches end of non-void function [-Werror=return-type] > > > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c: In function ‘kfd_fill_iolink_info_for_cpu’: > > ../arch/um/include/asm/processor-generic.h:103:19: error: called object is not a function or function pointer > > #define cpu_data (&boot_cpu_data) > > ~^~~~~~~~~~~~~~~ > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1688:27: note: in expansion of macro ‘cpu_data’ > > struct cpuinfo_x86 *c = &cpu_data(0); > > ^~~~~~~~ > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1691:7: error: dereferencing pointer to incomplete type ‘struct cpuinfo_x86’ > > if (c->x86_vendor == X86_VENDOR_AMD) > > ^~ > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1691:23: error: ‘X86_VENDOR_AMD’ undeclared (first use in this function); did you mean ‘X86_VENDOR_ANY’? > > if (c->x86_vendor == X86_VENDOR_AMD) > > ^~~~~~~~~~~~~~ > > X86_VENDOR_ANY > > > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c: In function ‘kfd_create_vcrat_image_cpu’: > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1742:11: warning: unused variable ‘entries’ [-Wunused-variable] > > uint32_t entries = 0; > > > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > > Signed-off-by: David Gow <davidgow@google.com> > > --- > > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 6 +++--- > > drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 2 +- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > index 9624bbe8b501..b1e2d117be3d 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > @@ -1682,7 +1682,7 @@ static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size, > > return 0; > > } > > > > -#ifdef CONFIG_X86_64 > > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) > > I don't think it makes sense to compile a hardware device driver in a > UML config. Instead of scattering UML #ifdefs through our code, I would > recommend adding this to Kconfig: > > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -254,7 +254,7 @@ source "drivers/gpu/drm/radeon/Kconfig" > > config DRM_AMDGPU > tristate "AMD GPU" > - depends on DRM && PCI && MMU > + depends on DRM && PCI && MMU && !UML > select FW_LOADER > select DRM_KMS_HELPER > select DRM_SCHED > > That would address patch 2 of this series as well. I agree. Otherwise, we are always going to be chasing these issues. Alex > > Regards, > Felix > > > > static int kfd_fill_iolink_info_for_cpu(int numa_node_id, int *avail_size, > > uint32_t *num_entries, > > struct crat_subtype_iolink *sub_type_hdr) > > @@ -1741,7 +1741,7 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) > > struct crat_subtype_generic *sub_type_hdr; > > int avail_size = *size; > > int numa_node_id; > > -#ifdef CONFIG_X86_64 > > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) > > uint32_t entries = 0; > > #endif > > int ret = 0; > > @@ -1806,7 +1806,7 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) > > sub_type_hdr->length); > > > > /* Fill in Subtype: IO Link */ > > -#ifdef CONFIG_X86_64 > > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) > > ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size, > > &entries, > > (struct crat_subtype_iolink *)sub_type_hdr); > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c > > index 948fbb39336e..b38fc530ffe2 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c > > @@ -1552,7 +1552,7 @@ static int kfd_cpumask_to_apic_id(const struct cpumask *cpumask) > > first_cpu_of_numa_node = cpumask_first(cpumask); > > if (first_cpu_of_numa_node >= nr_cpu_ids) > > return -1; > > -#ifdef CONFIG_X86_64 > > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) > > return cpu_data(first_cpu_of_numa_node).apicid; > > #else > > return first_cpu_of_numa_node;
On Sat, Feb 19, 2022 at 12:39 AM Felix Kuehling <felix.kuehling@amd.com> wrote: > > > Am 2022-02-18 um 02:57 schrieb David Gow: > > From: Randy Dunlap <rdunlap@infradead.org> > > > > cpuinfo_x86 and its associated macros are not available under ARCH=um, > > even though CONFIG_X86_64 is defined. > > > > This patch (and discussion) were originally posted here: > > https://lkml.org/lkml/2022/1/24/1547 > > > > This produces the following build errors: > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1556:9: note: in expansion of macro ‘cpu_data’ > > return cpu_data(first_cpu_of_numa_node).apicid; > > ^~~~~~~~ > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_topology.c:1560:1: error: control reaches end of non-void function [-Werror=return-type] > > > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c: In function ‘kfd_fill_iolink_info_for_cpu’: > > ../arch/um/include/asm/processor-generic.h:103:19: error: called object is not a function or function pointer > > #define cpu_data (&boot_cpu_data) > > ~^~~~~~~~~~~~~~~ > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1688:27: note: in expansion of macro ‘cpu_data’ > > struct cpuinfo_x86 *c = &cpu_data(0); > > ^~~~~~~~ > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1691:7: error: dereferencing pointer to incomplete type ‘struct cpuinfo_x86’ > > if (c->x86_vendor == X86_VENDOR_AMD) > > ^~ > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1691:23: error: ‘X86_VENDOR_AMD’ undeclared (first use in this function); did you mean ‘X86_VENDOR_ANY’? > > if (c->x86_vendor == X86_VENDOR_AMD) > > ^~~~~~~~~~~~~~ > > X86_VENDOR_ANY > > > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c: In function ‘kfd_create_vcrat_image_cpu’: > > ../drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_crat.c:1742:11: warning: unused variable ‘entries’ [-Wunused-variable] > > uint32_t entries = 0; > > > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > > Signed-off-by: David Gow <davidgow@google.com> > > --- > > drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 6 +++--- > > drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 2 +- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > index 9624bbe8b501..b1e2d117be3d 100644 > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c > > @@ -1682,7 +1682,7 @@ static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size, > > return 0; > > } > > > > -#ifdef CONFIG_X86_64 > > +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) > > I don't think it makes sense to compile a hardware device driver in a > UML config. Instead of scattering UML #ifdefs through our code, I would > recommend adding this to Kconfig: > There are cases where I think it could make sense to have a hardware driver under UML, though I agree they're pretty rare. In particular, there's the virtio PCI support in UML, which one could potentially hook up a GPU to. The case I care more about is the ability to run KUnit tests under UML: if amdgpu wanted to have KUnit tests, it could still run them in qemu (or on real hardware), but UML is faster and more convenient, if the code being tested can compile under it. So I have a slight preference personally for fixing this, to unblock those uses. That being said, it's definitely not worth placing a significant burden on you maintaining these things if no-one uses them. And since there doesn't appear to be any such use at the moment[1], I've no strong objection to just disabling this for now (it can always be re-enabled and fixed if it becomes useful later). Cheers, -- David [1] The proposed DRM KUnit tests don't require any actual hardware drivers, as I understand it: https://lore.kernel.org/all/20220117232259.180459-5-michal.winiarski@intel.com/T/
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c index 9624bbe8b501..b1e2d117be3d 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c @@ -1682,7 +1682,7 @@ static int kfd_fill_mem_info_for_cpu(int numa_node_id, int *avail_size, return 0; } -#ifdef CONFIG_X86_64 +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) static int kfd_fill_iolink_info_for_cpu(int numa_node_id, int *avail_size, uint32_t *num_entries, struct crat_subtype_iolink *sub_type_hdr) @@ -1741,7 +1741,7 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) struct crat_subtype_generic *sub_type_hdr; int avail_size = *size; int numa_node_id; -#ifdef CONFIG_X86_64 +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) uint32_t entries = 0; #endif int ret = 0; @@ -1806,7 +1806,7 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size) sub_type_hdr->length); /* Fill in Subtype: IO Link */ -#ifdef CONFIG_X86_64 +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) ret = kfd_fill_iolink_info_for_cpu(numa_node_id, &avail_size, &entries, (struct crat_subtype_iolink *)sub_type_hdr); diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c index 948fbb39336e..b38fc530ffe2 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_topology.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_topology.c @@ -1552,7 +1552,7 @@ static int kfd_cpumask_to_apic_id(const struct cpumask *cpumask) first_cpu_of_numa_node = cpumask_first(cpumask); if (first_cpu_of_numa_node >= nr_cpu_ids) return -1; -#ifdef CONFIG_X86_64 +#if defined(CONFIG_X86_64) && !defined(CONFIG_UML) return cpu_data(first_cpu_of_numa_node).apicid; #else return first_cpu_of_numa_node;