From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [kvm-unit-tests PATCH v2 01/10] asm-generic: add portio accessors to io.h Date: Mon, 18 Jan 2016 17:40:04 +0100 Message-ID: <20160118164003.GB14830@potion.brq.redhat.com> 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> 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: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:38320 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932123AbcARQkN (ORCPT ); Mon, 18 Jan 2016 11:40:13 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id A7ABF8E01B for ; Mon, 18 Jan 2016 16:40:12 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20160118135234.GC4075@hawk.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: 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 explai= ned >> that in*/out* is here because we'll also generalize the x86 PCI code >> (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 figu= re >> 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 t= o > share code between arm and x86 with a bunch of #ifdefs. #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. > As I didn't y= et > write a shared pci-testdev driver though, then this patch can be drop= ped > for now, if you'd prefer. However at some point it may need to come b= ack. I'd continue doing what ioread*() in x86/vmexit.c does. Using a loosely-tagged union also means that we don't need to implement in*/out* for everyone. And because structs are a pain to use, having two separate values instead of struct { enum {PIO, MMIO} space; union {u16 port; void * address;};}; can be excused. >> - functions are hidden behind a single #ifndef, like ARCH_HAS_PORT_= IO. >> (Ideally defined as part of global configuration, because it's ha= rder >> to fail that way.) >=20 > I prefer to stay consistent with Linux. It's ugly, but we already hav= e > the ugliness for read* and write*. Good point, copy-pasting code is convenient and we don't plan to outliv= e Linux anyway ... I'd rather drop this patch now and wait for the first use, but it has m= y Reviewed-by: Radim Kr=C4=8Dm=C3=A1=C5=99 >> - "unsigned long addr" is changed to "u16 port"; >> x86 ought to have that and we should use different names if we ne= ed >> 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. 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. > For mmio to work we may need 'unsigned = long > addr', but that shouldn't stop x86 from defining its version as 'u16 = port'. It should. The type system makes certain things easier, like using the expected value, and having two separate types for in*/out* defeats it without bringing any benefit. If it turns out that we need more than u16 addr, x86 in*/out* would be better as 'unsigned long port' with 'BUG(port >=3D 64k)' inside. > (Although Linux defines it as 'int' actually). >=20 > It does appear that in Linux arch/x86/include/asm/io.h doesn't includ= e > asm-generic/io.h. So, if you'd prefer x86 to not include it here eith= er, > then we can drop the inclusion and the '#define inb inb' type stuff f= rom > lib/x86/asm/io.h. Hm, I'd pick the mistake we do now.