qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 13/23] hyperv: qdev-ify SynIC


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH 13/23] hyperv: qdev-ify SynIC
Date: Wed, 14 Jun 2017 09:46:20 -0300
User-agent: Mutt/1.8.0 (2017-02-23)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]