Re: [PULL 00/32] VFIO updates 2020-10-26 (for QEMU 5.2 soft-freeze)

[Max Reitz]

On 28.10.20 03:00, Alex Williamson wrote:
> On Tue, 27 Oct 2020 23:42:57 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
>> On Mon, 26 Oct 2020 at 19:39, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>>> ----------------------------------------------------------------
>>> VFIO update 2020-10-26
>>>
>>>  * Migration support (Kirti Wankhede)
>>>  * s390 DMA limiting (Matthew Rosato)
>>>  * zPCI hardware info (Matthew Rosato)
>>>  * Lock guard (Amey Narkhede)
>>>  * Print fixes (Zhengui li)  
>>
>> I get a conflict here in
>> include/standard-headers/linux/fuse.h:
>>
>> ++<<<<<<< HEAD
>>  +#define FUSE_ATTR_FLAGS               (1 << 27)
>> ++=======
>> + #define FUSE_SUBMOUNTS                (1 << 27)
>> ++>>>>>>> remotes/awilliam/tags/vfio-update-20201026.0  

Oh no.

>> I assume these should not both be trying to use the same value,
>> so something has gone wrong somewhere. The conflicting commit
>> now in master is Max's 97d741cc96dd08 ("linux/fuse.h: Pull in from Linux").
>>
>> Can you sort out the correct resolution between you, please?
>> (My guess is that Max's commit is the erroneous one because
>> it doesn't look like it was created via a standard update
>> from the kernel headers.)

It is the erroneous one, because it was based on an earlier version of
the kernel series.

> So as near as I can tell, QEMU commit 97d741cc96dd ("linux/fuse.h: Pull
> in from Linux") is fantasy land.  The only thing I can find of this
> FUSE_ATTR_FLAGS outside Max's QEMU series is this[1] posting where the
> fuse maintainer announces that he's replaced FUSE_ATTR_FLAGS with
> FUSE_SUBMOUNTS, but the usage is "slightly different".  Reading that
> thread, it seems that virtiofsd probably needed an update but I can't
> see that it ever happened.

No, it didn't happen yet.  The series should have got a v2.

As an alternative to reverting, I could try to fix it up on top, but I
don't think that's really preferable.  So I would vote for reverting.

> I'm not comfortable trying to update Max's series to try to determine
> if FUSE_SUBMOUNTS can be interchanged with FUSE_ATTR_FLAGS, where the
> latter appears to be used to express the new field in struct fuse_attr
> exists, but the former appears to be a feature.  My guess would be that
> maybe FUSE_KERNEL_MINOR_VERSION needs to be tested instead for this new
> field??

It can't be interchanged 1:1.  The series should be updated, but not
with such a hack as using some other indicator to see whether the flag
is there, but with properly using FUSE_SUBMOUNTS.

(I suppose technically it's OK for the virtiofsd side to interpret
FUSE_SUBMOUNTS as meaning the field to be present, because
FUSE_SUBMOUNTS does imply that.  But I wouldn't want to test that
hypothesis, and instead just write a clean v2.)

> Anyway, I hate to pull the big hammer, but I think Max's series is
> bogus.  The only thing I can propose is to revert it in its entirety,
> after which this series applies cleanly.  I'll post a patch to do that
> as I think the code currently in master is on pretty shaky ground with
> respect to interpreting flag bits differently from those the kernel
> defines.

Sounds, well, not good, but definitely reasonable.

Thanks!

Max