Re: [PATCH v5 0/8] virtio-net: add support for SR-IOV emulation

[Akihiko Odaki]

On 2024/07/31 17:58, Cédric Le Goater wrote:
> On 7/30/24 19:56, Michael S. Tsirkin wrote:
> > On Tue, Jul 30, 2024 at 09:26:20PM +0900, Akihiko Odaki wrote:
> > > On 2024/07/30 20:37, Michael S. Tsirkin wrote:
> > > > On Mon, Jul 15, 2024 at 02:19:06PM +0900, Akihiko Odaki wrote:
> > > > > Based-on: <20240714-rombar-v2-0-af1504ef55de@daynix.com>
> > > > > ("[PATCH v2 0/4] hw/pci: Convert rom_bar into OnOffAuto")
> > > >
> > > > OK I will revert this for now. We'll try again after the release,
> > > > there will be time to address s390.
> > >
> > > I'd like to know if anybody wants to use igb on a s390x machine 
> > > configured
> > > with libvirt. Such a configuration is already kind of broken, and it is
> > > likely to require significant effort on both side of libvirt and QEMU 
> > > to fix
> > > it.
> >
> >
> > I assume Cédric wouldn't report it if was unused.
> >
> >
> > > As an alternative, I'm also introducing SR-IOV support to 
> > > virtio-net-pci. It
> > > does not suffer the same problem with igb thanks to its flexible
> > > configuration mechanism.
> > >
> > > Regards,
> > > Akihiko Odaki
> >
> > Sounds like this needs more review time, anyway.
>
> Using an emulated IGB device in a s390x VM is not a common scenario.
> The IGB device is not supported downstream (RHEL on Z). However, the
> change broke the s390x machines I use for upstream QEMU development,
> I removed the IGB device as a fix for now.
>
> The Z PCI device model has 'uid' and 'fid' properties which are set
> by the VMM, and in this case, they are auto-generated, hence the
> conflicting ids with libvirt. This is Z specific but, possibly there
> are subtle use cases on other platforms which could have similar
> consequences. Something to be aware of.
>
>
> Also, and this is why I thought it was important to report. Creating
> PCI devices at machine init time (with -nodefaults) in the back of the
> management layer is a no-no. libvirt requests to have full control
> on the machine topology and this change seems like a violation of
> this rule, even if VFs are kind of special.
>
> Daniel, did I understand correctly the above constraint on the machine
> definition ?

It is problematic even if it is not init time. QEMU shouldn't implicitly 
add a PCI device without letting the management layer know.

For the virtio-net SR-IOV support which I have been preparing, I'm 
avoiding automatically adding PCI devices but instead letting the user 
create VFs. However, fixing existing SR-IOV devices (igb and nvme) will 
require more efforts and possible breaking changes of the command line.

> I can't say if we should revert for 9.1. Again, this Z is specific.
> The changes were introduced to catch errors early when the PF device
> is realized. There is no doubt they are useful. Some rework is needed
> to avoid the conflicting id issue though.

I think it is a good idea to revert these patches for now, and add them 
back along with the virtio-net-pci SR-IOV support when the next merge 
window starts though I don't require to do so. If someone experiences 
this problem in the next merge window, we can advise to use 
virtio-net-pci, or to specify qemu:commandline to workaround it. We can 
start thinking of making a breaking change if neither of them satisfies 
the requirement.

Regards,
Akihiko Odaki