From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Subject: Re: [kvm-unit-tests PATCH v2 01/10] asm-generic: add portio accessors to io.h Date: Mon, 18 Jan 2016 18:02:16 +0100 Message-ID: <20160118170216.GJ4075@hawk.localdomain> References: <1452876695-9240-1-git-send-email-drjones@redhat.com> <1452876695-9240-2-git-send-email-drjones@redhat.com> <20160115213409.GA12949@potion.brq.redhat.com> <20160118135234.GC4075@hawk.localdomain> <20160118164003.GB14830@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, pbonzini@redhat.com, mst@redhat.com, agordeev@redhat.com To: Radim =?utf-8?B?S3LEjW3DocWZ?= Return-path: Received: from mx1.redhat.com ([209.132.183.28]:49553 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755597AbcARRCV (ORCPT ); Mon, 18 Jan 2016 12:02:21 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id E6CD918B24D for ; Mon, 18 Jan 2016 17:02:20 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20160118164003.GB14830@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jan 18, 2016 at 05:40:04PM +0100, Radim Kr=C4=8Dm=C3=A1=C5=99 w= rote: > 2016-01-18 14:52+0100, Andrew Jones: > > On Fri, Jan 15, 2016 at 10:34:09PM +0100, Radim Kr=C4=8Dm=C3=A1=C5=99= wrote: > >> 2016-01-15 17:51+0100, Andrew Jones: > >> My first reaction was "throw this abomination out!", but Drew expl= ained > >> that in*/out* is here because we'll also generalize the x86 PCI co= de > >> (which uses PIO and MMIO) and that Linux has the same code. > >>=20 > >> arm, arm64, and unicore32 define PCI_IOBASE in Linux. I didn't fi= gure > >> out why they want to use a PIO based abstraction for MMIO, so the > >> interface is fine with me as long as > >=20 > > Having these aliases for read*/write* are less evil than attempting= to > > share code between arm and x86 with a bunch of #ifdefs. >=20 > #ifs in plain sight are definitely evil, but we could have wrapped > read*/write*/in*/out* in an interface that doesn't violate most basic > types and is hidden with at most one #ifdef. >=20 > > As I didn't= yet > > write a shared pci-testdev driver though, then this patch can be dr= opped > > for now, if you'd prefer. However at some point it may need to come= back. >=20 > I'd continue doing what ioread*() in x86/vmexit.c does. > Using a loosely-tagged union also means that we don't need to impleme= nt > in*/out* for everyone. >=20 > And because structs are a pain to use, having two separate values > instead of > struct { > enum {PIO, MMIO} space; > union {u16 port; void * address;};}; >=20 > can be excused. >=20 > >> - functions are hidden behind a single #ifndef, like ARCH_HAS_POR= T_IO. > >> (Ideally defined as part of global configuration, because it's = harder > >> to fail that way.) > >=20 > > I prefer to stay consistent with Linux. It's ugly, but we already h= ave > > the ugliness for read* and write*. >=20 > Good point, copy-pasting code is convenient and we don't plan to outl= ive > Linux anyway ... >=20 > I'd rather drop this patch now and wait for the first use, but it has= my >=20 > Reviewed-by: Radim Kr=C4=8Dm=C3=A1=C5=99 Thanks, but I'll just drop this patch for now. We'll see how Alex implements the common pci-testdev. If he needs this stuff to allow it t= o more easily be shared with x86, then he can start this battle up again. I'll= get some popcorn and observe from the sideline :-) v3 on the way. drew >=20 > >> - "unsigned long addr" is changed to "u16 port"; > >> x86 ought to have that and we should use different names if we = need > >> different types, because behavior couldn't be the same then. > >=20 > > in*/out* should be portio for x86, and just aliases for read*/write= * > > mmio for other architectures. >=20 > Compile error would be the best implementation on arches without port > space, because they shouldn't access interface for something that > doesn't have a meaning for them. Aliasing read*/write* is reasonable > for arches that map port space into memory space. >=20 > > For mmio to work we may need 'unsigne= d long > > addr', but that shouldn't stop x86 from defining its version as 'u1= 6 port'. >=20 > It should. The type system makes certain things easier, like using t= he > expected value, and having two separate types for in*/out* defeats it > without bringing any benefit. >=20 > If it turns out that we need more than u16 addr, x86 in*/out* would b= e > better as 'unsigned long port' with 'BUG(port >=3D 64k)' inside. >=20 > > (Although Linux defines it as 'int' actually). > >=20 > > It does appear that in Linux arch/x86/include/asm/io.h doesn't incl= ude > > asm-generic/io.h. So, if you'd prefer x86 to not include it here ei= ther, > > then we can drop the inclusion and the '#define inb inb' type stuff= from > > lib/x86/asm/io.h. >=20 > Hm, I'd pick the mistake we do now. > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html