Re: [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio specification version 3

[Michael S. Tsirkin]

On Thu, Jan 09, 2020 at 02:15:51PM +0800, Liu, Jing2 wrote:
> 
> On 1/5/2020 7:04 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 25, 2019 at 10:50:23AM +0800, Zha Bin wrote:
> > > From: Liu Jiang<gerry@xxxxxxxxxxxxxxxxx>
> > > 
> > > Userspace VMMs (e.g. Qemu microvm, Firecracker) take advantage of using
> > > virtio over mmio devices as a lightweight machine model for modern
> > > cloud. The standard virtio over MMIO transport layer only supports one
> > > legacy interrupt, which is much heavier than virtio over PCI transport
> > > layer using MSI. Legacy interrupt has long work path and causes specific
> > > VMExits in following cases, which would considerably slow down the
> > > performance:
> > > 
> > > 1) read interrupt status register
> > > 2) update interrupt status register
> > > 3) write IOAPIC EOI register
> > > 
> > > We proposed to update virtio over MMIO to version 3[1] to add the
> > > following new features and enhance the performance.
> > > 
> > > 1) Support Message Signaled Interrupt(MSI), which increases the
> > >     interrupt performance for virtio multi-queue devices
> > > 2) Support per-queue doorbell, so the guest kernel may directly write
> > >     to the doorbells provided by virtio devices.
> > Do we need to come up with new "doorbell" terminology?
> > virtio spec calls these available event notifications,
> > let's stick to this.
> 
> Yes, let's keep virtio words, which just calls notifications.
> 
> > > The following is the network tcp_rr performance testing report, tested
> > > with virtio-pci device, vanilla virtio-mmio device and patched
> > > virtio-mmio device (run test 3 times for each case):
> > > 
> > > 	netperf -t TCP_RR -H 192.168.1.36 -l 30 -- -r 32,1024
> > > 
> > > 		Virtio-PCI    Virtio-MMIO   Virtio-MMIO(MSI)
> > > 	trans/s	    9536	6939		9500
> > > 	trans/s	    9734	7029		9749
> > > 	trans/s	    9894	7095		9318
> > > 
> > > [1]https://lkml.org/lkml/2019/12/20/113
> > > 
> > > Signed-off-by: Liu Jiang<gerry@xxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Zha Bin<zhabin@xxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Chao Peng<chao.p.peng@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Jing Liu<jing2.liu@xxxxxxxxxxxxxxx>
> > Do we need a new version though? What is wrong with
> > a feature bit? This way we can create compatible devices
> > and drivers.
> 
> We considered using 1 feature bit of 24~37 to specify MSI capability, but
> 
> this feature bit only means for mmio transport layer, but not representing
> 
> comment feature negotiation of the virtio device. So we're not sure if this
> is a good choice.

We are not short on bits, just don't use bits below 32
since these are for legacy devices.


> > > [...]
> > > +static void mmio_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> > > +{
> > > +	struct device *dev = desc->dev;
> > > +	struct virtio_device *vdev = dev_to_virtio(dev);
> > > +	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> > > +	void __iomem *pos = vm_dev->base;
> > > +	uint16_t cmd = VIRTIO_MMIO_MSI_CMD(VIRTIO_MMIO_MSI_CMD_UPDATE,
> > > +			desc->platform.msi_index);
> > > +
> > > +	writel(msg->address_lo, pos + VIRTIO_MMIO_MSI_ADDRESS_LOW);
> > > +	writel(msg->address_hi, pos + VIRTIO_MMIO_MSI_ADDRESS_HIGH);
> > > +	writel(msg->data, pos + VIRTIO_MMIO_MSI_DATA);
> > > +	writew(cmd, pos + VIRTIO_MMIO_MSI_COMMAND);
> > > +}
> > All this can happen when IRQ affinity changes while device
> > is sending interrupts. An interrupt sent between the writel
> > operations will then be directed incorrectly.
> 
> When investigating kernel MSI behavior, I found in most case there's no
> action during IRQ affinity changes to avoid the interrupt coming.
> 
> For example, when migrate_one_irq, it masks the irq before
> irq_do_set_affinity. But for others, like user setting any irq affinity
> 
> via /proc/, it only holds desc->lock instead of disable/mask irq. In such
> case, how can it ensure the interrupt sending between writel ops?

Could be a bug too. E.g. PCI spec explicitly says it's illegal to
change non-masked interrupts exactly for this reason.



> 
> > > [...]
> > > +
> > > +/* RO: MSI feature enabled mask */
> > > +#define VIRTIO_MMIO_MSI_ENABLE_MASK	0x8000
> > I don't understand the comment. Is this a way for
> > a version 3 device to say "I want/do not want MSI"?
> > Why not just use a feature bit? We are not short on these.
> 
> This is just used for current MSI enabled/disabled status, after all MSI
> configurations setup finished.
> 
> Not for showing MSI capability.
> 
> In other words, since the concern of feature bit, we choose to update the
> virtio mmio
> 
> version that devices with v3 have MSI capability and notifications.
> 
> 
> Thanks,
> 
> Jing

MSI looks like an optimization. I don't see how that
justifies incrementing a major version and breaking
compat with all existing guests.

-- 
MST