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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Jan 2020 14:15:51 +0800
From:   "Liu, Jing2" <jing2.liu@...ux.intel.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>,
        Zha Bin <zhabin@...ux.alibaba.com>
Cc:     linux-kernel@...r.kernel.org, jasowang@...hat.com, slp@...hat.com,
        virtio-dev@...ts.oasis-open.org, gerry@...ux.alibaba.com,
        jing2.liu@...el.com, chao.p.peng@...el.com
Subject: Re: [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio
 specification version 3


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@...ux.alibaba.com>
>>
>> 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@...ux.alibaba.com>
>> Signed-off-by: Zha Bin<zhabin@...ux.alibaba.com>
>> Signed-off-by: Chao Peng<chao.p.peng@...ux.intel.com>
>> Signed-off-by: Jing Liu<jing2.liu@...ux.intel.com>
> 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.

>> [...]
>>   
>> +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?


>> [...]
>> +
>> +/* 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