On Sat, 1 Jul 2023 16:28:30 +0900 Akihiko Odaki <[email protected]> wrote:
> On 2023/07/01 0:29, Michael S. Tsirkin wrote: > > On Fri, Jun 30, 2023 at 08:36:38PM +0900, Akihiko Odaki wrote: > >> On 2023/06/30 19:37, Ani Sinha wrote: > >>> > >>> > >>>> On 30-Jun-2023, at 3:30 PM, Michael S. Tsirkin <[email protected]> wrote: > >>>> > >>>> On Fri, Jun 30, 2023 at 02:52:52PM +0530, Ani Sinha wrote: > >>>>> > >>>>> > >>>>>> On 30-Jun-2023, at 2:13 PM, Michael S. Tsirkin <[email protected]> wrote: > >>>>>> > >>>>>> On Fri, Jun 30, 2023 at 02:06:59PM +0530, Ani Sinha wrote: > >>>>>>> > >>>>>>> > >>>>>>>> On 30-Jun-2023, at 2:02 PM, Michael S. Tsirkin <[email protected]> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>> On Fri, Jun 30, 2023 at 01:11:33PM +0530, Ani Sinha wrote: > >>>>>>>>>> > >>>>>>>>>> Thus the check for unoccupied function 0 needs to use pci_is_vf() > >>>>>>>>>> instead of checking ARI capability, and that can happen in > >>>>>>>>>> do_pci_register_device(). > >>>>>>>>>> > >>>>>>>>>>> Also where do you propose we move the check? > >>>>>>>>>> > >>>>>>>>>> In pci_qdev_realize(), somewhere after pc->realize() and before > >>>>>>>>>> option ROM loading. > >>>>>>>>> > >>>>>>>>> Hmm, I tried this. The issue here is something like this would be > >>>>>>>>> now allowed since the PF has ARI capability: > >>>>>>>>> > >>>>>>>>> -device pcie-root-port,id=p -device igb,bus=p,addr=0x2.0x0 > >>>>>>>>> > >>>>>>>>> The above should not be allowed and when used, we do not see the > >>>>>>>>> igb ethernet device from the guest OS. > >>>>>>>> > >>>>>>>> I think it's allowed because it expects you to hotplug function 0 > >>>>>>>> later, > >>>>>>> > >>>>>>> This is about the igb device being plugged into the non-zero slot of > >>>>>>> the pci-root-port. The guest OS ignores it. > >>>>>> > >>>>>> yes but if you later add a device with ARI and with next field pointing > >>>>>> slot 2 guest will suddently find both. > >>>>> > >>>>> Hmm, I tried this: > >>>>> > >>>>> -device pcie-root-port,id=p \ > >>>>> -device igb,bus=p,addr=0x2.0x0 \ > >>>>> -device igb,bus=p,addr=0x0.0x0 \ > >>>>> > >>>>> The guest only found the second igb device not the first. You can try > >>>>> too. > >>>> > >>>> Because next parameter in pcie_ari_init does not match. > >>> > >>> OK send me a command line that I can test it with. I can’t come up with a > >>> case that actually works in practice. > >> > >> I don't think there is one because the code for PCI multifunction does not > >> care ARI. In my opinion, we need yet another check to make non-SR-IOV > >> multifunction and ARI capability mutually exclusive; if a function has the > >> ARI capability and it is not a VF, an attempt to assign non-zero function > >> number for it should fail. is it stated somewhere in spec(s) that ARI and !SR-IOV are mutually exclusive? > > > > Why is that? My understanding is that ARI capable devices should also > > set the multifunction bit in the header. It's not terribly clear from > > the spec though. > > Something like the following will not work properly with ARI-capable > device (think of a as an ARI-capable device): > -device a,addr=0x1.0x0,multifunction=on -device a,addr=0x1.0x1 (I had a crazy idea, to use it like that so we could put more devices on port without resorting to adding extra bridges) Can you elaborate some more why it won't work? > This is because the next function numbers advertised with ARI are not > updated with the multifunction configuration, but they are hardcoded in > the device implementation. In this sense, the traditional (non-SR/IOV) > multifunction mechanism QEMU has will not work with ARI-capable devices. > > > > >> But it should be a distinct check as it will need to check the function > >> number bits. > >> > >>> > >>>> > >>>> > >>>>>> > >>>>>>>> no? > >>>>>>>> > >>>>>>>> I am quite worried about all this work going into blocking > >>>>>>>> what we think is disallowed configurations. We should have > >>>>>>>> maybe blocked them originally, but now that we didn't > >>>>>>>> there's a non zero chance of regressions, > >>>>>>> > >>>>>>> Sigh, > >>>>>> > >>>>>> There's value in patches 1-4 I think - the last patch helped you find > >>>>>> these. so there's value in this work. > >>>>>> > >>>>>>> no medals here for being brave :-) > >>>>>> > >>>>>> Try removing support for a 3.5mm jack next. Oh wait ... > >>>>> > >>>>> Indeed. Everyone uses bluetooth these days. I for one is happy that the > >>>>> jack is gone (and they were bold enough to do it while Samsung and > >>>>> others still carry the useless port ) :-) > >> > >> Hello from a guy using a shiny M2 Macbook Air carrying the legacy jack with > >> a 100-yen earphone. Even people who ported Linux to this machine spent > >> efforts to get the jack to work on Linux ;) > >> > >>>>> > >>>>>> > >>>>>>>> and the benefit > >>>>>>>> is not guaranteed. > >>>>>>>> > >>>>>>>> -- > >>>>>>>> MST > >>> > > >
