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

[Ramesh Thomas <ramesh.thomas@intel.com>]

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
>