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

[Roman Kagan]

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.

Thanks,
Roman.