virtio changes 5/2016

[ladipro]

ff1ab8b touches common PCI code but only the virtio-net driver is affected. Please let me know if there are any questions or concerns.

Thank you,
Ladi

[ladipro]

Friendly ping.

[ipxe-devel]

On 09/06/16 07:41, Ladi Prosek wrote:

Friendly ping.

ACK. In the post-ARM64-work queue. Thanks for putting this into a git
tree; that's easier for me to handle than e-mails, at least until github
next decides to break their API.

Michael

[ipxe-devel]

On 09/06/16 07:41, Ladi Prosek wrote:

Friendly ping.

I've cherry-picked the two commits that seemed obviously correct to me:

[virtio] Renumber virtio_pci_region flags
[virtio] Fix virtio-pci logging

I don't understand the motivation for the remaining two:

[pci] Add pci_enable_device and pci_restore_device
[virtio] Enable and restore PCI device correctly

If a PCI device has no I/O BAR, then it can simply implement
PCI_COMMAND_IO as hardwired to zero. Similarly for PCI_COMMAND_MEM.

Do virtio devices treat PCI_COMMAND in some special way which requires
this additional (mild) complexity?

Thanks,

Michael

[ladipro]

Thanks - virtio devices may be configured to expose both I/O and memory BARs, with either type being enough to drive the device. It is basically a back-compat setup where older drivers can still use I/O ports and newer may choose to use the so called "modern" interface.

We would like to avoid enabling PCI_COMMAND_IO / PCI_COMMAND_MEM if the driver is not going to use it.

[ipxe-devel]

On 20/06/16 15:37, Ladi Prosek wrote:

Thanks - virtio devices may be configured to expose both I/O and memory
BARs, with either type being enough to drive the device. It is basically
a back-compat setup where older drivers can still use I/O ports and
newer may choose to use the so called "modern" interface.

We would like to avoid enabling PCI_COMMAND_IO / PCI_COMMAND_MEM if the
driver is not going to use it.

Why? What goes wrong if iPXE just enables both? Does this prevent iPXE
from using the modern interface?

Michael

[ipxe-devel]

On Mo, 2016-06-20 at 15:44 +0100, Michael Brown wrote:

On 20/06/16 15:37, Ladi Prosek wrote:

Thanks - virtio devices may be configured to expose both I/O and memory
BARs, with either type being enough to drive the device. It is basically
a back-compat setup where older drivers can still use I/O ports and
newer may choose to use the so called "modern" interface.

We would like to avoid enabling PCI_COMMAND_IO / PCI_COMMAND_MEM if the
driver is not going to use it.

Why?

It is nice to have. It will make sure the driver will only use the
interface it intends to use and doesn't mix IO-based legacy device
access and MMIO-based modern device access by accident.

What goes wrong if iPXE just enables both?

Nothing (in theory).

cheers,
Gerd

[ipxe-devel]

On 20/06/16 16:30, Gerd Hoffmann wrote:

On Mo, 2016-06-20 at 15:44 +0100, Michael Brown wrote:

On 20/06/16 15:37, Ladi Prosek wrote:

Thanks - virtio devices may be configured to expose both I/O and memory
BARs, with either type being enough to drive the device. It is basically
a back-compat setup where older drivers can still use I/O ports and
newer may choose to use the so called "modern" interface.

We would like to avoid enabling PCI_COMMAND_IO / PCI_COMMAND_MEM if the
driver is not going to use it.

Why?

It is nice to have. It will make sure the driver will only use the
interface it intends to use and doesn't mix IO-based legacy device
access and MMIO-based modern device access by accident.

OK. If that kind of safety check is wanted, it should probably be
ensured at compile time, e.g. by having some kind of operations table
instead of scattered "if (is_modern) {}" tests.

I don't think this "nice to have" justifies the code size increase, but
I'll merge it if both you and Ladi disagree with me.

Michael

[ladipro]

I'm fine with dropping this if @marcel-apf doesn't object.

[marcel-apf]

Hi,
We can't hard-wire the PCI_COMMAND_IO to 0 since virtio are(may be) transitional devices that support both IO and MEM. Since the device supports both modes is up to firmware/OS to choose a working mode. Once the mode is chosen it is expected to update the command register accordingly. This is why PCI spec has separate bits for IO/MEM . If we dig into virtio implementation we may find that we don't care if both bits are on and we may also be able to fix any potential problems.

If every byte is important for ipxe you may leave the patches out, consciously creating an "unfriendly" driver (QEMU will create an io-region for that and will have some 'if' statements with politically correct comments: "Some drivers enable IO even if .." :) )

Bottom line, I would add the patches but definitely there are not a must.
I hope I helped,
Marcel

[ipxe-devel]

On 21/06/16 09:49, Marcel Apfelbaum wrote:

We can't hard-wire the PCI_COMMAND_IO to 0 since virtio are(may be)
transitional devices that support both IO and MEM. Since the device
supports both modes is up to firmware/OS to choose a working mode. Once
the mode is chosen it is expected to update the command register
accordingly. This is why PCI spec has separate bits for IO/MEM . If we
dig into virtio implementation we may find that we don't care if both
bits are on and we may also be able to fix any potential problems.

If every byte is important for ipxe you may leave the patches out,
consciously creating an "unfriendly" driver (QEMU will create an
io-region for that and will have some 'if' statements with politically
correct comments: "Some drivers enable IO even if .." :) )

Bottom line, I would add the patches but definitely there are not a must.
I hope I helped,

Thanks for the input!

If the current code would potentially cause problems for the virtio host
side (in current or older implementations), then we should definitely
add the patches to iPXE.

Looking at the current hw/virtio/virtio-pci.c in qemu: it seems to react
to only the PCI_COMMAND_MASTER bit of PCI_COMMAND anyway. Does it
actually make any difference to qemu if we set both the IO and MEM bits?

Thanks,

Michael

[ipxe-devel]

Hi,

Looking at the current hw/virtio/virtio-pci.c in qemu: it seems to react
to only the PCI_COMMAND_MASTER bit of PCI_COMMAND anyway. Does it
actually make any difference to qemu if we set both the IO and MEM bits?

It does, but that happens in the generic pci code as it is the same for
all pci devices. With IO not set the io region for the pci io bar is
not enabled, and qemu can skip the region when searching for a io
handler after trapping an io instruction.

So, in general it is a good idea to leave stuff disabled if you don't
need it as this saves resources. Some cpu cycles in emulation code in
case of qemu, and possibly it reduces power consumption a bit on
physical hardware.

But not doing so should not cause big problems either, especially in
case the software runs for a short amount of time only like ipxe does.

For reference: seabios recently started doing that too with this commit:

https://code.coreboot.org/p/seabios/source/commit/f1b1d76159a8e7091e783bbc296b928226ccf153/

... and switching all drivers over to use the new pci_enable_{io,mem}bar
functions in subsequent patches.

cheers,
Gerd