| Message ID | 20140520150510.GH1657@ERROL.INI.CMU.EDU |
|---|---|
| State | New |
| Headers | show |
Hi, Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: > Allow selection of different card models from the qemu > command line, to better accomodate a wider range of guests. > > Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> If that patch carried a Signed-off-by line, you should retain it. Your From: line already indicates that it has been rewritten since. > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > --- > > Based on the conversation in [2,3,4], and given that I would still like > OS X to work as close to "out of the box" as possible, I took a stab > at isolating just the bits that allow command-line selection of the > specific e1000 device model from Romain's patch set [1]. Hope that's > OK with everyone :) > > Beyond a few small style improvements, I'm still not 100% familiar with > QOM, so I wonder if there isn't a cleaner way to offer "e1000" as some > sort of "alias" to the default 82540EM, instead of listing two different > devices (e1000 and 82540EM) with otherwise 100% identical properties. > However, if the patch is acceptable as-is, I'm happy to leave it be... :) > > I tested this on Windows 7 (where the default e1000 continues to work > as before), Linux (F20, which is fine with either model), and OS X > (10.9 requires E1000_DEV_ID_82545EM_COPPER, which also works with prior > versions starting at 10.6; and the default e1000 works on versions <= 10.8). > > Comments, suggestions, (or even a straight-forward upstream acceptance > of the patch) much appreciated ! :) > > Thanks, > Gabriel > > [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01397.html > [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01831.html > [3] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01841.html > [4] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg03669.html > > hw/net/e1000.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 80 insertions(+), 17 deletions(-) > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index 8387443..92323a3 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -69,23 +69,29 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL); > > /* > * HW models: > - * E1000_DEV_ID_82540EM works with Windows and Linux > + * E1000_DEV_ID_82540EM works with Windows, Linux, and OS X <= 10.8 > * E1000_DEV_ID_82573L OK with windoze and Linux 2.6.22, > * appears to perform better than 82540EM, but breaks with Linux 2.6.18 > * E1000_DEV_ID_82544GC_COPPER appears to work; not well tested > + * E1000_DEV_ID_82545EM_COPPER works with Linux and OS X >= 10.6 > * Others never tested > */ > -enum { E1000_DEVID = E1000_DEV_ID_82540EM }; > > /* > * May need to specify additional MAC-to-PHY entries -- > * Intel's Windows driver refuses to initialize unless they match > */ > -enum { > - PHY_ID2_INIT = E1000_DEVID == E1000_DEV_ID_82573L ? 0xcc2 : > - E1000_DEVID == E1000_DEV_ID_82544GC_COPPER ? 0xc30 : > - /* default to E1000_DEV_ID_82540EM */ 0xc20 > -}; > +static uint16_t static inline uint16_t maybe? > +e1000_phy_id2_init(uint16_t dev_id) { > + switch(dev_id) { switch (dev_id) { > + case E1000_DEV_ID_82573L: > + return 0xcc2; > + case E1000_DEV_ID_82544GC_COPPER: > + return 0xc30; > + default: > + return 0xc20; /* default for 82540EM and others */ > + } > +} > > typedef struct E1000State_st { > /*< private >*/ > @@ -151,7 +157,7 @@ typedef struct E1000State_st { > uint32_t compat_flags; > } E1000State; > > -#define TYPE_E1000 "e1000" > +#define TYPE_E1000 "e1000-base" > > #define E1000(obj) \ > OBJECT_CHECK(E1000State, (obj), TYPE_E1000) > @@ -235,7 +241,7 @@ static const char phy_regcap[0x20] = { > static const uint16_t phy_reg_init[] = { > [PHY_CTRL] = 0x1140, > [PHY_STATUS] = 0x794d, /* link initially up with not completed autoneg */ > - [PHY_ID1] = 0x141, [PHY_ID2] = PHY_ID2_INIT, > + [PHY_ID1] = 0x141, /* [PHY_ID2] configured per DevId, from e1000_reset() */ > [PHY_1000T_CTRL] = 0x0e00, [M88E1000_PHY_SPEC_CTRL] = 0x360, > [M88E1000_EXT_PHY_SPEC_CTRL] = 0x0d60, [PHY_AUTONEG_ADV] = 0xde1, > [PHY_LP_ABILITY] = 0x1e0, [PHY_1000T_STATUS] = 0x3c00, > @@ -271,8 +277,9 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) > PCIDevice *d = PCI_DEVICE(s); PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(s); // note: not d > uint32_t pending_ints; > uint32_t mit_delay; > + uint16_t dev_id = PCI_DEVICE_GET_CLASS(d)->device_id; uint16_t dev_id = pdc->device_id; By convention, don't use these macros inline in FOO(bar)->baz style expressions, use a variable. Applies elsewhere as well. > > - if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) { > + if (val && (dev_id >= E1000_DEV_ID_82547EI_MOBILE)) { > /* Only for 8257x */ > val |= E1000_ICR_INT_ASSERTED; > } > @@ -377,6 +384,7 @@ static void e1000_reset(void *opaque) > E1000State *d = opaque; > uint8_t *macaddr = d->conf.macaddr.a; > int i; > + uint16_t dev_id = PCI_DEVICE_GET_CLASS(d)->device_id; > > timer_del(d->autoneg_timer); > timer_del(d->mit_timer); > @@ -385,6 +393,7 @@ static void e1000_reset(void *opaque) > d->mit_ide = 0; > memset(d->phy_reg, 0, sizeof d->phy_reg); > memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init); > + d->phy_reg[PHY_ID2] = e1000_phy_id2_init(dev_id); > memset(d->mac_reg, 0, sizeof d->mac_reg); > memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init); > d->rxbuf_min_shift = 1; > @@ -1440,9 +1449,13 @@ static const VMStateDescription vmstate_e1000 = { > } > }; > > +/* > + * EEPROM contents documented in Tables 5-2 and 5-3, pp. 98-102. > + * Note: A valid DevId will be inserted during pci_e1000_init(). > + */ > static const uint16_t e1000_eeprom_template[64] = { > 0x0000, 0x0000, 0x0000, 0x0000, 0xffff, 0x0000, 0x0000, 0x0000, > - 0x3000, 0x1000, 0x6403, E1000_DEVID, 0x8086, E1000_DEVID, 0x8086, 0x3040, > + 0x3000, 0x1000, 0x6403, 0 /*DevId*/, 0x8086, 0 /*DevId*/, 0x8086, 0x3040, > 0x0008, 0x2000, 0x7e14, 0x0048, 0x1000, 0x00d8, 0x0000, 0x2700, > 0x6cc9, 0x3150, 0x0722, 0x040b, 0x0984, 0x0000, 0xc000, 0x0706, > 0x1008, 0x0000, 0x0f04, 0x7fff, 0x4d01, 0xffff, 0xffff, 0xffff, > @@ -1511,6 +1524,7 @@ static int pci_e1000_init(PCIDevice *pci_dev) > uint16_t checksum = 0; > int i; > uint8_t *macaddr; > + uint16_t dev_id = PCI_DEVICE_GET_CLASS(pci_dev)->device_id; > > pci_conf = pci_dev->config; > > @@ -1531,6 +1545,7 @@ static int pci_e1000_init(PCIDevice *pci_dev) > macaddr = d->conf.macaddr.a; > for (i = 0; i < 3; i++) > d->eeprom_data[i] = (macaddr[2*i+1]<<8) | macaddr[2*i]; > + d->eeprom_data[11] = d->eeprom_data[13] = dev_id; > for (i = 0; i < EEPROM_CHECKSUM_REG; i++) > checksum += d->eeprom_data[i]; > checksum = (uint16_t) EEPROM_SUM - checksum; > @@ -1564,17 +1579,25 @@ static Property e1000_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +typedef struct E1000Info_st { > + const char *name; > + uint16_t vendor_id; > + uint16_t device_id; > + uint8_t revision; > +} E1000Info; > + > static void e1000_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + const E1000Info *info = data; > > k->init = pci_e1000_init; > k->exit = pci_e1000_uninit; > k->romfile = "efi-e1000.rom"; > - k->vendor_id = PCI_VENDOR_ID_INTEL; > - k->device_id = E1000_DEVID; > - k->revision = 0x03; > + k->vendor_id = info->vendor_id; > + k->device_id = info->device_id; > + k->revision = info->revision; > k->class_id = PCI_CLASS_NETWORK_ETHERNET; > set_bit(DEVICE_CATEGORY_NETWORK, dc->categories); > dc->desc = "Intel Gigabit Ethernet"; > @@ -1583,16 +1606,56 @@ static void e1000_class_init(ObjectClass *klass, void *data) > dc->props = e1000_properties; > } > > -static const TypeInfo e1000_info = { > +static const TypeInfo e1000_base_info = { > .name = TYPE_E1000, > .parent = TYPE_PCI_DEVICE, > .instance_size = sizeof(E1000State), > - .class_init = e1000_class_init, > + .abstract = true, > +}; > + > +static const E1000Info e1000_devices[] = { > + { > + .name = "e1000", /* default "alias" for 82540EM */ > + .vendor_id = PCI_VENDOR_ID_INTEL, > + .device_id = E1000_DEV_ID_82540EM, > + .revision = 0x03, > + }, > + { > + .name = "82540EM", > + .vendor_id = PCI_VENDOR_ID_INTEL, > + .device_id = E1000_DEV_ID_82540EM, > + .revision = 0x03, > + }, > + { > + .name = "82544GC", > + .vendor_id = PCI_VENDOR_ID_INTEL, > + .device_id = E1000_DEV_ID_82544GC_COPPER, > + .revision = 0x03, > + }, > + { > + .name = "82545EM", > + .vendor_id = PCI_VENDOR_ID_INTEL, > + .device_id = E1000_DEV_ID_82545EM_COPPER, > + .revision = 0x03, > + }, > }; > > static void e1000_register_types(void) > { > - type_register_static(&e1000_info); > + int i; > + > + type_register_static(&e1000_base_info); > + for (i = 0; i < ARRAY_SIZE(e1000_devices); i++) { > + const E1000Info *info = &e1000_devices[i]; > + TypeInfo type_info = {}; > + > + type_info.name = info->name; > + type_info.parent = TYPE_E1000; > + type_info.class_data = (void *)info; > + type_info.class_init = e1000_class_init; > + > + type_register(&type_info); > + } > } > > type_init(e1000_register_types) The QOM modeling looks right, and I can't think of a better way to handle "e1000" type name compatibility reliably. What's missing is an update to tests/e1000-test.c to test those three new devices as part of "make check" as well. May involve moving qtest_start()/qtest_end() from main() to what is now the nop() test function and calling qtest_add_func() four times. Regards, Andreas
On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote: > Hi, > > Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: > > Allow selection of different card models from the qemu > > command line, to better accomodate a wider range of guests. > > > > Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> > > If that patch carried a Signed-off-by line, you should retain it. Actually I think that would be confusing. Romain didn't sign off on *this* patch, he signed off on a previous one. A signature by Gabriel indicates Developer's Certificate of Origin 1.1 which has an option to incorporate other's work - it does not seem to require signatures by these others. > Your > From: line already indicates that it has been rewritten since. > > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu> > > --- > > > > Based on the conversation in [2,3,4], and given that I would still like > > OS X to work as close to "out of the box" as possible, I took a stab > > at isolating just the bits that allow command-line selection of the > > specific e1000 device model from Romain's patch set [1]. Hope that's > > OK with everyone :) > > > > Beyond a few small style improvements, I'm still not 100% familiar with > > QOM, so I wonder if there isn't a cleaner way to offer "e1000" as some > > sort of "alias" to the default 82540EM, instead of listing two different > > devices (e1000 and 82540EM) with otherwise 100% identical properties. > > However, if the patch is acceptable as-is, I'm happy to leave it be... :) > > > > I tested this on Windows 7 (where the default e1000 continues to work > > as before), Linux (F20, which is fine with either model), and OS X > > (10.9 requires E1000_DEV_ID_82545EM_COPPER, which also works with prior > > versions starting at 10.6; and the default e1000 works on versions <= 10.8). > > > > Comments, suggestions, (or even a straight-forward upstream acceptance > > of the patch) much appreciated ! :) > > > > Thanks, > > Gabriel > > > > [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01397.html > > [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01831.html > > [3] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01841.html > > [4] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg03669.html > > > > hw/net/e1000.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 80 insertions(+), 17 deletions(-) > > > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > > index 8387443..92323a3 100644 > > --- a/hw/net/e1000.c > > +++ b/hw/net/e1000.c > > @@ -69,23 +69,29 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL); > > > > /* > > * HW models: > > - * E1000_DEV_ID_82540EM works with Windows and Linux > > + * E1000_DEV_ID_82540EM works with Windows, Linux, and OS X <= 10.8 > > * E1000_DEV_ID_82573L OK with windoze and Linux 2.6.22, > > * appears to perform better than 82540EM, but breaks with Linux 2.6.18 > > * E1000_DEV_ID_82544GC_COPPER appears to work; not well tested > > + * E1000_DEV_ID_82545EM_COPPER works with Linux and OS X >= 10.6 > > * Others never tested > > */ > > -enum { E1000_DEVID = E1000_DEV_ID_82540EM }; > > > > /* > > * May need to specify additional MAC-to-PHY entries -- > > * Intel's Windows driver refuses to initialize unless they match > > */ > > -enum { > > - PHY_ID2_INIT = E1000_DEVID == E1000_DEV_ID_82573L ? 0xcc2 : > > - E1000_DEVID == E1000_DEV_ID_82544GC_COPPER ? 0xc30 : > > - /* default to E1000_DEV_ID_82540EM */ 0xc20 > > -}; > > +static uint16_t > > static inline uint16_t maybe? inline in c file is an optimization hint for a compiler, so really useless without a benchmark. It's useful in headers since some compilers warn about unused non-inline static functions. > > +e1000_phy_id2_init(uint16_t dev_id) { > > + switch(dev_id) { > > switch (dev_id) { > > > + case E1000_DEV_ID_82573L: > > + return 0xcc2; > > + case E1000_DEV_ID_82544GC_COPPER: > > + return 0xc30; > > + default: > > + return 0xc20; /* default for 82540EM and others */ > > + } > > +} > > > > typedef struct E1000State_st { > > /*< private >*/ > > @@ -151,7 +157,7 @@ typedef struct E1000State_st { > > uint32_t compat_flags; > > } E1000State; > > > > -#define TYPE_E1000 "e1000" > > +#define TYPE_E1000 "e1000-base" > > > > #define E1000(obj) \ > > OBJECT_CHECK(E1000State, (obj), TYPE_E1000) > > @@ -235,7 +241,7 @@ static const char phy_regcap[0x20] = { > > static const uint16_t phy_reg_init[] = { > > [PHY_CTRL] = 0x1140, > > [PHY_STATUS] = 0x794d, /* link initially up with not completed autoneg */ > > - [PHY_ID1] = 0x141, [PHY_ID2] = PHY_ID2_INIT, > > + [PHY_ID1] = 0x141, /* [PHY_ID2] configured per DevId, from e1000_reset() */ > > [PHY_1000T_CTRL] = 0x0e00, [M88E1000_PHY_SPEC_CTRL] = 0x360, > > [M88E1000_EXT_PHY_SPEC_CTRL] = 0x0d60, [PHY_AUTONEG_ADV] = 0xde1, > > [PHY_LP_ABILITY] = 0x1e0, [PHY_1000T_STATUS] = 0x3c00, > > @@ -271,8 +277,9 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) > > PCIDevice *d = PCI_DEVICE(s); > > PCIDeviceClass *pdc = PCI_DEVICE_GET_CLASS(s); // note: not d > > > uint32_t pending_ints; > > uint32_t mit_delay; > > + uint16_t dev_id = PCI_DEVICE_GET_CLASS(d)->device_id; > > uint16_t dev_id = pdc->device_id; > > By convention, don't use these macros inline in FOO(bar)->baz style > expressions, use a variable. > Applies elsewhere as well. > > > > - if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) { > > + if (val && (dev_id >= E1000_DEV_ID_82547EI_MOBILE)) { > > /* Only for 8257x */ I think above is an old bug: E1000_DEV_ID_82547EI_MOBILE is not 8257x, is it? Also ICH8 is not a 8257x either. so I think a flag in E1000State would be cleaner: bool is_8257x. in init, do a switch statement on device ID and set of 8257x ones. avoiding QOM on data path is also a good idea. > > val |= E1000_ICR_INT_ASSERTED; > > } > > @@ -377,6 +384,7 @@ static void e1000_reset(void *opaque) > > E1000State *d = opaque; > > uint8_t *macaddr = d->conf.macaddr.a; > > int i; > > + uint16_t dev_id = PCI_DEVICE_GET_CLASS(d)->device_id; > > > > timer_del(d->autoneg_timer); > > timer_del(d->mit_timer); > > @@ -385,6 +393,7 @@ static void e1000_reset(void *opaque) > > d->mit_ide = 0; > > memset(d->phy_reg, 0, sizeof d->phy_reg); > > memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init); > > + d->phy_reg[PHY_ID2] = e1000_phy_id2_init(dev_id); I would just pass E1000State to e1000_phy_id2_init. > > memset(d->mac_reg, 0, sizeof d->mac_reg); > > memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init); > > d->rxbuf_min_shift = 1; > > @@ -1440,9 +1449,13 @@ static const VMStateDescription vmstate_e1000 = { > > } > > }; > > > > +/* > > + * EEPROM contents documented in Tables 5-2 and 5-3, pp. 98-102. > > + * Note: A valid DevId will be inserted during pci_e1000_init(). > > + */ > > static const uint16_t e1000_eeprom_template[64] = { > > 0x0000, 0x0000, 0x0000, 0x0000, 0xffff, 0x0000, 0x0000, 0x0000, > > - 0x3000, 0x1000, 0x6403, E1000_DEVID, 0x8086, E1000_DEVID, 0x8086, 0x3040, > > + 0x3000, 0x1000, 0x6403, 0 /*DevId*/, 0x8086, 0 /*DevId*/, 0x8086, 0x3040, > > 0x0008, 0x2000, 0x7e14, 0x0048, 0x1000, 0x00d8, 0x0000, 0x2700, > > 0x6cc9, 0x3150, 0x0722, 0x040b, 0x0984, 0x0000, 0xc000, 0x0706, > > 0x1008, 0x0000, 0x0f04, 0x7fff, 0x4d01, 0xffff, 0xffff, 0xffff, > > @@ -1511,6 +1524,7 @@ static int pci_e1000_init(PCIDevice *pci_dev) > > uint16_t checksum = 0; > > int i; > > uint8_t *macaddr; > > + uint16_t dev_id = PCI_DEVICE_GET_CLASS(pci_dev)->device_id; > > > > pci_conf = pci_dev->config; > > > > @@ -1531,6 +1545,7 @@ static int pci_e1000_init(PCIDevice *pci_dev) > > macaddr = d->conf.macaddr.a; > > for (i = 0; i < 3; i++) > > d->eeprom_data[i] = (macaddr[2*i+1]<<8) | macaddr[2*i]; > > + d->eeprom_data[11] = d->eeprom_data[13] = dev_id; > > for (i = 0; i < EEPROM_CHECKSUM_REG; i++) > > checksum += d->eeprom_data[i]; > > checksum = (uint16_t) EEPROM_SUM - checksum; > > @@ -1564,17 +1579,25 @@ static Property e1000_properties[] = { > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > +typedef struct E1000Info_st { > > + const char *name; > > + uint16_t vendor_id; > > + uint16_t device_id; > > + uint8_t revision; > > +} E1000Info; > > + > > static void e1000_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > > + const E1000Info *info = data; > > > > k->init = pci_e1000_init; > > k->exit = pci_e1000_uninit; > > k->romfile = "efi-e1000.rom"; > > - k->vendor_id = PCI_VENDOR_ID_INTEL; > > - k->device_id = E1000_DEVID; > > - k->revision = 0x03; > > + k->vendor_id = info->vendor_id; > > + k->device_id = info->device_id; > > + k->revision = info->revision; > > k->class_id = PCI_CLASS_NETWORK_ETHERNET; > > set_bit(DEVICE_CATEGORY_NETWORK, dc->categories); > > dc->desc = "Intel Gigabit Ethernet"; > > @@ -1583,16 +1606,56 @@ static void e1000_class_init(ObjectClass *klass, void *data) > > dc->props = e1000_properties; > > } > > > > -static const TypeInfo e1000_info = { > > +static const TypeInfo e1000_base_info = { > > .name = TYPE_E1000, > > .parent = TYPE_PCI_DEVICE, > > .instance_size = sizeof(E1000State), > > - .class_init = e1000_class_init, > > + .abstract = true, > > +}; > > + > > +static const E1000Info e1000_devices[] = { > > + { > > + .name = "e1000", /* default "alias" for 82540EM */ > > + .vendor_id = PCI_VENDOR_ID_INTEL, > > + .device_id = E1000_DEV_ID_82540EM, > > + .revision = 0x03, > > + }, > > + { > > + .name = "82540EM", > > + .vendor_id = PCI_VENDOR_ID_INTEL, > > + .device_id = E1000_DEV_ID_82540EM, > > + .revision = 0x03, > > + }, > > + { > > + .name = "82544GC", > > + .vendor_id = PCI_VENDOR_ID_INTEL, > > + .device_id = E1000_DEV_ID_82544GC_COPPER, > > + .revision = 0x03, > > + }, > > + { > > + .name = "82545EM", > > + .vendor_id = PCI_VENDOR_ID_INTEL, > > + .device_id = E1000_DEV_ID_82545EM_COPPER, > > + .revision = 0x03, > > + }, How about prefixing "e1000-" to the name? Will make it a bit more friendly to users. > > }; > > > > static void e1000_register_types(void) > > { > > - type_register_static(&e1000_info); > > + int i; > > + > > + type_register_static(&e1000_base_info); > > + for (i = 0; i < ARRAY_SIZE(e1000_devices); i++) { > > + const E1000Info *info = &e1000_devices[i]; > > + TypeInfo type_info = {}; > > + > > + type_info.name = info->name; > > + type_info.parent = TYPE_E1000; > > + type_info.class_data = (void *)info; > > + type_info.class_init = e1000_class_init; > > + > > + type_register(&type_info); > > + } > > } > > > > type_init(e1000_register_types) > > The QOM modeling looks right, and I can't think of a better way to > handle "e1000" type name compatibility reliably. > > What's missing is an update to tests/e1000-test.c to test those three > new devices as part of "make check" as well. May involve moving > qtest_start()/qtest_end() from main() to what is now the nop() test > function and calling qtest_add_func() four times. > > Regards, > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Am 21.05.2014 11:04, schrieb Michael S. Tsirkin: > On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote: >> Hi, >> >> Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: >>> Allow selection of different card models from the qemu >>> command line, to better accomodate a wider range of guests. >>> >>> Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> >> >> If that patch carried a Signed-off-by line, you should retain it. > > Actually I think that would be confusing. Romain didn't sign off > on *this* patch, he signed off on a previous one. > A signature by Gabriel indicates Developer's Certificate of Origin 1.1 > which has an option to incorporate other's work - it > does not seem to require signatures by these others. With the same argument you could drop anyone's Sob you get as a maintainer. But the purpose of Sob is to track through whose hands a patch went, not just who last touched it. My point here was that Based-on-patch-by is very unusual. The alternative would be to leave the original From: Romain Dolbeau, his Sob, then a [gsomlo: Dropped this, added that] followed by Sob. Cheers, Andreas
On Wed, May 21, 2014 at 11:12:42AM +0200, Andreas Färber wrote: > Am 21.05.2014 11:04, schrieb Michael S. Tsirkin: > > On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote: > >> Hi, > >> > >> Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: > >>> Allow selection of different card models from the qemu > >>> command line, to better accomodate a wider range of guests. > >>> > >>> Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> > >> > >> If that patch carried a Signed-off-by line, you should retain it. > > > > Actually I think that would be confusing. Romain didn't sign off > > on *this* patch, he signed off on a previous one. > > A signature by Gabriel indicates Developer's Certificate of Origin 1.1 > > which has an option to incorporate other's work - it > > does not seem to require signatures by these others. > > With the same argument you could drop anyone's Sob you get as a > maintainer. I could but it would not be nice to submitters, and it drops useful info (author's Sob). So if someone thinks there's problematic code here and comes complaining, we want to be able to say "this code came from XYZ". > But the purpose of Sob is to track through whose hands a > patch went, not just who last touched it. Went untouched or mostly untouched. Did you bother checking? I looked and Romain's patch isn't very similar to this one. > My point here was that Based-on-patch-by is very unusual. What's the harm? Gabriel's just being nice and crediting other's work. > The alternative would be to leave the original From: Romain Dolbeau, his > Sob, then a [gsomlo: Dropped this, added that] followed by Sob. > > Cheers, > Andreas That's just asking submitter to do a lot of extra work, I don't see why would we place roadblocks in submitter's paths like this. Linux certainly does not and we didn't ask for this in the past. Further, the patch author in git will also be the original author then which is only fair if the patch is changed in very minor ways. In which case keeping the original Sob around *would* be right. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Am 21.05.2014 11:25, schrieb Michael S. Tsirkin: > On Wed, May 21, 2014 at 11:12:42AM +0200, Andreas Färber wrote: >> Am 21.05.2014 11:04, schrieb Michael S. Tsirkin: >>> On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote: >>>> Hi, >>>> >>>> Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: >>>>> Allow selection of different card models from the qemu >>>>> command line, to better accomodate a wider range of guests. >>>>> >>>>> Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> >>>> >>>> If that patch carried a Signed-off-by line, you should retain it. >>> >>> Actually I think that would be confusing. Romain didn't sign off >>> on *this* patch, he signed off on a previous one. >>> A signature by Gabriel indicates Developer's Certificate of Origin 1.1 >>> which has an option to incorporate other's work - it >>> does not seem to require signatures by these others. >> >> With the same argument you could drop anyone's Sob you get as a >> maintainer. > > I could but it would not be nice to submitters, and it drops useful info > (author's Sob). So if someone thinks there's problematic code here and > comes complaining, we want to be able to say "this code came from XYZ". > > >> But the purpose of Sob is to track through whose hands a >> patch went, not just who last touched it. > > Went untouched or mostly untouched. > Did you bother checking? > I looked and Romain's patch isn't very similar to this one. > >> My point here was that Based-on-patch-by is very unusual. > > What's the harm? > Gabriel's just being nice and crediting other's work. > >> The alternative would be to leave the original From: Romain Dolbeau, his >> Sob, then a [gsomlo: Dropped this, added that] followed by Sob. >> >> Cheers, >> Andreas > > That's just asking submitter to do a lot of extra work, > I don't see why would we place roadblocks in submitter's paths > like this. Linux certainly does not and we didn't ask for this > in the past. > > Further, the patch author in git will also be the original author then > which is only fair if the patch is changed in very minor ways. > In which case keeping the original Sob around *would* be right. Either the patch is based on the patch the submitter claims it is based on, or it is not based on that patch. If it is, then the Sob should be retained because not doing so is dropping useful information as you put it. You will find both ways, From new and old+new Sob or From original and [], in git history, depending on how much changed (which I have not checked here). If it is not based on Romain's patch, then Suggested-by would be much more to the point - and something the maintainer (Stefan) could easily edit when signing off, if there were nothing else to change. Andreas
On Wed, May 21, 2014 at 11:48:48AM +0200, Andreas Färber wrote: > Am 21.05.2014 11:25, schrieb Michael S. Tsirkin: > > On Wed, May 21, 2014 at 11:12:42AM +0200, Andreas Färber wrote: > >> Am 21.05.2014 11:04, schrieb Michael S. Tsirkin: > >>> On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote: > >>>> Hi, > >>>> > >>>> Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: > >>>>> Allow selection of different card models from the qemu > >>>>> command line, to better accomodate a wider range of guests. > >>>>> > >>>>> Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> > >>>> > >>>> If that patch carried a Signed-off-by line, you should retain it. > >>> > >>> Actually I think that would be confusing. Romain didn't sign off > >>> on *this* patch, he signed off on a previous one. > >>> A signature by Gabriel indicates Developer's Certificate of Origin 1.1 > >>> which has an option to incorporate other's work - it > >>> does not seem to require signatures by these others. > >> > >> With the same argument you could drop anyone's Sob you get as a > >> maintainer. > > > > I could but it would not be nice to submitters, and it drops useful info > > (author's Sob). So if someone thinks there's problematic code here and > > comes complaining, we want to be able to say "this code came from XYZ". > > > > > >> But the purpose of Sob is to track through whose hands a > >> patch went, not just who last touched it. > > > > Went untouched or mostly untouched. > > Did you bother checking? > > I looked and Romain's patch isn't very similar to this one. > > > >> My point here was that Based-on-patch-by is very unusual. > > > > What's the harm? > > Gabriel's just being nice and crediting other's work. > > > >> The alternative would be to leave the original From: Romain Dolbeau, his > >> Sob, then a [gsomlo: Dropped this, added that] followed by Sob. > >> > >> Cheers, > >> Andreas > > > > That's just asking submitter to do a lot of extra work, > > I don't see why would we place roadblocks in submitter's paths > > like this. Linux certainly does not and we didn't ask for this > > in the past. > > > > Further, the patch author in git will also be the original author then > > which is only fair if the patch is changed in very minor ways. > > In which case keeping the original Sob around *would* be right. > > Either the patch is based on the patch the submitter claims it is based > on, or it is not based on that patch. > > If it is, then the Sob should be retained because not doing so is > dropping useful information as you put it. It isn't useful if most of the patch has been changed, because Sob without the patch itself has no meaning. > You will find both ways, From > new and old+new Sob or From original and [], in git history, depending > on how much changed (which I have not checked here). Saying "Based on patch" in commit log is common practice too. You really can not redefine the meaning of Sob - it is widely understood to mean a very specific thing, Developer's certificate of origin 1.1 (which, by the way, qemu source really should include a copy of), which in particular says: (b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or so if you base upon legal previous work and can certify that, it is absolutely not required to track authors of that one and add their signatures. What if I copy code from virtual box's qemu? Would you make me troll their log for signatures? When to do that and when not is a judgement call on behalf of the submitter. > > If it is not based on Romain's patch, then Suggested-by would be much > more to the point - and something the maintainer (Stefan) could easily > edit when signing off, if there were nothing else to change. > > Andreas Suggested-by implies there wasn't a patch, just a suggestion. Maintainer can add any text on to the patch, that's also accepted practice. I am merely saying it does not make sense to push back on contributors who are just trying to be nice. Really there's no rule I can see that says you should not add random tags to the commit log, and I don't see what would making up such rules gain us either. This is why I'm still responding to this thread really, you seem to make up new requirements that submitters need to meet, such things would have to get buy-in from more maintainers before we ask everyone to comply. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Am 21.05.2014 12:22, schrieb Michael S. Tsirkin: > On Wed, May 21, 2014 at 11:48:48AM +0200, Andreas Färber wrote: >> Am 21.05.2014 11:25, schrieb Michael S. Tsirkin: >>> On Wed, May 21, 2014 at 11:12:42AM +0200, Andreas Färber wrote: >>>> Am 21.05.2014 11:04, schrieb Michael S. Tsirkin: >>>>> On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote: >>>>>> Hi, >>>>>> >>>>>> Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: >>>>>>> Allow selection of different card models from the qemu >>>>>>> command line, to better accomodate a wider range of guests. >>>>>>> >>>>>>> Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> >>>>>> >>>>>> If that patch carried a Signed-off-by line, you should retain it. >>>>> >>>>> Actually I think that would be confusing. Romain didn't sign off >>>>> on *this* patch, he signed off on a previous one. >>>>> A signature by Gabriel indicates Developer's Certificate of Origin 1.1 >>>>> which has an option to incorporate other's work - it >>>>> does not seem to require signatures by these others. >>>> >>>> With the same argument you could drop anyone's Sob you get as a >>>> maintainer. >>> >>> I could but it would not be nice to submitters, and it drops useful info >>> (author's Sob). So if someone thinks there's problematic code here and >>> comes complaining, we want to be able to say "this code came from XYZ". >>> >>> >>>> But the purpose of Sob is to track through whose hands a >>>> patch went, not just who last touched it. >>> >>> Went untouched or mostly untouched. >>> Did you bother checking? >>> I looked and Romain's patch isn't very similar to this one. >>> >>>> My point here was that Based-on-patch-by is very unusual. >>> >>> What's the harm? >>> Gabriel's just being nice and crediting other's work. >>> >>>> The alternative would be to leave the original From: Romain Dolbeau, his >>>> Sob, then a [gsomlo: Dropped this, added that] followed by Sob. >>>> >>>> Cheers, >>>> Andreas >>> >>> That's just asking submitter to do a lot of extra work, >>> I don't see why would we place roadblocks in submitter's paths >>> like this. Linux certainly does not and we didn't ask for this >>> in the past. >>> >>> Further, the patch author in git will also be the original author then >>> which is only fair if the patch is changed in very minor ways. >>> In which case keeping the original Sob around *would* be right. >> >> Either the patch is based on the patch the submitter claims it is based >> on, or it is not based on that patch. >> >> If it is, then the Sob should be retained because not doing so is >> dropping useful information as you put it. > > It isn't useful if most of the patch has been changed, > because Sob without the patch itself has no meaning. > >> You will find both ways, From >> new and old+new Sob or From original and [], in git history, depending >> on how much changed (which I have not checked here). > > Saying "Based on patch" in commit log is common practice too. > > You really can not redefine the meaning of Sob - it is > widely understood to mean a very specific thing, > Developer's certificate of origin 1.1 (which, by the way, qemu source > really should include a copy of), which in particular says: > > (b) The contribution is based upon previous work that, to the best > of my knowledge, is covered under an appropriate open source > license and I have the right under that license to submit that > work with modifications, whether created in whole or in part > by me, under the same open source license (unless I am > permitted to submit under a different license), as indicated > in the file; or > > so if you base upon legal previous work and can certify that, it is > absolutely not required to track authors of that one and add their > signatures. What if I copy code from virtual box's qemu? Would you make > me troll their log for signatures? When to do that and when not is a > judgement call on behalf of the submitter. > >> >> If it is not based on Romain's patch, then Suggested-by would be much >> more to the point - and something the maintainer (Stefan) could easily >> edit when signing off, if there were nothing else to change. >> >> Andreas > > Suggested-by implies there wasn't a patch, just a suggestion. To me, it doesn't imply that. > Maintainer can add any text on to the patch, that's also > accepted practice. I am merely saying it does not make sense to push > back on contributors who are just trying to be nice. > > Really there's no rule I can see that says you should not > add random tags to the commit log, and I don't see what would > making up such rules gain us either. > > This is why I'm still responding to this thread really, > you seem to make up new requirements that submitters need to > meet, such things would have to get buy-in from more maintainers > before we ask everyone to comply. I feel you are turning my words around in my mouth. Let's agree that we don't agree here. * I never redefined the DCO. The DCO applies to a single Sob and by my reading does not restrict having more than one Sob at all, and you agreed that this is common practice. * I did not reject the patch on that basis, I remarked it alongside contential review. What the contributor makes of it depends on him, and he hasn't spoken up so far, so you're not his lawyer either. * People are free to invent new *-bys, but they shouldn't when there exists one of different name and precise meaning, in particular Sob. This is not against random *-by, it's specifically about Based-on-patch-by which sounded redundant to Sob for me. * We are having GSoC students around, and they shouldn't be picking up bad examples just because no one else cares to comment on or fix commit messages like I do. Linux also lets a lot of typos slip through, doesn't mean we need to when we notice them. CC'ing PMM, who has stated opinions on how to do these things too, so it's unfair to pretend that it were just me making this up. Andreas
On Wed, May 21, 2014 at 12:42:13PM +0200, Andreas Färber wrote: > Am 21.05.2014 12:22, schrieb Michael S. Tsirkin: > > On Wed, May 21, 2014 at 11:48:48AM +0200, Andreas Färber wrote: > >> Am 21.05.2014 11:25, schrieb Michael S. Tsirkin: > >>> On Wed, May 21, 2014 at 11:12:42AM +0200, Andreas Färber wrote: > >>>> Am 21.05.2014 11:04, schrieb Michael S. Tsirkin: > >>>>> On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote: > >>>>>> Hi, > >>>>>> > >>>>>> Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: > >>>>>>> Allow selection of different card models from the qemu > >>>>>>> command line, to better accomodate a wider range of guests. > >>>>>>> > >>>>>>> Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> > >>>>>> > >>>>>> If that patch carried a Signed-off-by line, you should retain it. > >>>>> > >>>>> Actually I think that would be confusing. Romain didn't sign off > >>>>> on *this* patch, he signed off on a previous one. > >>>>> A signature by Gabriel indicates Developer's Certificate of Origin 1.1 > >>>>> which has an option to incorporate other's work - it > >>>>> does not seem to require signatures by these others. > >>>> > >>>> With the same argument you could drop anyone's Sob you get as a > >>>> maintainer. > >>> > >>> I could but it would not be nice to submitters, and it drops useful info > >>> (author's Sob). So if someone thinks there's problematic code here and > >>> comes complaining, we want to be able to say "this code came from XYZ". > >>> > >>> > >>>> But the purpose of Sob is to track through whose hands a > >>>> patch went, not just who last touched it. > >>> > >>> Went untouched or mostly untouched. > >>> Did you bother checking? > >>> I looked and Romain's patch isn't very similar to this one. > >>> > >>>> My point here was that Based-on-patch-by is very unusual. > >>> > >>> What's the harm? > >>> Gabriel's just being nice and crediting other's work. > >>> > >>>> The alternative would be to leave the original From: Romain Dolbeau, his > >>>> Sob, then a [gsomlo: Dropped this, added that] followed by Sob. > >>>> > >>>> Cheers, > >>>> Andreas > >>> > >>> That's just asking submitter to do a lot of extra work, > >>> I don't see why would we place roadblocks in submitter's paths > >>> like this. Linux certainly does not and we didn't ask for this > >>> in the past. > >>> > >>> Further, the patch author in git will also be the original author then > >>> which is only fair if the patch is changed in very minor ways. > >>> In which case keeping the original Sob around *would* be right. > >> > >> Either the patch is based on the patch the submitter claims it is based > >> on, or it is not based on that patch. > >> > >> If it is, then the Sob should be retained because not doing so is > >> dropping useful information as you put it. > > > > It isn't useful if most of the patch has been changed, > > because Sob without the patch itself has no meaning. > > > >> You will find both ways, From > >> new and old+new Sob or From original and [], in git history, depending > >> on how much changed (which I have not checked here). > > > > Saying "Based on patch" in commit log is common practice too. > > > > You really can not redefine the meaning of Sob - it is > > widely understood to mean a very specific thing, > > Developer's certificate of origin 1.1 (which, by the way, qemu source > > really should include a copy of), which in particular says: > > > > (b) The contribution is based upon previous work that, to the best > > of my knowledge, is covered under an appropriate open source > > license and I have the right under that license to submit that > > work with modifications, whether created in whole or in part > > by me, under the same open source license (unless I am > > permitted to submit under a different license), as indicated > > in the file; or > > > > so if you base upon legal previous work and can certify that, it is > > absolutely not required to track authors of that one and add their > > signatures. What if I copy code from virtual box's qemu? Would you make > > me troll their log for signatures? When to do that and when not is a > > judgement call on behalf of the submitter. > > > >> > >> If it is not based on Romain's patch, then Suggested-by would be much > >> more to the point - and something the maintainer (Stefan) could easily > >> edit when signing off, if there were nothing else to change. > >> > >> Andreas > > > > Suggested-by implies there wasn't a patch, just a suggestion. > > To me, it doesn't imply that. > > > Maintainer can add any text on to the patch, that's also > > accepted practice. I am merely saying it does not make sense to push > > back on contributors who are just trying to be nice. > > > > Really there's no rule I can see that says you should not > > add random tags to the commit log, and I don't see what would > > making up such rules gain us either. > > > > This is why I'm still responding to this thread really, > > you seem to make up new requirements that submitters need to > > meet, such things would have to get buy-in from more maintainers > > before we ask everyone to comply. > > I feel you are turning my words around in my mouth. Let's agree that we > don't agree here. Hmm I certainly didn't intend to, so maybe there's a misunderstanding. > * I never redefined the DCO. The DCO applies to a single Sob and by my > reading does not restrict having more than one Sob at all, and you > agreed that this is common practice. > * I did not reject the patch on that basis, I remarked it alongside > contential review. Ah OK. It's where you said "you should retain it" that made me think you are saying tracking Sob from original sources is mandatory. so for the record, are you basically saying: "you can replace Based-on-patch-by with Signed-off-by, it's more or less equivalent, and more standard" Then I agree. Can you confirm please? I would add that - it's possible to include text similar to "Based on patch by ABC" in plain text in addition to, or instead of the tag. - If the patch is mostly identical to the original, with trivial small modifications such as fixing typos, it's also possible to add From: ABC in front of the patch, this will then be the author recorded in the project history It's also possible to include the message id of the original patch if you really want to, services such as mid.gmane.org can later be used to look up messages based on the message id. > What the contributor makes of it depends on him, and > he hasn't spoken up so far, so you're not his lawyer either. > * People are free to invent new *-bys, but they shouldn't when there > exists one of different name and precise meaning, in particular Sob. > This is not against random *-by, it's specifically about > Based-on-patch-by which sounded redundant to Sob for me. > * We are having GSoC students around, and they shouldn't be picking up > bad examples just because no one else cares to comment on or fix commit > messages like I do. Linux also lets a lot of typos slip through, doesn't > mean we need to when we notice them. Right. It's exactly for the benefit of the new people that I am responding here, we should try to make the rules explicit, to reduce the barrier of entry. > CC'ing PMM, who has stated opinions on how to do these things too, so > it's unfair to pretend that it were just me making this up. > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Am 21.05.2014 14:11, schrieb Michael S. Tsirkin: > On Wed, May 21, 2014 at 12:42:13PM +0200, Andreas Färber wrote: >> Am 21.05.2014 12:22, schrieb Michael S. Tsirkin: >>> On Wed, May 21, 2014 at 11:48:48AM +0200, Andreas Färber wrote: >>>> Am 21.05.2014 11:25, schrieb Michael S. Tsirkin: >>>>> On Wed, May 21, 2014 at 11:12:42AM +0200, Andreas Färber wrote: >>>>>> Am 21.05.2014 11:04, schrieb Michael S. Tsirkin: >>>>>>> On Wed, May 21, 2014 at 10:40:54AM +0200, Andreas Färber wrote: >>>>>>>> Am 20.05.2014 17:05, schrieb Gabriel L. Somlo: >>>>>>>>> Allow selection of different card models from the qemu >>>>>>>>> command line, to better accomodate a wider range of guests. >>>>>>>>> >>>>>>>>> Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> >>>>>>>> >>>>>>>> If that patch carried a Signed-off-by line, you should retain it. >>>>>>> >>>>>>> Actually I think that would be confusing. Romain didn't sign off >>>>>>> on *this* patch, he signed off on a previous one. >>>>>>> A signature by Gabriel indicates Developer's Certificate of Origin 1.1 >>>>>>> which has an option to incorporate other's work - it >>>>>>> does not seem to require signatures by these others. >>>>>> >>>>>> With the same argument you could drop anyone's Sob you get as a >>>>>> maintainer. >>>>> >>>>> I could but it would not be nice to submitters, and it drops useful info >>>>> (author's Sob). So if someone thinks there's problematic code here and >>>>> comes complaining, we want to be able to say "this code came from XYZ". >>>>> >>>>> >>>>>> But the purpose of Sob is to track through whose hands a >>>>>> patch went, not just who last touched it. >>>>> >>>>> Went untouched or mostly untouched. >>>>> Did you bother checking? >>>>> I looked and Romain's patch isn't very similar to this one. >>>>> >>>>>> My point here was that Based-on-patch-by is very unusual. >>>>> >>>>> What's the harm? >>>>> Gabriel's just being nice and crediting other's work. >>>>> >>>>>> The alternative would be to leave the original From: Romain Dolbeau, his >>>>>> Sob, then a [gsomlo: Dropped this, added that] followed by Sob. >>>>>> >>>>>> Cheers, >>>>>> Andreas >>>>> >>>>> That's just asking submitter to do a lot of extra work, >>>>> I don't see why would we place roadblocks in submitter's paths >>>>> like this. Linux certainly does not and we didn't ask for this >>>>> in the past. >>>>> >>>>> Further, the patch author in git will also be the original author then >>>>> which is only fair if the patch is changed in very minor ways. >>>>> In which case keeping the original Sob around *would* be right. >>>> >>>> Either the patch is based on the patch the submitter claims it is based >>>> on, or it is not based on that patch. >>>> >>>> If it is, then the Sob should be retained because not doing so is >>>> dropping useful information as you put it. >>> >>> It isn't useful if most of the patch has been changed, >>> because Sob without the patch itself has no meaning. >>> >>>> You will find both ways, From >>>> new and old+new Sob or From original and [], in git history, depending >>>> on how much changed (which I have not checked here). >>> >>> Saying "Based on patch" in commit log is common practice too. >>> >>> You really can not redefine the meaning of Sob - it is >>> widely understood to mean a very specific thing, >>> Developer's certificate of origin 1.1 (which, by the way, qemu source >>> really should include a copy of), which in particular says: >>> >>> (b) The contribution is based upon previous work that, to the best >>> of my knowledge, is covered under an appropriate open source >>> license and I have the right under that license to submit that >>> work with modifications, whether created in whole or in part >>> by me, under the same open source license (unless I am >>> permitted to submit under a different license), as indicated >>> in the file; or >>> >>> so if you base upon legal previous work and can certify that, it is >>> absolutely not required to track authors of that one and add their >>> signatures. What if I copy code from virtual box's qemu? Would you make >>> me troll their log for signatures? When to do that and when not is a >>> judgement call on behalf of the submitter. >>> >>>> >>>> If it is not based on Romain's patch, then Suggested-by would be much >>>> more to the point - and something the maintainer (Stefan) could easily >>>> edit when signing off, if there were nothing else to change. >>> >>> Suggested-by implies there wasn't a patch, just a suggestion. >> >> To me, it doesn't imply that. >> >>> Maintainer can add any text on to the patch, that's also >>> accepted practice. I am merely saying it does not make sense to push >>> back on contributors who are just trying to be nice. >>> >>> Really there's no rule I can see that says you should not >>> add random tags to the commit log, and I don't see what would >>> making up such rules gain us either. >>> >>> This is why I'm still responding to this thread really, >>> you seem to make up new requirements that submitters need to >>> meet, such things would have to get buy-in from more maintainers >>> before we ask everyone to comply. >> >> I feel you are turning my words around in my mouth. Let's agree that we >> don't agree here. > > Hmm I certainly didn't intend to, so maybe there's a misunderstanding. > >> * I never redefined the DCO. The DCO applies to a single Sob and by my >> reading does not restrict having more than one Sob at all, and you >> agreed that this is common practice. >> * I did not reject the patch on that basis, I remarked it alongside >> contential review. > > Ah OK. It's where you said "you should retain it" that made me think > you are saying tracking Sob from original sources is mandatory. In case my use of "retain" was unclear, I specifically meant: Never *drop* Signed-off-bys from a patch you are modifying and resending. > so for the record, are you basically saying: > "you can replace Based-on-patch-by with Signed-off-by, it's > more or less equivalent, and more standard" > > > Then I agree. > Can you confirm please? Confirmed, assuming that original patch has such Signed-off-by(s). > I would add that > > > - it's possible to include text similar to > "Based on patch by ABC" in plain text > in addition to, or instead of the tag. Yep, when in doubt as explanation. Not machine-parseable of course, so my recommendation would be "in addition to", but otherwise equivalent to Some-newly-invented-by. > - If the patch is mostly identical to the original, with trivial > small modifications such as fixing typos, it's also possible to add > From: ABC > in front of the patch, this will then be the author recorded > in the project history Best, to not have it end up in the wrong place: git commit --amend --author="name <email>" Or actually the easiest variant: git am original-author-s.patch; git commit --amend -a -s > It's also possible to include the message id of the original patch if > you really want to, services such as mid.gmane.org can later be used to > look up messages based on the message id. To be precise, you probably meant a Message-id: line as added by Anthony's patches tool. e.g. http://git.qemu-project.org/?p=qemu.git;a=commit;h=e3da9921ebc554fad3224a9fdda9a7425ffd9ef7 Archive links OTOH can become stale after some months/years. But not wrong. And certainly useful in cover letter or, as here, below ---. Andreas
diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 8387443..92323a3 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -69,23 +69,29 @@ static int debugflags = DBGBIT(TXERR) | DBGBIT(GENERAL); /* * HW models: - * E1000_DEV_ID_82540EM works with Windows and Linux + * E1000_DEV_ID_82540EM works with Windows, Linux, and OS X <= 10.8 * E1000_DEV_ID_82573L OK with windoze and Linux 2.6.22, * appears to perform better than 82540EM, but breaks with Linux 2.6.18 * E1000_DEV_ID_82544GC_COPPER appears to work; not well tested + * E1000_DEV_ID_82545EM_COPPER works with Linux and OS X >= 10.6 * Others never tested */ -enum { E1000_DEVID = E1000_DEV_ID_82540EM }; /* * May need to specify additional MAC-to-PHY entries -- * Intel's Windows driver refuses to initialize unless they match */ -enum { - PHY_ID2_INIT = E1000_DEVID == E1000_DEV_ID_82573L ? 0xcc2 : - E1000_DEVID == E1000_DEV_ID_82544GC_COPPER ? 0xc30 : - /* default to E1000_DEV_ID_82540EM */ 0xc20 -}; +static uint16_t +e1000_phy_id2_init(uint16_t dev_id) { + switch(dev_id) { + case E1000_DEV_ID_82573L: + return 0xcc2; + case E1000_DEV_ID_82544GC_COPPER: + return 0xc30; + default: + return 0xc20; /* default for 82540EM and others */ + } +} typedef struct E1000State_st { /*< private >*/ @@ -151,7 +157,7 @@ typedef struct E1000State_st { uint32_t compat_flags; } E1000State; -#define TYPE_E1000 "e1000" +#define TYPE_E1000 "e1000-base" #define E1000(obj) \ OBJECT_CHECK(E1000State, (obj), TYPE_E1000) @@ -235,7 +241,7 @@ static const char phy_regcap[0x20] = { static const uint16_t phy_reg_init[] = { [PHY_CTRL] = 0x1140, [PHY_STATUS] = 0x794d, /* link initially up with not completed autoneg */ - [PHY_ID1] = 0x141, [PHY_ID2] = PHY_ID2_INIT, + [PHY_ID1] = 0x141, /* [PHY_ID2] configured per DevId, from e1000_reset() */ [PHY_1000T_CTRL] = 0x0e00, [M88E1000_PHY_SPEC_CTRL] = 0x360, [M88E1000_EXT_PHY_SPEC_CTRL] = 0x0d60, [PHY_AUTONEG_ADV] = 0xde1, [PHY_LP_ABILITY] = 0x1e0, [PHY_1000T_STATUS] = 0x3c00, @@ -271,8 +277,9 @@ set_interrupt_cause(E1000State *s, int index, uint32_t val) PCIDevice *d = PCI_DEVICE(s); uint32_t pending_ints; uint32_t mit_delay; + uint16_t dev_id = PCI_DEVICE_GET_CLASS(d)->device_id; - if (val && (E1000_DEVID >= E1000_DEV_ID_82547EI_MOBILE)) { + if (val && (dev_id >= E1000_DEV_ID_82547EI_MOBILE)) { /* Only for 8257x */ val |= E1000_ICR_INT_ASSERTED; } @@ -377,6 +384,7 @@ static void e1000_reset(void *opaque) E1000State *d = opaque; uint8_t *macaddr = d->conf.macaddr.a; int i; + uint16_t dev_id = PCI_DEVICE_GET_CLASS(d)->device_id; timer_del(d->autoneg_timer); timer_del(d->mit_timer); @@ -385,6 +393,7 @@ static void e1000_reset(void *opaque) d->mit_ide = 0; memset(d->phy_reg, 0, sizeof d->phy_reg); memmove(d->phy_reg, phy_reg_init, sizeof phy_reg_init); + d->phy_reg[PHY_ID2] = e1000_phy_id2_init(dev_id); memset(d->mac_reg, 0, sizeof d->mac_reg); memmove(d->mac_reg, mac_reg_init, sizeof mac_reg_init); d->rxbuf_min_shift = 1; @@ -1440,9 +1449,13 @@ static const VMStateDescription vmstate_e1000 = { } }; +/* + * EEPROM contents documented in Tables 5-2 and 5-3, pp. 98-102. + * Note: A valid DevId will be inserted during pci_e1000_init(). + */ static const uint16_t e1000_eeprom_template[64] = { 0x0000, 0x0000, 0x0000, 0x0000, 0xffff, 0x0000, 0x0000, 0x0000, - 0x3000, 0x1000, 0x6403, E1000_DEVID, 0x8086, E1000_DEVID, 0x8086, 0x3040, + 0x3000, 0x1000, 0x6403, 0 /*DevId*/, 0x8086, 0 /*DevId*/, 0x8086, 0x3040, 0x0008, 0x2000, 0x7e14, 0x0048, 0x1000, 0x00d8, 0x0000, 0x2700, 0x6cc9, 0x3150, 0x0722, 0x040b, 0x0984, 0x0000, 0xc000, 0x0706, 0x1008, 0x0000, 0x0f04, 0x7fff, 0x4d01, 0xffff, 0xffff, 0xffff, @@ -1511,6 +1524,7 @@ static int pci_e1000_init(PCIDevice *pci_dev) uint16_t checksum = 0; int i; uint8_t *macaddr; + uint16_t dev_id = PCI_DEVICE_GET_CLASS(pci_dev)->device_id; pci_conf = pci_dev->config; @@ -1531,6 +1545,7 @@ static int pci_e1000_init(PCIDevice *pci_dev) macaddr = d->conf.macaddr.a; for (i = 0; i < 3; i++) d->eeprom_data[i] = (macaddr[2*i+1]<<8) | macaddr[2*i]; + d->eeprom_data[11] = d->eeprom_data[13] = dev_id; for (i = 0; i < EEPROM_CHECKSUM_REG; i++) checksum += d->eeprom_data[i]; checksum = (uint16_t) EEPROM_SUM - checksum; @@ -1564,17 +1579,25 @@ static Property e1000_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +typedef struct E1000Info_st { + const char *name; + uint16_t vendor_id; + uint16_t device_id; + uint8_t revision; +} E1000Info; + static void e1000_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + const E1000Info *info = data; k->init = pci_e1000_init; k->exit = pci_e1000_uninit; k->romfile = "efi-e1000.rom"; - k->vendor_id = PCI_VENDOR_ID_INTEL; - k->device_id = E1000_DEVID; - k->revision = 0x03; + k->vendor_id = info->vendor_id; + k->device_id = info->device_id; + k->revision = info->revision; k->class_id = PCI_CLASS_NETWORK_ETHERNET; set_bit(DEVICE_CATEGORY_NETWORK, dc->categories); dc->desc = "Intel Gigabit Ethernet"; @@ -1583,16 +1606,56 @@ static void e1000_class_init(ObjectClass *klass, void *data) dc->props = e1000_properties; } -static const TypeInfo e1000_info = { +static const TypeInfo e1000_base_info = { .name = TYPE_E1000, .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(E1000State), - .class_init = e1000_class_init, + .abstract = true, +}; + +static const E1000Info e1000_devices[] = { + { + .name = "e1000", /* default "alias" for 82540EM */ + .vendor_id = PCI_VENDOR_ID_INTEL, + .device_id = E1000_DEV_ID_82540EM, + .revision = 0x03, + }, + { + .name = "82540EM", + .vendor_id = PCI_VENDOR_ID_INTEL, + .device_id = E1000_DEV_ID_82540EM, + .revision = 0x03, + }, + { + .name = "82544GC", + .vendor_id = PCI_VENDOR_ID_INTEL, + .device_id = E1000_DEV_ID_82544GC_COPPER, + .revision = 0x03, + }, + { + .name = "82545EM", + .vendor_id = PCI_VENDOR_ID_INTEL, + .device_id = E1000_DEV_ID_82545EM_COPPER, + .revision = 0x03, + }, }; static void e1000_register_types(void) { - type_register_static(&e1000_info); + int i; + + type_register_static(&e1000_base_info); + for (i = 0; i < ARRAY_SIZE(e1000_devices); i++) { + const E1000Info *info = &e1000_devices[i]; + TypeInfo type_info = {}; + + type_info.name = info->name; + type_info.parent = TYPE_E1000; + type_info.class_data = (void *)info; + type_info.class_init = e1000_class_init; + + type_register(&type_info); + } } type_init(e1000_register_types)
Allow selection of different card models from the qemu command line, to better accomodate a wider range of guests. Based-on-patch-by: Romain Dolbeau <romain@dolbeau.org> Signed-off-by: Gabriel Somlo <somlo@cmu.edu> --- Based on the conversation in [2,3,4], and given that I would still like OS X to work as close to "out of the box" as possible, I took a stab at isolating just the bits that allow command-line selection of the specific e1000 device model from Romain's patch set [1]. Hope that's OK with everyone :) Beyond a few small style improvements, I'm still not 100% familiar with QOM, so I wonder if there isn't a cleaner way to offer "e1000" as some sort of "alias" to the default 82540EM, instead of listing two different devices (e1000 and 82540EM) with otherwise 100% identical properties. However, if the patch is acceptable as-is, I'm happy to leave it be... :) I tested this on Windows 7 (where the default e1000 continues to work as before), Linux (F20, which is fine with either model), and OS X (10.9 requires E1000_DEV_ID_82545EM_COPPER, which also works with prior versions starting at 10.6; and the default e1000 works on versions <= 10.8). Comments, suggestions, (or even a straight-forward upstream acceptance of the patch) much appreciated ! :) Thanks, Gabriel [1] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01397.html [2] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01831.html [3] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01841.html [4] http://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg03669.html hw/net/e1000.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 80 insertions(+), 17 deletions(-)