On Wed, Jun 14, 2017 at 12:58:04PM +0300, Roman Kagan wrote:
> On Tue, Jun 13, 2017 at 03:34:34PM -0300, Eduardo Habkost wrote:
> > On Tue, Jun 06, 2017 at 09:19:38PM +0300, Roman Kagan wrote:
> > > Make Hyper-V SynIC a device which is attached as a child to X86CPU. For
> > > now it only makes SynIC visibile in the qom hierarchy and exposes a few
> > > properties which are maintained in sync with the respecitve msrs of the
> > > parent cpu.
> [...]
> > > +static SynICState *get_synic(X86CPU *cpu)
> > > +{
> > > + SynICState *synic =
> > > + SYNIC(object_resolve_path_component(OBJECT(cpu), "synic"));
> > > + assert(synic);
> > > + return synic;
> >
> > How often this will run? I think a new X86CPU struct field would
> > be acceptable to avoid resolving a QOM path on every hyperv exit.
>
> It's only used at the points where synic state is updated: at
> realize/save/load/unrealize and when the guest configures synics via
> msrs (i.e. a few times per cpu at guest startup). None of those is
> performance-critical.
>
> > Do you even need QOM for this?
>
> I need to reach the synic from the cpu on slow paths, and IMHO the
> pointer on X86CPU is not justified here. Besides there are two memory
> regions associated with every synic (in a followup patch) and I thought
> it made sense to have a dedicated parent QOM node for them.
Makes sense.
>
> > > +static Property synic_props[] = {
> > > + DEFINE_PROP_BOOL("enabled", SynICState, enabled, false),
> > > + DEFINE_PROP_UINT64("msg-page-addr", SynICState, msg_page_addr, 0),
> > > + DEFINE_PROP_UINT64("evt-page-addr", SynICState, evt_page_addr, 0),
> >
> > What do you need the QOM properties for? Are they supposed to be
> > configurable by the user?
>
> No. Actually I wanted to define them as read-only, but I failed to find
> a way to do so.
>
> > Are they supposed to be queried from outside QEMU? On which cases?
>
> ATM I only see them as informational fields that may prove useful during
> debugging.
>
You can use object_property_add_uint64_ptr() on instance_init to
add read-only properties.
If the enabled/msg_page_addr/evt_page_addr struct fields exist
only because of the properties, you can also use
object_property_add() and write your own getter that will query
the MSRs only when reading the property. I normally don't like
custom getters/setters, but it sounds acceptable for a read-only
property that's only for debugging.
Either way you choose to implement it, I would add a comment
noting that the properties are there just for debugging.
BTW, would you still want to add this amount of boilerplate code
just for debugging if we had a generic msr_get HMP command?
There's a series in the list (submitted on April) adding msr_get
and msr_set HMP commands, but it was never merged.
--
Eduardo
