On 16/01/2017 18:26, Fam Zheng wrote:>> Is the endianness correct for big-endian host here? > > I think so. The fc_host sysfs uses u64 to represent port_name and node_name, > this patch does the same, so using virtio_* helpers for these fields should > handle the endianness correctly.I was suspicious about it because they are defined as "u8 x[8]" in the virtio_scsi_config struct. So you would need to read with virtio_cread_bytes and pass the result to wwn_to_u64. For example, if you have 0x500123456789abcd this would be 0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd in virtio_scsi_config, and then virtio_cread64 would read it as a little-endian u64, 0xcdab896745230150. Maybe your QEMU patch is also writing things as little-endian 64-bit integers, rather than 8-element arrays of bytes? Paolo> Maybe we should use u64 in struct virtio_scsi_config as well?
On Tue, 01/17 14:17, Paolo Bonzini wrote:> > > On 16/01/2017 18:26, Fam Zheng wrote: > >> Is the endianness correct for big-endian host here? > > > > I think so. The fc_host sysfs uses u64 to represent port_name and node_name, > > this patch does the same, so using virtio_* helpers for these fields should > > handle the endianness correctly. > > I was suspicious about it because they are defined as "u8 x[8]" in the > virtio_scsi_config struct. So you would need to read with > virtio_cread_bytes and pass the result to wwn_to_u64. > > For example, if you have 0x500123456789abcd this would be > > 0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd > > in virtio_scsi_config, and then virtio_cread64 would read it as a > little-endian u64, 0xcdab896745230150. Maybe your QEMU patch is also > writing things as little-endian 64-bit integers, rather than 8-element > arrays of bytes?Yes, they all used 64-bit integers in a "less surprising" endian. I think there is an endianness conecpt to WWN, as in 0x500123456789abcd; and there is an native endianness to virtio, which is little-endian. If we use a "u8 x[8]" type in the spec and want the WWN's MSB, namely the 0x50 stuff, to be the first byte, is it worth to explicitly document that to avoid confusion? Fam
On Tue, Jan 17, 2017 at 10:05:00PM +0800, Fam Zheng wrote:> On Tue, 01/17 14:17, Paolo Bonzini wrote: > > > > > > On 16/01/2017 18:26, Fam Zheng wrote: > > >> Is the endianness correct for big-endian host here? > > > > > > I think so. The fc_host sysfs uses u64 to represent port_name and node_name, > > > this patch does the same, so using virtio_* helpers for these fields should > > > handle the endianness correctly. > > > > I was suspicious about it because they are defined as "u8 x[8]" in the > > virtio_scsi_config struct. So you would need to read with > > virtio_cread_bytes and pass the result to wwn_to_u64. > > > > For example, if you have 0x500123456789abcd this would be > > > > 0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd > > > > in virtio_scsi_config, and then virtio_cread64 would read it as a > > little-endian u64, 0xcdab896745230150. Maybe your QEMU patch is also > > writing things as little-endian 64-bit integers, rather than 8-element > > arrays of bytes? > > Yes, they all used 64-bit integers in a "less surprising" endian. I think there > is an endianness conecpt to WWN, as in 0x500123456789abcd; and there is an > native endianness to virtio, which is little-endian. If we use a "u8 x[8]" type > in the spec and want the WWN's MSB, namely the 0x50 stuff, to be the first byte, > is it worth to explicitly document that to avoid confusion? > > FamCan't hurt, for sure.