Re: [Qemu-devel] [PATCH v3 14/16] acpi: Add a way for devices to add ACPI tables

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 13 Jul 2015 16:55:29 -0500
Corey Minyard <minyard@acm.org> wrote:

> On 07/09/2015 03:25 AM, Igor Mammedov wrote:
> > On Wed, 8 Jul 2015 22:39:24 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >>
> >> On 08/07/2015 21:26, Igor Mammedov wrote:
> >>>> This was suggested by Michael, so I think you should read the reviews
> >>>> of earlier versions first.
> >>> That is basically the same as hooks in v1 only the other way around
> >>> with all drawbacks attached.
> >>>
> >>> Just dropping this universal way to scatter ACPI code all over QEMU
> >>> and calling ipmi_encode_one_acpi() "[15/16] ipmi: Add ACPI table entries"
> >>> directly from build_ssdt() would simplify series quite a bit.
> >> On the other hand, it would be much harder to make IPMI optional unless
> >> you want to add a bunch of ugly stubs.  Same for SMBIOS.  Especially
> > it's no the case if one uses QOM properties to fetch info.
> > it's a bit verbose but works well without need for ugly stubs.
> >
> >> when you can have the same ACPI and SMBIOS tables in both x86 and ARM
> >> virtual machines, it becomes a little more the device business to
> >> provide ACPI and SMBIOS information.
> > For a generic way for device to supply information to ACPI,
> > I was thinking about introducing interface that device might implement
> > for example if it should have _CRS in its ACPI description.
> > Then APCI core could query for devices that implement interface
> > and build AML code based on resource values (i.e. base address,
> > size, irq #, e.t.c) it gets from device.
> 
> I'm just getting back from vacation and catching up.
> 
> I would agree that implementing an interface is a more consistent
> way to do this than the way it's done now.  If it's ok with everyone,
> I'll work on implementing this.
Implementing interface shouldn't block this series.
I'm ok with this series ACPI wise if you drop 14/16 and do as suggested
above + address comment on 15/16.
It would require minimal changes and get us an interesting device
to play with without significant delays.
I would do the same with SMBIOS side as well.

Implementing interface could be made on top along with conversion
of current code that fetches resource info from devices manually.

> 
> I'm not sure that resource values are enough to do this, though.
> IPMI has some custom fields (_IFT, _SRV, _STR) and lots of optional
> fields (_ADR, _UID, _STA), how would you do these fields?
fields names are part of ACPI spec,
maybe interface should have methods for returning these fields.

_ADR - for example for PCI devices should address of device on bus
_SRV - return packed revision number
_IFT - return interface number
_STR, _UID, _STA - I'd leave them to board/firmware to decide how to
name/enumerate/detect presence (i.e. in acpi-build.c)

> 
> -corey
> 
> >
> >> So overall I'm happy with the way it was done here.  I and Michael
> >> disagreed on several details, but not enough to fight over it...
> > I won't fight over it either, but it's my opinion how it should be
> > done.
> >
> >> Paolo
> >>
> >>> I'd do the same for SMBIOS entries as well, i.e. drop similar
> >>> universal way for devices to supply SMBIOS info (it's not their
> >>> business) and just call ipmi_encode_one_smbios()  to add ipmi specific
> >>> entry if device is present. Again simplifying series even more.
> >>>
> >>> that roughly would save us 300 lines of not really necessary code
> >>> and keep things consistent with a way we are managing ssdt now.
>