[PATCH] igb: read PBA number from flash

[Gal Hammer]

Fixed flash presence check for 82576 controllers so the part
number string is read and displayed correctly.

Signed-off-by: Gal Hammer 
---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c 
b/drivers/net/ethernet/intel/igb/igb_main.c
index d9c3a6b169f9..245e62b0a97e 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3388,7 +3388,9 @@ static int igb_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
  "Width x1" : "unknown"), netdev->dev_addr);
}
 
-   if ((hw->mac.type >= e1000_i210 ||
+   if ((hw->mac.type == e1000_82576 &&
+rd32(E1000_EECD) & E1000_EECD_PRES) ||
+   (hw->mac.type >= e1000_i210 ||
 igb_get_flash_presence_i210(hw))) {
ret_val = igb_read_part_string(hw, part_str,
   E1000_PBANUM_LENGTH);
-- 
2.26.2

Re: [PATCH v5] drivers/misc: vm_gen_counter: initial driver implementation

[Gal Hammer]

On Tue, Mar 13, 2018 at 7:40 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
> Thanks for the patch! Yet something to improve (see below):

Thanks for the review.

> On Thu, Mar 01, 2018 at 04:22:15PM +0200, Or Idgar wrote:
>> From: Or Idgar <orid...@gmail.com>
>
> I see addresses at gmail, virtualoco and redhat.com At this point I
> don't really know which address to use to contact you :) All of them?

Use his gmail or virtualoco one.

> Also, I think it's a good idea to CC this more widely. Consider CC-ing
> qemu contributors to the vm gen id (use git log to get the list), Ben
> Warren who wrote a prototype driver a while ago, qemu mailing list,
> maybe more.
>
>>
>> This patch is a driver which expose the Virtual Machine Generation ID
>> via sysfs.
>>
>> The ID is a UUID value used to differentiate between virtual
>> machines.
>>
>> The VM-Generation ID is a feature defined by Microsoft (paper:
>> http://go.microsoft.com/fwlink/?LinkId=260709) and supported by multiple
>> hypervisor vendors.
>>
>> Signed-off-by: Or Idgar <orid...@gmail.com>
>
> I think it's a good idea to use sysfs for this. However,
> there are a couple of missing interfaces here:
>
> 1. Userspace needs a way to know when this value changes.
>I see no change notifications here and that does not seem right.

We have the next version which includes notification. The problem is
that right now QEMU doesn't include a mean to change the generation id
on-the-fly, so it was not published it.

> 2. Userspace needs to be able to read these without
>system calls. Pls add mmap support to the raw format.
>(Phys address is not guaranteed to be page-aligned so you will
> probably want an offset attribute for that as well).

I don't agree that this should be a requirement. According to the
specs, the value doesn't change frequently. A notification about a
change the re-reading the value should be enough for now.

> While it's possible to add these later in theory, it's
> easier if userspace can rely on all interfaces being
> in place just by detecting the directory presence.
>
>> ---
>>
>> Changes in v5:
>> - added to VMGENID module dependency on ACPI module.
>>
>>  Documentation/ABI/testing/sysfs-hypervisor |  13 +++
>>  drivers/misc/Kconfig   |   7 ++
>>  drivers/misc/Makefile  |   1 +
>>  drivers/misc/vmgenid.c | 142 
>> +
>
> Do you want to add this under QEMU MACHINE EMULATOR in MAINTAINERS?
> This way e.g. qemu-devel will be copied.

This feature is supported by other hypervisors and this driver should
be able to support them in the next versions. I don't see a reason to
bound it to QEMU.

>>  4 files changed, 163 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-hypervisor
>>  create mode 100644 drivers/misc/vmgenid.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-hypervisor 
>> b/Documentation/ABI/testing/sysfs-hypervisor
>> new file mode 100644
>> index ..2f9a7b8eab70
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-hypervisor
>> @@ -0,0 +1,13 @@
>> +What:/sys/hypervisor/vm_gen_counter
>
> It's not a counter, is it?
> I'd go with "vm_generation_id" here. Eschew abbreviation.

The names were chosen so they'll match Microsoft's specification and
code examples.

>> +Date:February 2018
>> +Contact: Or Idgar <id...@virtualoco.com>
>> + Gal Hammer <gham...@redhat.com>
>> +Description: Expose the virtual machine generation ID. The directory
>> + contains two files: "generation_id" and "raw". Both files
>> + represent the same information.
>> +
>> + "generation_id" file is a UUID string
>
> And this I'd call "uuid" then.

Why? The name says what the value is, not its type. This is not common
to see values' types in the sysfs directory.

>> + representation.
>> +
>> + "raw" file is a 128-bit integer
>> + representation (binary).
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 03605f8fc0dc..a39feff6a867 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -500,6 +500,13 @@ config MISC_RTSX
>>   tristate
>>   default MISC_RTSX_PCI || MISC_RTSX_USB
>>
>> +config VMGENID
>> + depends on ACPI
>> + tristate "Virtual Machine Generation ID driver"
>> + help
>> +   This is a Vi

Re: [PATCH v5] drivers/misc: vm_gen_counter: initial driver implementation

[Gal Hammer]

On Tue, Mar 13, 2018 at 7:40 PM, Michael S. Tsirkin  wrote:
> Thanks for the patch! Yet something to improve (see below):

Thanks for the review.

> On Thu, Mar 01, 2018 at 04:22:15PM +0200, Or Idgar wrote:
>> From: Or Idgar 
>
> I see addresses at gmail, virtualoco and redhat.com At this point I
> don't really know which address to use to contact you :) All of them?

Use his gmail or virtualoco one.

> Also, I think it's a good idea to CC this more widely. Consider CC-ing
> qemu contributors to the vm gen id (use git log to get the list), Ben
> Warren who wrote a prototype driver a while ago, qemu mailing list,
> maybe more.
>
>>
>> This patch is a driver which expose the Virtual Machine Generation ID
>> via sysfs.
>>
>> The ID is a UUID value used to differentiate between virtual
>> machines.
>>
>> The VM-Generation ID is a feature defined by Microsoft (paper:
>> http://go.microsoft.com/fwlink/?LinkId=260709) and supported by multiple
>> hypervisor vendors.
>>
>> Signed-off-by: Or Idgar 
>
> I think it's a good idea to use sysfs for this. However,
> there are a couple of missing interfaces here:
>
> 1. Userspace needs a way to know when this value changes.
>I see no change notifications here and that does not seem right.

We have the next version which includes notification. The problem is
that right now QEMU doesn't include a mean to change the generation id
on-the-fly, so it was not published it.

> 2. Userspace needs to be able to read these without
>system calls. Pls add mmap support to the raw format.
>(Phys address is not guaranteed to be page-aligned so you will
> probably want an offset attribute for that as well).

I don't agree that this should be a requirement. According to the
specs, the value doesn't change frequently. A notification about a
change the re-reading the value should be enough for now.

> While it's possible to add these later in theory, it's
> easier if userspace can rely on all interfaces being
> in place just by detecting the directory presence.
>
>> ---
>>
>> Changes in v5:
>> - added to VMGENID module dependency on ACPI module.
>>
>>  Documentation/ABI/testing/sysfs-hypervisor |  13 +++
>>  drivers/misc/Kconfig   |   7 ++
>>  drivers/misc/Makefile  |   1 +
>>  drivers/misc/vmgenid.c | 142 
>> +
>
> Do you want to add this under QEMU MACHINE EMULATOR in MAINTAINERS?
> This way e.g. qemu-devel will be copied.

This feature is supported by other hypervisors and this driver should
be able to support them in the next versions. I don't see a reason to
bound it to QEMU.

>>  4 files changed, 163 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-hypervisor
>>  create mode 100644 drivers/misc/vmgenid.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-hypervisor 
>> b/Documentation/ABI/testing/sysfs-hypervisor
>> new file mode 100644
>> index ..2f9a7b8eab70
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-hypervisor
>> @@ -0,0 +1,13 @@
>> +What:/sys/hypervisor/vm_gen_counter
>
> It's not a counter, is it?
> I'd go with "vm_generation_id" here. Eschew abbreviation.

The names were chosen so they'll match Microsoft's specification and
code examples.

>> +Date:February 2018
>> +Contact: Or Idgar 
>> + Gal Hammer 
>> +Description: Expose the virtual machine generation ID. The directory
>> + contains two files: "generation_id" and "raw". Both files
>> + represent the same information.
>> +
>> + "generation_id" file is a UUID string
>
> And this I'd call "uuid" then.

Why? The name says what the value is, not its type. This is not common
to see values' types in the sysfs directory.

>> + representation.
>> +
>> + "raw" file is a 128-bit integer
>> + representation (binary).
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 03605f8fc0dc..a39feff6a867 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -500,6 +500,13 @@ config MISC_RTSX
>>   tristate
>>   default MISC_RTSX_PCI || MISC_RTSX_USB
>>
>> +config VMGENID
>> + depends on ACPI
>> + tristate "Virtual Machine Generation ID driver"
>> + help
>> +   This is a Virtual Machine Generation ID driver which provides
>> +   a virtual machine unique identifier.
>> +
>>  source

Re: [PATCH v5] drivers/misc: vm_gen_counter: initial driver implementation

[Gal Hammer]

On Wed, Mar 14, 2018 at 9:25 PM, Michael S. Tsirkin  wrote:
> On Wed, Mar 14, 2018 at 07:25:36PM +0100, Greg KH wrote:
>> On Tue, Mar 13, 2018 at 07:40:51PM +0200, Michael S. Tsirkin wrote:
>> > I think it's a good idea to use sysfs for this. However,
>> > there are a couple of missing interfaces here:
>> >
>> > 1. Userspace needs a way to know when this value changes.
>> >I see no change notifications here and that does not seem right.
>>
>> How can these change?
>
> It's a hardware register. It changes when hardware feels like it :)
> In particular, it changes whenever VM is migrated or snapshotted.

This value doesn't change when a VM is migrated. It is unlikely that
this value will be changed so frequently that a direct access to the
memory is required. Even in QEMU, the current implementation was
merged without an option to change the generation id on-the-fly. One
must run a new instance in order to set a new value, which means that
no application will be running during that time.

>> > 2. Userspace needs to be able to read these without
>> >system calls.
>>
>> Ick, what?  Why not?
>>
>> > Pls add mmap support to the raw format.
>>
>> For a single integer?  Why do you need mmap for this?  What is so
>> "performant" that needs to touch a sysfs file?
>> >(Phys address is not guaranteed to be page-aligned so you will
>> > probably want an offset attribute for that as well).
>>
>> Ick ick ick, that's why it's good to just stick with a sysfs file.

I agree with Greg here. The user is able to read the value, and then
wait for a notification if she cares about changes.

>> Have you tested just how long this takes to see if the open/read/close
>> is really the bottleneck, or if the io on reading the value is the
>> bottleneck?
>>
>> thanks,
>>
>> greg k-h
>
> Well an application needs to check this value basically after
> every database transaction. So I'm pretty sure it's a performance
> sensitive path. But yes, I didn't profile any apps since they
> are yet to be written to use this interface.
> I'm fine deferring point 2 for now.
>
> --
> MST

Thanks,

Gal.

Re: [PATCH v5] drivers/misc: vm_gen_counter: initial driver implementation

[Gal Hammer]

On Wed, Mar 14, 2018 at 9:25 PM, Michael S. Tsirkin  wrote:
> On Wed, Mar 14, 2018 at 07:25:36PM +0100, Greg KH wrote:
>> On Tue, Mar 13, 2018 at 07:40:51PM +0200, Michael S. Tsirkin wrote:
>> > I think it's a good idea to use sysfs for this. However,
>> > there are a couple of missing interfaces here:
>> >
>> > 1. Userspace needs a way to know when this value changes.
>> >I see no change notifications here and that does not seem right.
>>
>> How can these change?
>
> It's a hardware register. It changes when hardware feels like it :)
> In particular, it changes whenever VM is migrated or snapshotted.

This value doesn't change when a VM is migrated. It is unlikely that
this value will be changed so frequently that a direct access to the
memory is required. Even in QEMU, the current implementation was
merged without an option to change the generation id on-the-fly. One
must run a new instance in order to set a new value, which means that
no application will be running during that time.

>> > 2. Userspace needs to be able to read these without
>> >system calls.
>>
>> Ick, what?  Why not?
>>
>> > Pls add mmap support to the raw format.
>>
>> For a single integer?  Why do you need mmap for this?  What is so
>> "performant" that needs to touch a sysfs file?
>> >(Phys address is not guaranteed to be page-aligned so you will
>> > probably want an offset attribute for that as well).
>>
>> Ick ick ick, that's why it's good to just stick with a sysfs file.

I agree with Greg here. The user is able to read the value, and then
wait for a notification if she cares about changes.

>> Have you tested just how long this takes to see if the open/read/close
>> is really the bottleneck, or if the io on reading the value is the
>> bottleneck?
>>
>> thanks,
>>
>> greg k-h
>
> Well an application needs to check this value basically after
> every database transaction. So I'm pretty sure it's a performance
> sensitive path. But yes, I didn't profile any apps since they
> are yet to be written to use this interface.
> I'm fine deferring point 2 for now.
>
> --
> MST

Thanks,

Gal.

Re: [PATCH] drivers/virt: vm_gen_counter: initial driver implementation

[Gal Hammer]

Hi,

On Thu, Feb 22, 2018 at 6:36 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Thu, Feb 22, 2018 at 06:20:38PM +0200, Or Idgar wrote:
>> From: Or Idgar <orid...@gmail.com>
>>
>> This patch is a driver which expose the Virtual Machine Generation ID
>> via sysfs. The ID is a UUID value used to differentiate between virtual
>> machines.
>>
>> The VM-Generation ID is a feature defined by Microsoft (paper:
>> http://go.microsoft.com/fwlink/?LinkId=260709) and supported by multiple
>> hypervisor vendors.
>>
>> Signed-off-by: Or Idgar <orid...@gmail.com>
>> ---
>>  Documentation/ABI/testing/sysfs-hypervisor |  13 +++
>>  drivers/misc/Kconfig   |   6 ++
>>  drivers/misc/Makefile  |   1 +
>>  drivers/misc/vmgenid.c | 141 
>> +
>>  4 files changed, 161 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-hypervisor
>>  create mode 100644 drivers/misc/vmgenid.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-hypervisor 
>> b/Documentation/ABI/testing/sysfs-hypervisor
>> new file mode 100644
>> index ..2f9a7b8eab70
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-hypervisor
>> @@ -0,0 +1,13 @@
>> +What:/sys/hypervisor/vm_gen_counter
>
> Shouldn't this go under the specific hypervisor you are running on?  Why
> in the "root" of /sys/hypervisor?

As far as I know, most of today's hypervisors support this feature, so
adding a subdirectory doesn't seem right. The guest user should be
able to access the UUID without a knowledge of the running host.

At the moment, the driver supports only QEMU, but we're planning to
enhance it once we'll have access to other vendor hypervisors.

>
>> +Date:February 2018
>> +Contact: Or Idgar <id...@virtualoco.com>
>> + Gal Hammer <gham...@redhat.com>
>> +Description: Expose the virtual machine generation ID. The directory
>> + contains two files: "generation_id" and "raw". Both files
>> + represent the same information.
>> +
>> + "generation_id" file is a UUID string
>> + representation.
>> +
>> + "raw" file is a 128-bit integer
>> + representation (binary).
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 03605f8fc0dc..5a74802bdfb4 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -500,6 +500,12 @@ config MISC_RTSX
>>   tristate
>>   default MISC_RTSX_PCI || MISC_RTSX_USB
>>
>> +config VMGENID
>> + tristate "Virtual Machine Generation ID driver"
>> + help
>> +   This is a Virtual Machine Generation ID driver which provides
>> +   a virtual machine unique identifier.
>> +
>>  source "drivers/misc/c2port/Kconfig"
>>  source "drivers/misc/eeprom/Kconfig"
>>  source "drivers/misc/cb710/Kconfig"
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index c3c8624f4d95..067aa666bb6a 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -57,6 +57,7 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)  += aspeed-lpc-snoop.o
>>  obj-$(CONFIG_PCI_ENDPOINT_TEST)  += pci_endpoint_test.o
>>  obj-$(CONFIG_OCXL)   += ocxl/
>>  obj-$(CONFIG_MISC_RTSX)  += cardreader/
>> +obj-$(CONFIG_VMGENID)    += vmgenid.o
>>
>>  lkdtm-$(CONFIG_LKDTM)+= lkdtm_core.o
>>  lkdtm-$(CONFIG_LKDTM)+= lkdtm_bugs.o
>> diff --git a/drivers/misc/vmgenid.c b/drivers/misc/vmgenid.c
>> new file mode 100644
>> index ..9d7f09f9e7bd
>> --- /dev/null
>> +++ b/drivers/misc/vmgenid.c
>> @@ -0,0 +1,141 @@
>> +/*
>> + * Virtual Machine Generation ID driver
>> + *
>> + * Copyright (C) 2018 Red Hat, Inc. All rights reserved.
>> + *   Authors:
>> + * Or Idgar <orid...@gmail.com>
>> + * Gal Hammer <gham...@redhat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>
> No SPDX line?  Please see the documentation for how to properly add this
> as the first line of any new file.

We'll give it another try.

> I want some hypervisor people to weigh in on this before I can take
> it...
>
> thanks,
>
> greg k-h

Thank you.

Gal.

Re: [PATCH] drivers/virt: vm_gen_counter: initial driver implementation

[Gal Hammer]

Hi,

On Thu, Feb 22, 2018 at 6:36 PM, Greg KH  wrote:
> On Thu, Feb 22, 2018 at 06:20:38PM +0200, Or Idgar wrote:
>> From: Or Idgar 
>>
>> This patch is a driver which expose the Virtual Machine Generation ID
>> via sysfs. The ID is a UUID value used to differentiate between virtual
>> machines.
>>
>> The VM-Generation ID is a feature defined by Microsoft (paper:
>> http://go.microsoft.com/fwlink/?LinkId=260709) and supported by multiple
>> hypervisor vendors.
>>
>> Signed-off-by: Or Idgar 
>> ---
>>  Documentation/ABI/testing/sysfs-hypervisor |  13 +++
>>  drivers/misc/Kconfig   |   6 ++
>>  drivers/misc/Makefile  |   1 +
>>  drivers/misc/vmgenid.c | 141 
>> +
>>  4 files changed, 161 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-hypervisor
>>  create mode 100644 drivers/misc/vmgenid.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-hypervisor 
>> b/Documentation/ABI/testing/sysfs-hypervisor
>> new file mode 100644
>> index ..2f9a7b8eab70
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-hypervisor
>> @@ -0,0 +1,13 @@
>> +What:/sys/hypervisor/vm_gen_counter
>
> Shouldn't this go under the specific hypervisor you are running on?  Why
> in the "root" of /sys/hypervisor?

As far as I know, most of today's hypervisors support this feature, so
adding a subdirectory doesn't seem right. The guest user should be
able to access the UUID without a knowledge of the running host.

At the moment, the driver supports only QEMU, but we're planning to
enhance it once we'll have access to other vendor hypervisors.

>
>> +Date:February 2018
>> +Contact: Or Idgar 
>> + Gal Hammer 
>> +Description: Expose the virtual machine generation ID. The directory
>> + contains two files: "generation_id" and "raw". Both files
>> + represent the same information.
>> +
>> + "generation_id" file is a UUID string
>> + representation.
>> +
>> + "raw" file is a 128-bit integer
>> + representation (binary).
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 03605f8fc0dc..5a74802bdfb4 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -500,6 +500,12 @@ config MISC_RTSX
>>   tristate
>>   default MISC_RTSX_PCI || MISC_RTSX_USB
>>
>> +config VMGENID
>> + tristate "Virtual Machine Generation ID driver"
>> + help
>> +   This is a Virtual Machine Generation ID driver which provides
>> +   a virtual machine unique identifier.
>> +
>>  source "drivers/misc/c2port/Kconfig"
>>  source "drivers/misc/eeprom/Kconfig"
>>  source "drivers/misc/cb710/Kconfig"
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index c3c8624f4d95..067aa666bb6a 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -57,6 +57,7 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)  += aspeed-lpc-snoop.o
>>  obj-$(CONFIG_PCI_ENDPOINT_TEST)  += pci_endpoint_test.o
>>  obj-$(CONFIG_OCXL)   += ocxl/
>>  obj-$(CONFIG_MISC_RTSX)  += cardreader/
>> +obj-$(CONFIG_VMGENID)        += vmgenid.o
>>
>>  lkdtm-$(CONFIG_LKDTM)+= lkdtm_core.o
>>  lkdtm-$(CONFIG_LKDTM)+= lkdtm_bugs.o
>> diff --git a/drivers/misc/vmgenid.c b/drivers/misc/vmgenid.c
>> new file mode 100644
>> index ..9d7f09f9e7bd
>> --- /dev/null
>> +++ b/drivers/misc/vmgenid.c
>> @@ -0,0 +1,141 @@
>> +/*
>> + * Virtual Machine Generation ID driver
>> + *
>> + * Copyright (C) 2018 Red Hat, Inc. All rights reserved.
>> + *   Authors:
>> + * Or Idgar 
>> + * Gal Hammer 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>
> No SPDX line?  Please see the documentation for how to properly add this
> as the first line of any new file.

We'll give it another try.

> I want some hypervisor people to weigh in on this before I can take
> it...
>
> thanks,
>
> greg k-h

Thank you.

Gal.