[v8,01/16] FDT: introduce global phandle allocation

Commit Message

Andre Przywara Nov. 4, 2016, 5:31 p.m. UTC

Allocating an FDT phandle (a unique identifier) using a static
variable in a static inline function defined in a header file works
only if all users are in the same source file. So trying to allocate
a handle from two different compilation units fails.
Introduce global phandle allocation and reference code to properly
allocate unique phandles.

Signed-off-by: Andre Przywara <[e-mail]>
---
 Makefile          |  1 +
 arm/fdt.c         |  2 +-
 arm/gic.c         |  2 +-
 include/kvm/fdt.h | 10 +++++-----
 kvm-fdt.c         | 26 ++++++++++++++++++++++++++
 5 files changed, 34 insertions(+), 7 deletions(-)
 create mode 100644 kvm-fdt.c

Comments

Marc Zyngier Dec. 9, 2016, 11:55 a.m. UTC | #1

Hi Andre,

On 04/11/16 17:31, Andre Przywara wrote:
> Allocating an FDT phandle (a unique identifier) using a static
> variable in a static inline function defined in a header file works
> only if all users are in the same source file. So trying to allocate
> a handle from two different compilation units fails.
> Introduce global phandle allocation and reference code to properly
> allocate unique phandles.
> 
> Signed-off-by: Andre Przywara <[e-mail]>
> ---
>  Makefile          |  1 +
>  arm/fdt.c         |  2 +-
>  arm/gic.c         |  2 +-
>  include/kvm/fdt.h | 10 +++++-----
>  kvm-fdt.c         | 26 ++++++++++++++++++++++++++
>  5 files changed, 34 insertions(+), 7 deletions(-)
>  create mode 100644 kvm-fdt.c
> 
> diff --git a/Makefile b/Makefile
> index 1f0196f..e4a4002 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -98,6 +98,7 @@ OBJS	+= kvm-ipc.o
>  OBJS	+= builtin-sandbox.o
>  OBJS	+= virtio/mmio.o
>  OBJS	+= hw/i8042.o
> +OBJS	+= kvm-fdt.o
>  
>  # Translate uname -m into ARCH string
>  ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
> diff --git a/arm/fdt.c b/arm/fdt.c
> index 381d48f..8bcfffb 100644
> --- a/arm/fdt.c
> +++ b/arm/fdt.c
> @@ -114,7 +114,7 @@ static int setup_fdt(struct kvm *kvm)
>  {
>  	struct device_header *dev_hdr;
>  	u8 staging_fdt[FDT_MAX_SIZE];
> -	u32 gic_phandle		= fdt__alloc_phandle();
> +	u32 gic_phandle		= fdt__get_phandle(PHANDLE_GIC);
>  	u64 mem_reg_prop[]	= {
>  		cpu_to_fdt64(kvm->arch.memory_guest_start),
>  		cpu_to_fdt64(kvm->ram_size),
> diff --git a/arm/gic.c b/arm/gic.c
> index d6d6dd0..b60437e 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -222,7 +222,7 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> -	_FDT(fdt_property_cell(fdt, "phandle", phandle));
> +	_FDT(fdt_property_cell(fdt, "phandle", fdt__get_phandle(PHANDLE_GIC)));
>  	_FDT(fdt_end_node(fdt));
>  }
>  
> diff --git a/include/kvm/fdt.h b/include/kvm/fdt.h
> index 53d85a4..cd2bb72 100644
> --- a/include/kvm/fdt.h
> +++ b/include/kvm/fdt.h
> @@ -8,6 +8,10 @@
>  #include <linux/types.h>
>  
>  #define FDT_MAX_SIZE	0x10000
> +#define FDT_INVALID_PHANDLE 0
> +#define FDT_IS_VALID_PHANDLE(phandle) ((phandle) != FDT_INVALID_PHANDLE)
> +
> +enum phandles {PHANDLE_GIC, PHANDLES_MAX};
>  
>  /* Those definitions are generic FDT values for specifying IRQ
>   * types and are used in the Linux kernel internally as well as in
> @@ -33,10 +37,6 @@ enum irq_type {
>  		}							\
>  	} while (0)
>  
> -static inline u32 fdt__alloc_phandle(void)
> -{
> -	static u32 phandle = 0;
> -	return ++phandle;
> -}
> +u32 fdt__get_phandle(enum phandles phandle);
>  
>  #endif /* KVM__FDT_H */
> diff --git a/kvm-fdt.c b/kvm-fdt.c
> new file mode 100644
> index 0000000..d05f3fe
> --- /dev/null
> +++ b/kvm-fdt.c
> @@ -0,0 +1,26 @@
> +/*
> + * Commonly used FDT functions.
> + */
> +
> +#include <stdio.h>
> +#include "kvm/fdt.h"
> +#include "kvm/util.h"
> +
> +u32 phandles[PHANDLES_MAX] = {};

It is a bit weird that you're initializing this to zero...

> +u32 next_phandle = 1;
> +
> +u32 fdt__get_phandle(enum phandles phandle)
> +{
> +	u32 ret;
> +
> +	if (phandle >= PHANDLES_MAX)
> +		return FDT_INVALID_PHANDLE;
> +
> +	ret = phandles[phandle];
> +	if (ret == FDT_INVALID_PHANDLE) {

and yet test against a #define that isn't the initializer.

Also, given that fdt__get_phandle() can now fail by returning
FDT_INVALID_HANDLE, maybe we should abort instead of returning something
that is definitely wrong and use it blindly (which is going to be fun to
debug...).


> +		ret = next_phandle++;
> +		phandles[phandle] = ret;
> +	}
> +
> +	return ret;
> +}
> 

Thanks,

	M.

Marc Zyngier Dec. 9, 2016, 12:03 p.m. UTC | #2

On 04/11/16 17:31, Andre Przywara wrote:
> Allocating an FDT phandle (a unique identifier) using a static
> variable in a static inline function defined in a header file works
> only if all users are in the same source file. So trying to allocate
> a handle from two different compilation units fails.
> Introduce global phandle allocation and reference code to properly
> allocate unique phandles.
> 
> Signed-off-by: Andre Przywara <[e-mail]>
> ---
>  Makefile          |  1 +
>  arm/fdt.c         |  2 +-
>  arm/gic.c         |  2 +-
>  include/kvm/fdt.h | 10 +++++-----
>  kvm-fdt.c         | 26 ++++++++++++++++++++++++++
>  5 files changed, 34 insertions(+), 7 deletions(-)
>  create mode 100644 kvm-fdt.c
> 
> diff --git a/Makefile b/Makefile
> index 1f0196f..e4a4002 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -98,6 +98,7 @@ OBJS	+= kvm-ipc.o
>  OBJS	+= builtin-sandbox.o
>  OBJS	+= virtio/mmio.o
>  OBJS	+= hw/i8042.o
> +OBJS	+= kvm-fdt.o
>  
>  # Translate uname -m into ARCH string
>  ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
> diff --git a/arm/fdt.c b/arm/fdt.c
> index 381d48f..8bcfffb 100644
> --- a/arm/fdt.c
> +++ b/arm/fdt.c
> @@ -114,7 +114,7 @@ static int setup_fdt(struct kvm *kvm)
>  {
>  	struct device_header *dev_hdr;
>  	u8 staging_fdt[FDT_MAX_SIZE];
> -	u32 gic_phandle		= fdt__alloc_phandle();
> +	u32 gic_phandle		= fdt__get_phandle(PHANDLE_GIC);
>  	u64 mem_reg_prop[]	= {
>  		cpu_to_fdt64(kvm->arch.memory_guest_start),
>  		cpu_to_fdt64(kvm->ram_size),
> diff --git a/arm/gic.c b/arm/gic.c
> index d6d6dd0..b60437e 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -222,7 +222,7 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> -	_FDT(fdt_property_cell(fdt, "phandle", phandle));
> +	_FDT(fdt_property_cell(fdt, "phandle", fdt__get_phandle(PHANDLE_GIC)));
>  	_FDT(fdt_end_node(fdt));
>  }
>  
> diff --git a/include/kvm/fdt.h b/include/kvm/fdt.h
> index 53d85a4..cd2bb72 100644
> --- a/include/kvm/fdt.h
> +++ b/include/kvm/fdt.h
> @@ -8,6 +8,10 @@
>  #include <linux/types.h>
>  
>  #define FDT_MAX_SIZE	0x10000
> +#define FDT_INVALID_PHANDLE 0
> +#define FDT_IS_VALID_PHANDLE(phandle) ((phandle) != FDT_INVALID_PHANDLE)
> +
> +enum phandles {PHANDLE_GIC, PHANDLES_MAX};

Another nit here: PHANDLE_GIC is pretty much ARM-specific, while FDT is
supposed to be generic. Can't we move the enum to be architecture
specific and not put this in an architecture agnostic one?

Thanks,

	M.

Andre Przywara Dec. 19, 2016, 6:43 p.m. UTC | #3

Hi Marc,

On 09/12/16 11:55, Marc Zyngier wrote:
> Hi Andre,
> 
> On 04/11/16 17:31, Andre Przywara wrote:
>> Allocating an FDT phandle (a unique identifier) using a static
>> variable in a static inline function defined in a header file works
>> only if all users are in the same source file. So trying to allocate
>> a handle from two different compilation units fails.
>> Introduce global phandle allocation and reference code to properly
>> allocate unique phandles.
>>
>> Signed-off-by: Andre Przywara <[e-mail]>
>> ---
>>  Makefile          |  1 +
>>  arm/fdt.c         |  2 +-
>>  arm/gic.c         |  2 +-
>>  include/kvm/fdt.h | 10 +++++-----
>>  kvm-fdt.c         | 26 ++++++++++++++++++++++++++
>>  5 files changed, 34 insertions(+), 7 deletions(-)
>>  create mode 100644 kvm-fdt.c
>>
>> diff --git a/Makefile b/Makefile
>> index 1f0196f..e4a4002 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -98,6 +98,7 @@ OBJS	+= kvm-ipc.o
>>  OBJS	+= builtin-sandbox.o
>>  OBJS	+= virtio/mmio.o
>>  OBJS	+= hw/i8042.o
>> +OBJS	+= kvm-fdt.o
>>  
>>  # Translate uname -m into ARCH string
>>  ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
>> diff --git a/arm/fdt.c b/arm/fdt.c
>> index 381d48f..8bcfffb 100644
>> --- a/arm/fdt.c
>> +++ b/arm/fdt.c
>> @@ -114,7 +114,7 @@ static int setup_fdt(struct kvm *kvm)
>>  {
>>  	struct device_header *dev_hdr;
>>  	u8 staging_fdt[FDT_MAX_SIZE];
>> -	u32 gic_phandle		= fdt__alloc_phandle();
>> +	u32 gic_phandle		= fdt__get_phandle(PHANDLE_GIC);
>>  	u64 mem_reg_prop[]	= {
>>  		cpu_to_fdt64(kvm->arch.memory_guest_start),
>>  		cpu_to_fdt64(kvm->ram_size),
>> diff --git a/arm/gic.c b/arm/gic.c
>> index d6d6dd0..b60437e 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -222,7 +222,7 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
>> -	_FDT(fdt_property_cell(fdt, "phandle", phandle));
>> +	_FDT(fdt_property_cell(fdt, "phandle", fdt__get_phandle(PHANDLE_GIC)));
>>  	_FDT(fdt_end_node(fdt));
>>  }
>>  
>> diff --git a/include/kvm/fdt.h b/include/kvm/fdt.h
>> index 53d85a4..cd2bb72 100644
>> --- a/include/kvm/fdt.h
>> +++ b/include/kvm/fdt.h
>> @@ -8,6 +8,10 @@
>>  #include <linux/types.h>
>>  
>>  #define FDT_MAX_SIZE	0x10000
>> +#define FDT_INVALID_PHANDLE 0
>> +#define FDT_IS_VALID_PHANDLE(phandle) ((phandle) != FDT_INVALID_PHANDLE)
>> +
>> +enum phandles {PHANDLE_GIC, PHANDLES_MAX};
>>  
>>  /* Those definitions are generic FDT values for specifying IRQ
>>   * types and are used in the Linux kernel internally as well as in
>> @@ -33,10 +37,6 @@ enum irq_type {
>>  		}							\
>>  	} while (0)
>>  
>> -static inline u32 fdt__alloc_phandle(void)
>> -{
>> -	static u32 phandle = 0;
>> -	return ++phandle;
>> -}
>> +u32 fdt__get_phandle(enum phandles phandle);
>>  
>>  #endif /* KVM__FDT_H */
>> diff --git a/kvm-fdt.c b/kvm-fdt.c
>> new file mode 100644
>> index 0000000..d05f3fe
>> --- /dev/null
>> +++ b/kvm-fdt.c
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Commonly used FDT functions.
>> + */
>> +
>> +#include <stdio.h>
>> +#include "kvm/fdt.h"
>> +#include "kvm/util.h"
>> +
>> +u32 phandles[PHANDLES_MAX] = {};
> 
> It is a bit weird that you're initializing this to zero...
> 
>> +u32 next_phandle = 1;
>> +
>> +u32 fdt__get_phandle(enum phandles phandle)
>> +{
>> +	u32 ret;
>> +
>> +	if (phandle >= PHANDLES_MAX)
>> +		return FDT_INVALID_PHANDLE;
>> +
>> +	ret = phandles[phandle];
>> +	if (ret == FDT_INVALID_PHANDLE) {
> 
> and yet test against a #define that isn't the initializer.

Well, yes. The problem is that AFAIK you cannot initialize an array
easily with all the values getting set to something other than zero.
So I could write
	u32 phandles[PHANDLES_MAX] = {FDT_INVALID_PHANDLE};
above, but as that would only set the first member to
FDT_INVALID_PHANDLE (and all the others to 0), so it would rely on the
define actually being zero to work reliably. So my thought was to avoid
readers falling into this trap by not specifying the reset value
explicitly. Also that's the reason that 0 is the invalid value, which I
don't like too much, tbh.

So shall I make this a comment then?

Or do I miss something here?

> Also, given that fdt__get_phandle() can now fail by returning
> FDT_INVALID_HANDLE, maybe we should abort instead of returning something
> that is definitely wrong and use it blindly (which is going to be fun to
> debug...).

Yes, good point. Given that this hints at an internal error, die() seems
to be the appropriate action here.

Cheers,
Andre.

> 
> 
>> +		ret = next_phandle++;
>> +		phandles[phandle] = ret;
>> +	}
>> +
>> +	return ret;
>> +}
>>
> 
> Thanks,
> 
> 	M.
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andre Przywara Dec. 19, 2016, 6:43 p.m. UTC | #4

Hi Marc,

On 09/12/16 12:03, Marc Zyngier wrote:
> On 04/11/16 17:31, Andre Przywara wrote:
>> Allocating an FDT phandle (a unique identifier) using a static
>> variable in a static inline function defined in a header file works
>> only if all users are in the same source file. So trying to allocate
>> a handle from two different compilation units fails.
>> Introduce global phandle allocation and reference code to properly
>> allocate unique phandles.
>>
>> Signed-off-by: Andre Przywara <[e-mail]>
>> ---
>>  Makefile          |  1 +
>>  arm/fdt.c         |  2 +-
>>  arm/gic.c         |  2 +-
>>  include/kvm/fdt.h | 10 +++++-----
>>  kvm-fdt.c         | 26 ++++++++++++++++++++++++++
>>  5 files changed, 34 insertions(+), 7 deletions(-)
>>  create mode 100644 kvm-fdt.c
>>
>> diff --git a/Makefile b/Makefile
>> index 1f0196f..e4a4002 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -98,6 +98,7 @@ OBJS	+= kvm-ipc.o
>>  OBJS	+= builtin-sandbox.o
>>  OBJS	+= virtio/mmio.o
>>  OBJS	+= hw/i8042.o
>> +OBJS	+= kvm-fdt.o
>>  
>>  # Translate uname -m into ARCH string
>>  ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
>> diff --git a/arm/fdt.c b/arm/fdt.c
>> index 381d48f..8bcfffb 100644
>> --- a/arm/fdt.c
>> +++ b/arm/fdt.c
>> @@ -114,7 +114,7 @@ static int setup_fdt(struct kvm *kvm)
>>  {
>>  	struct device_header *dev_hdr;
>>  	u8 staging_fdt[FDT_MAX_SIZE];
>> -	u32 gic_phandle		= fdt__alloc_phandle();
>> +	u32 gic_phandle		= fdt__get_phandle(PHANDLE_GIC);
>>  	u64 mem_reg_prop[]	= {
>>  		cpu_to_fdt64(kvm->arch.memory_guest_start),
>>  		cpu_to_fdt64(kvm->ram_size),
>> diff --git a/arm/gic.c b/arm/gic.c
>> index d6d6dd0..b60437e 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -222,7 +222,7 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
>> -	_FDT(fdt_property_cell(fdt, "phandle", phandle));
>> +	_FDT(fdt_property_cell(fdt, "phandle", fdt__get_phandle(PHANDLE_GIC)));
>>  	_FDT(fdt_end_node(fdt));
>>  }
>>  
>> diff --git a/include/kvm/fdt.h b/include/kvm/fdt.h
>> index 53d85a4..cd2bb72 100644
>> --- a/include/kvm/fdt.h
>> +++ b/include/kvm/fdt.h
>> @@ -8,6 +8,10 @@
>>  #include <linux/types.h>
>>  
>>  #define FDT_MAX_SIZE	0x10000
>> +#define FDT_INVALID_PHANDLE 0
>> +#define FDT_IS_VALID_PHANDLE(phandle) ((phandle) != FDT_INVALID_PHANDLE)
>> +
>> +enum phandles {PHANDLE_GIC, PHANDLES_MAX};
> 
> Another nit here: PHANDLE_GIC is pretty much ARM-specific, while FDT is
> supposed to be generic. Can't we move the enum to be architecture
> specific and not put this in an architecture agnostic one?

Yes, that indeed makes sense.
This requires an include file for every architecture, but well ...

Cheers,
Andre.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Jones Dec. 20, 2016, 9:43 a.m. UTC | #5

On Mon, Dec 19, 2016 at 06:43:29PM +0000, Andre Przywara wrote:
> Well, yes. The problem is that AFAIK you cannot initialize an array
> easily with all the values getting set to something other than zero.

u32 phandles[PHANDLES_MAX] = { [0 ... PHANDLES_MAX-1] = FDT_INVALID_PHANDLE };

(Unrelated, but could you please add format.subjectprefix = PATCH kvmtool
 to your git config. I just suggested via a kvmtool README patch...)

Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andre Przywara Feb. 1, 2017, 4:44 p.m. UTC | #6

Hi Marc, Will,

On 09/12/16 12:03, Marc Zyngier wrote:
> On 04/11/16 17:31, Andre Przywara wrote:
>> Allocating an FDT phandle (a unique identifier) using a static
>> variable in a static inline function defined in a header file works
>> only if all users are in the same source file. So trying to allocate
>> a handle from two different compilation units fails.
>> Introduce global phandle allocation and reference code to properly
>> allocate unique phandles.
>>
>> Signed-off-by: Andre Przywara <[e-mail]>
>> ---
>>  Makefile          |  1 +
>>  arm/fdt.c         |  2 +-
>>  arm/gic.c         |  2 +-
>>  include/kvm/fdt.h | 10 +++++-----
>>  kvm-fdt.c         | 26 ++++++++++++++++++++++++++
>>  5 files changed, 34 insertions(+), 7 deletions(-)
>>  create mode 100644 kvm-fdt.c
>>
>> diff --git a/Makefile b/Makefile
>> index 1f0196f..e4a4002 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -98,6 +98,7 @@ OBJS	+= kvm-ipc.o
>>  OBJS	+= builtin-sandbox.o
>>  OBJS	+= virtio/mmio.o
>>  OBJS	+= hw/i8042.o
>> +OBJS	+= kvm-fdt.o
>>  
>>  # Translate uname -m into ARCH string
>>  ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
>> diff --git a/arm/fdt.c b/arm/fdt.c
>> index 381d48f..8bcfffb 100644
>> --- a/arm/fdt.c
>> +++ b/arm/fdt.c
>> @@ -114,7 +114,7 @@ static int setup_fdt(struct kvm *kvm)
>>  {
>>  	struct device_header *dev_hdr;
>>  	u8 staging_fdt[FDT_MAX_SIZE];
>> -	u32 gic_phandle		= fdt__alloc_phandle();
>> +	u32 gic_phandle		= fdt__get_phandle(PHANDLE_GIC);
>>  	u64 mem_reg_prop[]	= {
>>  		cpu_to_fdt64(kvm->arch.memory_guest_start),
>>  		cpu_to_fdt64(kvm->ram_size),
>> diff --git a/arm/gic.c b/arm/gic.c
>> index d6d6dd0..b60437e 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -222,7 +222,7 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
>> -	_FDT(fdt_property_cell(fdt, "phandle", phandle));
>> +	_FDT(fdt_property_cell(fdt, "phandle", fdt__get_phandle(PHANDLE_GIC)));
>>  	_FDT(fdt_end_node(fdt));
>>  }
>>  
>> diff --git a/include/kvm/fdt.h b/include/kvm/fdt.h
>> index 53d85a4..cd2bb72 100644
>> --- a/include/kvm/fdt.h
>> +++ b/include/kvm/fdt.h
>> @@ -8,6 +8,10 @@
>>  #include <linux/types.h>
>>  
>>  #define FDT_MAX_SIZE	0x10000
>> +#define FDT_INVALID_PHANDLE 0
>> +#define FDT_IS_VALID_PHANDLE(phandle) ((phandle) != FDT_INVALID_PHANDLE)
>> +
>> +enum phandles {PHANDLE_GIC, PHANDLES_MAX};
> 
> Another nit here: PHANDLE_GIC is pretty much ARM-specific, while FDT is
> supposed to be generic. Can't we move the enum to be architecture
> specific and not put this in an architecture agnostic one?

So while trying to find the best possible solution for this seemingly
simple problem, I was wondering why we had this allocation function in
the first place?
It seems a bit overkill to allocate a handle if we could just go with
static values.
I changed the first two patches now to have an enum per architecture to
list all possible handles and then just using those values directly
instead of going through another layer of indirection.

So is there anything that will require dynamic phandles in the future?
This version proposed here can't really cope with them anyway and in the
moment it's just about _one_ GIC phandle and _one_ ITS phandle, so a
static assignment is much simpler.

Will we need multiple ITSes for device passthrough? Or would it just be
one ITS for all hardware devices and one for emulated virtio?

Or do I miss anything else here?

Cheers,
Andre.

> 
> Thanks,
> 
> 	M.
>

Marc Zyngier Feb. 1, 2017, 5:13 p.m. UTC | #7

On Wed, Feb 01 2017 at  4:44:19 pm GMT, André Przywara <[e-mail]> wrote:

Hi Andre,

> Hi Marc, Will,
>
> On 09/12/16 12:03, Marc Zyngier wrote:
>> On 04/11/16 17:31, Andre Przywara wrote:
>>> Allocating an FDT phandle (a unique identifier) using a static
>>> variable in a static inline function defined in a header file works
>>> only if all users are in the same source file. So trying to allocate
>>> a handle from two different compilation units fails.
>>> Introduce global phandle allocation and reference code to properly
>>> allocate unique phandles.
>>>
>>> Signed-off-by: Andre Przywara <[e-mail]>
>>> ---
>>>  Makefile          |  1 +
>>>  arm/fdt.c         |  2 +-
>>>  arm/gic.c         |  2 +-
>>>  include/kvm/fdt.h | 10 +++++-----
>>>  kvm-fdt.c         | 26 ++++++++++++++++++++++++++
>>>  5 files changed, 34 insertions(+), 7 deletions(-)
>>>  create mode 100644 kvm-fdt.c
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 1f0196f..e4a4002 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -98,6 +98,7 @@ OBJS	+= kvm-ipc.o
>>>  OBJS	+= builtin-sandbox.o
>>>  OBJS	+= virtio/mmio.o
>>>  OBJS	+= hw/i8042.o
>>> +OBJS	+= kvm-fdt.o
>>>  
>>>  # Translate uname -m into ARCH string
>>>  ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
>>> diff --git a/arm/fdt.c b/arm/fdt.c
>>> index 381d48f..8bcfffb 100644
>>> --- a/arm/fdt.c
>>> +++ b/arm/fdt.c
>>> @@ -114,7 +114,7 @@ static int setup_fdt(struct kvm *kvm)
>>>  {
>>>  	struct device_header *dev_hdr;
>>>  	u8 staging_fdt[FDT_MAX_SIZE];
>>> -	u32 gic_phandle		= fdt__alloc_phandle();
>>> +	u32 gic_phandle		= fdt__get_phandle(PHANDLE_GIC);
>>>  	u64 mem_reg_prop[]	= {
>>>  		cpu_to_fdt64(kvm->arch.memory_guest_start),
>>>  		cpu_to_fdt64(kvm->ram_size),
>>> diff --git a/arm/gic.c b/arm/gic.c
>>> index d6d6dd0..b60437e 100644
>>> --- a/arm/gic.c
>>> +++ b/arm/gic.c
>>> @@ -222,7 +222,7 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>>>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>>>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
>>> -	_FDT(fdt_property_cell(fdt, "phandle", phandle));
>>> +	_FDT(fdt_property_cell(fdt, "phandle", fdt__get_phandle(PHANDLE_GIC)));
>>>  	_FDT(fdt_end_node(fdt));
>>>  }
>>>  
>>> diff --git a/include/kvm/fdt.h b/include/kvm/fdt.h
>>> index 53d85a4..cd2bb72 100644
>>> --- a/include/kvm/fdt.h
>>> +++ b/include/kvm/fdt.h
>>> @@ -8,6 +8,10 @@
>>>  #include <linux/types.h>
>>>  
>>>  #define FDT_MAX_SIZE	0x10000
>>> +#define FDT_INVALID_PHANDLE 0
>>> +#define FDT_IS_VALID_PHANDLE(phandle) ((phandle) != FDT_INVALID_PHANDLE)
>>> +
>>> +enum phandles {PHANDLE_GIC, PHANDLES_MAX};
>> 
>> Another nit here: PHANDLE_GIC is pretty much ARM-specific, while FDT is
>> supposed to be generic. Can't we move the enum to be architecture
>> specific and not put this in an architecture agnostic one?
>
> So while trying to find the best possible solution for this seemingly
> simple problem, I was wondering why we had this allocation function in
> the first place?
> It seems a bit overkill to allocate a handle if we could just go with
> static values.
> I changed the first two patches now to have an enum per architecture to
> list all possible handles and then just using those values directly
> instead of going through another layer of indirection.

Yes, that probably make sense, at least for the time being.

> So is there anything that will require dynamic phandles in the future?
> This version proposed here can't really cope with them anyway and in the
> moment it's just about _one_ GIC phandle and _one_ ITS phandle, so a
> static assignment is much simpler.
>
> Will we need multiple ITSes for device passthrough? Or would it just be
> one ITS for all hardware devices and one for emulated virtio?

Having multiple ITSes is definitely on the cards (pass-through and
emulated devices are one case, where once of them would be driven using
GICv4 and the other be purely virtual). This probably would translate
into having multiple PCIe host controllers.

So maybe we don't need the full breath of an allocator yet, but I reckon
that we shouldn't pretend that there is no use for it, forever.

Thanks,

	M.

Andre Przywara Feb. 2, 2017, 4:31 p.m. UTC | #8

Hi Marc,

On 01/02/17 17:13, Marc Zyngier wrote:
> On Wed, Feb 01 2017 at  4:44:19 pm GMT, André Przywara <[e-mail]> wrote:
> 
> Hi Andre,
> 
>> Hi Marc, Will,
>>
>> On 09/12/16 12:03, Marc Zyngier wrote:
>>> On 04/11/16 17:31, Andre Przywara wrote:
>>>> Allocating an FDT phandle (a unique identifier) using a static
>>>> variable in a static inline function defined in a header file works
>>>> only if all users are in the same source file. So trying to allocate
>>>> a handle from two different compilation units fails.
>>>> Introduce global phandle allocation and reference code to properly
>>>> allocate unique phandles.
>>>>
>>>> Signed-off-by: Andre Przywara <[e-mail]>
>>>> ---
>>>>  Makefile          |  1 +
>>>>  arm/fdt.c         |  2 +-
>>>>  arm/gic.c         |  2 +-
>>>>  include/kvm/fdt.h | 10 +++++-----
>>>>  kvm-fdt.c         | 26 ++++++++++++++++++++++++++
>>>>  5 files changed, 34 insertions(+), 7 deletions(-)
>>>>  create mode 100644 kvm-fdt.c
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index 1f0196f..e4a4002 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -98,6 +98,7 @@ OBJS	+= kvm-ipc.o
>>>>  OBJS	+= builtin-sandbox.o
>>>>  OBJS	+= virtio/mmio.o
>>>>  OBJS	+= hw/i8042.o
>>>> +OBJS	+= kvm-fdt.o
>>>>  
>>>>  # Translate uname -m into ARCH string
>>>>  ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
>>>> diff --git a/arm/fdt.c b/arm/fdt.c
>>>> index 381d48f..8bcfffb 100644
>>>> --- a/arm/fdt.c
>>>> +++ b/arm/fdt.c
>>>> @@ -114,7 +114,7 @@ static int setup_fdt(struct kvm *kvm)
>>>>  {
>>>>  	struct device_header *dev_hdr;
>>>>  	u8 staging_fdt[FDT_MAX_SIZE];
>>>> -	u32 gic_phandle		= fdt__alloc_phandle();
>>>> +	u32 gic_phandle		= fdt__get_phandle(PHANDLE_GIC);
>>>>  	u64 mem_reg_prop[]	= {
>>>>  		cpu_to_fdt64(kvm->arch.memory_guest_start),
>>>>  		cpu_to_fdt64(kvm->ram_size),
>>>> diff --git a/arm/gic.c b/arm/gic.c
>>>> index d6d6dd0..b60437e 100644
>>>> --- a/arm/gic.c
>>>> +++ b/arm/gic.c
>>>> @@ -222,7 +222,7 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>>>>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>>>>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>>>>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
>>>> -	_FDT(fdt_property_cell(fdt, "phandle", phandle));
>>>> +	_FDT(fdt_property_cell(fdt, "phandle", fdt__get_phandle(PHANDLE_GIC)));
>>>>  	_FDT(fdt_end_node(fdt));
>>>>  }
>>>>  
>>>> diff --git a/include/kvm/fdt.h b/include/kvm/fdt.h
>>>> index 53d85a4..cd2bb72 100644
>>>> --- a/include/kvm/fdt.h
>>>> +++ b/include/kvm/fdt.h
>>>> @@ -8,6 +8,10 @@
>>>>  #include <linux/types.h>
>>>>  
>>>>  #define FDT_MAX_SIZE	0x10000
>>>> +#define FDT_INVALID_PHANDLE 0
>>>> +#define FDT_IS_VALID_PHANDLE(phandle) ((phandle) != FDT_INVALID_PHANDLE)
>>>> +
>>>> +enum phandles {PHANDLE_GIC, PHANDLES_MAX};
>>>
>>> Another nit here: PHANDLE_GIC is pretty much ARM-specific, while FDT is
>>> supposed to be generic. Can't we move the enum to be architecture
>>> specific and not put this in an architecture agnostic one?
>>
>> So while trying to find the best possible solution for this seemingly
>> simple problem, I was wondering why we had this allocation function in
>> the first place?
>> It seems a bit overkill to allocate a handle if we could just go with
>> static values.
>> I changed the first two patches now to have an enum per architecture to
>> list all possible handles and then just using those values directly
>> instead of going through another layer of indirection.
> 
> Yes, that probably make sense, at least for the time being.
> 
>> So is there anything that will require dynamic phandles in the future?
>> This version proposed here can't really cope with them anyway and in the
>> moment it's just about _one_ GIC phandle and _one_ ITS phandle, so a
>> static assignment is much simpler.
>>
>> Will we need multiple ITSes for device passthrough? Or would it just be
>> one ITS for all hardware devices and one for emulated virtio?
> 
> Having multiple ITSes is definitely on the cards (pass-through and
> emulated devices are one case, where once of them would be driven using
> GICv4 and the other be purely virtual). This probably would translate
> into having multiple PCIe host controllers.

OK, I see. At the moment the kvmtool code seems to rely on having only a
single PCI controller using a static setup. As changing this would
require some rework (for instance to allow dynamic MMIO allocation), I
would like to refrain from adding multiple ITS support prematurely now.
We should do this later in conjunction with multiple PCI controller
support, I believe.

> So maybe we don't need the full breath of an allocator yet, but I reckon
> that we shouldn't pretend that there is no use for it, forever.

OK, got it.

I will post my version with the static phandle setup, feel free to shoot
it down if this is too much of a simplification or if you have the
feeling that removing the allocation will bite us later.

Cheers,
Andre.

Patch

diff --git a/Makefile b/Makefile
index 1f0196f..e4a4002 100644
--- a/Makefile
+++ b/Makefile
@@ -98,6 +98,7 @@  OBJS	+= kvm-ipc.o
 OBJS	+= builtin-sandbox.o
 OBJS	+= virtio/mmio.o
 OBJS	+= hw/i8042.o
+OBJS	+= kvm-fdt.o
 
 # Translate uname -m into ARCH string
 ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
diff --git a/arm/fdt.c b/arm/fdt.c
index 381d48f..8bcfffb 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -114,7 +114,7 @@  static int setup_fdt(struct kvm *kvm)
 {
 	struct device_header *dev_hdr;
 	u8 staging_fdt[FDT_MAX_SIZE];
-	u32 gic_phandle		= fdt__alloc_phandle();
+	u32 gic_phandle		= fdt__get_phandle(PHANDLE_GIC);
 	u64 mem_reg_prop[]	= {
 		cpu_to_fdt64(kvm->arch.memory_guest_start),
 		cpu_to_fdt64(kvm->ram_size),
diff --git a/arm/gic.c b/arm/gic.c
index d6d6dd0..b60437e 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -222,7 +222,7 @@  void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
 	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
 	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
-	_FDT(fdt_property_cell(fdt, "phandle", phandle));
+	_FDT(fdt_property_cell(fdt, "phandle", fdt__get_phandle(PHANDLE_GIC)));
 	_FDT(fdt_end_node(fdt));
 }
 
diff --git a/include/kvm/fdt.h b/include/kvm/fdt.h
index 53d85a4..cd2bb72 100644
--- a/include/kvm/fdt.h
+++ b/include/kvm/fdt.h
@@ -8,6 +8,10 @@ 
 #include <linux/types.h>
 
 #define FDT_MAX_SIZE	0x10000
+#define FDT_INVALID_PHANDLE 0
+#define FDT_IS_VALID_PHANDLE(phandle) ((phandle) != FDT_INVALID_PHANDLE)
+
+enum phandles {PHANDLE_GIC, PHANDLES_MAX};
 
 /* Those definitions are generic FDT values for specifying IRQ
  * types and are used in the Linux kernel internally as well as in
@@ -33,10 +37,6 @@  enum irq_type {
 		}							\
 	} while (0)
 
-static inline u32 fdt__alloc_phandle(void)
-{
-	static u32 phandle = 0;
-	return ++phandle;
-}
+u32 fdt__get_phandle(enum phandles phandle);
 
 #endif /* KVM__FDT_H */
diff --git a/kvm-fdt.c b/kvm-fdt.c
new file mode 100644
index 0000000..d05f3fe
--- /dev/null
+++ b/kvm-fdt.c
@@ -0,0 +1,26 @@ 
+/*
+ * Commonly used FDT functions.
+ */
+
+#include <stdio.h>
+#include "kvm/fdt.h"
+#include "kvm/util.h"
+
+u32 phandles[PHANDLES_MAX] = {};
+u32 next_phandle = 1;
+
+u32 fdt__get_phandle(enum phandles phandle)
+{
+	u32 ret;
+
+	if (phandle >= PHANDLES_MAX)
+		return FDT_INVALID_PHANDLE;
+
+	ret = phandles[phandle];
+	if (ret == FDT_INVALID_PHANDLE) {
+		ret = next_phandle++;
+		phandles[phandle] = ret;
+	}
+
+	return ret;
+}