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

[Eduardo Habkost]

On Wed, Jun 14, 2017 at 06:11:03PM +0300, Roman Kagan wrote:
> On Wed, Jun 14, 2017 at 09:46:20AM -0300, Eduardo Habkost wrote:
> > 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:
> > > > > +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.
> 
> Actually that was what I did at first, but then I decided it was useful
> to have the fields directly on SynICState (see followup patches).
> 
> > 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.
> 
> Yes I guess it would do.  So I'm now tempted to just drop the
> properties, and leave the fields invisible through QOM.

If you are going to keep the fields on SynICState, then you just
need one object_property_add_*_ptr() line for each property, so
maybe it's still worth it.  It's up to you.

-- 
Eduardo