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
>>> + }
>>> +}
>>> +
>>> +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
