On Thu, Mar 26, 2020 at 3:08 PM Jason Wang <jasowang at redhat.com> wrote:> > From: Zhu Lingshan <lingshan.zhu at intel.com> > > This commit introduced two layers to drive IFC VF: > > (1) ifcvf_base layer, which handles IFC VF NIC hardware operations and > configurations. > > (2) ifcvf_main layer, which complies to VDPA bus framework, > implemented device operations for VDPA bus, handles device probe, > bus attaching, vring operations, etc. > > Signed-off-by: Zhu Lingshan <lingshan.zhu at intel.com> > Signed-off-by: Bie Tiwei <tiwei.bie at intel.com> > Signed-off-by: Wang Xiao <xiao.w.wang at intel.com> > Signed-off-by: Jason Wang <jasowang at redhat.com>> + > +#define IFCVF_QUEUE_ALIGNMENT PAGE_SIZE > +#define IFCVF_QUEUE_MAX 32768 > +static u16 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) > +{ > + return IFCVF_QUEUE_ALIGNMENT; > +}This fails to build on arm64 with 64kb page size (found in linux-next): /drivers/vdpa/ifcvf/ifcvf_main.c: In function 'ifcvf_vdpa_get_vq_align': arch/arm64/include/asm/page-def.h:17:20: error: conversion from 'long unsigned int' to 'u16' {aka 'short unsigned int'} changes value from '65536' to '0' [-Werror=overflow] 17 | #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT) | ^ drivers/vdpa/ifcvf/ifcvf_base.h:37:31: note: in expansion of macro 'PAGE_SIZE' 37 | #define IFCVF_QUEUE_ALIGNMENT PAGE_SIZE | ^~~~~~~~~ drivers/vdpa/ifcvf/ifcvf_main.c:231:9: note: in expansion of macro 'IFCVF_QUEUE_ALIGNMENT' 231 | return IFCVF_QUEUE_ALIGNMENT; | ^~~~~~~~~~~~~~~~~~~~~ It's probably good enough to just not allow the driver to be built in that configuration as it's fairly rare but unfortunately there is no simple Kconfig symbol for it. In a similar driver, we did config VMXNET3 tristate "VMware VMXNET3 ethernet driver" depends on PCI && INET depends on !(PAGE_SIZE_64KB || ARM64_64K_PAGES || \ IA64_PAGE_SIZE_64KB || MICROBLAZE_64K_PAGES || \ PARISC_PAGE_SIZE_64KB || PPC_64K_PAGES) I think we should probably make PAGE_SIZE_64KB a global symbol in arch/Kconfig and have it selected by the other symbols so drivers like yours can add a dependency for it. Arnd
On 2020/4/9 ??6:41, Arnd Bergmann wrote:> On Thu, Mar 26, 2020 at 3:08 PM Jason Wang <jasowang at redhat.com> wrote: >> From: Zhu Lingshan <lingshan.zhu at intel.com> >> >> This commit introduced two layers to drive IFC VF: >> >> (1) ifcvf_base layer, which handles IFC VF NIC hardware operations and >> configurations. >> >> (2) ifcvf_main layer, which complies to VDPA bus framework, >> implemented device operations for VDPA bus, handles device probe, >> bus attaching, vring operations, etc. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu at intel.com> >> Signed-off-by: Bie Tiwei <tiwei.bie at intel.com> >> Signed-off-by: Wang Xiao <xiao.w.wang at intel.com> >> Signed-off-by: Jason Wang <jasowang at redhat.com> >> + >> +#define IFCVF_QUEUE_ALIGNMENT PAGE_SIZE >> +#define IFCVF_QUEUE_MAX 32768 >> +static u16 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) >> +{ >> + return IFCVF_QUEUE_ALIGNMENT; >> +} > This fails to build on arm64 with 64kb page size (found in linux-next): > > /drivers/vdpa/ifcvf/ifcvf_main.c: In function 'ifcvf_vdpa_get_vq_align': > arch/arm64/include/asm/page-def.h:17:20: error: conversion from 'long > unsigned int' to 'u16' {aka 'short unsigned int'} changes value from > '65536' to '0' [-Werror=overflow] > 17 | #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT) > | ^ > drivers/vdpa/ifcvf/ifcvf_base.h:37:31: note: in expansion of macro 'PAGE_SIZE' > 37 | #define IFCVF_QUEUE_ALIGNMENT PAGE_SIZE > | ^~~~~~~~~ > drivers/vdpa/ifcvf/ifcvf_main.c:231:9: note: in expansion of macro > 'IFCVF_QUEUE_ALIGNMENT' > 231 | return IFCVF_QUEUE_ALIGNMENT; > | ^~~~~~~~~~~~~~~~~~~~~ > > It's probably good enough to just not allow the driver to be built in that > configuration as it's fairly rare but unfortunately there is no simple Kconfig > symbol for it.Or I think the 64KB alignment is probably more than enough. Ling Shan, can we use smaller value here? Thanks> > In a similar driver, we did > > config VMXNET3 > tristate "VMware VMXNET3 ethernet driver" > depends on PCI && INET > depends on !(PAGE_SIZE_64KB || ARM64_64K_PAGES || \ > IA64_PAGE_SIZE_64KB || MICROBLAZE_64K_PAGES || \ > PARISC_PAGE_SIZE_64KB || PPC_64K_PAGES) > > I think we should probably make PAGE_SIZE_64KB a global symbol > in arch/Kconfig and have it selected by the other symbols so drivers > like yours can add a dependency for it. > > Arnd >
Michael S. Tsirkin
2020-Apr-09 20:25 UTC
[PATCH V9 9/9] virtio: Intel IFC VF driver for VDPA
On Thu, Apr 09, 2020 at 12:41:13PM +0200, Arnd Bergmann wrote:> On Thu, Mar 26, 2020 at 3:08 PM Jason Wang <jasowang at redhat.com> wrote: > > > > From: Zhu Lingshan <lingshan.zhu at intel.com> > > > > This commit introduced two layers to drive IFC VF: > > > > (1) ifcvf_base layer, which handles IFC VF NIC hardware operations and > > configurations. > > > > (2) ifcvf_main layer, which complies to VDPA bus framework, > > implemented device operations for VDPA bus, handles device probe, > > bus attaching, vring operations, etc. > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu at intel.com> > > Signed-off-by: Bie Tiwei <tiwei.bie at intel.com> > > Signed-off-by: Wang Xiao <xiao.w.wang at intel.com> > > Signed-off-by: Jason Wang <jasowang at redhat.com> > > > + > > +#define IFCVF_QUEUE_ALIGNMENT PAGE_SIZE > > +#define IFCVF_QUEUE_MAX 32768 > > +static u16 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) > > +{ > > + return IFCVF_QUEUE_ALIGNMENT; > > +} > > This fails to build on arm64 with 64kb page size (found in linux-next): > > /drivers/vdpa/ifcvf/ifcvf_main.c: In function 'ifcvf_vdpa_get_vq_align': > arch/arm64/include/asm/page-def.h:17:20: error: conversion from 'long > unsigned int' to 'u16' {aka 'short unsigned int'} changes value from > '65536' to '0' [-Werror=overflow] > 17 | #define PAGE_SIZE (_AC(1, UL) << PAGE_SHIFT) > | ^ > drivers/vdpa/ifcvf/ifcvf_base.h:37:31: note: in expansion of macro 'PAGE_SIZE' > 37 | #define IFCVF_QUEUE_ALIGNMENT PAGE_SIZE > | ^~~~~~~~~ > drivers/vdpa/ifcvf/ifcvf_main.c:231:9: note: in expansion of macro > 'IFCVF_QUEUE_ALIGNMENT' > 231 | return IFCVF_QUEUE_ALIGNMENT; > | ^~~~~~~~~~~~~~~~~~~~~ > > It's probably good enough to just not allow the driver to be built in that > configuration as it's fairly rare but unfortunately there is no simple Kconfig > symbol for it. > > In a similar driver, we did > > config VMXNET3 > tristate "VMware VMXNET3 ethernet driver" > depends on PCI && INET > depends on !(PAGE_SIZE_64KB || ARM64_64K_PAGES || \ > IA64_PAGE_SIZE_64KB || MICROBLAZE_64K_PAGES || \ > PARISC_PAGE_SIZE_64KB || PPC_64K_PAGES) > > I think we should probably make PAGE_SIZE_64KB a global symbol > in arch/Kconfig and have it selected by the other symbols so drivers > like yours can add a dependency for it. > > ArndIt's probably easier to make the alignment u32 - I don't really know why it's u16, all callers seem to assign the result to a u32 value.