Patchwork [v3,12/14] pvpanic: add API to access io port

login
register
mail settings
Submitter Michael S. Tsirkin
Date July 24, 2013, 4:02 p.m.
Message ID <1374681580-17439-13-git-send-email-mst@redhat.com>
Download mbox | patch
Permalink /patch/261449/
State New
Headers show

Comments

Michael S. Tsirkin - July 24, 2013, 4:02 p.m.
Add API to find pvpanic device and get its io port.
Will be used to fill in guest info structure.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/misc/pvpanic.c    | 11 +++++++++++
 include/hw/i386/pc.h |  1 +
 2 files changed, 12 insertions(+)
Gerd Hoffmann - July 25, 2013, 10:29 a.m.
On 07/24/13 18:02, Michael S. Tsirkin wrote:
> Add API to find pvpanic device and get its io port.
> Will be used to fill in guest info structure.

> +uint16_t pvpanic_port(void)
> +{
> +    Object *o = object_resolve_path_type("", TYPE_ISA_PVPANIC_DEVICE, NULL);
> +    PVPanicState *s;
> +    if (!o) {
> +        return 0;
> +    }

       return object_property_get_int(o, "ioport");
}

Then you don't need PVPanicState access and can place the code into
acpi-build.c.

cheers,
  Gerd
Michael S. Tsirkin - July 25, 2013, 10:55 a.m.
On Thu, Jul 25, 2013 at 12:29:52PM +0200, Gerd Hoffmann wrote:
> On 07/24/13 18:02, Michael S. Tsirkin wrote:
> > Add API to find pvpanic device and get its io port.
> > Will be used to fill in guest info structure.
> 
> > +uint16_t pvpanic_port(void)
> > +{
> > +    Object *o = object_resolve_path_type("", TYPE_ISA_PVPANIC_DEVICE, NULL);
> > +    PVPanicState *s;
> > +    if (!o) {
> > +        return 0;
> > +    }
> 
>        return object_property_get_int(o, "ioport");
> }
> 
> Then you don't need PVPanicState access and can place the code into
> acpi-build.c.
> 
> cheers,
>   Gerd

I can change the implementation but I don't think it's
a good idea to copy property names around:
it's too fragile, compiler won't warn us if we
change the name or value semantics, or make
a mistake in acpi-build.c
Same applies to TYPE_ISA_PVPANIC_DEVICE: better
to expose an API than expose the type externally.
Michael S. Tsirkin - July 25, 2013, 10:58 a.m.
On Thu, Jul 25, 2013 at 01:55:26PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 25, 2013 at 12:29:52PM +0200, Gerd Hoffmann wrote:
> > On 07/24/13 18:02, Michael S. Tsirkin wrote:
> > > Add API to find pvpanic device and get its io port.
> > > Will be used to fill in guest info structure.
> > 
> > > +uint16_t pvpanic_port(void)
> > > +{
> > > +    Object *o = object_resolve_path_type("", TYPE_ISA_PVPANIC_DEVICE, NULL);
> > > +    PVPanicState *s;
> > > +    if (!o) {
> > > +        return 0;
> > > +    }
> > 
> >        return object_property_get_int(o, "ioport");
> > }
> > 
> > Then you don't need PVPanicState access and can place the code into
> > acpi-build.c.
> > 
> > cheers,
> >   Gerd
> 
> I can change the implementation but I don't think it's
> a good idea to copy property names around:
> it's too fragile, compiler won't warn us if we
> change the name or value semantics, or make
> a mistake in acpi-build.c

And even within pvpanic.c, this will make me copy-paste
the "ioport" string or create yet another macro.

    s = ISA_PVPANIC_DEVICE(o);
    return s->ioport;

seems nicer.

> Same applies to TYPE_ISA_PVPANIC_DEVICE: better
> to expose an API than expose the type externally.



> -- 
> MST
Gerd Hoffmann - July 25, 2013, 11:05 a.m.
Hi,

> I can change the implementation but I don't think it's
> a good idea to copy property names around:
> it's too fragile, compiler won't warn us if we
> change the name or value semantics,

I'm not worried.  Changing the strings will break the command line
interface too (qemu -device pvpanic,ioport=...), so that isn't going to
happen.

cheers,
  Gerd
Michael S. Tsirkin - July 25, 2013, 11:22 a.m.
On Thu, Jul 25, 2013 at 01:05:12PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > I can change the implementation but I don't think it's
> > a good idea to copy property names around:
> > it's too fragile, compiler won't warn us if we
> > change the name or value semantics,
> 
> I'm not worried.  Changing the strings will break the command line
> interface too (qemu -device pvpanic,ioport=...), so that isn't going to
> happen.
> 
> cheers,
>   Gerd

What will catch this breakage?
There are 0 users actually tweaking the port
number so I'm sure no one will notice this.

In any case, catching errors at compile time
is much better than at runtime.

What exactly are advantages of duplicating
property names in this way? I don't see any.
Gerd Hoffmann - July 25, 2013, 12:03 p.m.
On 07/25/13 13:22, Michael S. Tsirkin wrote:
> On Thu, Jul 25, 2013 at 01:05:12PM +0200, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> I can change the implementation but I don't think it's
>>> a good idea to copy property names around:
>>> it's too fragile, compiler won't warn us if we
>>> change the name or value semantics,
>>
>> I'm not worried.  Changing the strings will break the command line
>> interface too (qemu -device pvpanic,ioport=...), so that isn't going to
>> happen.
>>
>> cheers,
>>   Gerd
> 
> What will catch this breakage?
> There are 0 users actually tweaking the port
> number so I'm sure no one will notice this.
> 
> In any case, catching errors at compile time
> is much better than at runtime.
> 
> What exactly are advantages of duplicating
> property names in this way? I don't see any.

You don't need access to pvpanic internals then and thus the code can be
moved over to the acpi generator.  At least in this case where all info
needed is already available via properties.

cheers,
  Gerd
Michael S. Tsirkin - July 25, 2013, 12:23 p.m.
On Thu, Jul 25, 2013 at 02:03:33PM +0200, Gerd Hoffmann wrote:
> On 07/25/13 13:22, Michael S. Tsirkin wrote:
> > On Thu, Jul 25, 2013 at 01:05:12PM +0200, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>> I can change the implementation but I don't think it's
> >>> a good idea to copy property names around:
> >>> it's too fragile, compiler won't warn us if we
> >>> change the name or value semantics,
> >>
> >> I'm not worried.  Changing the strings will break the command line
> >> interface too (qemu -device pvpanic,ioport=...), so that isn't going to
> >> happen.
> >>
> >> cheers,
> >>   Gerd
> > 
> > What will catch this breakage?
> > There are 0 users actually tweaking the port
> > number so I'm sure no one will notice this.
> > 
> > In any case, catching errors at compile time
> > is much better than at runtime.
> > 
> > What exactly are advantages of duplicating
> > property names in this way? I don't see any.
> 
> You don't need access to pvpanic internals then and thus the code can be
> moved over to the acpi generator.  At least in this case where all info
> needed is already available via properties.
> 
> cheers,
>   Gerd

We'll have to disagree here.
There's no access to internals with an API.
I prefer using APIs, since they are compiler-checked.
Andreas Färber - July 27, 2013, 11:58 p.m.
Am 25.07.2013 14:23, schrieb Michael S. Tsirkin:
> On Thu, Jul 25, 2013 at 02:03:33PM +0200, Gerd Hoffmann wrote:
>> On 07/25/13 13:22, Michael S. Tsirkin wrote:
>>> On Thu, Jul 25, 2013 at 01:05:12PM +0200, Gerd Hoffmann wrote:
>>>>> I can change the implementation but I don't think it's
>>>>> a good idea to copy property names around:
>>>>> it's too fragile, compiler won't warn us if we
>>>>> change the name or value semantics,
>>>>
>>>> I'm not worried.  Changing the strings will break the command line
>>>> interface too (qemu -device pvpanic,ioport=...), so that isn't going to
>>>> happen.
>>>
>>> What will catch this breakage?
>>> There are 0 users actually tweaking the port
>>> number so I'm sure no one will notice this.
>>>
>>> In any case, catching errors at compile time
>>> is much better than at runtime.
>>>
>>> What exactly are advantages of duplicating
>>> property names in this way? I don't see any.
>>
>> You don't need access to pvpanic internals then and thus the code can be
>> moved over to the acpi generator.  At least in this case where all info
>> needed is already available via properties.
> 
> We'll have to disagree here.
> There's no access to internals with an API.
> I prefer using APIs, since they are compiler-checked.

Sorry, I don't understand, is there a typo? A qdev "ioport" property is
hardly internal, and QOM has an API to access it.

For QOM properties we have ABI stability rules in place, not only for
the textual command line: No one is allowed to change the type of a
property in an incompatible way. And dropping it would be a
command-line-incompatible change in this case. Properties don't
magically disappear at runtime, and since you're writing this code you
can rely on the property being present and the person attempting to
remove it noticing it while testing.

You're writing redundant code here, and it's not your device IIRC.

I vaguely remember seeing you or Xen guys adding some check that the
pvpanic device can only be instantiated once, right? In that case I
would consider it best to add an
object_property_add_child(qdev_get_machine(), "pvpanic", foo, NULL);
call into pvpanic device creation, then you can access it from ACPI code
without needing to know the type via

obj = object_path_resolve_component(qdev_get_machine(), "pvpanic");
if (obj != NULL) {
    val = object_property_get_int(obj, "ioport", &err);
    assert_no_error(&err);
}

and pvpanic code remains untouched.

Andreas

Patch

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 7bb49a5..bb403e9 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -122,6 +122,17 @@  void pvpanic_init(ISABus *bus)
     pvpanic_fw_cfg(dev, fw_cfg);
 }
 
+uint16_t pvpanic_port(void)
+{
+    Object *o = object_resolve_path_type("", TYPE_ISA_PVPANIC_DEVICE, NULL);
+    PVPanicState *s;
+    if (!o) {
+        return 0;
+    }
+    s = ISA_PVPANIC_DEVICE(o);
+    return s->ioport;
+}
+
 static Property pvpanic_isa_properties[] = {
     DEFINE_PROP_UINT16("ioport", PVPanicState, ioport, 0x505),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 76af5cd..e718a59 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -228,6 +228,7 @@  void pc_system_firmware_init(MemoryRegion *rom_memory);
 
 /* pvpanic.c */
 void pvpanic_init(ISABus *bus);
+uint16_t pvpanic_port(void);
 
 /* e820 types */
 #define E820_RAM        1