From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Jones <drjones@redhat.com>
Subject: Re: [PATCH RFC 14/15] pci: Add pci-testdev PCI bus test device
Date: Mon, 30 May 2016 08:09:20 +0200
Message-ID: <20160530060920.gfo64mqog4c6kedt@hawk.localdomain>
References: <cover.1460190352.git.agordeev@redhat.com>
 <d9a0f494a3a476c4dcbbd0adcd789085f1c14723.1460190352.git.agordeev@redhat.com>
 <20160422172350.6jhc42cohxgiehlb@hawk.localdomain>
 <20160523080239.GA2051@dhcp-27-118.brq.redhat.com>
 <20160523151716.ivqzu5f4gscmifbs@hawk.localdomain>
 <20160529174815.GA4130@dhcp-27-118.brq.redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, Thomas Huth <thuth@redhat.com>
To: Alexander Gordeev <agordeev@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:50551 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751093AbcE3GJZ (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 30 May 2016 02:09:25 -0400
Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id D98867F0A6
	for <kvm@vger.kernel.org>; Mon, 30 May 2016 06:09:24 +0000 (UTC)
Content-Disposition: inline
In-Reply-To: <20160529174815.GA4130@dhcp-27-118.brq.redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Sun, May 29, 2016 at 07:48:16PM +0200, Alexander Gordeev wrote:
> On Mon, May 23, 2016 at 05:17:16PM +0200, Andrew Jones wrote:
> > > > > +static void pio_writel(u32 value, volatile void *addr)
> > > > > +{
> > > > > +	outl(value, (unsigned long)addr);
> > > > > +}
> > > > 
> > > > Ah, so the above is why you were attempting to add the io-accessor
> > > > wrappers around mmio-accessors to asm-generic. Rather than do that,
> > > 
> > > I also tried to unify io-accessors across archs Linux style.
> > 
> > Yeah, but not everything Linux does is nice :-)
> > 
> > > 
> > > > I think we can just allow the unit test to populate
> > > > pci_testdev_io_ops. E.g. a unit test on ARM would populate it (I
> > > > think) like
> > > > 
> > > >      .io_readb       = __raw_readb,
> > > >      .io_readw       = __raw_readw,
> > > >      .io_readl       = __raw_readl,
> > > >      .io_writeb      = __raw_writeb,
> > > >      .io_writew      = __raw_writew,
> > > >      .io_writel      = __raw_writel,
> > > > 
> > > > while an x86 unit test would use ins and outs.
> > > > 
> > > > You could maybe even still provide the ops structs pre-populated,
> > > > but you'll need to use some #ifdef __arm__ type of stuff then.
> > > 
> > > Out of these two options the latter makes more sense to me, since
> > > type of access to PCI resources is derived from that resource type
> > > rather than from unit test parameters. And that deriving could be
> > > done "automatically".
> > > 
> > > So I would do the #ifdefs, but that will look like a worsened 
> > > version of generic io-accessors ;)
> > > 
> > > So what is your preference here?
> > 
> > My latter suggestion, and I think I was wrong about needing ifdefs.
> > Anyway, let's see how it turns out.
> 
> I am struggling to make it any better. Ifdefs would look less than nice
> indeed, but the only alternative I can think of is putting pci-testdev
> io accessors to <arch>/asm. But pci-testdev io accessors in asm would
> be just ugly, because there is nothing special about pci-testdev
> specific io-accessors. They are still arch's io accessors. So why not
> just make io accessors unified and Linux-style?

OK, let's give the io function wrappers another round.

> 
> > > > > diff --git a/lib/pci.h b/lib/pci.h
> > > > > index 36dd67e19838..03452b059412 100644
> > > > > --- a/lib/pci.h
> > > > > +++ b/lib/pci.h
> > > > > @@ -25,6 +25,8 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> > > > >  void pci_type_desc(int type, char *desc, int len);
> > > > >  void pci_print(void);
> > > > >  
> > > > > +int pci_testdev(void);
> > > > > +
> > > > >  /*
> > > > >   * pci-testdev is a driver for the pci-testdev qemu pci device. The
> > > > >   * device enables testing mmio and portio exits, and measuring their
> > > > > @@ -33,7 +35,12 @@ void pci_print(void);
> > > > >  #define PCI_VENDOR_ID_REDHAT		0x1b36
> > > > >  #define PCI_DEVICE_ID_REDHAT_TEST	0x0005
> > > > >  
> > > > > +/*
> > > > > + * pci-testdev supports at least three types of tests (via mmio and
> > > > > + * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd
> > > > > + */
> > > > >  #define PCI_TESTDEV_NUM_BARS		2
> > > > > +#define PCI_TESTDEV_NUM_TESTS		3
> > > > 
> > > > you don't seem to use this define anywhere. maybe the next patch
> 
> Yeah, PCI_TESTDEV_NUM_TESTS is used by the next patch. But it looks
> cleaner to me to introduce it here - as this patch kind of makes
> pci-testdev "complete". While the next patch is just pci-testdev
> enabler for ARM.
> 
> Leave it here or move?

It's fine here.

Thanks,
drew

> 
> Thanks!
> 
> > > > thanks,
> > > > drew
> --
> 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