Re: [PATCH v10 12/17] vpci/header: emulate PCI_COMMAND register for guests

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Dec 01, 2023 at 02:05:54AM +0000, Volodymyr Babchuk wrote:
> 
> Hi Roger, Stewart,
> 
> Roger Pau Monné <roger.pau@xxxxxxxxxx> writes:
> 
> > On Thu, Oct 12, 2023 at 10:09:18PM +0000, Volodymyr Babchuk wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >> 
> >> Xen and/or Dom0 may have put values in PCI_COMMAND which they expect
> >> to remain unaltered. PCI_COMMAND_SERR bit is a good example: while the
> >> guest's view of this will want to be zero initially, the host having set
> >> it to 1 may not easily be overwritten with 0, or else we'd effectively
> >> imply giving the guest control of the bit. Thus, PCI_COMMAND register needs
> >> proper emulation in order to honor host's settings.
> >> 
> >> According to "PCI LOCAL BUS SPECIFICATION, REV. 3.0", section "6.2.2
> >> Device Control" the reset state of the command register is typically 0,
> >> so when assigning a PCI device use 0 as the initial state for the guest's 
> >> view
> >> of the command register.
> >> 
> >> Here is the full list of command register bits with notes about
> >> emulation, along with QEMU behavior in the same situation:
> >> 
> >> PCI_COMMAND_IO - QEMU does not allow a guest to change value of this bit
> >> in real device. Instead it is always set to 1. A guest can write to this
> >> register, but writes are ignored.
> >> 
> >> PCI_COMMAND_MEMORY - QEMU behaves exactly as with PCI_COMMAND_IO. In
> >> Xen case, we handle writes to this bit by mapping/unmapping BAR
> >> regions. For devices assigned to DomUs, memory decoding will be
> >> disabled and the initialization.
> >> 
> >> PCI_COMMAND_MASTER - Allow guest to control it. QEMU passes through
> >> writes to this bit.
> >> 
> >> PCI_COMMAND_SPECIAL - Guest can generate special cycles only if it has
> >> access to host bridge that supports software generation of special
> >> cycles. In our case guest has no access to host bridges at all. Value
> >> after reset is 0. QEMU passes through writes of this bit, we will do
> >> the same.
> >> 
> >> PCI_COMMAND_INVALIDATE - Allows "Memory Write and Invalidate" commands
> >> to be generated. It requires additional configuration via Cacheline
> >> Size register. We are not emulating this register right now and we
> >> can't expect guest to properly configure it. QEMU "emulates" access to
> >> Cachline Size register by ignoring all writes to it. QEMU passes through
> >> writes of PCI_COMMAND_INVALIDATE bit, we will do the same.
> >> 
> >> PCI_COMMAND_VGA_PALETTE - Enable VGA palette snooping. QEMU passes
> >> through writes of this bit, we will do the same.
> >> 
> >> PCI_COMMAND_PARITY - Controls how device response to parity
> >> errors. QEMU ignores writes to this bit, we will do the same.
> >> 
> >> PCI_COMMAND_WAIT - Reserved. Should be 0, but QEMU passes
> >> through writes of this bit, so we will do the same.
> >> 
> >> PCI_COMMAND_SERR - Controls if device can assert SERR. QEMU ignores
> >> writes to this bit, we will do the same.
> >> 
> >> PCI_COMMAND_FAST_BACK - Optional bit that allows fast back-to-back
> >> transactions. It is configured by firmware, so we don't want guest to
> >> control it. QEMU ignores writes to this bit, we will do the same.
> >> 
> >> PCI_COMMAND_INTX_DISABLE - Disables INTx signals. If MSI(X) is
> >> enabled, device is prohibited from asserting INTx as per
> >> specification. Value after reset is 0. In QEMU case, it checks of INTx
> >> was mapped for a device. If it is not, then guest can't control
> >> PCI_COMMAND_INTX_DISABLE bit. In our case, we prohibit a guest to
> >> change value of this bit if MSI(X) is enabled.
> >
> > FWIW, bits 11-15 are RsvdP, so we will need to add support for them
> > after the series from Stewart that adds support for register masks is
> > accepted.
> 
> Stewart's series implement much better register handling than this
> patch. What about dropping this change at all in favor of Stewart's
> implementation? I'll be 100% okay with this.

That's fine.  I expect Stewart's series will go in quite soon, and
then we can likely rework this commit on top of them?

Thanks, Roger.