Re: [Qemu-devel] [PATCH v2 06/11] memory: add ref/unref calls

[Paolo Bonzini <pbonzini@redhat.com>]

Il 02/07/2013 09:11, Jan Kiszka ha scritto:
> On 2013-07-02 08:42, Paolo Bonzini wrote:
>> Il 01/07/2013 20:33, Jan Kiszka ha scritto:
>>> On 2013-06-28 18:58, Paolo Bonzini wrote:
>>>> Add ref/unref calls at the following places:
>>>>
>>>> - places where memory regions are stashed by a listener and
>>>>   used outside the BQL (including in Xen or KVM).
>>>>
>>>> - memory_region_find callsites
>>>
>>> You missed some recently added ones. Check hw/acpi/piix4.c and
>>> hw/isa/lpc_ich9.c (both will require some refactorings for this).
>>
>> Here are the required changes:
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 3b95c69..0310053 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -380,6 +380,16 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>>      acpi_pm1_evt_power_down(&s->ar);
>>  }
>>  
>> +static bool memory_region_present(MemoryRegion *io, hwaddr port)
>> +{
>> +    MemoryRegion *mr = memory_region_find(io, port, 1);
>> +    if (!mr) {
>> +        return false;
>> +    }
>> +    memory_region_unref(mr);
>> +    return true;
>> +}
>> +
>>  static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>>  {
>>      PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
>> @@ -388,10 +398,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>>  
>>      pci_conf = s->dev.config;
>>      pci_conf[0x5f] = 0x10 |
>> -        (memory_region_find(io_as, 0x378, 1).mr ? 0x80 : 0);
>> +        (memory_region_present(io_as, 0x378, 1) ? 0x80 : 0);
>>      pci_conf[0x63] = 0x60;
>> -    pci_conf[0x67] = (memory_region_find(io_as, 0x3f8, 1).mr ? 0x08 : 0) |
>> -        (memory_region_find(io_as, 0x2f8, 1).mr ? 0x90 : 0);
>> +    pci_conf[0x67] = (memory_region_present(io_as, 0x3f8, 1) ? 0x08 : 0) |
>> +        (memory_region_present(io_as, 0x2f8, 1) ? 0x90 : 0);
>>  }
>>  
>>  static int piix4_pm_initfn(PCIDevice *dev)
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 82f8ea6..205ac26 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -478,24 +478,33 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
>>  {
>>      ICH9LPCState *s = container_of(n, ICH9LPCState, machine_ready);
>>      MemoryRegion *io_as = pci_address_space_io(&s->d);
>> +    MemoryRegion *mr;
>>      uint8_t *pci_conf;
>>  
>>      pci_conf = s->d.config;
>> -    if (memory_region_find(io_as, 0x3f8, 1).mr) {
>> +    mr = memory_region_find(io_as, 0x3f8, 1).mr;
>> +    if (mr) {
>>          /* com1 */
>>          pci_conf[0x82] |= 0x01;
>> +        memory_region_unref(mr);
>>      }
>> -    if (memory_region_find(io_as, 0x2f8, 1).mr) {
>> +    mr = memory_region_find(io_as, 0x2f8, 1).mr;
>> +    if (mr) {
>>          /* com2 */
>>          pci_conf[0x82] |= 0x02;
>> +        memory_region_unref(mr);
>>      }
>> -    if (memory_region_find(io_as, 0x378, 1).mr) {
>> +    mr = memory_region_find(io_as, 0x378, 1).mr;
>> +    if (mr) {
>>          /* lpt */
>>          pci_conf[0x82] |= 0x04;
>> +        memory_region_unref(mr);
>>      }
>> -    if (memory_region_find(io_as, 0x3f0, 1).mr) {
>> +    mr = memory_region_find(io_as, 0x3f0, 1).mr;
>> +    if (mr) {
>>          /* floppy */
>>          pci_conf[0x82] |= 0x08;
>> +        memory_region_unref(mr);
>>      }
>>  }
>>  
>>
>>
> 
> Hmm, two solutions for one problem - can we improve this in the next round?

Sure, I can adapt the hw/acpi/piix4.c to use ifs in the same style as
hw/isa/lpc_ich9.c (I find the code easier to read).

Paolo