linux-kernel - Re: [PATCH v2 0/5] virtio mmio specification enhancement

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]

Message-ID: <20200211053953-mutt-send-email-mst@kernel.org>
Date:   Tue, 11 Feb 2020 05:57:09 -0500
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Chao Peng <chao.p.peng@...ux.intel.com>
Cc:     Zha Bin <zhabin@...ux.alibaba.com>, linux-kernel@...r.kernel.org,
        jasowang@...hat.com, slp@...hat.com,
        virtio-dev@...ts.oasis-open.org, qemu-devel@...gnu.org,
        gerry@...ux.alibaba.com, jing2.liu@...ux.intel.com
Subject: Re: [PATCH v2 0/5] virtio mmio specification enhancement

On Tue, Feb 11, 2020 at 04:05:41PM +0000, Chao Peng wrote:
> On Mon, Feb 10, 2020 at 06:44:50AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Feb 10, 2020 at 05:05:16PM +0800, Zha Bin wrote:
> > > We have compared the number of files and the lines of code between
> > > virtio-mmio and virio-pci.
> > > 
> > > 				Virtio-PCI	    Virtio-MMIO	
> > > 	number of files(Linux)	    161			1
> > > 	lines of code(Linux)	    78237		538
> > 
> > 
> > 
> > Something's very wrong here. virtio PCI is 161 files?
> > Are you counting the whole PCI subsystem?
> 
> Right, that is just a rough statistics.

Please try not to make them look so wrong then.
E.g. you don't include drivers/base/platform-msi.c for
mmio do you? Your patch brings a bunch of code in there.

> Surely enough, some drivers will
> never get enabled in a typcial config.
> 
> > Sure enough:
> > 
> > $ find drivers/pci -name '*c' |wc -l
> > 150
> 
> and plus:
> $ find arch/x86/pci/ -name '*c' |wc -l
> 22

But what's the point? This is code that is maintained by PCI core
people anyway.

> > 
> > That's not reasonable, this includes a bunch of drivers that
> > never run on a typical hypervisor.
> > 
> > MMIO is also not as small as you are trying to show:
> > 
> > $ cloc drivers/virtio/virtio_mmio.c include/uapi/linux/virtio_mmio.h
> >        2 text files.
> >        2 unique files.                              
> >        0 files ignored.
> > 
> > github.com/AlDanial/cloc v 1.82  T=0.01 s (230.7 files/s, 106126.5 lines/s)
> > -------------------------------------------------------------------------------
> > Language                     files          blank        comment           code
> > -------------------------------------------------------------------------------
> > C                                1            144            100            535
> > C/C++ Header                     1             39             66             36
> > -------------------------------------------------------------------------------
> > SUM:                             2            183            166            571
> > -------------------------------------------------------------------------------
> > 
> > 
> > I don't doubt MMIO is smaller than PCI. Of course that's because it has
> > no features to speak of - just this patch already doubles it's size. If
> > we keep doing that because we want the features then they will reach
> > the same size in about 4 iterations.
> 
> Since current virtio-mmio size is small enough, so adding any notable
> feature would easily double it.

But really unlike PCI this is just PV stuff that is not reused by
anyone. We end up maintaining all this by ourselves.

> I have no objection that it may one day
> reach the same level of PCI, but in this patch some are actually
> generic changes and for MSI specific code we provide the option to
> confige away.
> 
> Thanks,
> Chao

The option will make it fall down at runtime but
it does not actually seem to remove all of the overhead.



> > 
> > 
> > -- 
> > MST