Re: [PATCH 04/10] Support for device capability

From: Marcelo Tosatti <mtosatti@redhat.com>
To: Sheng Yang <sheng@linux.intel.com>
Cc: Avi Kivity <avi@redhat.com>,
	Anthony Liguori <anthony@codemonkey.ws>,
	kvm@vger.kernel.org
Subject: Re: [PATCH 04/10] Support for device capability
Date: Wed, 4 Mar 2009 10:57:16 -0300	[thread overview]
Message-ID: <20090304135716.GB21902@amt.cnet> (raw)
In-Reply-To: <200903041056.36603.sheng@linux.intel.com>

On Wed, Mar 04, 2009 at 10:56:36AM +0800, Sheng Yang wrote:
> On Tuesday 03 March 2009 20:42:07 Marcelo Tosatti wrote:
> > Hi Sheng,
> >
> > On Mon, Mar 02, 2009 at 04:29:27PM +0800, Sheng Yang wrote:
> > > This framework can be easily extended to support device capability, like
> > > MSI/MSI-x.
> > >
> > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > ---
> > >  qemu/hw/pci.c |   85
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu/hw/pci.h |
> > >   30 ++++++++++++++++++++
> > >  2 files changed, 115 insertions(+), 0 deletions(-)
> > >
> > >
> > > @@ -205,6 +215,15 @@ struct PCIDevice {
> > >
> > >      /* Current IRQ levels.  Used internally by the generic PCI code.  */
> > >      int irq_state[4];
> > > +
> > > +    /* Device capability configuration space */
> > > +    struct {
> > > +        int supported;
> > > +        uint8_t config[PCI_CAPABILITY_CONFIG_MAX_LENGTH];
> > > +        unsigned int start, length;
> > > +        PCICapConfigReadFunc *config_read;
> > > +        PCICapConfigWriteFunc *config_write;
> > > +    } cap;
> > >  };
> >
> > Why do you have a copy of the capabilities config space? Why not just
> > access PCIDevice->config directly?
> 
> I am not sure upstream would accept which. Separate the logic seems more clear 
> and easy to accept to me...

You have to maintain two copies of the same data. Whats the point? The
logic can be separate with a unified config space, no? 