qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 2/7] pci: memory access API and IOMMU support
Date: Thu, 02 Sep 2010 08:24:05 -0500
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.11) Gecko/20100713 Lightning/1.0b1 Thunderbird/3.0.6

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]