| Message ID | 51D27638.4030804@redhat.com |
|---|---|
| State | New |
| Headers | show |
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? Jan
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
On 2013-07-02 10:18, Paolo Bonzini wrote: > 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). I was more referring to memory_region_present vs. open-coding. Jan
Il 02/07/2013 10:36, Jan Kiszka ha scritto: >>> >> 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). > I was more referring to memory_region_present vs. open-coding. Understood, but once you replace ?: with "if", the memory_region_present wrapper loses most of the appeal. So I actually prefer the open-coded one. Paolo
On 2013-07-02 11:28, Paolo Bonzini wrote: > Il 02/07/2013 10:36, Jan Kiszka ha scritto: >>>>>> 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). >> I was more referring to memory_region_present vs. open-coding. > > Understood, but once you replace ?: with "if", the memory_region_present > wrapper loses most of the appeal. So I actually prefer the open-coded one. Well, count the memory_region_unref calls. Jan
Il 02/07/2013 11:31, Jan Kiszka ha scritto: > On 2013-07-02 11:28, Paolo Bonzini wrote: >> Il 02/07/2013 10:36, Jan Kiszka ha scritto: >>>>>>> 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). >>> I was more referring to memory_region_present vs. open-coding. >> >> Understood, but once you replace ?: with "if", the memory_region_present >> wrapper loses most of the appeal. So I actually prefer the open-coded one. > > Well, count the memory_region_unref calls. Fair enough... I'll move memory_region_present to memory.h/c. Paolo
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); } }