Date: Sun, 10 Sep 2023 09:12:01 +0000 From: =?utf-8?Q?Mina_Gali=C4=87?= <freebsd@igalic.co> To: vmaffione@freebsd.org Cc: freebsd-hackers@freebsd.org, freebsd-virtualization@freebsd.org Subject: Re: Question about virtio_alloc_virtqueues Message-ID: <m3DDzb6EGIA3TLedpJuuGRsGyrjvCvsyFcquV7F5hO23QvtjyQo9F-Ekg6Fp154YSgVHi4n_7boHlgUGn0U3IBYcL18PZb_wnX5F_GgI9Qo=@igalic.co> In-Reply-To: <CA%2B_eA9h_AAh95sJ7q1p3J4xCZcd73WWzHkA=BfR6dFYA=%2BG5uw@mail.gmail.com> References: <ia9RXdtQafOfdOfg9wml1ApOEb4ivdf8lbRf74J1XYzA8HIDoEWQ75ScEQY8DpmUHlfMraZmgY0XlgaMWid46UBEIOeCBiOQxsFI4TIoIW4=@igalic.co> <CA%2B_eA9h_AAh95sJ7q1p3J4xCZcd73WWzHkA=BfR6dFYA=%2BG5uw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] that sounds like we can just remove the flags parameter from virtio_alloc_virtqueues() because it doesn't do anything I'm surprised i haven't seen… noticed any warnings about the flags parameter being unused. Kind regards, Mina-------- Original Message -------- On 9 Sept 2023, 11:53, Vincenzo Maffione wrote: > Hi, > Looking at the code, it looks like that comment was written at a time where per-virtqueue MSIX interrupts were not available, and virtio_alloc_virtqueues() API was already designed to receive a flag to indicate that per-virtqueue MSIX interrupts were requested. The code you quoted would likely have been a simple placeholder, to be replaced with something like "flags |= VIRTIO_PERVQ_INTR" once the support had come. It's a very common pattern to start with setting flags = 0, and then bitwise or |= flags depending on the situation. > > However: > > - per-virtqueue MSIX interrupts are now available (e.g. look at vtpci_setup_interrupts()), and > - virtiqueues interrupt setup happens in a separate API, i.e. virtio_setup_intr(), which takes care all the possible cases > > So as far as I can tell that comment is not relevant anymore and can be removed together with the flags. > Unless I'm wrong, ofc. > > Cheers, > Vincenzo > > Il giorno ven 8 set 2023 alle ore 21:05 Mina Galić <freebsd@igalic.co> ha scritto: > >> Hi folks, >> >> for the past two or so weeks, I've been trying to document the >> virtio functions: You can see some of my progress here: >> >> https://codeberg.org/meena/freebsd-src/commits/branch/improve/virtio >> >> I'm currently trying to document virtio_alloc_virtqueues. >> The second argument, flags, is only ever called with 0, and >> never passed on to anything. So I thought I'd remove it. >> However, there *is* this comment in if_vtnet.c: >> >> /* >> * TODO: Enable interrupt binding if this is multiqueue. This will >> * only matter when per-virtqueue MSIX is available. >> */ >> if (sc->vtnet_flags & VTNET_FLAG_MQ) >> flags |= 0; >> >> after which flags are still set to 0. for now?? >> What does this mean? >> >> Kind regards, >> >> Mina Galić [-- Attachment #2 --] that sounds like we can just remove the flags parameter from virtio_alloc_virtqueues() because it doesn't do anything<br><br>I'm surprised i haven't seen… noticed any warnings about the flags parameter being unused.<br><br>Kind regards,<br><br>Mina-------- Original Message --------<br>On 9 Sept 2023, 11:53, Vincenzo Maffione < vmaffione@freebsd.org> wrote:<blockquote class="protonmail_quote"><br><div dir="ltr"><div>Hi,</div><div> Looking at the code, it looks like that comment was written at a time where per-virtqueue MSIX interrupts were not available, and virtio_alloc_virtqueues() API was already designed to receive a flag to indicate that per-virtqueue MSIX interrupts were requested. The code you quoted would likely have been a simple placeholder, to be replaced with something like "flags |= VIRTIO_PERVQ_INTR" once the support had come. It's a very common pattern to start with setting flags = 0, and then bitwise or |= flags depending on the situation.<br></div><div><br></div><div>However:</div><div><ul><li>per-virtqueue MSIX interrupts are now available (e.g. look at vtpci_setup_interrupts()), and</li><li>virtiqueues interrupt setup happens in a separate API, i.e. virtio_setup_intr(), which takes care all the possible cases<br></li></ul><div>So as far as I can tell that comment is not relevant anymore and can be removed together with the flags.</div><div>Unless I'm wrong, ofc.<br></div><div><br></div><div>Cheers,</div><div> Vincenzo<br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Il giorno ven 8 set 2023 alle ore 21:05 Mina Galić <<a href="mailto:freebsd@igalic.co">freebsd@igalic.co</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi folks,<br> <br> for the past two or so weeks, I've been trying to document the<br> virtio functions: You can see some of my progress here:<br> <br> <a href="https://codeberg.org/meena/freebsd-src/commits/branch/improve/virtio" rel="noreferrer" target="_blank">https://codeberg.org/meena/freebsd-src/commits/branch/improve/virtio</a><br>; <br> I'm currently trying to document virtio_alloc_virtqueues.<br> The second argument, flags, is only ever called with 0, and<br> never passed on to anything. So I thought I'd remove it.<br> However, there *is* this comment in if_vtnet.c:<br> <br> /*<br> * TODO: Enable interrupt binding if this is multiqueue. This will<br> * only matter when per-virtqueue MSIX is available.<br> */<br> if (sc->vtnet_flags & VTNET_FLAG_MQ)<br> flags |= 0;<br> <br> <br> after which flags are still set to 0. for now??<br> What does this mean?<br> <br> Kind regards,<br> <br> Mina Galić<br> <br> </blockquote></div> </div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?m3DDzb6EGIA3TLedpJuuGRsGyrjvCvsyFcquV7F5hO23QvtjyQo9F-Ekg6Fp154YSgVHi4n_7boHlgUGn0U3IBYcL18PZb_wnX5F_GgI9Qo=>