| Message ID | 20240522150651.1999584-3-gbayer@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Support 8-byte PCI loads and stores | expand |
The removal of the check for iowrite64 and ioread64 causes build error because those macros don't get defined anywhere if CONFIG_GENERIC_IOMAP is not defined. However, I do think the removal of the checks is correct. It is better to include linux/io-64-nonatomic-lo-hi.h which define those macros mapping to generic implementations in lib/iomap.c. If the architecture does not implement 64 bit rw functions (readq/writeq), then it does 32 bit back to back. I have sent a patch with the change that includes the above header file. Please review and include in this patch series if ok. Thanks, Ramesh On 5/22/2024 8:06 AM, Gerd Bayer wrote: > From: Ben Segal <bpsegal@us.ibm.com> > > Many PCI adapters can benefit or even require full 64bit read > and write access to their registers. In order to enable work on > user-space drivers for these devices add two new variations > vfio_pci_core_io{read|write}64 of the existing access methods > when the architecture supports 64-bit ioreads and iowrites. > > Signed-off-by: Ben Segal <bpsegal@us.ibm.com> > Co-developed-by: Gerd Bayer <gbayer@linux.ibm.com> > Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com> > --- > drivers/vfio/pci/vfio_pci_rdwr.c | 18 +++++++++++++++++- > include/linux/vfio_pci_core.h | 5 ++++- > 2 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c > index d07bfb0ab892..07351ea76604 100644 > --- a/drivers/vfio/pci/vfio_pci_rdwr.c > +++ b/drivers/vfio/pci/vfio_pci_rdwr.c > @@ -61,7 +61,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_iowrite##size); > VFIO_IOWRITE(8) > VFIO_IOWRITE(16) > VFIO_IOWRITE(32) > -#ifdef iowrite64 > +#ifdef CONFIG_64BIT > VFIO_IOWRITE(64) > #endif > > @@ -89,6 +89,9 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_ioread##size); > VFIO_IOREAD(8) > VFIO_IOREAD(16) > VFIO_IOREAD(32) > +#ifdef CONFIG_64BIT > +VFIO_IOREAD(64) > +#endif > > #define VFIO_IORDWR(size) \ > static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\ > @@ -124,6 +127,10 @@ static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\ > VFIO_IORDWR(8) > VFIO_IORDWR(16) > VFIO_IORDWR(32) > +#if CONFIG_64BIT > +VFIO_IORDWR(64) > +#endif > + > /* > * Read or write from an __iomem region (MMIO or I/O port) with an excluded > * range which is inaccessible. The excluded range drops writes and fills > @@ -148,6 +155,15 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem, > else > fillable = 0; > > +#if CONFIG_64BIT > + if (fillable >= 8 && !(off % 8)) { > + ret = vfio_pci_iordwr64(vdev, iswrite, test_mem, > + io, buf, off, &filled); > + if (ret) > + return ret; > + > + } else > +#endif > if (fillable >= 4 && !(off % 4)) { > ret = vfio_pci_iordwr32(vdev, iswrite, test_mem, > io, buf, off, &filled); > diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h > index a2c8b8bba711..5f9b02d4a3e9 100644 > --- a/include/linux/vfio_pci_core.h > +++ b/include/linux/vfio_pci_core.h > @@ -146,7 +146,7 @@ int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev, \ > VFIO_IOWRITE_DECLATION(8) > VFIO_IOWRITE_DECLATION(16) > VFIO_IOWRITE_DECLATION(32) > -#ifdef iowrite64 > +#ifdef CONFIG_64BIT > VFIO_IOWRITE_DECLATION(64) > #endif > > @@ -157,5 +157,8 @@ int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev, \ > VFIO_IOREAD_DECLATION(8) > VFIO_IOREAD_DECLATION(16) > VFIO_IOREAD_DECLATION(32) > +#ifdef CONFIG_64BIT > +VFIO_IOREAD_DECLATION(64) > +#endif > > #endif /* VFIO_PCI_CORE_H */
Hi Ramesh, On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote: > The removal of the check for iowrite64 and ioread64 causes build > error because those macros don't get defined anywhere if > CONFIG_GENERIC_IOMAP is not defined. However, I do think the removal > of the checks is correct. Wait, I believe it is the other way around. If your config *is* specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide implementations for back-to-back 32bit operations to emulate 64bit accesses - and you have to "select" which of the two types of emulation (hi/lo or lo/hi order) get mapped onto ioread64(be) or iowrite64(be) by including linux/io-64-nonatomic-lo-hi.h (or -hi-lo.h). > It is better to include linux/io-64-nonatomic-lo-hi.h which define > those macros mapping to generic implementations in lib/iomap.c. If > the architecture does not implement 64 bit rw functions > (readq/writeq), then it does 32 bit back to back. I have sent a > patch with the change that includes the above header file. Please > review and include in this patch series if ok. I did find your patch, thank you. I had a very hard time to find a kernel config that actually showed the unresolved symbols situation: Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your patch applied, I could compile successfully. Do you have an easier way steer a kernel config into this dead-end? > Thanks, > Ramesh Frankly, I'd rather not make any assumptions in this rather generic vfio/pci layer about whether hi-lo or lo-hi is the right order to emulate a 64bit access when the base architecture does not support 64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that there's a definitive implementation of ioread64/iowrite64, I'd rather revert to make the conditional compiles depend on those definitions. But maybe Alex has an opinion on this, too? Thanks, Gerd
On Wed, 2024-05-22 at 17:06 +0200, Gerd Bayer wrote: > diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c > b/drivers/vfio/pci/vfio_pci_rdwr.c > index d07bfb0ab892..07351ea76604 100644 > --- a/drivers/vfio/pci/vfio_pci_rdwr.c > +++ b/drivers/vfio/pci/vfio_pci_rdwr.c > @@ -124,6 +127,10 @@ static int vfio_pci_iordwr##size(struct > vfio_pci_core_device *vdev,\ > VFIO_IORDWR(8) > VFIO_IORDWR(16) > VFIO_IORDWR(32) > +#if CONFIG_64BIT During my experimenations to reproduce Ramesh's complaint about unresolved symbols, I found that this should have been #ifdef > +VFIO_IORDWR(64) > +#endif > + > /* > * Read or write from an __iomem region (MMIO or I/O port) with an > excluded > * range which is inaccessible. The excluded range drops writes and > fills > @@ -148,6 +155,15 @@ ssize_t vfio_pci_core_do_io_rw(struct > vfio_pci_core_device *vdev, bool test_mem, > else > fillable = 0; > > +#if CONFIG_64BIT ... as should this have been. > + if (fillable >= 8 && !(off % 8)) { > + ret = vfio_pci_iordwr64(vdev, iswrite, > test_mem, > + io, buf, off, > &filled); > + if (ret) > + return ret; > + > + } else > +#endif > if (fillable >= 4 && !(off % 4)) { > ret = vfio_pci_iordwr32(vdev, iswrite, > test_mem, > io, buf, off, > &filled); > diff --git a/include/linux/vfio_pci_core.h > b/include/linux/vfio_pci_core.h > index a2c8b8bba711..5f9b02d4a3e9 100644 > --- a/include/linux/vfio_pci_core.h > +++ b/include/linux/vfio_pci_core.h > @@ -146,7 +146,7 @@ int vfio_pci_core_iowrite##size(struct > vfio_pci_core_device *vdev, \ > VFIO_IOWRITE_DECLATION(8) > VFIO_IOWRITE_DECLATION(16) > VFIO_IOWRITE_DECLATION(32) > -#ifdef iowrite64 > +#ifdef CONFIG_64BIT > VFIO_IOWRITE_DECLATION(64) > #endif > > @@ -157,5 +157,8 @@ int vfio_pci_core_ioread##size(struct > vfio_pci_core_device *vdev, \ > VFIO_IOREAD_DECLATION(8) > VFIO_IOREAD_DECLATION(16) > VFIO_IOREAD_DECLATION(32) > +#ifdef CONFIG_64BIT > +VFIO_IOREAD_DECLATION(64) > +#endif > > #endif /* VFIO_PCI_CORE_H */ So there will be a v5. Thanks, Gerd
On 5/23/2024 8:01 AM, Gerd Bayer wrote: > Hi Ramesh, > > On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote: >> The removal of the check for iowrite64 and ioread64 causes build >> error because those macros don't get defined anywhere if >> CONFIG_GENERIC_IOMAP is not defined. However, I do think the removal >> of the checks is correct. > > Wait, I believe it is the other way around. If your config *is* > specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide > implementations for back-to-back 32bit operations to emulate 64bit > accesses - and you have to "select" which of the two types of emulation > (hi/lo or lo/hi order) get mapped onto ioread64(be) or iowrite64(be) by > including linux/io-64-nonatomic-lo-hi.h (or -hi-lo.h). Sorry, yes I meant to write they don't get defined anywhere in your code path if CONFIG_GENERIC_IOMAP *is defined*. The only place in your code path where iowrit64 and ioread64 get defined is in asm/io.h. Those definitions are surrounded by #ifndef CONFIG_GENERIC_IOMAP. CONFIG_GENERIC_IOMAP gets defined for x86. > >> It is better to include linux/io-64-nonatomic-lo-hi.h which define >> those macros mapping to generic implementations in lib/iomap.c. If >> the architecture does not implement 64 bit rw functions >> (readq/writeq), then it does 32 bit back to back. I have sent a >> patch with the change that includes the above header file. Please >> review and include in this patch series if ok. > > I did find your patch, thank you. I had a very hard time to find a > kernel config that actually showed the unresolved symbols situation: > Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your > patch applied, I could compile successfully. > Do you have an easier way steer a kernel config into this dead-end? The generic implementation takes care of all conditions. I guess some build bot would report error on build failures. But checks like #ifdef iowrite64 would hide the missing definitions error. > >> Thanks, >> Ramesh > > Frankly, I'd rather not make any assumptions in this rather generic > vfio/pci layer about whether hi-lo or lo-hi is the right order to > emulate a 64bit access when the base architecture does not support > 64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that > there's a definitive implementation of ioread64/iowrite64, I'd rather There is already an assumption of the order in the current implementation regardless e.g. vfio_pci_core_do_io_rw(). If there is no iowrite64 found, the code does back to back 34 bit writes without checking for any particular order requirements. io-64-nonatomic-lo-hi.h and io-64-nonatomic-hi-lo.h would define ioread64/iowrite64 only if they are not already defined in asm/io.h. Also since there is a check for CONFIG_64BIT, most likely a 64 bit readq/writeq will get used in the lib/iomap.c implementations. I think we can pick either lo-hi or hi-lo for the unlikely 32 bit fall through when CONFIG_64BIT is defined. > revert to make the conditional compiles depend on those definitions. > > But maybe Alex has an opinion on this, too? > > Thanks, > Gerd > >
On Thu, 2024-05-23 at 14:47 -0700, Ramesh Thomas wrote: > On 5/23/2024 8:01 AM, Gerd Bayer wrote: > > Hi Ramesh, > > > > On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote: > > > The removal of the check for iowrite64 and ioread64 causes build > > > error because those macros don't get defined anywhere if > > > CONFIG_GENERIC_IOMAP is not defined. However, I do think the > > > removal of the checks is correct. > > > > Wait, I believe it is the other way around. If your config *is* > > specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide > > implementations for back-to-back 32bit operations to emulate 64bit > > accesses - and you have to "select" which of the two types of > > emulation (hi/lo or lo/hi order) get mapped onto ioread64(be) or > > iowrite64(be) by including linux/io-64-nonatomic-lo-hi.h (or -hi- > > lo.h). > > Sorry, yes I meant to write they don't get defined anywhere in your > code path if CONFIG_GENERIC_IOMAP *is defined*. The only place in > your code path where iowrit64 and ioread64 get defined is in > asm/io.h. Those definitions are surrounded by #ifndef > CONFIG_GENERIC_IOMAP. CONFIG_GENERIC_IOMAP gets defined for x86. Now I got it - I think. And I see that plain x86 is aleady affected by this issue. > > > It is better to include linux/io-64-nonatomic-lo-hi.h which > > > define those macros mapping to generic implementations in > > > lib/iomap.c. > > > If the architecture does not implement 64 bit rw functions > > > (readq/writeq), then it does 32 bit back to back. I have sent a > > > patch with the change that includes the above header file. Please > > > review and include in this patch series if ok. > > > > I did find your patch, thank you. I had a very hard time to find a > > kernel config that actually showed the unresolved symbols > > situation: > > Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your > > patch applied, I could compile successfully. > > Do you have an easier way steer a kernel config into this dead-end? > > The generic implementation takes care of all conditions. I guess some > build bot would report error on build failures. But checks like > #ifdef iowrite64 would hide the missing definitions error. Yes definitely, we need to avoid this. > > > > > Thanks, > > > Ramesh > > > > Frankly, I'd rather not make any assumptions in this rather generic > > vfio/pci layer about whether hi-lo or lo-hi is the right order to > > > emulate a 64bit access when the base architecture does not support > > 64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that > > there's a definitive implementation of ioread64/iowrite64, I'd > > rather > > There is already an assumption of the order in the current > implementation regardless e.g. vfio_pci_core_do_io_rw(). If there is > no iowrite64 found, the code does back to back 34 bit writes without > checking for any particular order requirements. > > io-64-nonatomic-lo-hi.h and io-64-nonatomic-hi-lo.h would define > ioread64/iowrite64 only if they are not already defined in asm/io.h. > > Also since there is a check for CONFIG_64BIT, most likely a 64 bit > readq/writeq will get used in the lib/iomap.c implementations. I > think we can pick either lo-hi or hi-lo for the unlikely 32 bit fall > through when CONFIG_64BIT is defined. I dug into lib/iomap.c some more today and I see your point, that it is desireable to make the 64bit accessors useable through vfio/pci when they're implemented in lib/iomap.c. And I follow your argument that in most cases these will map onto readq/writeq - only programmed IO (PIO) has to emulate this with 2 32bit back-to-back accesses. If only the code in lib/iomap.c was structured differently - and made readq/writeq available under ioread64/iowrite64 proper and only fell back to the nonatomic hi-lo or lo-hi emulation with 32bit accesses if PIO is used. As much as I'd like to have it differently, it seems like it was a lengthy process to have that change accepted at the time: https://lore.kernel.org/all/20181106205234.25792-1-logang@deltatee.com/ I'm not sure if we can clean that up, easily. Plus there are appear to be plenty of users of io-64-nonatomic-{lo-hi|-hi-lo}.h in tree already - 103 and 18, resp. > > revert to make the conditional compiles depend on those > > definitions. But maybe Alex has an opinion on this, too? > > > > Thanks, > > Gerd So I'd like to hear from Alex and Tian (who was not a big fan) if we should support 64bit accessors in vfio/pci (primarily) on x86 with this series, or not at all, or split that work off, maybe? Thanks, Gerd
On 5/24/2024 6:42 AM, Gerd Bayer wrote: > On Thu, 2024-05-23 at 14:47 -0700, Ramesh Thomas wrote: >> On 5/23/2024 8:01 AM, Gerd Bayer wrote: >>> Hi Ramesh, >>> >>> On Wed, 2024-05-22 at 16:38 -0700, Ramesh Thomas wrote: >>>> The removal of the check for iowrite64 and ioread64 causes build >>>> error because those macros don't get defined anywhere if >>>> CONFIG_GENERIC_IOMAP is not defined. However, I do think the >>>> removal of the checks is correct. >>> >>> Wait, I believe it is the other way around. If your config *is* >>> specifying CONFIG_GENERIC_IOMAP, lib/iomap.c will provide >>> implementations for back-to-back 32bit operations to emulate 64bit >>> accesses - and you have to "select" which of the two types of >>> emulation (hi/lo or lo/hi order) get mapped onto ioread64(be) or >>> iowrite64(be) by including linux/io-64-nonatomic-lo-hi.h (or -hi- >>> lo.h). >> >> Sorry, yes I meant to write they don't get defined anywhere in your >> code path if CONFIG_GENERIC_IOMAP *is defined*. The only place in >> your code path where iowrit64 and ioread64 get defined is in >> asm/io.h. Those definitions are surrounded by #ifndef >> CONFIG_GENERIC_IOMAP. CONFIG_GENERIC_IOMAP gets defined for x86. > > Now I got it - I think. And I see that plain x86 is aleady affected by > this issue. > >>>> It is better to include linux/io-64-nonatomic-lo-hi.h which >>>> define those macros mapping to generic implementations in >>>> lib/iomap.c. >>>> If the architecture does not implement 64 bit rw functions >>>> (readq/writeq), then it does 32 bit back to back. I have sent a >>>> patch with the change that includes the above header file. Please >>>> review and include in this patch series if ok. >>> >>> I did find your patch, thank you. I had a very hard time to find a >>> kernel config that actually showed the unresolved symbols >>> situation: >>> Some 64bit MIPS config, that relied on GENERIC_IOMAP. And with your >>> patch applied, I could compile successfully. >>> Do you have an easier way steer a kernel config into this dead-end? >> >> The generic implementation takes care of all conditions. I guess some >> build bot would report error on build failures. But checks like >> #ifdef iowrite64 would hide the missing definitions error. > > Yes definitely, we need to avoid this. > >>> >>>> Thanks, >>>> Ramesh >>> >>> Frankly, I'd rather not make any assumptions in this rather generic >>> vfio/pci layer about whether hi-lo or lo-hi is the right order to > >>> emulate a 64bit access when the base architecture does not support >>> 64bit accesses naturally. So, if CONFIG_64BIT is no guarantee that >>> there's a definitive implementation of ioread64/iowrite64, I'd >>> rather >> >> There is already an assumption of the order in the current >> implementation regardless e.g. vfio_pci_core_do_io_rw(). If there is >> no iowrite64 found, the code does back to back 34 bit writes without >> checking for any particular order requirements. >> >> io-64-nonatomic-lo-hi.h and io-64-nonatomic-hi-lo.h would define >> ioread64/iowrite64 only if they are not already defined in asm/io.h. >> >> Also since there is a check for CONFIG_64BIT, most likely a 64 bit >> readq/writeq will get used in the lib/iomap.c implementations. I >> think we can pick either lo-hi or hi-lo for the unlikely 32 bit fall >> through when CONFIG_64BIT is defined. > > I dug into lib/iomap.c some more today and I see your point, that it is > desireable to make the 64bit accessors useable through vfio/pci when > they're implemented in lib/iomap.c. And I follow your argument that in > most cases these will map onto readq/writeq - only programmed IO (PIO) > has to emulate this with 2 32bit back-to-back accesses. > > If only the code in lib/iomap.c was structured differently - and made > readq/writeq available under ioread64/iowrite64 proper and only fell > back to the nonatomic hi-lo or lo-hi emulation with 32bit accesses if > PIO is used. It determines if it is PIO or MMIO based on the address. If the address is >= PIO_RESERVED then it calls readq/writeq. PIO_RESERVED is arch dependent. Looks like if asm/io.h didn't define ioread64/iowrite64 already, then the intention is to use the generic implementation, especially when it defines CONFIG_GENERIC_IOMAP. lib/iomap.c and io-64-nonatomic-{lo-hi|-hi-lo}.h include linux/io.h which includes asm/io.h. > > As much as I'd like to have it differently, it seems like it was a > lengthy process to have that change accepted at the time: > https://lore.kernel.org/all/20181106205234.25792-1-logang@deltatee.com/ > > I'm not sure if we can clean that up, easily. Plus there are appear to > be plenty of users of io-64-nonatomic-{lo-hi|-hi-lo}.h in tree already > - 103 and 18, resp. > >>> revert to make the conditional compiles depend on those >>> definitions. But maybe Alex has an opinion on this, too? >>> >>> Thanks, >>> Gerd > > So I'd like to hear from Alex and Tian (who was not a big fan) if we > should support 64bit accessors in vfio/pci (primarily) on x86 with this > series, or not at all, or split that work off, maybe? > > Thanks, > Gerd >
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c index d07bfb0ab892..07351ea76604 100644 --- a/drivers/vfio/pci/vfio_pci_rdwr.c +++ b/drivers/vfio/pci/vfio_pci_rdwr.c @@ -61,7 +61,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_iowrite##size); VFIO_IOWRITE(8) VFIO_IOWRITE(16) VFIO_IOWRITE(32) -#ifdef iowrite64 +#ifdef CONFIG_64BIT VFIO_IOWRITE(64) #endif @@ -89,6 +89,9 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_ioread##size); VFIO_IOREAD(8) VFIO_IOREAD(16) VFIO_IOREAD(32) +#ifdef CONFIG_64BIT +VFIO_IOREAD(64) +#endif #define VFIO_IORDWR(size) \ static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\ @@ -124,6 +127,10 @@ static int vfio_pci_iordwr##size(struct vfio_pci_core_device *vdev,\ VFIO_IORDWR(8) VFIO_IORDWR(16) VFIO_IORDWR(32) +#if CONFIG_64BIT +VFIO_IORDWR(64) +#endif + /* * Read or write from an __iomem region (MMIO or I/O port) with an excluded * range which is inaccessible. The excluded range drops writes and fills @@ -148,6 +155,15 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem, else fillable = 0; +#if CONFIG_64BIT + if (fillable >= 8 && !(off % 8)) { + ret = vfio_pci_iordwr64(vdev, iswrite, test_mem, + io, buf, off, &filled); + if (ret) + return ret; + + } else +#endif if (fillable >= 4 && !(off % 4)) { ret = vfio_pci_iordwr32(vdev, iswrite, test_mem, io, buf, off, &filled); diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index a2c8b8bba711..5f9b02d4a3e9 100644 --- a/include/linux/vfio_pci_core.h +++ b/include/linux/vfio_pci_core.h @@ -146,7 +146,7 @@ int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev, \ VFIO_IOWRITE_DECLATION(8) VFIO_IOWRITE_DECLATION(16) VFIO_IOWRITE_DECLATION(32) -#ifdef iowrite64 +#ifdef CONFIG_64BIT VFIO_IOWRITE_DECLATION(64) #endif @@ -157,5 +157,8 @@ int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev, \ VFIO_IOREAD_DECLATION(8) VFIO_IOREAD_DECLATION(16) VFIO_IOREAD_DECLATION(32) +#ifdef CONFIG_64BIT +VFIO_IOREAD_DECLATION(64) +#endif #endif /* VFIO_PCI_CORE_H */