| Message ID | 20240522232125.548643-1-ramesh.thomas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Add iowrite64 and ioread64 support for vfio pci | expand |
On Wed, May 22, 2024 at 04:21:25PM -0700, Ramesh Thomas wrote: > ioread64 and iowrite64 macros called by vfio pci implementations are > defined in asm/io.h if CONFIG_GENERIC_IOMAP is not defined. Include > linux/io-64-nonatomic-lo-hi.h to define iowrite64 and ioread64 macros > when they are not defined. io-64-nonatomic-lo-hi.h maps the macros to > generic implementation in lib/iomap.c. The generic implementation > does 64 bit rw if readq/writeq is defined for the architecture, > otherwise it would do 32 bit back to back rw. > > Note that there are two versions of the generic implementation that > differs in the order the 32 bit words are written if 64 bit support is > not present. This is not the little/big endian ordering, which is > handled separately. This patch uses the lo followed by hi word ordering > which is consistent with current back to back implementation in the > vfio/pci code. > > Refer patch series the requirement originated from: > https://lore.kernel.org/all/20240522150651.1999584-1-gbayer@linux.ibm.com/ > > Signed-off-by: Ramesh Thomas <ramesh.thomas@intel.com> > --- > drivers/vfio/pci/vfio_pci_priv.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h > index 5e4fa69aee16..5eab5abf2ff2 100644 > --- a/drivers/vfio/pci/vfio_pci_priv.h > +++ b/drivers/vfio/pci/vfio_pci_priv.h > @@ -3,6 +3,7 @@ > #define VFIO_PCI_PRIV_H > > #include <linux/vfio_pci_core.h> > +#include <linux/io-64-nonatomic-lo-hi.h> Why include it here though? It should go in vfio_pci_rdwr.c and this patch should remove all the "#ifdef iowrite64"'s from that file too. But the idea looks right to me Thanks, Jason
Hi Jason, On 5/24/2024 7:00 AM, Jason Gunthorpe wrote: > On Wed, May 22, 2024 at 04:21:25PM -0700, Ramesh Thomas wrote: >> ioread64 and iowrite64 macros called by vfio pci implementations are >> defined in asm/io.h if CONFIG_GENERIC_IOMAP is not defined. Include >> linux/io-64-nonatomic-lo-hi.h to define iowrite64 and ioread64 macros >> when they are not defined. io-64-nonatomic-lo-hi.h maps the macros to >> generic implementation in lib/iomap.c. The generic implementation >> does 64 bit rw if readq/writeq is defined for the architecture, >> otherwise it would do 32 bit back to back rw. >> >> Note that there are two versions of the generic implementation that >> differs in the order the 32 bit words are written if 64 bit support is >> not present. This is not the little/big endian ordering, which is >> handled separately. This patch uses the lo followed by hi word ordering >> which is consistent with current back to back implementation in the >> vfio/pci code. >> >> Refer patch series the requirement originated from: >> https://lore.kernel.org/all/20240522150651.1999584-1-gbayer@linux.ibm.com/ >> >> Signed-off-by: Ramesh Thomas <ramesh.thomas@intel.com> >> --- >> drivers/vfio/pci/vfio_pci_priv.h | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h >> index 5e4fa69aee16..5eab5abf2ff2 100644 >> --- a/drivers/vfio/pci/vfio_pci_priv.h >> +++ b/drivers/vfio/pci/vfio_pci_priv.h >> @@ -3,6 +3,7 @@ >> #define VFIO_PCI_PRIV_H >> >> #include <linux/vfio_pci_core.h> >> +#include <linux/io-64-nonatomic-lo-hi.h> > > Why include it here though? > > It should go in vfio_pci_rdwr.c and this patch should remove all the > "#ifdef iowrite64"'s from that file too. I was trying to make it future proof, but I agree it should be included only where iowrite64/ioread64 is getting called. I will make both the changes. Thanks, Ramesh > > But the idea looks right to me > > Thanks, > Jason
Hi Ramesh, hi Jason, being back from a short vacation, I think I'm sold on enabling x86 for the 64bit accessors in vfio/pci. On Tue, 2024-05-28 at 15:48 -0700, Ramesh Thomas wrote: > Hi Jason, > > > On 5/24/2024 7:00 AM, Jason Gunthorpe wrote: > > On Wed, May 22, 2024 at 04:21:25PM -0700, Ramesh Thomas wrote: > > > ioread64 and iowrite64 macros called by vfio pci implementations > > > are > > > defined in asm/io.h if CONFIG_GENERIC_IOMAP is not defined. > > > Include > > > linux/io-64-nonatomic-lo-hi.h to define iowrite64 and ioread64 > > > macros > > > when they are not defined. io-64-nonatomic-lo-hi.h maps the > > > macros to > > > generic implementation in lib/iomap.c. The generic implementation > > > does 64 bit rw if readq/writeq is defined for the architecture, > > > otherwise it would do 32 bit back to back rw. > > > > > > Note that there are two versions of the generic implementation > > > that > > > differs in the order the 32 bit words are written if 64 bit > > > support is > > > not present. This is not the little/big endian ordering, which is > > > handled separately. This patch uses the lo followed by hi word > > > ordering > > > which is consistent with current back to back implementation in > > > the > > > vfio/pci code. > > > > > > Refer patch series the requirement originated from: > > > https://lore.kernel.org/all/20240522150651.1999584-1-gbayer@linux.ibm.com/ > > > > > > Signed-off-by: Ramesh Thomas <ramesh.thomas@intel.com> > > > --- > > > drivers/vfio/pci/vfio_pci_priv.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/vfio/pci/vfio_pci_priv.h > > > b/drivers/vfio/pci/vfio_pci_priv.h > > > index 5e4fa69aee16..5eab5abf2ff2 100644 > > > --- a/drivers/vfio/pci/vfio_pci_priv.h > > > +++ b/drivers/vfio/pci/vfio_pci_priv.h > > > @@ -3,6 +3,7 @@ > > > #define VFIO_PCI_PRIV_H > > > > > > #include <linux/vfio_pci_core.h> > > > +#include <linux/io-64-nonatomic-lo-hi.h> > > > > Why include it here though? > > > > It should go in vfio_pci_rdwr.c and this patch should remove all > > the "#ifdef iowrite64"'s from that file too. > > I was trying to make it future proof, but I agree it should be > included only where iowrite64/ioread64 is getting called. I will make > both the changes. > > Thanks, > Ramesh > > > > > But the idea looks right to me > > > > Thanks, > > Jason So how should we go about this? To keep the scope that I can test manageable, my proposal would be: I'll post a v5 of my series with the conditional compiles for "ioread64"/"iowrite64" (effectively still excluding x86) - and you Ramesh run this patch (add the include + change #ifdef's) as an explicit patch on top? Thanks, Gerd
On 6/4/2024 4:46 AM, Gerd Bayer wrote: > Hi Ramesh, hi Jason, > > being back from a short vacation, I think I'm sold on enabling x86 for > the 64bit accessors in vfio/pci. > > On Tue, 2024-05-28 at 15:48 -0700, Ramesh Thomas wrote: >> Hi Jason, >> >> >> On 5/24/2024 7:00 AM, Jason Gunthorpe wrote: >>> On Wed, May 22, 2024 at 04:21:25PM -0700, Ramesh Thomas wrote: >>>> ioread64 and iowrite64 macros called by vfio pci implementations >>>> are >>>> defined in asm/io.h if CONFIG_GENERIC_IOMAP is not defined. >>>> Include >>>> linux/io-64-nonatomic-lo-hi.h to define iowrite64 and ioread64 >>>> macros >>>> when they are not defined. io-64-nonatomic-lo-hi.h maps the >>>> macros to >>>> generic implementation in lib/iomap.c. The generic implementation >>>> does 64 bit rw if readq/writeq is defined for the architecture, >>>> otherwise it would do 32 bit back to back rw. >>>> >>>> Note that there are two versions of the generic implementation >>>> that >>>> differs in the order the 32 bit words are written if 64 bit >>>> support is >>>> not present. This is not the little/big endian ordering, which is >>>> handled separately. This patch uses the lo followed by hi word >>>> ordering >>>> which is consistent with current back to back implementation in >>>> the >>>> vfio/pci code. >>>> >>>> Refer patch series the requirement originated from: >>>> https://lore.kernel.org/all/20240522150651.1999584-1-gbayer@linux.ibm.com/ >>>> >>>> Signed-off-by: Ramesh Thomas <ramesh.thomas@intel.com> >>>> --- >>>> drivers/vfio/pci/vfio_pci_priv.h | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/vfio/pci/vfio_pci_priv.h >>>> b/drivers/vfio/pci/vfio_pci_priv.h >>>> index 5e4fa69aee16..5eab5abf2ff2 100644 >>>> --- a/drivers/vfio/pci/vfio_pci_priv.h >>>> +++ b/drivers/vfio/pci/vfio_pci_priv.h >>>> @@ -3,6 +3,7 @@ >>>> #define VFIO_PCI_PRIV_H >>>> >>>> #include <linux/vfio_pci_core.h> >>>> +#include <linux/io-64-nonatomic-lo-hi.h> >>> >>> Why include it here though? >>> >>> It should go in vfio_pci_rdwr.c and this patch should remove all >>> the "#ifdef iowrite64"'s from that file too. >> >> I was trying to make it future proof, but I agree it should be >> included only where iowrite64/ioread64 is getting called. I will make >> both the changes. >> >> Thanks, >> Ramesh >> >>> >>> But the idea looks right to me >>> >>> Thanks, >>> Jason > > So how should we go about this? > To keep the scope that I can test manageable, my proposal would be: > > I'll post a v5 of my series with the conditional compiles for > "ioread64"/"iowrite64" (effectively still excluding x86) - and you > Ramesh run this patch (add the include + change #ifdef's) as an > explicit patch on top? Hi Gerd, Sounds good. Meanwhile I am also trying to get this tested in x86. Thanks, Ramesh > > Thanks, > Gerd >
diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h index 5e4fa69aee16..5eab5abf2ff2 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -3,6 +3,7 @@ #define VFIO_PCI_PRIV_H #include <linux/vfio_pci_core.h> +#include <linux/io-64-nonatomic-lo-hi.h> /* Special capability IDs predefined access */ #define PCI_CAP_ID_INVALID 0xFF /* default raw access */
ioread64 and iowrite64 macros called by vfio pci implementations are defined in asm/io.h if CONFIG_GENERIC_IOMAP is not defined. Include linux/io-64-nonatomic-lo-hi.h to define iowrite64 and ioread64 macros when they are not defined. io-64-nonatomic-lo-hi.h maps the macros to generic implementation in lib/iomap.c. The generic implementation does 64 bit rw if readq/writeq is defined for the architecture, otherwise it would do 32 bit back to back rw. Note that there are two versions of the generic implementation that differs in the order the 32 bit words are written if 64 bit support is not present. This is not the little/big endian ordering, which is handled separately. This patch uses the lo followed by hi word ordering which is consistent with current back to back implementation in the vfio/pci code. Refer patch series the requirement originated from: https://lore.kernel.org/all/20240522150651.1999584-1-gbayer@linux.ibm.com/ Signed-off-by: Ramesh Thomas <ramesh.thomas@intel.com> --- drivers/vfio/pci/vfio_pci_priv.h | 1 + 1 file changed, 1 insertion(+)