Re: [PATCH 3/3] virtio: Add a new layer to abstract pci access method

On 2016/01/18 22:46, Yuanhan Liu wrote:

On Mon, Jan 18, 2016 at 06:13:09PM +0900, Tetsuya Mukawa wrote:

quoted

+struct virtio_pci_access_ops {
+	uint8_t (*legacy_read8)(struct virtio_hw *hw, uint8_t *addr);
+	uint16_t (*legacy_read16)(struct virtio_hw *hw, uint16_t *addr);
+	uint32_t (*legacy_read32)(struct virtio_hw *hw, uint32_t *addr);
+	void (*legacy_write8)(struct virtio_hw *hw,
+				uint8_t *addr, uint8_t val);
+	void (*legacy_write16)(struct virtio_hw *hw,
+				uint16_t *addr, uint16_t val);
+	void (*legacy_write32)(struct virtio_hw *hw,
+				uint32_t *addr, uint32_t val);
+
+	uint8_t (*modern_read8)(struct virtio_hw *hw, uint8_t *addr);
+	uint16_t (*modern_read16)(struct virtio_hw *hw, uint16_t *addr);
+	uint32_t (*modern_read32)(struct virtio_hw *hw, uint32_t *addr);
+	void (*modern_write8)(struct virtio_hw *hw,
+				uint8_t *addr, uint8_t val);
+	void (*modern_write16)(struct virtio_hw *hw,
+				uint16_t *addr, uint16_t val);
+	void (*modern_write32)(struct virtio_hw *hw,
+				uint32_t *addr, uint32_t val);

One thing about abstraction is that you need define one set of operations,
instead of two similar sets. Thus, you need define following operations
only:

  - read8
  - read16
  - read32
  - write8
  - write16
  - write32

And make a proper assignment after the modern/legacy detection.

Thanks for comments.
Sure, will do.

quoted

+
+	int (*map_pci_cfg)(struct virtio_hw *hw);
+	void (*unmap_pci_cfg)(struct virtio_hw *hw);
+	void *(*get_cfg_addr)(struct virtio_hw *hw,
+				struct virtio_pci_cap *cap);
+	int (*read_pci_cfg)(struct virtio_hw *hw,
+				void *buf, size_t len, off_t offset);

It'd be good if you can post the patches that use above abstract
operations, so that people can tell if they are properly defined.

Yes, I will submit it again.

Thanks,
Tetsuya