[PATCH] vfio/pci: Add iowrite64 and ioread64 support for vfio pci

[PATCH] vfio/pci: Add iowrite64 and ioread64 support for vfio pci

[Ramesh Thomas]

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>
 
 /* Special capability IDs predefined access */
 #define PCI_CAP_ID_INVALID		0xFF	/* default raw access */
-- 
2.34.1

Re: [PATCH] vfio/pci: Add iowrite64 and ioread64 support for vfio pci

[Jason Gunthorpe]

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

Re: [PATCH] vfio/pci: Add iowrite64 and ioread64 support for vfio pci

[Ramesh Thomas]

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

Re: [PATCH] vfio/pci: Add iowrite64 and ioread64 support for vfio pci

[Gerd Bayer]

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

Re: [PATCH] vfio/pci: Add iowrite64 and ioread64 support for vfio pci

[Ramesh Thomas]

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
>