Re: [Qemu-devel] [PATCH 2/7] pci: memory access API and IOMMU support

[Anthony Liguori]

On 09/02/2010 04:08 AM, Eduard - Gabriel Munteanu wrote:
> On Thu, Sep 02, 2010 at 09:00:46AM +0300, Michael S. Tsirkin wrote:
>    
> > On Wed, Sep 01, 2010 at 10:10:30PM +0200, Stefan Weil wrote:
> >      
> > > > +static inline void pci_memory_read(PCIDevice *dev,
> > > > + pcibus_t addr,
> > > > + uint8_t *buf,
> > > > + pcibus_t len)
> > > > +{
> > > > + pci_memory_rw(dev, addr, buf, len, 0);
> > > > +}
> > > > +
> > > > +static inline void pci_memory_write(PCIDevice *dev,
> > > > + pcibus_t addr,
> > > > + const uint8_t *buf,
> > > > + pcibus_t len)
> > > > +{
> > > > + pci_memory_rw(dev, addr, (uint8_t *) buf, len, 1);
> > > > +}
> > > > +
> > > > #endif
> > > >          
> > > The functions pci_memory_read and pci_memory_write not only read
> > > or write byte data but many different data types which leads to
> > > a lot of type casts in your other patches.
> > >
> > > I'd prefer "void *buf" and "const void *buf" in the argument lists.
> > > Then all those type casts could be removed.
> > >
> > > Regards
> > > Stefan Weil
> > >        
> > Further, I am not sure pcibus_t is a good type to use here.
> > This also forces use of pci specific types in e.g. ide, or resorting to
> > casts as this patch does. We probably should use a more generic type
> > for this.
> >      
> It only forces use of PCI-specific types in the IDE controller, which is
> already a PCI device.
>    

But IDE controllers are not always PCI devices...  This isn't an issue 
with your patch, per-say, but with how we're modelling the IDE 
controller today.  There's no great solution but I think your patch is 
an improvement over what we have today.

I do agree with Stefan though that void * would make a lot more sense.

Regards,

Anthony Liguori

>         Eduard
>
>    
> > --
> > MST
> >      
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>