Re: [Qemu-devel] [PATCH] sun4u: implement power device

[Marcel Apfelbaum <marcel@redhat.com>]

Hi Philippe,

On 16/01/2018 2:54, Philippe Mathieu-Daudé wrote:
> CC'ing PCI maintainers.
> 
> Hi Mark,
> 
> On 01/15/2018 05:58 PM, Mark Cave-Ayland wrote:
>> This inbuilt device contains a single 4-byte register, of which bit 24 is used
>> to power down the machine on a real Ultra 5.
>>
>> The power device exists at offset 0x724000 on a real machine, but due to the
>> current configuration of the BARs in QEMU it must be located lower in PCI IO
>> space.
> Is is some issue in pci_bar_address()?
> 

The QEMU IO layout:

     /*
      * QEMU I/O address space usage:
      *   0000 - 0fff    legacy isa, pci config, pci root bus, ...
      *   1000 - 9fff    free
      *   a000 - afff    hotplug (cpu, pci via acpi, i440fx/piix only)
      *   b000 - bfff    power management (PORT_ACPI_PM_BASE)
      *                  [ qemu 1.4+ implements pci config registers
      *                    properly so guests can place the registers
      *                    where they want, on older versions its fixed ]
      *   c000 - ffff    free, traditionally used for pci io
      */

As you can see we don't have IO address space over ffff.

Thanks,
Marcel

>>
>> For the moment we place the power device at offset 0x7240 as a reminder of its
>> original location and raise the base PCI IO address from 0x4000 to 0x8000.
> 
> If we can't fix it, I rather prefer a #define with a comment in the
> code, IMHO this is safer and easier to remember than looking in git history.
> 
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/sparc64/sun4u.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index ec45ec2801..26ab6e9a9c 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -205,6 +205,59 @@ typedef struct ResetData {
>>       uint64_t prom_addr;
>>   } ResetData;
>>   
>> +#define TYPE_SUN4U_POWER "power"
>> +#define SUN4U_POWER(obj) OBJECT_CHECK(PowerDevice, (obj), TYPE_SUN4U_POWER)
>> +
>> +typedef struct PowerDevice {
>> +    SysBusDevice parent_obj;
>> +
>> +    MemoryRegion power_mmio;
>> +} PowerDevice;
>> +
>> +/* Power */
>> +static void power_mem_write(void *opaque, hwaddr addr,
>> +                            uint64_t val, unsigned size)
>> +{
>> +    /* According to a real Ultra 5, bit 24 controls the power */
>> +    if (val & 0x1000000) {
>> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps power_mem_ops = {
>> +    .write = power_mem_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>> +};
>> +
>> +static void power_realize(DeviceState *dev, Error **errp)
>> +{
>> +    PowerDevice *d = SUN4U_POWER(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +
>> +    memory_region_init_io(&d->power_mmio, OBJECT(dev), &power_mem_ops, d,
>> +                          "power", sizeof(uint32_t));
>> +
>> +    sysbus_init_mmio(sbd, &d->power_mmio);
>> +}
>> +
>> +static void power_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = power_realize;
>> +}
>> +
>> +static const TypeInfo power_info = {
>> +    .name          = TYPE_SUN4U_POWER,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(PowerDevice),
>> +    .class_init    = power_class_init,
>> +};
>> +
>>   static void ebus_isa_irq_handler(void *opaque, int n, int level)
>>   {
>>       EbusState *s = EBUS(opaque);
>> @@ -221,6 +274,7 @@ static void ebus_isa_irq_handler(void *opaque, int n, int level)
>>   static void ebus_realize(PCIDevice *pci_dev, Error **errp)
>>   {
>>       EbusState *s = EBUS(pci_dev);
>> +    SysBusDevice *sbd;
>>       DeviceState *dev;
>>       qemu_irq *isa_irq;
>>       DriveInfo *fd[MAX_FD];
>> @@ -270,6 +324,13 @@ static void ebus_realize(PCIDevice *pci_dev, Error **errp)
>>       qdev_prop_set_uint32(dev, "dma", -1);
>>       qdev_init_nofail(dev);
>>   
>> +    /* Power */
>> +    dev = qdev_create(NULL, TYPE_SUN4U_POWER);
>> +    qdev_init_nofail(dev);
>> +    sbd = SYS_BUS_DEVICE(dev);
>> +    memory_region_add_subregion(pci_address_space_io(pci_dev), 0x7240,
>> +                                sysbus_mmio_get_region(sbd, 0));
>> +
>>       /* PCI */
>>       pci_dev->config[0x04] = 0x06; // command = bus master, pci mem
>>       pci_dev->config[0x05] = 0x00;
>> @@ -282,7 +343,7 @@ static void ebus_realize(PCIDevice *pci_dev, Error **errp)
>>                                0, 0x1000000);
>>       pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->bar0);
>>       memory_region_init_alias(&s->bar1, OBJECT(s), "bar1", get_system_io(),
>> -                             0, 0x4000);
>> +                             0, 0x8000);
> 
> What about using 2 MR, bar1_lo/bar1_hi, registering bar1_lo at offset @0
> and bar1_hi at @0x724000?
> 
>>       pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->bar1);
>>   }
>>   
>> @@ -693,6 +754,7 @@ static const TypeInfo sun4v_type = {
>>   
>>   static void sun4u_register_types(void)
>>   {
>> +    type_register_static(&power_info);
>>       type_register_static(&ebus_info);
>>       type_register_static(&prom_info);
>>       type_register_static(&ram_info);
>>
>