[Qemu-devel] Re: [PATCH 2/9] eepro100: Fix endianness issues

[Stefan Weil <weil@mail.berlios.de>]

Am 31.03.2011 23:52, schrieb Michael S. Tsirkin:
> On Thu, Mar 31, 2011 at 10:33:24PM +0200, Stefan Weil wrote:
>> Like other Intel devices, e100 (eepro100) uses little endian byte order.
>>
>> This patch was tested with these combinations:
>>
>> i386 host, i386 + mipsel guests (le-le)
>> mipsel host, i386 guest (le-le)
>> i386 host, mips + ppc guests (le-be)
>> mips host, i386 guest (be-le)
>>
>> mips and mipsel hosts were emulated machines.
>>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>> hw/eepro100.c | 113 
>> ++++++++++++++++++++++++++++++++++++++++-----------------
>> 1 files changed, 80 insertions(+), 33 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index f89ff17..c789767 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -20,11 +20,10 @@
>> * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> *
>> * Tested features (i82559):
>> - * PXE boot (i386) ok
>> + * PXE boot (i386 guest, i386 / mips / mipsel / ppc host) ok
>> * Linux networking (i386) ok
>> *
>> * Untested:
>> - * non-i386 platforms
>> * Windows networking
>> *
>> * References:
>> @@ -130,7 +129,7 @@ typedef struct {
>>
>> /* Offsets to the various registers.
>> All accesses need not be longword aligned. */
>> -enum speedo_offsets {
>> +typedef enum {
>> SCBStatus = 0, /* Status Word. */
>> SCBAck = 1,
>> SCBCmd = 2, /* Rx/Command Unit command and status. */
>> @@ -145,7 +144,7 @@ enum speedo_offsets {
>> SCBpmdr = 27, /* Power Management Driver. */
>> SCBgctrl = 28, /* General Control. */
>> SCBgstat = 29, /* General Status. */
>> -};
>> +} E100RegisterOffset;
>>
>> /* A speedo3 transmit buffer descriptor with two buffers... */
>> typedef struct {
>> @@ -307,7 +306,32 @@ static const uint16_t eepro100_mdi_mask[] = {
>> 0xffff, 0xffff, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
>> };
>>
>> -/* XXX: optimize */
>> +/* Read a 16 bit little endian value from physical memory. */
>> +static uint16_t lduw_le_phys(target_phys_addr_t addr)
>> +{
>> + /* Load 16 bit (little endian) word from emulated hardware. */
>> + uint16_t val;
>> + cpu_physical_memory_read(addr, (uint8_t *)&val, sizeof(val));
>> + return le16_to_cpu(val);
>> +}
>> +
>> +/* Read a 32 bit little endian value from physical memory. */
>> +static uint32_t ldl_le_phys(target_phys_addr_t addr)
>> +{
>> + /* Load 32 bit (little endian) word from emulated hardware. */
>> + uint32_t val;
>> + cpu_physical_memory_read(addr, (uint8_t *)&val, sizeof(val));
>> + return le32_to_cpu(val);
>> +}
>> +
>> +/* Write a 16 bit little endian value to physical memory. */
>> +static void stw_le_phys(target_phys_addr_t addr, uint16_t val)
>> +{
>> + val = cpu_to_le16(val);
>> + cpu_physical_memory_write(addr, (const uint8_t *)&val, sizeof(val));
>> +}
>> +
>> +/* Write a 32 bit little endian value to physical memory. */
>
> So why not opencode e.g.
> le32_to_cpu(ldl_phys(addr))
>
> wrappers really worth it? What do I miss?
>
> If you insist on these online wrappers, pls prefix
> them with eepro100_.
> Also, why not use lduw_phys and friends internally?
> cpu_physical_ is slower ...
>
>> static void stl_le_phys(target_phys_addr_t addr, uint32_t val)
>> {
>> val = cpu_to_le32(val);
>> @@ -339,6 +363,32 @@ static unsigned compute_mcast_idx(const uint8_t 
>> * ep)
>> return (crc & BITS(7, 2)) >> 2;
>> }
>>
>> +/* Read a 16 bit control/status (CSR) register. */
>> +static uint16_t e100_read_reg2(EEPRO100State *s, E100RegisterOffset 
>> addr)
>> +{
>> + return le16_to_cpup((uint16_t *)&s->mem[addr]);
>> +}
>> +
>> +/* Read a 32 bit control/status (CSR) register. */
>> +static uint32_t e100_read_reg4(EEPRO100State *s, E100RegisterOffset 
>> addr)
>> +{
>> + return le32_to_cpup((uint32_t *)&s->mem[addr]);
>> +}
>> +
>> +/* Write a 16 bit control/status (CSR) register. */
>> +static void e100_write_reg2(EEPRO100State *s, E100RegisterOffset addr,
>> + uint16_t val)
>> +{
>> + cpu_to_le16w((uint16_t *)&s->mem[addr], val);
>> +}
>> +
>> +/* Read a 32 bit control/status (CSR) register. */
>> +static void e100_write_reg4(EEPRO100State *s, E100RegisterOffset addr,
>> + uint32_t val)
>> +{
>> + cpu_to_le32w((uint32_t *)&s->mem[addr], val);
>> +}
>> +
>
> Note that cpu_to_le32w requires an aligned address, unlike
> memcpy, and there's no guarantee
> addr is aligned apparently?
>
> If true you need to memcpy to a 32 bit variable, then
> cpu_to_le32w ther result.
>

[snip]

Thank you for your review, especially for the hints at lduw_phys
and potential alignment issues. I'll apply them to a new version
of this patch.

There was already a function without prefix (stl_le_phys),
and the new ones belong to the same "family". There is nothing
e100 specific in them, so they might be added to qemu-common.h
as well. That was (and is) the reason why I did not add a prefix.