[01/18] smbios: Add a function to directly add an entry

From: Corey Minyard <cminyard@mvista.com>

There was no way to directly add a table entry to the SMBIOS table,
even though the BIOS supports this.  So add a function to do this.
This is in preparation for the IPMI handler adding it's SMBIOS table
entry.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/smbios.c |   27 +++++++++++++++++++++++++++
 hw/smbios.h |   15 ++++++++-------
 2 files changed, 35 insertions(+), 7 deletions(-)

minyard@acm.org writes:

> From: Corey Minyard <cminyard@mvista.com>
>
> There was no way to directly add a table entry to the SMBIOS table,
> even though the BIOS supports this.  So add a function to do this.
> This is in preparation for the IPMI handler adding it's SMBIOS table
> entry.
>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

I don't expect that hardware ever adds SMBIOS entries.  Rather, the BIOS
adds the entries by probing the hardware.

So I think you should solve this in SeaBIOS, instead of trying to do it
in QEMU.  I think that also solves the problem you have with
pre-firmware init.

Regards,

Anthony Liguori

> ---
>  hw/smbios.c |   27 +++++++++++++++++++++++++++
>  hw/smbios.h |   15 ++++++++-------
>  2 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/hw/smbios.c b/hw/smbios.c
> index c57237d..c0c1be1 100644
> --- a/hw/smbios.c
> +++ b/hw/smbios.c
> @@ -178,6 +178,33 @@ static void smbios_build_type_1_fields(const char *t)
>                           strlen(buf) + 1, buf);
>  }
>  
> +int smbios_table_entry_add(struct smbios_structure_header *entry)
> +{
> +    struct smbios_table *table;
> +    struct smbios_structure_header *header;
> +    unsigned int size = entry->length;
> +
> +    if (!smbios_entries) {
> +        smbios_entries_len = sizeof(uint16_t);
> +        smbios_entries = g_malloc0(smbios_entries_len);
> +    }
> +    smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
> +                               sizeof(*table) + size);
> +    table = (struct smbios_table *)(smbios_entries + smbios_entries_len);
> +    table->header.type = SMBIOS_TABLE_ENTRY;
> +    table->header.length = cpu_to_le16(sizeof(*table) + size);
> +
> +    header = (struct smbios_structure_header *)(table->data);
> +    memcpy(header, entry, size);
> +
> +    smbios_check_collision(header->type, SMBIOS_TABLE_ENTRY);
> +
> +    smbios_entries_len += sizeof(*table) + size;
> +    (*(uint16_t *)smbios_entries) =
> +        cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
> +    return 0;
> +}
> +
>  int smbios_entry_add(const char *t)
>  {
>      char buf[1024];
> diff --git a/hw/smbios.h b/hw/smbios.h
> index 94e3641..6431a15 100644
> --- a/hw/smbios.h
> +++ b/hw/smbios.h
> @@ -13,21 +13,22 @@
>   *
>   */
>  
> +/* This goes at the beginning of every SMBIOS structure. */
> +struct smbios_structure_header {
> +    uint8_t type;
> +    uint8_t length;
> +    uint16_t handle;
> +} QEMU_PACKED;
> +
>  int smbios_entry_add(const char *t);
>  void smbios_add_field(int type, int offset, int len, void *data);
>  uint8_t *smbios_get_table(size_t *length);
> +int smbios_table_entry_add(struct smbios_structure_header *entry);
>  
>  /*
>   * SMBIOS spec defined tables
>   */
>  
> -/* This goes at the beginning of every SMBIOS structure. */
> -struct smbios_structure_header {
> -    uint8_t type;
> -    uint8_t length;
> -    uint16_t handle;
> -} QEMU_PACKED;
> -
>  /* SMBIOS type 0 - BIOS Information */
>  struct smbios_type_0 {
>      struct smbios_structure_header header;
> -- 
> 1.7.4.1

On 07/30/2012 10:37 AM, Anthony Liguori wrote:
> minyard@acm.org writes:
>
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> There was no way to directly add a table entry to the SMBIOS table,
>> even though the BIOS supports this.  So add a function to do this.
>> This is in preparation for the IPMI handler adding it's SMBIOS table
>> entry.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> I don't expect that hardware ever adds SMBIOS entries.  Rather, the BIOS
> adds the entries by probing the hardware.

Well, memory entries are added by QEMU, why not let the BIOS probe for 
that?  Plus, I really doubt that BIOSes on real systems probe for this.  
I'd guess they are hard-coded.

>
> So I think you should solve this in SeaBIOS, instead of trying to do it
> in QEMU.  I think that also solves the problem you have with
> pre-firmware init.

The user can pass the I/O base and IRQ values in on the QEMU command 
line, and they can be arbitrary values.  The BIOS is not going to be 
able to probe for those.

-corey

Corey Minyard <cminyard@mvista.com> writes:

> On 07/30/2012 10:37 AM, Anthony Liguori wrote:
>> minyard@acm.org writes:
>>
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> There was no way to directly add a table entry to the SMBIOS table,
>>> even though the BIOS supports this.  So add a function to do this.
>>> This is in preparation for the IPMI handler adding it's SMBIOS table
>>> entry.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> I don't expect that hardware ever adds SMBIOS entries.  Rather, the BIOS
>> adds the entries by probing the hardware.
>
> Well, memory entries are added by QEMU, why not let the BIOS probe for 
> that?

QEMU doesn't add any entries by default.  SeaBIOS owns SMBIOS.  QEMU
uses a backchannel to hand SeaBIOS tables that SeaBIOS can then expose.
The reason we use a table based interface is because type 0 and type 1
tables can have vendor extensions that are in a vendor specific format.

But SeaBIOS unquestionably owns SMBIOS generation.

>  Plus, I really doubt that BIOSes on real systems probe for this.  
> I'd guess they are hard-coded.

I think you'd be surprised how little is hard coded on modern BIOSes.

>> So I think you should solve this in SeaBIOS, instead of trying to do it
>> in QEMU.  I think that also solves the problem you have with
>> pre-firmware init.
>
> The user can pass the I/O base and IRQ values in on the QEMU command 
> line, and they can be arbitrary values.  The BIOS is not going to be 
> able to probe for those.

Then pass the information that the BIOS needs through fw_cfg.  That's
what it's there for.

Regards,

Anthony Liguoi

>
> -corey

On 07/30/2012 12:25 PM, Anthony Liguori wrote:
> Corey Minyard <cminyard@mvista.com> writes:
>
>> On 07/30/2012 10:37 AM, Anthony Liguori wrote:
>>> minyard@acm.org writes:
>>>
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> There was no way to directly add a table entry to the SMBIOS table,
>>>> even though the BIOS supports this.  So add a function to do this.
>>>> This is in preparation for the IPMI handler adding it's SMBIOS table
>>>> entry.
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> I don't expect that hardware ever adds SMBIOS entries.  Rather, the BIOS
>>> adds the entries by probing the hardware.
>> Well, memory entries are added by QEMU, why not let the BIOS probe for
>> that?
> QEMU doesn't add any entries by default.  SeaBIOS owns SMBIOS.  QEMU
> uses a backchannel to hand SeaBIOS tables that SeaBIOS can then expose.
> The reason we use a table based interface is because type 0 and type 1
> tables can have vendor extensions that are in a vendor specific format.
>
> But SeaBIOS unquestionably owns SMBIOS generation.
>
>>   Plus, I really doubt that BIOSes on real systems probe for this.
>> I'd guess they are hard-coded.
> I think you'd be surprised how little is hard coded on modern BIOSes.
>
>>> So I think you should solve this in SeaBIOS, instead of trying to do it
>>> in QEMU.  I think that also solves the problem you have with
>>> pre-firmware init.
>> The user can pass the I/O base and IRQ values in on the QEMU command
>> line, and they can be arbitrary values.  The BIOS is not going to be
>> able to probe for those.
> Then pass the information that the BIOS needs through fw_cfg.  That's
> what it's there for.

Ok, I understand.  Thanks, I'll look at doing it this way.

-corey

On Mon, Jul 30, 2012 at 12:25:16PM -0500, Anthony Liguori wrote:
> Corey Minyard <cminyard@mvista.com> writes:
> > On 07/30/2012 10:37 AM, Anthony Liguori wrote:
> >> minyard@acm.org writes:
> >>> There was no way to directly add a table entry to the SMBIOS table,
> >>> even though the BIOS supports this.  So add a function to do this.
> >>> This is in preparation for the IPMI handler adding it's SMBIOS table
> >>> entry.
> >> I don't expect that hardware ever adds SMBIOS entries.  Rather, the BIOS
> >> adds the entries by probing the hardware.
> >
> > Well, memory entries are added by QEMU, why not let the BIOS probe for 
> > that?
> QEMU doesn't add any entries by default.  SeaBIOS owns SMBIOS.  QEMU
> uses a backchannel to hand SeaBIOS tables that SeaBIOS can then expose.
> The reason we use a table based interface is because type 0 and type 1
> tables can have vendor extensions that are in a vendor specific format.
> 
> But SeaBIOS unquestionably owns SMBIOS generation.
[...]
> >> So I think you should solve this in SeaBIOS, instead of trying to do it
> >> in QEMU.  I think that also solves the problem you have with
> >> pre-firmware init.
> > The user can pass the I/O base and IRQ values in on the QEMU command 
> > line, and they can be arbitrary values.  The BIOS is not going to be 
> > able to probe for those.
> 
> Then pass the information that the BIOS needs through fw_cfg.  That's
> what it's there for.

This approach, unfortunately, leads to extra code and "double
handling" of infomation.

The ultimate consumer of the data wants a binary struct which looks
like:

struct smbios_type_38 {
    struct smbios_structure_header header;
    u8 interface_type;
    u8 ipmi_version;
    u8 i2c_slave_addr;
    u8 nv_storage_dev_addr;
    u64 base_addr;
    u8 base_addr_mod_and_irq_info;
    u8 interrupt_number;
};

The proposed solution is to put the info in a binary struct which
looks like:

struct ipmi_info {
    u8 str_version;
    u8 interface;
    u8 reg_space;
    u8 reg_spacing;
    u8 slave_addr;
    u8 irq;
    u8 version;
    u8 reserved1;
    u64 base_addr;
} PACKED;

and then have SeaBIOS translate the latter binary struct into the
former.  However, there's no compelling reason to introduce a new
binary struct instead of using the industry standard binary struct.

I'd prefer QEMU to just pass the SMBIOS struct and let SeaBIOS pass it
through to the OS.  I also think the existing fw_cfg mechanism for
passing SMBIOS tables should be used.  I understand that this causes
some contortions due to the current SMBIOS fw_cfg impementation in
QEMU, but it seems wrong to introduce a new fw_cfg port that
accomplishes the same goal as an existing fw_cfg port.

-Kevin

On 08/01/2012 08:15 PM, Kevin O'Connor wrote:
>
> This approach, unfortunately, leads to extra code and "double
> handling" of infomation.
>
> The ultimate consumer of the data wants a binary struct which looks
> like:
>
> struct smbios_type_38 {
>      struct smbios_structure_header header;
>      u8 interface_type;
>      u8 ipmi_version;
>      u8 i2c_slave_addr;
>      u8 nv_storage_dev_addr;
>      u64 base_addr;
>      u8 base_addr_mod_and_irq_info;
>      u8 interrupt_number;
> };
>
> The proposed solution is to put the info in a binary struct which
> looks like:
>
> struct ipmi_info {
>      u8 str_version;
>      u8 interface;
>      u8 reg_space;
>      u8 reg_spacing;
>      u8 slave_addr;
>      u8 irq;
>      u8 version;
>      u8 reserved1;
>      u64 base_addr;
> } PACKED;
>
> and then have SeaBIOS translate the latter binary struct into the
> former.  However, there's no compelling reason to introduce a new
> binary struct instead of using the industry standard binary struct.
>
> I'd prefer QEMU to just pass the SMBIOS struct and let SeaBIOS pass it
> through to the OS.  I also think the existing fw_cfg mechanism for
> passing SMBIOS tables should be used.  I understand that this causes
> some contortions due to the current SMBIOS fw_cfg impementation in
> QEMU, but it seems wrong to introduce a new fw_cfg port that
> accomplishes the same goal as an existing fw_cfg port.

Well, I should also probably add the ACPI name space definition for this 
information, too, and the SMBIOS information is not capable of passing 
all the information required for this (though the above structure can).

I've been studying this, but I don't see an obvious way to dynamically 
add something to the ACPI name space.  At least an easy way.

-corey

Corey Minyard <cminyard@mvista.com> writes:

> On 08/01/2012 08:15 PM, Kevin O'Connor wrote:
>>
>> This approach, unfortunately, leads to extra code and "double
>> handling" of infomation.
>>
>> The ultimate consumer of the data wants a binary struct which looks
>> like:
>>
>> struct smbios_type_38 {
>>      struct smbios_structure_header header;
>>      u8 interface_type;
>>      u8 ipmi_version;
>>      u8 i2c_slave_addr;
>>      u8 nv_storage_dev_addr;
>>      u64 base_addr;
>>      u8 base_addr_mod_and_irq_info;
>>      u8 interrupt_number;
>> };
>>
>> The proposed solution is to put the info in a binary struct which
>> looks like:
>>
>> struct ipmi_info {
>>      u8 str_version;
>>      u8 interface;
>>      u8 reg_space;
>>      u8 reg_spacing;
>>      u8 slave_addr;
>>      u8 irq;
>>      u8 version;
>>      u8 reserved1;
>>      u64 base_addr;
>> } PACKED;
>>
>> and then have SeaBIOS translate the latter binary struct into the
>> former.  However, there's no compelling reason to introduce a new
>> binary struct instead of using the industry standard binary struct.
>>
>> I'd prefer QEMU to just pass the SMBIOS struct and let SeaBIOS pass it
>> through to the OS.  I also think the existing fw_cfg mechanism for
>> passing SMBIOS tables should be used.  I understand that this causes
>> some contortions due to the current SMBIOS fw_cfg impementation in
>> QEMU, but it seems wrong to introduce a new fw_cfg port that
>> accomplishes the same goal as an existing fw_cfg port.
>
> Well, I should also probably add the ACPI name space definition for this 
> information, too, and the SMBIOS information is not capable of passing 
> all the information required for this (though the above structure can).
>
> I've been studying this, but I don't see an obvious way to dynamically 
> add something to the ACPI name space.  At least an easy way.

Okay, I was actually going to ask if there was an ACPI table for this.

Maybe this argues in favor of doing a fw_cfg interface?

Another question--is it really necessary for all of this to be user
specified?  Can't we just use a static SMBIOS/ACPI entry?  Then SeaBIOS
only needs to be concerned with whether or not an IPMI device exists.

Regards,

Anthony Liguori

>
> -corey

On 08/01/2012 09:40 PM, Anthony Liguori wrote:
> Corey Minyard <cminyard@mvista.com> writes:
>
>> On 08/01/2012 08:15 PM, Kevin O'Connor wrote:
>> Well, I should also probably add the ACPI name space definition for this
>> information, too, and the SMBIOS information is not capable of passing
>> all the information required for this (though the above structure can).
>>
>> I've been studying this, but I don't see an obvious way to dynamically
>> add something to the ACPI name space.  At least an easy way.
> Okay, I was actually going to ask if there was an ACPI table for this.
>
> Maybe this argues in favor of doing a fw_cfg interface?
>
> Another question--is it really necessary for all of this to be user
> specified?  Can't we just use a static SMBIOS/ACPI entry?  Then SeaBIOS
> only needs to be concerned with whether or not an IPMI device exists.

That's a good question At least the interrupt is important for the user 
to be able to specify.  The specific interface type may also be 
important if the user is trying to accomplish some specific emulation.  
Two other standard emulations exist, too, one in memory and one over 
I2C.  I'd eventually like to add those, if for nothing else my ability 
to test the interfaces.

If the user is trying to emulate some specific machine, setting the 
address is also important, and I need to add the ability to specify 
register spacing and the address space.  This will become more important 
for non-x86 machines.

-corey

> Regards,
>
> Anthony Liguori
>
>> -corey

Corey Minyard <tcminyard@gmail.com> writes:

> On 08/01/2012 09:40 PM, Anthony Liguori wrote:
>> Corey Minyard <cminyard@mvista.com> writes:
>>
>>> On 08/01/2012 08:15 PM, Kevin O'Connor wrote:
>>> Well, I should also probably add the ACPI name space definition for this
>>> information, too, and the SMBIOS information is not capable of passing
>>> all the information required for this (though the above structure can).
>>>
>>> I've been studying this, but I don't see an obvious way to dynamically
>>> add something to the ACPI name space.  At least an easy way.
>> Okay, I was actually going to ask if there was an ACPI table for this.
>>
>> Maybe this argues in favor of doing a fw_cfg interface?
>>
>> Another question--is it really necessary for all of this to be user
>> specified?  Can't we just use a static SMBIOS/ACPI entry?  Then SeaBIOS
>> only needs to be concerned with whether or not an IPMI device exists.
>
> That's a good question At least the interrupt is important for the user 
> to be able to specify.  The specific interface type may also be 
> important if the user is trying to accomplish some specific emulation.  

Why is it important to specify the interrupt?  Is this important for a
typical user, or important for the IPMI maintainer who needs to test a
bunch of different scenarios? :-)

If it's the later, we can probably express the interrupt number as a
#define in SeaBIOS, but still make it configurable in QEMU.  Then you
could build multiple copies of SeaBIOS and then just point QEMU at the
right version.

> Two other standard emulations exist, too, one in memory and one over 
> I2C.  I'd eventually like to add those, if for nothing else my ability 
> to test the interfaces.

Right, see above.  It may be easier to just build multiple copies of the
BIOS then to try and make this all dynamic.

Regards,

Anthony Liguori

>
> If the user is trying to emulate some specific machine, setting the 
> address is also important, and I need to add the ability to specify 
> register spacing and the address space.  This will become more important 
> for non-x86 machines.
>
> -corey
>
>> Regards,
>>
>> Anthony Liguori
>>
>>> -corey

On 08/02/2012 01:32 PM, Anthony Liguori wrote:
> Corey Minyard <tcminyard@gmail.com> writes:
>
>> On 08/01/2012 09:40 PM, Anthony Liguori wrote:
>>> Corey Minyard <cminyard@mvista.com> writes:
>>>
>>>> On 08/01/2012 08:15 PM, Kevin O'Connor wrote:
>>>> Well, I should also probably add the ACPI name space definition for this
>>>> information, too, and the SMBIOS information is not capable of passing
>>>> all the information required for this (though the above structure can).
>>>>
>>>> I've been studying this, but I don't see an obvious way to dynamically
>>>> add something to the ACPI name space.  At least an easy way.
>>> Okay, I was actually going to ask if there was an ACPI table for this.
>>>
>>> Maybe this argues in favor of doing a fw_cfg interface?
>>>
>>> Another question--is it really necessary for all of this to be user
>>> specified?  Can't we just use a static SMBIOS/ACPI entry?  Then SeaBIOS
>>> only needs to be concerned with whether or not an IPMI device exists.
>> That's a good question At least the interrupt is important for the user
>> to be able to specify.  The specific interface type may also be
>> important if the user is trying to accomplish some specific emulation.
> Why is it important to specify the interrupt?  Is this important for a
> typical user, or important for the IPMI maintainer who needs to test a
> bunch of different scenarios? :-)

I'm not too worried about the IPMI maintainer, he can hack in what he 
likes :).

I would be worried about conflicts on interrupts with other devices.  I 
really don't know how people use qemu out in the wild, though.  If they 
are trying to get close to some specific machine, or if nobody really 
cares about stuff like that.

I also don't know if people will be wanting this on other 
architectures.  IPMI is certainly available on ia64.  In fact it's quite 
common there.  I've seen it on practically everything else, though it's 
not so common.  It's in the PPC device trees for sure, and in some uboot 
device trees on other arches.

> If it's the later, we can probably express the interrupt number as a
> #define in SeaBIOS, but still make it configurable in QEMU.  Then you
> could build multiple copies of SeaBIOS and then just point QEMU at the
> right version.

That philosophy sounds like a recipe for version overload.  I'd prefer 
to avoid that.

>
>> Two other standard emulations exist, too, one in memory and one over
>> I2C.  I'd eventually like to add those, if for nothing else my ability
>> to test the interfaces.
> Right, see above.  It may be easier to just build multiple copies of the
> BIOS then to try and make this all dynamic.
In my experience, if you need the flexibility and don't make it dynamic, 
you make things harder in the long run.  But adding unnecessary 
flexibility is extra work without value.

IMHO, we should either have a single IPMI interface type at a fixed 
location with a fixed interrupt, or we should make it flexible. Even if 
we make it fixed, the BIOS will have to be told if the device is present 
and will have to dynamically chose to add the SMBIOS table and ACPI name 
space entries.

Thanks,

-corey

> Regards,
>
> Anthony Liguori
>
>> If the user is trying to emulate some specific machine, setting the
>> address is also important, and I need to add the ability to specify
>> register spacing and the address space.  This will become more important
>> for non-x86 machines.
>>
>> -corey
>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>> -corey

Corey Minyard <cminyard@mvista.com> writes:

> On 08/02/2012 01:32 PM, Anthony Liguori wrote:
>> Corey Minyard <tcminyard@gmail.com> writes:
>>
>>> On 08/01/2012 09:40 PM, Anthony Liguori wrote:
>>>> Corey Minyard <cminyard@mvista.com> writes:
>>>>
>>>>> On 08/01/2012 08:15 PM, Kevin O'Connor wrote:
>>>>> Well, I should also probably add the ACPI name space definition for this
>>>>> information, too, and the SMBIOS information is not capable of passing
>>>>> all the information required for this (though the above structure can).
>>>>>
>>>>> I've been studying this, but I don't see an obvious way to dynamically
>>>>> add something to the ACPI name space.  At least an easy way.
>>>> Okay, I was actually going to ask if there was an ACPI table for this.
>>>>
>>>> Maybe this argues in favor of doing a fw_cfg interface?
>>>>
>>>> Another question--is it really necessary for all of this to be user
>>>> specified?  Can't we just use a static SMBIOS/ACPI entry?  Then SeaBIOS
>>>> only needs to be concerned with whether or not an IPMI device exists.
>>> That's a good question At least the interrupt is important for the user
>>> to be able to specify.  The specific interface type may also be
>>> important if the user is trying to accomplish some specific emulation.
>> Why is it important to specify the interrupt?  Is this important for a
>> typical user, or important for the IPMI maintainer who needs to test a
>> bunch of different scenarios? :-)
>
> I'm not too worried about the IPMI maintainer, he can hack in what he 
> likes :).
>
> I would be worried about conflicts on interrupts with other devices.  I 
> really don't know how people use qemu out in the wild, though.  If they 
> are trying to get close to some specific machine, or if nobody really 
> cares about stuff like that.

It's an LPC device?  Ther aren't going to be many of those device types
that would be user controllable (basically TPM and IPMI) so I don't
think interrupt conflicts are a real likely issue.

> I also don't know if people will be wanting this on other 
> architectures.  IPMI is certainly available on ia64.

I doubt QEMU will ever support ia64 since noone seems to care about it anymore.

> In fact it's quite 
> common there.  I've seen it on practically everything else, though it's 
> not so common.  It's in the PPC device trees for sure, and in some uboot 
> device trees on other arches.

Right, but this is specifically about SMBIOS/ACPI support which won't be
on other architectures.

>
>> If it's the later, we can probably express the interrupt number as a
>> #define in SeaBIOS, but still make it configurable in QEMU.  Then you
>> could build multiple copies of SeaBIOS and then just point QEMU at the
>> right version.
>
> That philosophy sounds like a recipe for version overload.  I'd prefer 
> to avoid that.
>
>>
>>> Two other standard emulations exist, too, one in memory and one over
>>> I2C.  I'd eventually like to add those, if for nothing else my ability
>>> to test the interfaces.
>> Right, see above.  It may be easier to just build multiple copies of the
>> BIOS then to try and make this all dynamic.
> In my experience, if you need the flexibility and don't make it dynamic, 
> you make things harder in the long run.  But adding unnecessary 
> flexibility is extra work without value.

Exactly.

> IMHO, we should either have a single IPMI interface type at a fixed 
> location with a fixed interrupt, or we should make it flexible.

I think fixed interrupt is what makes the most sense now.  If there's a
 pressing need in the future to do otherwise, we can revisit.

Regards,

Anthony Liguori

> Even if 
> we make it fixed, the BIOS will have to be told if the device is present 
> and will have to dynamically chose to add the SMBIOS table and ACPI name 
> space entries.
>
> Thanks,
>
> -corey
>
>> Regards,
>>
>> Anthony Liguori
>>
>>> If the user is trying to emulate some specific machine, setting the
>>> address is also important, and I need to add the ability to specify
>>> register spacing and the address space.  This will become more important
>>> for non-x86 machines.
>>>
>>> -corey
>>>
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>>>
>>>>> -corey

On 08/02/2012 04:05 PM, Anthony Liguori wrote:
> Corey Minyard <cminyard@mvista.com> writes:
>
>> On 08/02/2012 01:32 PM, Anthony Liguori wrote:
>>> Corey Minyard <tcminyard@gmail.com> writes:
>>>
>>>> On 08/01/2012 09:40 PM, Anthony Liguori wrote:
>>>>> Corey Minyard <cminyard@mvista.com> writes:
>>>>>
>>>>>> On 08/01/2012 08:15 PM, Kevin O'Connor wrote:
>>>>>> Well, I should also probably add the ACPI name space definition for this
>>>>>> information, too, and the SMBIOS information is not capable of passing
>>>>>> all the information required for this (though the above structure can).
>>>>>>
>>>>>> I've been studying this, but I don't see an obvious way to dynamically
>>>>>> add something to the ACPI name space.  At least an easy way.
>>>>> Okay, I was actually going to ask if there was an ACPI table for this.
>>>>>
>>>>> Maybe this argues in favor of doing a fw_cfg interface?
>>>>>
>>>>> Another question--is it really necessary for all of this to be user
>>>>> specified?  Can't we just use a static SMBIOS/ACPI entry?  Then SeaBIOS
>>>>> only needs to be concerned with whether or not an IPMI device exists.
>>>> That's a good question At least the interrupt is important for the user
>>>> to be able to specify.  The specific interface type may also be
>>>> important if the user is trying to accomplish some specific emulation.
>>> Why is it important to specify the interrupt?  Is this important for a
>>> typical user, or important for the IPMI maintainer who needs to test a
>>> bunch of different scenarios? :-)
>> I'm not too worried about the IPMI maintainer, he can hack in what he
>> likes :).
>>
>> I would be worried about conflicts on interrupts with other devices.  I
>> really don't know how people use qemu out in the wild, though.  If they
>> are trying to get close to some specific machine, or if nobody really
>> cares about stuff like that.
> It's an LPC device?  Ther aren't going to be many of those device types
> that would be user controllable (basically TPM and IPMI) so I don't
> think interrupt conflicts are a real likely issue.
>

The implementation depends.  But for SMBIOS concerns, you are probably 
correct.

> Right, but this is specifically about SMBIOS/ACPI support which won't be
> on other architectures.

No, it's not.  This is about passing information to the firmware. At 
least PPC and SPARC use the same mechanisms.

>
>>> If it's the later, we can probably express the interrupt number as a
>>> #define in SeaBIOS, but still make it configurable in QEMU.  Then you
>>> could build multiple copies of SeaBIOS and then just point QEMU at the
>>> right version.
>> That philosophy sounds like a recipe for version overload.  I'd prefer
>> to avoid that.
>>
>>>> Two other standard emulations exist, too, one in memory and one over
>>>> I2C.  I'd eventually like to add those, if for nothing else my ability
>>>> to test the interfaces.
>>> Right, see above.  It may be easier to just build multiple copies of the
>>> BIOS then to try and make this all dynamic.
>> In my experience, if you need the flexibility and don't make it dynamic,
>> you make things harder in the long run.  But adding unnecessary
>> flexibility is extra work without value.
> Exactly.
>
>> IMHO, we should either have a single IPMI interface type at a fixed
>> location with a fixed interrupt, or we should make it flexible.
> I think fixed interrupt is what makes the most sense now.  If there's a
>   pressing need in the future to do otherwise, we can revisit.

I wanted to think about this a bit, and in my mind if you have to pass 
anything, you might as well pass everything.  It's not that big a 
difference.  Since you are going to have to pass something along, the 
difference between passing a flag saying "IPMI is available" and passing 
a structure with the information doesn't seem to be that much.  The main 
difference is the firmware is going to pull the data out of a passed in 
structure verses setting it to fixed values. Passing the structure gives 
the user the ability to specify anything and have it just work.

Thanks,

-corey

>
> Regards,
>
> Anthony Liguori
>
>> Even if
>> we make it fixed, the BIOS will have to be told if the device is present
>> and will have to dynamically chose to add the SMBIOS table and ACPI name
>> space entries.
>>
>> Thanks,
>>
>> -corey
>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>> If the user is trying to emulate some specific machine, setting the
>>>> address is also important, and I need to add the ability to specify
>>>> register spacing and the address space.  This will become more important
>>>> for non-x86 machines.
>>>>
>>>> -corey
>>>>
>>>>> Regards,
>>>>>
>>>>> Anthony Liguori
>>>>>
>>>>>> -corey