[Qemu-devel] Re: [Qemu-commits] [COMMIT e813376] Sparc32: fix fdc io_base

[Qemu-devel] Re: [Qemu-commits] [COMMIT e813376] Sparc32: fix fdc io_base

[Anthony Liguori]

Anthony Liguori wrote:
> From: Blue Swirl <blauwirbel@gmail.com>
>
> On some Sparc32 machines, fdc is located above 4G limit, so uint32_t is not
> appropriate type for io_base.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>
> diff --git a/hw/fdc.c b/hw/fdc.c
> index fa154a3..4ad5e5e 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -33,6 +33,7 @@
>  #include "qemu-timer.h"
>  #include "isa.h"
>  #include "sysbus.h"
> +#include "qdev-addr.h"
>  
>  /********************************************************/
>  /* debug Floppy devices */
> @@ -1972,7 +1973,7 @@ static SysBusDeviceInfo fdc_info = {
>      .qdev.props = (Property[]) {
>          {
>              .name = "io_base",
> -            .info = &qdev_prop_uint32,
> +            .info = &qdev_prop_taddr,
>   

fdc probably shouldn't use target_phys_addr_t and instead should just 
use a uint64_t for io_base.  target_phys is a CPU type, devices 
shouldn't depend on it.

What do you think?

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT e813376] Sparc32: fix fdc io_base

[Paul Brook]

> >      .qdev.props = (Property[]) {
> >          {
> >              .name = "io_base",
> > -            .info = &qdev_prop_uint32,
> > +            .info = &qdev_prop_taddr,
>
> fdc probably shouldn't use target_phys_addr_t and instead should just
> use a uint64_t for io_base.  target_phys is a CPU type, devices
> shouldn't depend on it.

The qdev support for this device is almost completely bogus.  The device code 
should not be dealing with the base address at all. It should be handled by a 
SysBus MMIO region. fdctrl_init should not be calling fdctrl_init_common. 
Instead everything should be done by the qdev init routine (fdctrl_init1).

The mem_mapped property is also fairly suspect. We almost certainly want two 
different devices. On SysBus device a MMIO region, and the other an ISA device 
(using IO ports) - Note that qdev ISA bus support does not exist yet.

Paul

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT e813376] Sparc32: fix fdc io_base

[Blue Swirl]

On Fri, Jul 17, 2009 at 5:58 PM, Paul Brook<paul@codesourcery.com> wrote:
>> >      .qdev.props = (Property[]) {
>> >          {
>> >              .name = "io_base",
>> > -            .info = &qdev_prop_uint32,
>> > +            .info = &qdev_prop_taddr,
>>
>> fdc probably shouldn't use target_phys_addr_t and instead should just
>> use a uint64_t for io_base.  target_phys is a CPU type, devices
>> shouldn't depend on it.
>
> The qdev support for this device is almost completely bogus.  The device code
> should not be dealing with the base address at all. It should be handled by a
> SysBus MMIO region. fdctrl_init should not be calling fdctrl_init_common.
> Instead everything should be done by the qdev init routine (fdctrl_init1).

Right, but we don't have sysbus_init_pio()/sysbus_pio_map() yet.

> The mem_mapped property is also fairly suspect. We almost certainly want two
> different devices. On SysBus device a MMIO region, and the other an ISA device
> (using IO ports) - Note that qdev ISA bus support does not exist yet.

Yes, I think the code looks strange today, properties should not be
used at all. It looked much better a few days ago. ;-)

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT e813376] Sparc32: fix fdc io_base

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On Fri, Jul 17, 2009 at 5:58 PM, Paul Brook<paul@codesourcery.com> wrote:
>> >      .qdev.props = (Property[]) {
>> >          {
>> >              .name = "io_base",
>> > -            .info = &qdev_prop_uint32,
>> > +            .info = &qdev_prop_taddr,
>>
>> fdc probably shouldn't use target_phys_addr_t and instead should just
>> use a uint64_t for io_base.  target_phys is a CPU type, devices
>> shouldn't depend on it.
>
> The qdev support for this device is almost completely bogus.  The device code
> should not be dealing with the base address at all. It should be handled by a
> SysBus MMIO region. fdctrl_init should not be calling fdctrl_init_common.
> Instead everything should be done by the qdev init routine (fdctrl_init1).
>
> The mem_mapped property is also fairly suspect. We almost certainly want two
> different devices. On SysBus device a MMIO region, and the other an ISA device
> (using IO ports) - Note that qdev ISA bus support does not exist yet.

How about this cleanup?

[-- Attachment #2: 0001-Clean-up-fdc-qdev-conversion.patch --]
[-- Type: application/x-patch, Size: 4287 bytes --]

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT e813376] Sparc32: fix fdc io_base

[Paul Brook]

> > The qdev support for this device is almost completely bogus.  
> >...
> > The mem_mapped property is also fairly suspect. We almost certainly want
> > two different devices. On SysBus device a MMIO region, and the other an
> > ISA device (using IO ports) - Note that qdev ISA bus support does not
> > exist yet.
>
> How about this cleanup?

It's a start. However the init code is still backwards. For example:

>@@ -1944,6 +1937,7 @@ fdctrl_t *sun4m_fdctrl_init
>     fdctrl = FROM_SYSBUS(fdctrl_t, s);
>+    fdctrl->sun4m = 1;
>     fdctrl_init_common(fdctrl, -1, io_base, fds);

This code should be in sun4m_fdc_init1.

sun4m_fdctrl_init should really live in sun4m.c, not fdc.c. This should make 
it clear whether you're honoring the abstraction boundaries.

>+    .qdev.props = (Property[]) {
>         {/* end of properties */}
>     }

It is not necessary to specify an empty property list.

Paul

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT e813376] Sparc32: fix fdc io_base

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Sat, Jul 18, 2009 at 2:37 AM, Paul Brook<paul@codesourcery.com> wrote:
>> > The qdev support for this device is almost completely bogus.
>> >...
>> > The mem_mapped property is also fairly suspect. We almost certainly want
>> > two different devices. On SysBus device a MMIO region, and the other an
>> > ISA device (using IO ports) - Note that qdev ISA bus support does not
>> > exist yet.
>>
>> How about this cleanup?
>
> It's a start. However the init code is still backwards. For example:
>
>>@@ -1944,6 +1937,7 @@ fdctrl_t *sun4m_fdctrl_init
>>     fdctrl = FROM_SYSBUS(fdctrl_t, s);
>>+    fdctrl->sun4m = 1;
>>     fdctrl_init_common(fdctrl, -1, io_base, fds);
>
> This code should be in sun4m_fdc_init1.

OK. Better now?

> sun4m_fdctrl_init should really live in sun4m.c, not fdc.c. This should make
> it clear whether you're honoring the abstraction boundaries.

I know, but I tried to preserve the existing interface at this stage
of conversion. Later I'm going to move all such init functions to
sun4m.c. Then I should add a SBus bus type.

[-- Attachment #2: 0001-Clean-up-fdc-qdev-conversion.patch --]
[-- Type: application/x-patch, Size: 7503 bytes --]

[Qemu-devel] Re: [Qemu-commits] [COMMIT e813376] Sparc32: fix fdc io_base

[Blue Swirl]

On Fri, Jul 17, 2009 at 5:24 PM, Anthony Liguori<anthony@codemonkey.ws> wrote:
> Anthony Liguori wrote:
>>
>> From: Blue Swirl <blauwirbel@gmail.com>
>>
>> On some Sparc32 machines, fdc is located above 4G limit, so uint32_t is
>> not
>> appropriate type for io_base.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>
>> diff --git a/hw/fdc.c b/hw/fdc.c
>> index fa154a3..4ad5e5e 100644
>> --- a/hw/fdc.c
>> +++ b/hw/fdc.c
>> @@ -33,6 +33,7 @@
>>  #include "qemu-timer.h"
>>  #include "isa.h"
>>  #include "sysbus.h"
>> +#include "qdev-addr.h"
>>   /********************************************************/
>>  /* debug Floppy devices */
>> @@ -1972,7 +1973,7 @@ static SysBusDeviceInfo fdc_info = {
>>     .qdev.props = (Property[]) {
>>         {
>>             .name = "io_base",
>> -            .info = &qdev_prop_uint32,
>> +            .info = &qdev_prop_taddr,
>>
>
> fdc probably shouldn't use target_phys_addr_t and instead should just use a
> uint64_t for io_base.  target_phys is a CPU type, devices shouldn't depend
> on it.
>
> What do you think?

Actually, currently target_phys_addr_t is used by most devices as the
machine bus address type. It's tied to CPU type but not as tightly as
for example TARGET_PAGE_SIZE or target_ulong.

In this case, uint64_t would work too.