On Thu, Jul 02, 2015 at 09:02:58PM +0200, Paolo Bonzini wrote:
>
>
> On 02/07/2015 21:00, Michael S. Tsirkin wrote:
> > On Thu, Jul 02, 2015 at 08:48:14PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 02/07/2015 15:00, Michael S. Tsirkin wrote:
> >>> + cfg = (void *)(proxy->pci_dev.config + proxy->config_cap);
> >>> + off = le32_to_cpu(cfg->cap.offset);
> >>> + len = le32_to_cpu(cfg->cap.length);
> >>> +
> >>> + if ((len == 1 || len == 2 || len == 4)) {
> >>> + address_space_write(&proxy->modern_as, off,
> >>> + MEMTXATTRS_UNSPECIFIED,
> >>> + cfg->pci_cfg_data, len);
> >>> + }
> >>
> >> This parses pci_cfg_data in target endianness I think. You just want to
> >> move the little-endian value from the config cap to the little-endian
> >> value in the modern_as, so you need to use ldl_le_p and
> >> address_space_stl_le.
> >
> > but isn't that two byteswaps? why isn't this same as memcpy?
>
> It is a memcpy if you write to RAM, but the MMIO ops take an unsigned
> integer, so you can still have one byteswap hiding; you have to be
> careful when you have this kind of "forwarder", and the simplest way to
> do it is to use an ldl/stl pair with the same endianness.
>
> Paolo
The fact that address_space_write/_read actually does a byteswap if
host!=target endian should probably be documented.
Or maybe it should be changed: it seems likely that non-target-specific devices
that use it do this incorrectly ATM. In particular, dma_memory_rw_relaxed calls
address_space_rw and since DMA originates with devices I think there's very
little chance that these actually want a different behaviour depending on the
target endian-ness.
Most likely, these only work correctly because DMA outside RAM
is highly unusual.
> >>> + }
> >>> +}
> >>> +
> >>> +static uint32_t virtio_read_config(PCIDevice *pci_dev,
> >>> + uint32_t address, int len)
> >>> +{
> >>> + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> >>> + struct virtio_pci_cfg_cap *cfg;
> >>> +
> >>> + if (proxy->config_cap &&
> >>> + ranges_overlap(address, len, proxy->config_cap + offsetof(struct
> >>> virtio_pci_cfg_cap,
> >>> +
> >>> pci_cfg_data),
> >>> + sizeof cfg->pci_cfg_data)) {
> >>> + uint32_t off;
> >>> + uint32_t len;
> >>> +
> >>> + cfg = (void *)(proxy->pci_dev.config + proxy->config_cap);
> >>> + off = le32_to_cpu(cfg->cap.offset);
> >>> + len = le32_to_cpu(cfg->cap.length);
> >>> +
> >>> + if ((len == 1 || len == 2 || len == 4)) {
> >>> + address_space_read(&proxy->modern_as, off,
> >>> + MEMTXATTRS_UNSPECIFIED,
> >>> + cfg->pci_cfg_data, len);
> >>
> >> Same here, use address_space_ldl_le to read into an int, and stl_le_p to
> >> write into cfg->pci_cfg_data.
> >>
> >> The best way to check it, of course, is to write a unit test! :) But
> >> you could also use a Linux BE guest on LE host.
> >>
> >> Everything else looks good.
> >>
> >> Paolo
