Re: [Qemu-devel] [Qemu-trivial] [PATCH] hw/net/eepro100: Implement read-only bits in MDI registers

[Michael Tokarev <mjt@tls.msk.ru>]

08.06.2014 16:27, Peter Maydell wrote:
> On 8 June 2014 12:31, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 07.06.2014 20:52, Peter Maydell wrote:
>>> Although we defined an eepro100_mdi_mask[] array indicating which bits
>>> in the registers are read-only, we weren't actually doing anything with
>>> it. Make the MDI register-read code use it rather than manually making
>>> registers 2 and 3 totally read-only and the rest totally read-write.
>>
>> s/register-read/register-write/ -- can be fixed when applying.
>>
>> I'm not sure this is "trivial enough", because the side effect is
>> not obvious, at least not to someone not familiar with eepro100
>> registers and their usage.
> 
> Mmm, but do you want to suggest a better queue? I did check
> that Linux could still boot and talk to the network via an eepro100.

[]

>> In this context, apparently we're losing the ability to write to
>> register 0 completely, since its mask is 0 but the original code
>> allows writing something to it.
> 
> Hmm? The mask is a mask of read-only bits, so if mask is zero
> then ANDing the register with the mask will clear it, ANDing the
> data with ~mask will do nothing, and then ORing the data into
> the register means we set every bit in the register. (This
> all happens after the register-specific case code, so the work
> that does to have some of the bits have special effects by
> changing the value of 'data' remains in place.)

That was inaccurate on my part.

Let's take a closer look.  Current code:

static const uint16_t eepro100_mdi_mask[] = {
    0x0000, 0xffff, 0xffff, 0xffff, 0xc01f, 0xffff, 0xffff, 0x0000,
    ...

static void eepro100_write_mdi() {
...
            switch (reg) {
            case 0:            /* Control Register */
                ..modify data...
                break;
            case 1:            /* Status Register */
                missing("not writable");
                data = s->mdimem[reg];
                break;
            case 2:            /* PHY Identification Register (Word 1) */
            case 3:            /* PHY Identification Register (Word 2) */
                missing("not implemented");
                break;
            case 4:            /* Auto-Negotiation Advertisement Register */
            case 5:            /* Auto-Negotiation Link Partner Ability Register */
                break;
            case 6:            /* Auto-Negotiation Expansion Register */
            default:
                missing("not implemented");
            }
            s->mdimem[reg] = data;

You're changing this last line with there (removing data= in case 1):

            s->mdimem[reg] &= eepro100_mdi_mask[reg];
            s->mdimem[reg] |= data & ~eepro100_mdi_mask[reg];

so eepro100_mdi_mask has bit set for UNwritable bits, and bit unset
for WRITABLE bits.  This is where I misunderstood it initially (because
it is sort of expected to be the opposite).  So, reg0 is fully writable
still (with the same mods which are applied initially).

But the the next 3 registers - 1, 2 and 3 - are becoming all UNwritable.
Original code has only register 1 unwritable, not registers 2 and 3 (and
all others).

Initially all registers but register 1 was fully writable, but now most
regs becomes UNwritable.  That's what I should have said in the first
email ;)

Maybe that's a non-issue anyway, maybe these regs aren't actually used
by the device emulation code, -- as I mentioned initially, this is not
exactly a trivial change, it isn't clear at all to someone who isn't
familiar with the registers and their usage -- in real hw and in there.

Thanks,

/mjt