| Message ID | 20250212112413.37553-2-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/microblaze: Allow running cross-endian vCPUs | expand |
On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: > Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. > Endianness can be BIG, LITTLE or unspecified (default). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > qapi/common.json | 16 ++++++++++++++++ > include/hw/qdev-properties-system.h | 7 +++++++ > hw/core/qdev-properties-system.c | 11 +++++++++++ > 3 files changed, 34 insertions(+) > > diff --git a/qapi/common.json b/qapi/common.json > index 6ffc7a37890..217feaaf683 100644 > --- a/qapi/common.json > +++ b/qapi/common.json > @@ -212,3 +212,19 @@ > ## > { 'struct': 'HumanReadableText', > 'data': { 'human-readable-text': 'str' } } > + > +## > +# @EndianMode: > +# > +# An enumeration of three options: little, big, and unspecified > +# > +# @little: Little endianness > +# > +# @big: Big endianness > +# > +# @unspecified: Endianness not specified > +# > +# Since: 10.0 > +## > +{ 'enum': 'EndianMode', > + 'data': [ 'little', 'big', 'unspecified' ] } Should 'unspecified' come first? ... so that it gets the value 0, i.e. when someone forgets to properly initialize a related variable, the chances are higher that it ends up as "unspecified" than as "little" ? Apart from that: Reviewed-by: Thomas Huth <thuth@redhat.com>
On 12/2/25 12:37, Thomas Huth wrote: > On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: >> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. >> Endianness can be BIG, LITTLE or unspecified (default). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> qapi/common.json | 16 ++++++++++++++++ >> include/hw/qdev-properties-system.h | 7 +++++++ >> hw/core/qdev-properties-system.c | 11 +++++++++++ >> 3 files changed, 34 insertions(+) >> >> diff --git a/qapi/common.json b/qapi/common.json >> index 6ffc7a37890..217feaaf683 100644 >> --- a/qapi/common.json >> +++ b/qapi/common.json >> @@ -212,3 +212,19 @@ >> ## >> { 'struct': 'HumanReadableText', >> 'data': { 'human-readable-text': 'str' } } >> + >> +## >> +# @EndianMode: >> +# >> +# An enumeration of three options: little, big, and unspecified >> +# >> +# @little: Little endianness >> +# >> +# @big: Big endianness >> +# >> +# @unspecified: Endianness not specified >> +# >> +# Since: 10.0 >> +## >> +{ 'enum': 'EndianMode', >> + 'data': [ 'little', 'big', 'unspecified' ] } > > Should 'unspecified' come first? ... so that it gets the value 0, i.e. > when someone forgets to properly initialize a related variable, the > chances are higher that it ends up as "unspecified" than as "little" ? Hmm but then in this series the dual-endianness regions are defined as: +static const MemoryRegionOps pic_ops[2] = { + [0 ... 1] = { + .read = pic_read, + .write = pic_write, + .endianness = DEVICE_BIG_ENDIAN, + .impl = { + .min_access_size = 4, + .max_access_size = 4, + }, + .valid = { + /* + * All XPS INTC registers are accessed through the PLB interface. + * The base address for these registers is provided by the + * configuration parameter, C_BASEADDR. Each register is 32 bits + * although some bits may be unused and is accessed on a 4-byte + * boundary offset from the base address. + */ + .min_access_size = 4, + .max_access_size = 4, + }, }, - .valid = { - .min_access_size = 4, - .max_access_size = 4 - } + [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, + [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, }; We could declare the array using the confusing __MAX definition (at the price of wasting the ENDIAN_MODE_UNSPECIFIED entry) as: static const MemoryRegionOps pic_ops[ENDIAN_MODE__MAX - 1] { } WDYT? > Apart from that: > Reviewed-by: Thomas Huth <thuth@redhat.com> > Thanks!
On 12/2/25 12:43, Philippe Mathieu-Daudé wrote: > On 12/2/25 12:37, Thomas Huth wrote: >> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: >>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. >>> Endianness can be BIG, LITTLE or unspecified (default). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> qapi/common.json | 16 ++++++++++++++++ >>> include/hw/qdev-properties-system.h | 7 +++++++ >>> hw/core/qdev-properties-system.c | 11 +++++++++++ >>> 3 files changed, 34 insertions(+) >>> >>> diff --git a/qapi/common.json b/qapi/common.json >>> index 6ffc7a37890..217feaaf683 100644 >>> --- a/qapi/common.json >>> +++ b/qapi/common.json >>> @@ -212,3 +212,19 @@ >>> ## >>> { 'struct': 'HumanReadableText', >>> 'data': { 'human-readable-text': 'str' } } >>> + >>> +## >>> +# @EndianMode: >>> +# >>> +# An enumeration of three options: little, big, and unspecified >>> +# >>> +# @little: Little endianness >>> +# >>> +# @big: Big endianness >>> +# >>> +# @unspecified: Endianness not specified >>> +# >>> +# Since: 10.0 >>> +## >>> +{ 'enum': 'EndianMode', >>> + 'data': [ 'little', 'big', 'unspecified' ] } >> >> Should 'unspecified' come first? ... so that it gets the value 0, i.e. >> when someone forgets to properly initialize a related variable, the >> chances are higher that it ends up as "unspecified" than as "little" ? BTW I'm not sure QAPI guaranty enums are following an order (at least, as in C, I wouldn't rely on that assumption).
On Wed, Feb 12, 2025 at 01:02:18PM +0100, Philippe Mathieu-Daudé wrote: > On 12/2/25 12:43, Philippe Mathieu-Daudé wrote: > > On 12/2/25 12:37, Thomas Huth wrote: > > > On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: > > > > Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. > > > > Endianness can be BIG, LITTLE or unspecified (default). > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > --- > > > > qapi/common.json | 16 ++++++++++++++++ > > > > include/hw/qdev-properties-system.h | 7 +++++++ > > > > hw/core/qdev-properties-system.c | 11 +++++++++++ > > > > 3 files changed, 34 insertions(+) > > > > > > > > diff --git a/qapi/common.json b/qapi/common.json > > > > index 6ffc7a37890..217feaaf683 100644 > > > > --- a/qapi/common.json > > > > +++ b/qapi/common.json > > > > @@ -212,3 +212,19 @@ > > > > ## > > > > { 'struct': 'HumanReadableText', > > > > 'data': { 'human-readable-text': 'str' } } > > > > + > > > > +## > > > > +# @EndianMode: > > > > +# > > > > +# An enumeration of three options: little, big, and unspecified > > > > +# > > > > +# @little: Little endianness > > > > +# > > > > +# @big: Big endianness > > > > +# > > > > +# @unspecified: Endianness not specified > > > > +# > > > > +# Since: 10.0 > > > > +## > > > > +{ 'enum': 'EndianMode', > > > > + 'data': [ 'little', 'big', 'unspecified' ] } > > > > > > Should 'unspecified' come first? ... so that it gets the value 0, > > > i.e. when someone forgets to properly initialize a related variable, > > > the chances are higher that it ends up as "unspecified" than as > > > "little" ? > > BTW I'm not sure QAPI guaranty enums are following an order > (at least, as in C, I wouldn't rely on that assumption). If we don't document a guaranteed order IMHO we should, mostly just for the sake for guaranteeing exactly what will be the 0 value . It is pretty common to want a particular enum constant to be special default for the 0 value. It allows enums to be retrofitted into existing code, with confidence that any code forgetting to initialize a variable/field will get the special default. Missed initialization is relatively common as a C bug, and we use -ftrivial-auto-var-init=zero to give well defined (usually safe) semantics. With regards, Daniel
On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: > On 12/2/25 12:37, Thomas Huth wrote: >> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: >>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. >>> Endianness can be BIG, LITTLE or unspecified (default). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> qapi/common.json | 16 ++++++++++++++++ >>> include/hw/qdev-properties-system.h | 7 +++++++ >>> hw/core/qdev-properties-system.c | 11 +++++++++++ >>> 3 files changed, 34 insertions(+) >>> >>> diff --git a/qapi/common.json b/qapi/common.json >>> index 6ffc7a37890..217feaaf683 100644 >>> --- a/qapi/common.json >>> +++ b/qapi/common.json >>> @@ -212,3 +212,19 @@ >>> ## >>> { 'struct': 'HumanReadableText', >>> 'data': { 'human-readable-text': 'str' } } >>> + >>> +## >>> +# @EndianMode: >>> +# >>> +# An enumeration of three options: little, big, and unspecified >>> +# >>> +# @little: Little endianness >>> +# >>> +# @big: Big endianness >>> +# >>> +# @unspecified: Endianness not specified >>> +# >>> +# Since: 10.0 >>> +## >>> +{ 'enum': 'EndianMode', >>> + 'data': [ 'little', 'big', 'unspecified' ] } >> >> Should 'unspecified' come first? ... so that it gets the value 0, i.e. when >> someone forgets to properly initialize a related variable, the chances are >> higher that it ends up as "unspecified" than as "little" ? > > Hmm but then in this series the dual-endianness regions are defined as: > > +static const MemoryRegionOps pic_ops[2] = { > + [0 ... 1] = { This is already confusing as you'd have to know that 0 and 1 here means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as well might be clearer). It's easy to miss what this does so maybe repeating the definitions for each case would be longer but less confusing and then it does not matter what the values are. Or what uses the ops.endianness now should look at the property introduced by this patch to avoid having to propagate it like below and drop the ops.endianness? Or it should move to the memory region and set when the ops are assigned? Regards, BALATON Zoltan > + .read = pic_read, > + .write = pic_write, > + .endianness = DEVICE_BIG_ENDIAN, > + .impl = { > + .min_access_size = 4, > + .max_access_size = 4, > + }, > + .valid = { > + /* > + * All XPS INTC registers are accessed through the PLB > interface. > + * The base address for these registers is provided by the > + * configuration parameter, C_BASEADDR. Each register is 32 bits > + * although some bits may be unused and is accessed on a 4-byte > + * boundary offset from the base address. > + */ > + .min_access_size = 4, > + .max_access_size = 4, > + }, > }, > - .valid = { > - .min_access_size = 4, > - .max_access_size = 4 > - } > + [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, > + [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, > }; > > We could declare the array using the confusing __MAX definition > (at the price of wasting the ENDIAN_MODE_UNSPECIFIED entry) as: > > static const MemoryRegionOps pic_ops[ENDIAN_MODE__MAX - 1] { } > > WDYT? > >> Apart from that: >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> > > Thanks! > >
On 12/2/25 13:56, BALATON Zoltan wrote: > On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: >> On 12/2/25 12:37, Thomas Huth wrote: >>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: >>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. >>>> Endianness can be BIG, LITTLE or unspecified (default). >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> --- >>>> qapi/common.json | 16 ++++++++++++++++ >>>> include/hw/qdev-properties-system.h | 7 +++++++ >>>> hw/core/qdev-properties-system.c | 11 +++++++++++ >>>> 3 files changed, 34 insertions(+) >>>> >>>> diff --git a/qapi/common.json b/qapi/common.json >>>> index 6ffc7a37890..217feaaf683 100644 >>>> --- a/qapi/common.json >>>> +++ b/qapi/common.json >>>> @@ -212,3 +212,19 @@ >>>> ## >>>> { 'struct': 'HumanReadableText', >>>> 'data': { 'human-readable-text': 'str' } } >>>> + >>>> +## >>>> +# @EndianMode: >>>> +# >>>> +# An enumeration of three options: little, big, and unspecified >>>> +# >>>> +# @little: Little endianness >>>> +# >>>> +# @big: Big endianness >>>> +# >>>> +# @unspecified: Endianness not specified >>>> +# >>>> +# Since: 10.0 >>>> +## >>>> +{ 'enum': 'EndianMode', >>>> + 'data': [ 'little', 'big', 'unspecified' ] } >>> >>> Should 'unspecified' come first? ... so that it gets the value 0, >>> i.e. when someone forgets to properly initialize a related variable, >>> the chances are higher that it ends up as "unspecified" than as >>> "little" ? >> >> Hmm but then in this series the dual-endianness regions are defined as: >> >> +static const MemoryRegionOps pic_ops[2] = { >> + [0 ... 1] = { > > This is already confusing as you'd have to know that 0 and 1 here means > ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as > well might be clearer). It's easy to miss what this does so maybe > repeating the definitions for each case would be longer but less > confusing and then it does not matter what the values are. > > Or what uses the ops.endianness now should look at the property > introduced by this patch to avoid having to propagate it like below and > drop the ops.endianness? Or it should move to the memory region and set > when the ops are assigned? I'm not understanding well what you ask, but maybe the answer is in v7 :)
On 12/2/25 14:53, Philippe Mathieu-Daudé wrote: > On 12/2/25 13:56, BALATON Zoltan wrote: >> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: >>> On 12/2/25 12:37, Thomas Huth wrote: >>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: >>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. >>>>> Endianness can be BIG, LITTLE or unspecified (default). >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> --- >>>>> qapi/common.json | 16 ++++++++++++++++ >>>>> include/hw/qdev-properties-system.h | 7 +++++++ >>>>> hw/core/qdev-properties-system.c | 11 +++++++++++ >>>>> 3 files changed, 34 insertions(+) >>>>> >>>>> diff --git a/qapi/common.json b/qapi/common.json >>>>> index 6ffc7a37890..217feaaf683 100644 >>>>> --- a/qapi/common.json >>>>> +++ b/qapi/common.json >>>>> @@ -212,3 +212,19 @@ >>>>> ## >>>>> { 'struct': 'HumanReadableText', >>>>> 'data': { 'human-readable-text': 'str' } } >>>>> + >>>>> +## >>>>> +# @EndianMode: >>>>> +# >>>>> +# An enumeration of three options: little, big, and unspecified >>>>> +# >>>>> +# @little: Little endianness >>>>> +# >>>>> +# @big: Big endianness >>>>> +# >>>>> +# @unspecified: Endianness not specified >>>>> +# >>>>> +# Since: 10.0 >>>>> +## >>>>> +{ 'enum': 'EndianMode', >>>>> + 'data': [ 'little', 'big', 'unspecified' ] } >>>> >>>> Should 'unspecified' come first? ... so that it gets the value 0, >>>> i.e. when someone forgets to properly initialize a related variable, >>>> the chances are higher that it ends up as "unspecified" than as >>>> "little" ? >>> >>> Hmm but then in this series the dual-endianness regions are defined as: >>> >>> +static const MemoryRegionOps pic_ops[2] = { >>> + [0 ... 1] = { >> >> This is already confusing as you'd have to know that 0 and 1 here >> means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants >> here as well might be clearer). It's easy to miss what this does so At this point 0 / 1 only mean "from the index #0 included to the index #1 included", 0 being the first one and 1 the last one. >> maybe repeating the definitions for each case would be longer but less >> confusing and then it does not matter what the values are. This is what I tried to do with: + [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, + [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, }; but in v7 we are back of picking an arbitrary value. >> Or what uses the ops.endianness now should look at the property >> introduced by this patch to avoid having to propagate it like below >> and drop the ops.endianness? Or it should move to the memory region >> and set when the ops are assigned? > > I'm not understanding well what you ask, but maybe the answer is in v7 :)
On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: > On 12/2/25 14:53, Philippe Mathieu-Daudé wrote: >> On 12/2/25 13:56, BALATON Zoltan wrote: >>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: >>>> On 12/2/25 12:37, Thomas Huth wrote: >>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: >>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. >>>>>> Endianness can be BIG, LITTLE or unspecified (default). >>>>>> >>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>>> --- >>>>>> qapi/common.json | 16 ++++++++++++++++ >>>>>> include/hw/qdev-properties-system.h | 7 +++++++ >>>>>> hw/core/qdev-properties-system.c | 11 +++++++++++ >>>>>> 3 files changed, 34 insertions(+) >>>>>> >>>>>> diff --git a/qapi/common.json b/qapi/common.json >>>>>> index 6ffc7a37890..217feaaf683 100644 >>>>>> --- a/qapi/common.json >>>>>> +++ b/qapi/common.json >>>>>> @@ -212,3 +212,19 @@ >>>>>> ## >>>>>> { 'struct': 'HumanReadableText', >>>>>> 'data': { 'human-readable-text': 'str' } } >>>>>> + >>>>>> +## >>>>>> +# @EndianMode: >>>>>> +# >>>>>> +# An enumeration of three options: little, big, and unspecified >>>>>> +# >>>>>> +# @little: Little endianness >>>>>> +# >>>>>> +# @big: Big endianness >>>>>> +# >>>>>> +# @unspecified: Endianness not specified >>>>>> +# >>>>>> +# Since: 10.0 >>>>>> +## >>>>>> +{ 'enum': 'EndianMode', >>>>>> + 'data': [ 'little', 'big', 'unspecified' ] } >>>>> >>>>> Should 'unspecified' come first? ... so that it gets the value 0, i.e. >>>>> when someone forgets to properly initialize a related variable, the >>>>> chances are higher that it ends up as "unspecified" than as "little" ? >>>> >>>> Hmm but then in this series the dual-endianness regions are defined as: >>>> >>>> +static const MemoryRegionOps pic_ops[2] = { >>>> + [0 ... 1] = { >>> >>> This is already confusing as you'd have to know that 0 and 1 here means >>> ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants here as well >>> might be clearer). It's easy to miss what this does so > > At this point 0 / 1 only mean "from the index #0 included to the index > #1 included", 0 being the first one and 1 the last one. > >>> maybe repeating the definitions for each case would be longer but less >>> confusing and then it does not matter what the values are. > > This is what I tried to do with: > > + [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, > + [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, > }; > > but in v7 we are back of picking an arbitrary value. > >>> Or what uses the ops.endianness now should look at the property introduced >>> by this patch to avoid having to propagate it like below and drop the >>> ops.endianness? Or it should move to the memory region and set when the >>> ops are assigned? >> >> I'm not understanding well what you ask, but maybe the answer is in v7 :) I'm not sure I understand it well either. I think what I was asking about is the same as what Thomas asked if this could be avoided to make it necessary to allocate two separate ops for this. Looks like from now on this ops struct should really loose the endianness value and this should be assigned when the ops is added to the io region because that's where it decides which endianness is it based on the property added in this series. But I don't know if that could be done or would need deeper changes as what later uses this ops struct might not have access to the property and if we have a single ops struct it may need to be copied to set different endianness intstead of just referencing it. So I'm not sure there's a better way but I think this change makes an already cryptic boiler plate even more confusing for people less knowledgeable about QEMU and C programming so it makes even harder to write devices. But as long as it's just a few devices that need to work with different endianness then it might be OK. But wasn't that what NATIVE_ENDIAN was meant for? What can't that be kept then? Regards, BALATON Zoltan
On 12/2/25 17:23, BALATON Zoltan wrote: > On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: >> On 12/2/25 14:53, Philippe Mathieu-Daudé wrote: >>> On 12/2/25 13:56, BALATON Zoltan wrote: >>>> On Wed, 12 Feb 2025, Philippe Mathieu-Daudé wrote: >>>>> On 12/2/25 12:37, Thomas Huth wrote: >>>>>> On 12/02/2025 12.24, Philippe Mathieu-Daudé wrote: >>>>>>> Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. >>>>>>> Endianness can be BIG, LITTLE or unspecified (default). >>>>>>> >>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>>>> --- >>>>>>> qapi/common.json | 16 ++++++++++++++++ >>>>>>> include/hw/qdev-properties-system.h | 7 +++++++ >>>>>>> hw/core/qdev-properties-system.c | 11 +++++++++++ >>>>>>> 3 files changed, 34 insertions(+) >>>>>>> +{ 'enum': 'EndianMode', >>>>>>> + 'data': [ 'little', 'big', 'unspecified' ] } >>>>>> >>>>>> Should 'unspecified' come first? ... so that it gets the value 0, >>>>>> i.e. when someone forgets to properly initialize a related >>>>>> variable, the chances are higher that it ends up as "unspecified" >>>>>> than as "little" ? >>>>> >>>>> Hmm but then in this series the dual-endianness regions are defined >>>>> as: >>>>> >>>>> +static const MemoryRegionOps pic_ops[2] = { >>>>> + [0 ... 1] = { >>>> >>>> This is already confusing as you'd have to know that 0 and 1 here >>>> means ENDIAN_MODE_LITTLE and ENDIAN_MODE_BIG (using those constants >>>> here as well might be clearer). It's easy to miss what this does so >> >> At this point 0 / 1 only mean "from the index #0 included to the index >> #1 included", 0 being the first one and 1 the last one. >> >>>> maybe repeating the definitions for each case would be longer but >>>> less confusing and then it does not matter what the values are. >> >> This is what I tried to do with: >> >> + [ENDIAN_MODE_BIG].endianness = DEVICE_BIG_ENDIAN, >> + [ENDIAN_MODE_LITTLE].endianness = DEVICE_LITTLE_ENDIAN, >> }; >> >> but in v7 we are back of picking an arbitrary value. >> >>>> Or what uses the ops.endianness now should look at the property >>>> introduced by this patch to avoid having to propagate it like below >>>> and drop the ops.endianness? Or it should move to the memory region >>>> and set when the ops are assigned? >>> >>> I'm not understanding well what you ask, but maybe the answer is in >>> v7 :) > > I'm not sure I understand it well either. I think what I was asking > about is the same as what Thomas asked if this could be avoided to make > it necessary to allocate two separate ops for this. Looks like from now > on this ops struct should really loose the endianness value and this > should be assigned when the ops is added to the io region because that's > where it decides which endianness is it based on the property added in > this series. But I don't know if that could be done or would need deeper > changes as what later uses this ops struct might not have access to the > property and if we have a single ops struct it may need to be copied to > set different endianness intstead of just referencing it. So I'm not > sure there's a better way but I think this change makes an already > cryptic boiler plate even more confusing for people less knowledgeable > about QEMU and C programming so it makes even harder to write devices. > But as long as it's just a few devices that need to work with different > endianness then it might be OK. But wasn't that what NATIVE_ENDIAN was > meant for? What can't that be kept then? Moving toward a single binary able to run heterogeneous machines, we can't rely on a particular target endianness, so we need to remove DEVICE_NATIVE_ENDIAN. The endianness is a property a device / machine, not of the binary.
diff --git a/qapi/common.json b/qapi/common.json index 6ffc7a37890..217feaaf683 100644 --- a/qapi/common.json +++ b/qapi/common.json @@ -212,3 +212,19 @@ ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } + +## +# @EndianMode: +# +# An enumeration of three options: little, big, and unspecified +# +# @little: Little endianness +# +# @big: Big endianness +# +# @unspecified: Endianness not specified +# +# Since: 10.0 +## +{ 'enum': 'EndianMode', + 'data': [ 'little', 'big', 'unspecified' ] } diff --git a/include/hw/qdev-properties-system.h b/include/hw/qdev-properties-system.h index 7ec37f6316c..ead4dfc2f02 100644 --- a/include/hw/qdev-properties-system.h +++ b/include/hw/qdev-properties-system.h @@ -30,6 +30,7 @@ extern const PropertyInfo qdev_prop_pcie_link_speed; extern const PropertyInfo qdev_prop_pcie_link_width; extern const PropertyInfo qdev_prop_cpus390entitlement; extern const PropertyInfo qdev_prop_iothread_vq_mapping_list; +extern const PropertyInfo qdev_prop_endian_mode; #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d) \ DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t) @@ -97,4 +98,10 @@ extern const PropertyInfo qdev_prop_iothread_vq_mapping_list; DEFINE_PROP(_name, _state, _field, qdev_prop_iothread_vq_mapping_list, \ IOThreadVirtQueueMappingList *) +#define DEFINE_PROP_ENDIAN(_name, _state, _field, _default) \ + DEFINE_PROP_UNSIGNED(_name, _state, _field, _default, \ + qdev_prop_endian_mode, EndianMode) +#define DEFINE_PROP_ENDIAN_NODEFAULT(_name, _state, _field) \ + DEFINE_PROP_ENDIAN(_name, _state, _field, ENDIAN_MODE_UNSPECIFIED) + #endif diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index a96675beb0d..89f954f569e 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -1283,3 +1283,14 @@ const PropertyInfo qdev_prop_iothread_vq_mapping_list = { .set = set_iothread_vq_mapping_list, .release = release_iothread_vq_mapping_list, }; + +/* --- Endian modes */ + +const PropertyInfo qdev_prop_endian_mode = { + .name = "EndianMode", + .description = "Endian mode, big/little/unspecified", + .enum_table = &EndianMode_lookup, + .get = qdev_propinfo_get_enum, + .set = qdev_propinfo_set_enum, + .set_default_value = qdev_propinfo_set_default_value_enum, +};
Introduce the EndianMode type and the DEFINE_PROP_ENDIAN() macros. Endianness can be BIG, LITTLE or unspecified (default). Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- qapi/common.json | 16 ++++++++++++++++ include/hw/qdev-properties-system.h | 7 +++++++ hw/core/qdev-properties-system.c | 11 +++++++++++ 3 files changed, 34 insertions(+)