[Qemu-devel] [RFC PATCH 0/2] Update HMP only upon mac change completion.

[Qemu-devel] [RFC PATCH 0/2] Update HMP only upon mac change completion.

[Vlad Yasevich]

Recent threads regarding e1000/rtl8139 and mac address change notifications
prompted some research into the respecitive hw data sheets as well as
available drivers.  What I found is that each hw has a mechanism that
can be used by our emulation layer to determine when the mac address
change has completed (when the OS finished writing the mac address),
and we can use these mechanisms to trigger HMP notifications.

This is only an RFC series.  It's been tested and works well.
I've split e1000 and rtl8139 changes as they are sufficiently
different.  e1000 make this very clean and easy, but rtl8139
isn't as nice.

Please take a look and I'd like to hear your comments.

Thanks
-vlad

Vlad Yasevich (2):
  e1000: Use Address_Available bit as HW latch
  rtl8139: update HMP only when the address is fully written

 hw/i386/pc_piix.c    |  4 ++++
 hw/i386/pc_q35.c     |  4 ++++
 hw/net/e1000.c       | 11 ++++++++++-
 hw/net/rtl8139.c     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/i386/pc.h |  8 ++++++++
 5 files changed, 75 insertions(+), 2 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [RFC PATCH 1/2] e1000: Use Address_Available bit as HW latch

[Vlad Yasevich]

e1000 provides a E1000_RAH_AV bit on every complete write
to the Receive Address Register.  We can use this bit
2 ways:
 1) To trigger HMP notifications.  When the bit is set the
    mac address is fully set and we can update the HMP.

 2) We can turn off he bit on the write to low order bits of
    the Receive Address Register, so that we would not try
    to match received traffic to this address when it is
    not completely set.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 hw/net/e1000.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ae63591..82978ea 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1106,10 +1106,19 @@ mac_writereg(E1000State *s, int index, uint32_t val)
 
     s->mac_reg[index] = val;
 
-    if (index == RA || index == RA + 1) {
+    switch (index) {
+    case RA:
+        /* Mask off AV bit on the write of the low dword.  The write of
+         * the high dword will set the bit.  This way a half-written
+         * mac address will not be used to filter on rx.
+         */
+        s->mac_reg[RA+1] &= ~E1000_RAH_AV;
+        break;
+    case (RA + 1):
         macaddr[0] = cpu_to_le32(s->mac_reg[RA]);
         macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]);
         qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr);
+        break;
     }
 }
 
-- 
1.8.4.2

[Qemu-devel] [RFC PATCH 2/2] rtl8139: update HMP only when the address is fully written

[Vlad Yasevich]

rtl8139 hardware requires 9346 config register to be set into
write mode before mac address can be changed even though it is
not documented.  Every driver inspected so far appears to do
this along with comments that this is an undocumented requirement.

We can use this to help us identify when the mac address has been
completely written.  Simple set a flag whenever mac has changed
and at the next transition of 9346 register from Write to Normal
mode, we update the HMP.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 hw/i386/pc_piix.c    |  4 ++++
 hw/i386/pc_q35.c     |  4 ++++
 hw/net/rtl8139.c     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/i386/pc.h |  8 ++++++++
 4 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2daa111..731ae3b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -372,6 +372,10 @@ static QEMUMachine pc_i440fx_machine_v1_7 = {
     PC_I440FX_1_7_MACHINE_OPTIONS,
     .name = "pc-i440fx-1.7",
     .init = pc_init_pci_1_7,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_1_7,
+        { /* end of list */ }
+    },
 };
 
 #define PC_I440FX_1_6_MACHINE_OPTIONS PC_I440FX_1_7_MACHINE_OPTIONS
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e2b8907..7b6aedf 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -292,6 +292,10 @@ static QEMUMachine pc_q35_machine_v1_7 = {
     PC_Q35_1_7_MACHINE_OPTIONS,
     .name = "pc-q35-1.7",
     .init = pc_q35_init_1_7,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_1_7,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_1_7_MACHINE_OPTIONS
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 7f2b4db..5c4caec 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -476,6 +476,7 @@ typedef struct RTL8139State {
 
     uint16_t CpCmd;
     uint8_t  TxThresh;
+    bool     mac_changed;
 
     NICState *nic;
     NICConf conf;
@@ -515,6 +516,9 @@ typedef struct RTL8139State {
 
     /* Support migration to/from old versions */
     int rtl8139_mmio_io_addr_dummy;
+#define RTL8139_FLAG_MAC_BIT 0
+#define RTL8139_FLAG_MAC_COMPLETE (1 << RTL8139_FLAG_MAC_BIT)
+    uint32_t    compat_flags;
 } RTL8139State;
 
 /* Writes tally counters to memory via DMA */
@@ -1215,6 +1219,7 @@ static void rtl8139_reset(DeviceState *d)
     /* restore MAC address */
     memcpy(s->phys, s->conf.macaddr.a, 6);
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
+    s->mac_changed = false;
 
     /* reset interrupt mask */
     s->IntrStatus = 0;
@@ -1563,6 +1568,14 @@ static void rtl8139_Cfg9346_write(RTL8139State *s, uint32_t val)
         /* Reset.  */
         val = 0;
         rtl8139_reset(d);
+    } else if (opmode == Cfg9346_Normal && s->mac_changed) {
+        /* Even though it is not documented, it is required to set
+         * opmode to Cfg9346_ConfigWrite when changing the mac address
+         * of the card and to set to Cfg9346_Normal when done.  We
+         * can use this as an idication to kick off the notification event.
+         */
+        qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
+        s->mac_changed = false;
     }
 
     s->Cfg9346 = val;
@@ -2743,7 +2756,12 @@ static void rtl8139_io_writeb(void *opaque, uint8_t addr, uint32_t val)
     {
         case MAC0 ... MAC0+5:
             s->phys[addr - MAC0] = val;
-            qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
+            if (s->compat_flags & RTL8139_FLAG_MAC_COMPLETE) {
+                s->mac_changed = true;
+            } else if (addr == MAC0+5) {
+                /* Emulate old style updates on the last write */
+                qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
+            }
             break;
         case MAC0+6 ... MAC0+7:
             /* reserved */
@@ -3256,6 +3274,13 @@ static int rtl8139_post_load(void *opaque, int version_id)
      * to link status bit in BasicModeStatus */
     qemu_get_queue(s->nic)->link_down = (s->BasicModeStatus & 0x04) == 0;
 
+    /* Emulate old behavior if we don't support mac change completion
+     * tracking
+     */
+    if (!(s->compat_flags & RTL8139_FLAG_MAC_COMPLETE)) {
+        s->mac_changed = false;
+    }
+
     return 0;
 }
 
@@ -3286,6 +3311,24 @@ static void rtl8139_pre_save(void *opaque)
     s->rtl8139_mmio_io_addr_dummy = 0;
 }
 
+static bool rtl8139_mac_state_needed(void *opaque)
+{
+    RTL8139State *s = opaque;
+
+    return (s->compat_flags & RTL8139_FLAG_MAC_COMPLETE) && s->mac_changed;
+}
+
+static const VMStateDescription vmstate_rtl8139_mac_state ={
+    .name = "rtl8139/mac_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_BOOL(mac_changed, RTL8139State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_rtl8139 = {
     .name = "rtl8139",
     .version_id = 4,
@@ -3371,6 +3414,9 @@ static const VMStateDescription vmstate_rtl8139 = {
             .vmsd = &vmstate_rtl8139_hotplug_ready,
             .needed = rtl8139_hotplug_ready_needed,
         }, {
+            .vmsd = &vmstate_rtl8139_mac_state,
+            .needed = rtl8139_mac_state_needed,
+        }, {
             /* empty */
         }
     }
@@ -3547,6 +3593,8 @@ static int pci_rtl8139_init(PCIDevice *dev)
 
 static Property rtl8139_properties[] = {
     DEFINE_NIC_PROPERTIES(RTL8139State, conf),
+    DEFINE_PROP_BIT("mac_complete", RTL8139State,
+                    compat_flags, RTL8139_FLAG_MAC_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 57e8d16..d1cfdde 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -247,7 +247,15 @@ uint16_t pvpanic_port(void);
 
 int e820_add_entry(uint64_t, uint64_t, uint32_t);
 
+#define PC_COMPAT_1_7 \
+        {\
+            .driver = "rtl8139",\
+            .property="mac_complete",\
+            .value = "off",\
+        }
+
 #define PC_COMPAT_1_6 \
+        PC_COMPAT_1_7, \
         {\
             .driver   = "e1000",\
             .property = "mitigation",\
-- 
1.8.4.2

Re: [Qemu-devel] [RFC PATCH 1/2] e1000: Use Address_Available bit as HW latch

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

On 11/21/2013 01:04 PM, Vlad Yasevich wrote:
> e1000 provides a E1000_RAH_AV bit on every complete write
> to the Receive Address Register.  We can use this bit
> 2 ways:
>  1) To trigger HMP notifications.  When the bit is set the
>     mac address is fully set and we can update the HMP.
> 
>  2) We can turn off he bit on the write to low order bits of

s/he/the/

>     the Receive Address Register, so that we would not try
>     to match received traffic to this address when it is
>     not completely set.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  hw/net/e1000.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 

>  
> -    if (index == RA || index == RA + 1) {
> +    switch (index) {
> +    case RA:
> +        /* Mask off AV bit on the write of the low dword.  The write of
> +         * the high dword will set the bit.  This way a half-written
> +         * mac address will not be used to filter on rx.
> +         */
> +        s->mac_reg[RA+1] &= ~E1000_RAH_AV;

Does real hardware also auto-clear this bit when writing the low word
(thus forcing all drivers to write high word last to make a change take
effect)?  Or are we risking the case of a driver that writes high word
first including the bit, and where real hardware just glitches over the
temporary half-written address where our emulation locks the user out
entirely?  (Asked by someone that has not read the datasheet, so take
with a grain of salt)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [RFC PATCH 2/2] rtl8139: update HMP only when the address is fully written

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

On 11/21/2013 01:04 PM, Vlad Yasevich wrote:
> rtl8139 hardware requires 9346 config register to be set into
> write mode before mac address can be changed even though it is
> not documented.  Every driver inspected so far appears to do
> this along with comments that this is an undocumented requirement.
> 
> We can use this to help us identify when the mac address has been
> completely written.  Simple set a flag whenever mac has changed

s/Simple/Simply/

> and at the next transition of 9346 register from Write to Normal
> mode, we update the HMP.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---

Comment-only review (ie. I didn't validate the code, just fixing grammar)

> +    } else if (opmode == Cfg9346_Normal && s->mac_changed) {
> +        /* Even though it is not documented, it is required to set
> +         * opmode to Cfg9346_ConfigWrite when changing the mac address
> +         * of the card and to set to Cfg9346_Normal when done.  We
> +         * can use this as an idication to kick off the notification event.

s/idication/indication/

> -            qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
> +            if (s->compat_flags & RTL8139_FLAG_MAC_COMPLETE) {
> +                s->mac_changed = true;
> +            } else if (addr == MAC0+5) {

Doesn't coding style recommend s/MAC0+5/MAC0 + 5/

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/2] e1000: Use Address_Available bit as HW latch

[Jason Wang]

On 11/22/2013 04:04 AM, Vlad Yasevich wrote:
> e1000 provides a E1000_RAH_AV bit on every complete write
> to the Receive Address Register.  We can use this bit
> 2 ways:
>  1) To trigger HMP notifications.  When the bit is set the
>     mac address is fully set and we can update the HMP.
>
>  2) We can turn off he bit on the write to low order bits of
>     the Receive Address Register, so that we would not try
>     to match received traffic to this address when it is
>     not completely set.
>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  hw/net/e1000.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index ae63591..82978ea 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -1106,10 +1106,19 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>  
>      s->mac_reg[index] = val;
>  
> -    if (index == RA || index == RA + 1) {
> +    switch (index) {
> +    case RA:
> +        /* Mask off AV bit on the write of the low dword.  The write of
> +         * the high dword will set the bit.  This way a half-written
> +         * mac address will not be used to filter on rx.
> +         */
> +        s->mac_reg[RA+1] &= ~E1000_RAH_AV;

If a stupid driver write high dword first, it won't receive any packets.
> +        break;
> +    case (RA + 1):
>          macaddr[0] = cpu_to_le32(s->mac_reg[RA]);
>          macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]);
>          qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr);

Guest may invalid the mac address by clearing the AV bit through writing
to high dword. So this may notify a wrong mac address.

Generally, we could teset the AV bit before notification, and try to do
the this on both high and low dword. This obeys specs and
receive_filter() above.

If we don't want half-written status, driver should clear AV bit before
each writing of new mac address. But looks like linux and freebsd does
not do this.  But the window is really small and harmless.
> +        break;
>      }
>  }
>  

Re: [Qemu-devel] [RFC PATCH 1/2] e1000: Use Address_Available bit as HW latch

[Vlad Yasevich]

On 11/22/2013 04:47 AM, Jason Wang wrote:
> On 11/22/2013 04:04 AM, Vlad Yasevich wrote:
>> e1000 provides a E1000_RAH_AV bit on every complete write
>> to the Receive Address Register.  We can use this bit
>> 2 ways:
>>  1) To trigger HMP notifications.  When the bit is set the
>>     mac address is fully set and we can update the HMP.
>>
>>  2) We can turn off he bit on the write to low order bits of
>>     the Receive Address Register, so that we would not try
>>     to match received traffic to this address when it is
>>     not completely set.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>  hw/net/e1000.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index ae63591..82978ea 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -1106,10 +1106,19 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>>  
>>      s->mac_reg[index] = val;
>>  
>> -    if (index == RA || index == RA + 1) {
>> +    switch (index) {
>> +    case RA:
>> +        /* Mask off AV bit on the write of the low dword.  The write of
>> +         * the high dword will set the bit.  This way a half-written
>> +         * mac address will not be used to filter on rx.
>> +         */
>> +        s->mac_reg[RA+1] &= ~E1000_RAH_AV;
> 
> If a stupid driver write high dword first, it won't receive any packets.

I need to ping Intel guys again.  I asked them what happens when only
the low register is set, but haven't heard back.

>> +        break;
>> +    case (RA + 1):
>>          macaddr[0] = cpu_to_le32(s->mac_reg[RA]);
>>          macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]);
>>          qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr);
> 
> Guest may invalid the mac address by clearing the AV bit through writing
> to high dword. So this may notify a wrong mac address.

In this case, testing for the AV bit would solve this issue.
> 
> Generally, we could teset the AV bit before notification, and try to do
> the this on both high and low dword. This obeys specs and
> receive_filter() above.

This will not really help since the AV bit would already be set from the
prior mac address.  So, if a stupid driver writes just the low word,
the AV bit would already be set.

> 
> If we don't want half-written status, driver should clear AV bit before
> each writing of new mac address. But looks like linux and freebsd does
> not do this.  But the window is really small and harmless.

We can emulate this.  Thanks for the idea.

-vlad

>> +        break;
>>      }
>>  }
>>  
> 

Re: [Qemu-devel] [RFC PATCH 1/2] e1000: Use Address_Available bit as HW latch

[Jason Wang]

On 11/22/2013 10:37 PM, Vlad Yasevich wrote:
> On 11/22/2013 04:47 AM, Jason Wang wrote:
>> > On 11/22/2013 04:04 AM, Vlad Yasevich wrote:
>>> >> e1000 provides a E1000_RAH_AV bit on every complete write
>>> >> to the Receive Address Register.  We can use this bit
>>> >> 2 ways:
>>> >>  1) To trigger HMP notifications.  When the bit is set the
>>> >>     mac address is fully set and we can update the HMP.
>>> >>
>>> >>  2) We can turn off he bit on the write to low order bits of
>>> >>     the Receive Address Register, so that we would not try
>>> >>     to match received traffic to this address when it is
>>> >>     not completely set.
>>> >>
>>> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>> >> ---
>>> >>  hw/net/e1000.c | 11 ++++++++++-
>>> >>  1 file changed, 10 insertions(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>>> >> index ae63591..82978ea 100644
>>> >> --- a/hw/net/e1000.c
>>> >> +++ b/hw/net/e1000.c
>>> >> @@ -1106,10 +1106,19 @@ mac_writereg(E1000State *s, int index, uint32_t val)
>>> >>  
>>> >>      s->mac_reg[index] = val;
>>> >>  
>>> >> -    if (index == RA || index == RA + 1) {
>>> >> +    switch (index) {
>>> >> +    case RA:
>>> >> +        /* Mask off AV bit on the write of the low dword.  The write of
>>> >> +         * the high dword will set the bit.  This way a half-written
>>> >> +         * mac address will not be used to filter on rx.
>>> >> +         */
>>> >> +        s->mac_reg[RA+1] &= ~E1000_RAH_AV;
>> > 
>> > If a stupid driver write high dword first, it won't receive any packets.
> I need to ping Intel guys again.  I asked them what happens when only
> the low register is set, but haven't heard back.
>

They probably don't have the willing to share the internals of card. I
hacked the e1000 driver to check the subtle and undocumented behaviour
in the past. Maybe we can do the same.