From mboxrd@z Thu Jan 1 00:00:00 1970 From: Blue Swirl Subject: Re: [PATCH 4/4] kvm: i386: Add classic PCI device assignment Date: Sat, 1 Sep 2012 09:20:21 +0000 Message-ID: References: <825e653c9cfe9d8e26185917cbe1f1dd7ae299e2.1346048917.git.jan.kiszka@web.de> <503C74DD.3020307@msgid.tls.msk.ru> <20120828172838.GD3661@redhat.com> <87fw76ak5l.fsf@codemonkey.ws> <87d32a3csy.fsf@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: kvm@vger.kernel.org, "Michael S. Tsirkin" , Marcelo Tosatti , Michael Tokarev , qemu-devel@nongnu.org, Alex Williamson , Jan Kiszka , Avi Kivity To: Anthony Liguori Return-path: In-Reply-To: <87d32a3csy.fsf@codemonkey.ws> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org On Tue, Aug 28, 2012 at 9:51 PM, Anthony Liguori wrote: > Blue Swirl writes: > >> On Tue, Aug 28, 2012 at 7:31 PM, Anthony Liguori wrote: >>> Blue Swirl writes: >>> >>>> On Tue, Aug 28, 2012 at 5:28 PM, Michael S. Tsirkin wrote: >>>>> On Tue, Aug 28, 2012 at 05:01:55PM +0000, Blue Swirl wrote: >>>>>> On Tue, Aug 28, 2012 at 7:35 AM, Michael Tokarev wrote: >>>>>> > On 27.08.2012 22:56, Blue Swirl wrote: >>>>>> > [] >>>>>> >>> +static uint32_t slow_bar_readb(void *opaque, target_phys_addr_t addr) >>>>>> >>> +{ >>>>>> >>> + AssignedDevRegion *d = opaque; >>>>>> >>> + uint8_t *in = d->u.r_virtbase + addr; >>>>>> >> >>>>>> >> Don't perform arithmetic with void pointers. >>>>>> > >>>>>> > There are a few places in common qemu code which does this for a very >>>>>> > long time. So I guess it is safe now. >>>>>> >>>>>> It's a non-standard GCC extension. >>>>> >>>>> So? We use many other GCC extensions. grep for typeof. >>>> >>>> Dependencies should not be introduced trivially. In this case, it's >>>> pretty easy to avoid void pointer arithmetic as Jan's next version >>>> shows. >>> >>> The standard is vague with respect void arithmetic. Most compilers >>> allow it. A very good analysis of the standard can be found below. >>> >>> http://stackoverflow.com/questions/3523145/pointer-arithmetic-for-void-pointer-in-c >> >> The analysis would seem to show that arithmetic may be acceptable, but >> it doesn't say that void pointers must be treated like char pointers. >> In my view, this would make sense: >> >> char *cptr; >> void *vptr; >> >> Since >> cptr++; >> is equivalent to >> cptr = (char *)((uintptr_t)cptr + sizeof(*cptr)); >> >> therefore >> >> vptr++; >> should be equivalent to >> vptr = (void *)((uintptr_t)vptr + sizeof(*vptr)); >> That is, vptr++ should be equivalent to vptr += 0 because sizeof(void) >> should be 0 if allowed. > > sizeof(void) == 1 > > With GCC at least. It's not valid C (0 is just how I think it should be if allowed). Also GCC can reject it even with std=gnu89 (default, C89 with GNU extensions): $ cat void.c unsigned long x = sizeof(void); $ gcc -pedantic void.c -c void.c:1: warning: invalid application of 'sizeof' to a void type > Regards, > > Anthony Liguori > >>> >>> Regards, >>> >>> Anthony Liguori >>> >>>> >>>>> >>>>> Is there a work in progress to build GCC with visual studio? >>>>> If yes what are the chances KVM device assignment >>>>> will work on windows? >>>> >>>> IIRC there was really a project to use KVM on Windows and another >>>> project to build QEMU with MSVC. >>>> >>>>> >>>>> Look QEMU codebase is what it is. Unless you rework all existing >>>>> code to confirm to your taste, I do not see why you NACK valid new code >>>>> unless it confirms to same. >>>> >>>> Yes, I'd be happy to fix the style with huge patches at once. But our >>>> fearless leader does not agree, so we are stuck with the codebase >>>> being what it is until it is fixed one step at a time. >>>> >>>>> >>>>>> > >>>>>> > /mjt >>>> -- >>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47038) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7jsj-00067o-JK for qemu-devel@nongnu.org; Sat, 01 Sep 2012 05:20:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T7jsh-0003b5-V6 for qemu-devel@nongnu.org; Sat, 01 Sep 2012 05:20:45 -0400 Received: from mail-iy0-f173.google.com ([209.85.210.173]:40826) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T7jsh-0003ay-OA for qemu-devel@nongnu.org; Sat, 01 Sep 2012 05:20:43 -0400 Received: by iakx26 with SMTP id x26so5808354iak.4 for ; Sat, 01 Sep 2012 02:20:43 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <87d32a3csy.fsf@codemonkey.ws> References: <825e653c9cfe9d8e26185917cbe1f1dd7ae299e2.1346048917.git.jan.kiszka@web.de> <503C74DD.3020307@msgid.tls.msk.ru> <20120828172838.GD3661@redhat.com> <87fw76ak5l.fsf@codemonkey.ws> <87d32a3csy.fsf@codemonkey.ws> From: Blue Swirl Date: Sat, 1 Sep 2012 09:20:21 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 4/4] kvm: i386: Add classic PCI device assignment List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: kvm@vger.kernel.org, "Michael S. Tsirkin" , Marcelo Tosatti , Michael Tokarev , qemu-devel@nongnu.org, Alex Williamson , Jan Kiszka , Avi Kivity On Tue, Aug 28, 2012 at 9:51 PM, Anthony Liguori wrote: > Blue Swirl writes: > >> On Tue, Aug 28, 2012 at 7:31 PM, Anthony Liguori wrote: >>> Blue Swirl writes: >>> >>>> On Tue, Aug 28, 2012 at 5:28 PM, Michael S. Tsirkin wrote: >>>>> On Tue, Aug 28, 2012 at 05:01:55PM +0000, Blue Swirl wrote: >>>>>> On Tue, Aug 28, 2012 at 7:35 AM, Michael Tokarev wrote: >>>>>> > On 27.08.2012 22:56, Blue Swirl wrote: >>>>>> > [] >>>>>> >>> +static uint32_t slow_bar_readb(void *opaque, target_phys_addr_t addr) >>>>>> >>> +{ >>>>>> >>> + AssignedDevRegion *d = opaque; >>>>>> >>> + uint8_t *in = d->u.r_virtbase + addr; >>>>>> >> >>>>>> >> Don't perform arithmetic with void pointers. >>>>>> > >>>>>> > There are a few places in common qemu code which does this for a very >>>>>> > long time. So I guess it is safe now. >>>>>> >>>>>> It's a non-standard GCC extension. >>>>> >>>>> So? We use many other GCC extensions. grep for typeof. >>>> >>>> Dependencies should not be introduced trivially. In this case, it's >>>> pretty easy to avoid void pointer arithmetic as Jan's next version >>>> shows. >>> >>> The standard is vague with respect void arithmetic. Most compilers >>> allow it. A very good analysis of the standard can be found below. >>> >>> http://stackoverflow.com/questions/3523145/pointer-arithmetic-for-void-pointer-in-c >> >> The analysis would seem to show that arithmetic may be acceptable, but >> it doesn't say that void pointers must be treated like char pointers. >> In my view, this would make sense: >> >> char *cptr; >> void *vptr; >> >> Since >> cptr++; >> is equivalent to >> cptr = (char *)((uintptr_t)cptr + sizeof(*cptr)); >> >> therefore >> >> vptr++; >> should be equivalent to >> vptr = (void *)((uintptr_t)vptr + sizeof(*vptr)); >> That is, vptr++ should be equivalent to vptr += 0 because sizeof(void) >> should be 0 if allowed. > > sizeof(void) == 1 > > With GCC at least. It's not valid C (0 is just how I think it should be if allowed). Also GCC can reject it even with std=gnu89 (default, C89 with GNU extensions): $ cat void.c unsigned long x = sizeof(void); $ gcc -pedantic void.c -c void.c:1: warning: invalid application of 'sizeof' to a void type > Regards, > > Anthony Liguori > >>> >>> Regards, >>> >>> Anthony Liguori >>> >>>> >>>>> >>>>> Is there a work in progress to build GCC with visual studio? >>>>> If yes what are the chances KVM device assignment >>>>> will work on windows? >>>> >>>> IIRC there was really a project to use KVM on Windows and another >>>> project to build QEMU with MSVC. >>>> >>>>> >>>>> Look QEMU codebase is what it is. Unless you rework all existing >>>>> code to confirm to your taste, I do not see why you NACK valid new code >>>>> unless it confirms to same. >>>> >>>> Yes, I'd be happy to fix the style with huge patches at once. But our >>>> fearless leader does not agree, so we are stuck with the codebase >>>> being what it is until it is fixed one step at a time. >>>> >>>>> >>>>>> > >>>>>> > /mjt >>>> -- >>>> 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