Re: [Qemu-devel] [PATCH] virtio-pci: implement cfg capability

[Michael S. Tsirkin]

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?

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