Re: [PATCH v4 2/3] vfio/pci: Support 8-byte PCI loads and stores

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

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
>