Re: [PATCH v3 26/28] target/ppc: Replace g_memdup() by g_memdup2()
On Fri, Sep 03, 2021 at 07:45:08PM +0200, Philippe Mathieu-Daudé wrote: > Per > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > The old API took the size of the memory to duplicate as a guint, > whereas most memory functions take memory sizes as a gsize. This > made it easy to accidentally pass a gsize to g_memdup(). For large > values, that would lead to a silent truncation of the size from 64 > to 32 bits, and result in a heap area being returned which is > significantly smaller than what the caller expects. This can likely > be exploited in various modules to cause a heap buffer overflow. > > Replace g_memdup() by the safer g_memdup2() wrapper. > > Signed-off-by: Philippe Mathieu-Daudé Acked-by: David Gibson > --- > target/ppc/mmu-hash64.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c > index 19832c4b46f..bc6f8748acb 100644 > --- a/target/ppc/mmu-hash64.c > +++ b/target/ppc/mmu-hash64.c > @@ -1122,7 +1122,7 @@ void ppc_hash64_init(PowerPCCPU *cpu) > return; > } > > -cpu->hash64_opts = g_memdup(pcc->hash64_opts, sizeof(*cpu->hash64_opts)); > +cpu->hash64_opts = g_memdup2(pcc->hash64_opts, > sizeof(*cpu->hash64_opts)); > } > > void ppc_hash64_finalize(PowerPCCPU *cpu) -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH v3 16/28] hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2()
On Fri, Sep 03, 2021 at 07:44:58PM +0200, Philippe Mathieu-Daudé wrote:
> Per
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>
> The old API took the size of the memory to duplicate as a guint,
> whereas most memory functions take memory sizes as a gsize. This
> made it easy to accidentally pass a gsize to g_memdup(). For large
> values, that would lead to a silent truncation of the size from 64
> to 32 bits, and result in a heap area being returned which is
> significantly smaller than what the caller expects. This can likely
> be exploited in various modules to cause a heap buffer overflow.
>
> Replace g_memdup() by the safer g_memdup2() wrapper.
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/ppc/spapr_pci.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7430bd63142..8e36cffab79 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2201,10 +2201,9 @@ static int spapr_pci_post_load(void *opaque, int
> version_id)
> int i;
>
> for (i = 0; i < sphb->msi_devs_num; ++i) {
> -key = g_memdup(>msi_devs[i].key,
> - sizeof(sphb->msi_devs[i].key));
> -value = g_memdup(>msi_devs[i].value,
> - sizeof(sphb->msi_devs[i].value));
> +key = g_memdup2(>msi_devs[i].key,
> sizeof(sphb->msi_devs[i].key));
> +value = g_memdup2(>msi_devs[i].value,
> + sizeof(sphb->msi_devs[i].value));
> g_hash_table_insert(sphb->msi, key, value);
> }
> g_free(sphb->msi_devs);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 16/28] hw/ppc/spapr_pci: Replace g_memdup() by g_memdup2_qemu()
On Fri, Sep 03, 2021 at 01:06:50PM +0200, Philippe Mathieu-Daudé wrote:
> Per
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
>
> The old API took the size of the memory to duplicate as a guint,
> whereas most memory functions take memory sizes as a gsize. This
> made it easy to accidentally pass a gsize to g_memdup(). For large
> values, that would lead to a silent truncation of the size from 64
> to 32 bits, and result in a heap area being returned which is
> significantly smaller than what the caller expects. This can likely
> be exploited in various modules to cause a heap buffer overflow.
>
> Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/ppc/spapr_pci.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7430bd63142..79c0e8d4f98 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2201,10 +2201,10 @@ static int spapr_pci_post_load(void *opaque, int
> version_id)
> int i;
>
> for (i = 0; i < sphb->msi_devs_num; ++i) {
> -key = g_memdup(>msi_devs[i].key,
> - sizeof(sphb->msi_devs[i].key));
> -value = g_memdup(>msi_devs[i].value,
> - sizeof(sphb->msi_devs[i].value));
> +key = g_memdup2_qemu(>msi_devs[i].key,
> + sizeof(sphb->msi_devs[i].key));
> +value = g_memdup2_qemu(>msi_devs[i].value,
> + sizeof(sphb->msi_devs[i].value));
> g_hash_table_insert(sphb->msi, key, value);
> }
> g_free(sphb->msi_devs);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 26/28] target/ppc: Replace g_memdup() by g_memdup2_qemu()
On Fri, Sep 03, 2021 at 01:07:00PM +0200, Philippe Mathieu-Daudé wrote: > Per > https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 > > The old API took the size of the memory to duplicate as a guint, > whereas most memory functions take memory sizes as a gsize. This > made it easy to accidentally pass a gsize to g_memdup(). For large > values, that would lead to a silent truncation of the size from 64 > to 32 bits, and result in a heap area being returned which is > significantly smaller than what the caller expects. This can likely > be exploited in various modules to cause a heap buffer overflow. > > Replace g_memdup() by the safer g_memdup2_qemu() wrapper. > > Signed-off-by: Philippe Mathieu-Daudé Acked-by: David Gibson > --- > target/ppc/mmu-hash64.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c > index 19832c4b46f..2ee6025a406 100644 > --- a/target/ppc/mmu-hash64.c > +++ b/target/ppc/mmu-hash64.c > @@ -1122,7 +1122,8 @@ void ppc_hash64_init(PowerPCCPU *cpu) > return; > } > > -cpu->hash64_opts = g_memdup(pcc->hash64_opts, sizeof(*cpu->hash64_opts)); > +cpu->hash64_opts = g_memdup2_qemu(pcc->hash64_opts, > + sizeof(*cpu->hash64_opts)); > } > > void ppc_hash64_finalize(PowerPCCPU *cpu) -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH 15/23] net: Avoid dynamic stack allocation
On Wed, May 05, 2021 at 11:10:39PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
>
> Signed-off-by: Philippe Mathieu-Daudé
fsl_etsec parts
Acked-by: David Gibson
> ---
> hw/net/fsl_etsec/rings.c | 9 -
> hw/net/rocker/rocker_of_dpa.c | 2 +-
> net/dump.c| 2 +-
> net/tap.c | 2 +-
> 4 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 8f084464155..1abdcb5a29c 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -381,8 +381,6 @@ static void fill_rx_bd(eTSEC *etsec,
> uint16_t to_write;
> hwaddr bufptr = bd->bufptr +
> ((hwaddr)(etsec->regs[TBDBPH].value & 0xF) << 32);
> -uint8_t padd[etsec->rx_padding];
> -uint8_t rem;
>
> RING_DEBUG("eTSEC fill Rx buffer @ 0x%016" HWADDR_PRIx
> " size:%zu(padding + crc:%u) + fcb:%u\n",
> @@ -423,11 +421,12 @@ static void fill_rx_bd(eTSEC *etsec,
> /* The remaining bytes are only for padding which is not actually
> * allocated in the data buffer.
> */
> -
> -rem = MIN(etsec->regs[MRBLR].value - bd->length, etsec->rx_padding);
> +uint8_t rem = MIN(etsec->regs[MRBLR].value - bd->length,
> + etsec->rx_padding);
>
> if (rem > 0) {
> -memset(padd, 0x0, sizeof(padd));
> +g_autofree uint8_t *padd = g_malloc0(etsec->rx_padding);
> +
> etsec->rx_padding -= rem;
> *size -= rem;
> bd->length+= rem;
> diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
> index b3b8c5bb6d4..3e400ceaef7 100644
> --- a/hw/net/rocker/rocker_of_dpa.c
> +++ b/hw/net/rocker/rocker_of_dpa.c
> @@ -1043,7 +1043,7 @@ static void of_dpa_flow_ig_tbl(OfDpaFlowContext *fc,
> uint32_t tbl_id)
> static ssize_t of_dpa_ig(World *world, uint32_t pport,
> const struct iovec *iov, int iovcnt)
> {
> -struct iovec iov_copy[iovcnt + 2];
> +g_autofree struct iovec *iov_copy = g_new(struct iovec, iovcnt + 2);
> OfDpaFlowContext fc = {
> .of_dpa = world_private(world),
> .in_pport = pport,
> diff --git a/net/dump.c b/net/dump.c
> index 4d538d82a69..b830302e27d 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -68,7 +68,7 @@ static ssize_t dump_receive_iov(DumpState *s, const struct
> iovec *iov, int cnt)
> int64_t ts;
> int caplen;
> size_t size = iov_size(iov, cnt);
> -struct iovec dumpiov[cnt + 1];
> +g_autofree struct iovec *dumpiov = g_new(struct iovec, cnt + 1);
>
> /* Early return in case of previous error. */
> if (s->fd < 0) {
> diff --git a/net/tap.c b/net/tap.c
> index bae895e2874..2b9ed8a2cd8 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -120,7 +120,7 @@ static ssize_t tap_receive_iov(NetClientState *nc, const
> struct iovec *iov,
> {
> TAPState *s = DO_UPCAST(TAPState, nc, nc);
> const struct iovec *iovp = iov;
> -struct iovec iov_copy[iovcnt + 1];
> +g_autofree struct iovec *iov_copy = g_new(struct iovec, iovcnt + 1);
> struct virtio_net_hdr_mrg_rxbuf hdr = { };
>
> if (s->host_vnet_hdr_len && !s->using_vnet_hdr) {
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 10/23] hw/ppc/pnv: Avoid dynamic stack allocation
On Wed, May 05, 2021 at 11:10:34PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/ppc/pnv.c | 4 ++--
> hw/ppc/spapr.c | 8
> hw/ppc/spapr_pci_nvlink2.c | 2 +-
> 3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 77af846cdfe..f6e3d37b751 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -141,7 +141,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void
> *fdt)
> int smt_threads = CPU_CORE(pc)->nr_threads;
> CPUPPCState *env = >env;
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> -uint32_t servers_prop[smt_threads];
> +g_autofree uint32_t *servers_prop = g_new(uint32_t, smt_threads);
> int i;
> uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
> 0x, 0x};
> @@ -244,7 +244,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void
> *fdt)
> servers_prop[i] = cpu_to_be32(pc->pir + i);
> }
> _FDT((fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
> - servers_prop, sizeof(servers_prop;
> + servers_prop, sizeof(*servers_prop) * smt_threads)));
> }
>
> static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 529ff056dd2..31c2c0d97bf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -176,8 +176,8 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset,
> PowerPCCPU *cpu,
>int smt_threads)
> {
> int i, ret = 0;
> -uint32_t servers_prop[smt_threads];
> -uint32_t gservers_prop[smt_threads * 2];
> +g_autofree uint32_t *servers_prop = g_new(uint32_t, smt_threads);
> +g_autofree uint32_t *gservers_prop = g_new(uint32_t, smt_threads * 2);
> int index = spapr_get_vcpu_id(cpu);
>
> if (cpu->compat_pvr) {
> @@ -195,12 +195,12 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int
> offset, PowerPCCPU *cpu,
> gservers_prop[i*2 + 1] = 0;
> }
> ret = fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
> - servers_prop, sizeof(servers_prop));
> + servers_prop, sizeof(*servers_prop) * smt_threads);
> if (ret < 0) {
> return ret;
> }
> ret = fdt_setprop(fdt, offset, "ibm,ppc-interrupt-gserver#s",
> - gservers_prop, sizeof(gservers_prop));
> + gservers_prop, sizeof(*gservers_prop) * smt_threads *
> 2);
>
> return ret;
> }
> diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
> index 8ef9b40a18d..bb61adb114c 100644
> --- a/hw/ppc/spapr_pci_nvlink2.c
> +++ b/hw/ppc/spapr_pci_nvlink2.c
> @@ -401,7 +401,7 @@ void spapr_phb_nvgpu_populate_pcidev_dt(PCIDevice *dev,
> void *fdt, int offset,
> continue;
> }
> if (dev == nvslot->gpdev) {
> -uint32_t npus[nvslot->linknum];
> +g_autofree uint32_t *npus = g_new(uint32_t, nvslot->linknum);
>
> for (j = 0; j < nvslot->linknum; ++j) {
> PCIDevice *npdev = nvslot->links[j].npdev;
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 21/23] target/ppc/kvm: Avoid dynamic stack allocation
On Wed, May 05, 2021 at 11:10:45PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> target/ppc/kvm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index ae62daddf7d..90d0230eb86 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2660,7 +2660,7 @@ int kvmppc_get_htab_fd(bool write, uint64_t index,
> Error **errp)
> int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
> {
> int64_t starttime = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -uint8_t buf[bufsize];
> +g_autofree uint8_t *buf = g_malloc(bufsize);
> ssize_t rc;
>
> do {
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 11/23] hw/intc/xics: Avoid dynamic stack allocation
On Wed, May 05, 2021 at 11:10:35PM +0200, Philippe Mathieu-Daudé wrote:
> Use autofree heap allocation instead of variable-length
> array on the stack.
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/intc/xics.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 68f9d44feb4..c293d00d5c4 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -566,8 +566,8 @@ static void ics_reset_irq(ICSIRQState *irq)
> static void ics_reset(DeviceState *dev)
> {
> ICSState *ics = ICS(dev);
> +g_autofree uint8_t *flags = g_malloc(ics->nr_irqs);
> int i;
> -uint8_t flags[ics->nr_irqs];
>
> for (i = 0; i < ics->nr_irqs; i++) {
> flags[i] = ics->irqs[i].flags;
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH v2 2/3] hw/pci-host/raven: Manually reset the OR_IRQ device
On Sun, May 02, 2021 at 10:31:20PM +0200, Philippe Mathieu-Daudé wrote:
> The OR_IRQ device is bus-less, thus isn't reset automatically.
> Add the raven_pcihost_reset() handler to manually reset the OR IRQ.
>
> Fixes: f40b83a4e31 ("40p: use OR gate to wire up raven PCI interrupts")
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/pci-host/prep.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 0a9162fba97..7481bbf99d4 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -230,6 +230,15 @@ static void raven_change_gpio(void *opaque, int n, int
> level)
> s->contiguous_map = level;
> }
>
> +static void raven_pcihost_reset_enter(Object *obj, ResetType type)
> +{
> +PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
> +
> +if (!s->is_legacy_prep) {
> +device_cold_reset(DEVICE(>or_irq));
> +}
> +}
> +
> static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
> {
> SysBusDevice *dev = SYS_BUS_DEVICE(d);
> @@ -419,11 +428,13 @@ static Property raven_pcihost_properties[] = {
> static void raven_pcihost_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> +ResettableClass *rc = RESETTABLE_CLASS(klass);
>
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> dc->realize = raven_pcihost_realizefn;
> device_class_set_props(dc, raven_pcihost_properties);
> dc->fw_name = "pci";
> +rc->phases.enter = raven_pcihost_reset_enter;
> }
>
> static const TypeInfo raven_pcihost_info = {
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler
On Tue, Apr 27, 2021 at 11:20:07AM +0200, Philippe Mathieu-Daudé wrote:
> On 4/27/21 3:45 AM, David Gibson wrote:
> > On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote:
> >> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset
> >> automatically. Register a reset handler to get reset with the
> >> machine.
> >>
> >> It doesn't seem to be an issue because it is that way since the
> >> device QDev'ifycation 8 years ago, in commit a83000f5e3f
> >> ("spapr-tce: make sPAPRTCETable a proper device").
> >> Still, correct to have a proper API usage.
> >
> > So, the reason this works now is that we explicitly call
> > device_reset() on the TCE table from the TCE tables "owner", either a
> > PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()).
> >
> > I think we want either that, or the register_reset(), not both.
>
> rtas_quiesce() seems to call a DeviceClass::reset() on the
> children of TYPE_SPAPR_VIO_BUS:
>
> Abstract TYPE_VIO_SPAPR_DEVICE has the TYPE_SPAPR_VIO_BUS bus_type,
> and registers the spapr_vio_busdev_reset() handler, which calls
> spapr_vio_quiesce_one()...
>
> So either we already have 2 resets, or the bus is never reset?
There are 2 resets, and this is intentional. We reset once at machine
reset time, via the bus. Once a booting OS is done with the firmware
it calls "quiesce" to put all the devices back into a safe state. The
easiest way to do that is just to invoke their reset callbacks, so
that's what we do.
> The bus is created in spapr_machine_init():
>
> /* Set up VIO bus */
> spapr->vio_bus = spapr_vio_bus_init();
>
> TYPE_SPAPR_MACHINE class registers spapr_machine_reset(), which
> manually calls qemu_devices_reset() and spapr_drc_reset_all(),
> but I can't understand if a callee resets vio_bus...
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler
On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote:
> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset
> automatically. Register a reset handler to get reset with the
> machine.
>
> It doesn't seem to be an issue because it is that way since the
> device QDev'ifycation 8 years ago, in commit a83000f5e3f
> ("spapr-tce: make sPAPRTCETable a proper device").
> Still, correct to have a proper API usage.
So, the reason this works now is that we explicitly call
device_reset() on the TCE table from the TCE tables "owner", either a
PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()).
I think we want either that, or the register_reset(), not both.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/ppc/spapr_iommu.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 24537ffcbd3..f7dad1dc0fe 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -24,6 +24,7 @@
> #include "sysemu/kvm.h"
> #include "kvm_ppc.h"
> #include "migration/vmstate.h"
> +#include "sysemu/reset.h"
> #include "sysemu/dma.h"
> #include "exec/address-spaces.h"
> #include "trace.h"
> @@ -302,6 +303,11 @@ static const VMStateDescription vmstate_spapr_tce_table
> = {
> }
> };
>
> +static void spapr_tce_reset_handler(void *dev)
> +{
> +device_legacy_reset(DEVICE(dev));
> +}
> +
> static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
> {
> SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev);
> @@ -324,6 +330,8 @@ static void spapr_tce_table_realize(DeviceState *dev,
> Error **errp)
>
> vmstate_register(VMSTATE_IF(tcet), tcet->liobn, _spapr_tce_table,
> tcet);
> +
> +qemu_register_reset(spapr_tce_reset_handler, dev);
> }
>
> void spapr_tce_set_need_vfio(SpaprTceTable *tcet, bool need_vfio)
> @@ -425,6 +433,8 @@ static void spapr_tce_table_unrealize(DeviceState *dev)
> {
> SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev);
>
> +qemu_unregister_reset(spapr_tce_reset_handler, dev);
> +
> vmstate_unregister(VMSTATE_IF(tcet), _spapr_tce_table, tcet);
>
> QLIST_REMOVE(tcet, list);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 4/5] hw/pci-host/raven: Manually reset the OR_IRQ device
On Sat, Apr 24, 2021 at 06:22:28PM +0200, Philippe Mathieu-Daudé wrote:
> The OR_IRQ device is bus-less, thus isn't reset automatically.
> Add the raven_pcihost_reset() handler to manually reset the OR IRQ.
>
> Fixes: f40b83a4e31 ("40p: use OR gate to wire up raven PCI interrupts")
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/pci-host/prep.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 0a9162fba97..275379e4c78 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -230,6 +230,15 @@ static void raven_change_gpio(void *opaque, int n, int
> level)
> s->contiguous_map = level;
> }
>
> +static void raven_pcihost_reset(DeviceState *dev)
> +{
> +PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
> +
> +if (!s->is_legacy_prep) {
> +device_legacy_reset(DEVICE(>or_irq));
> +}
> +}
> +
> static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
> {
> SysBusDevice *dev = SYS_BUS_DEVICE(d);
> @@ -422,6 +431,7 @@ static void raven_pcihost_class_init(ObjectClass *klass,
> void *data)
>
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> dc->realize = raven_pcihost_realizefn;
> + dc->reset = raven_pcihost_reset;
> device_class_set_props(dc, raven_pcihost_properties);
> dc->fw_name = "pci";
> }
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH-for-6.1 07/10] hw/block/pflash_cfi02: Simplify pflash_cfi02_register() prototype
On Fri, Mar 26, 2021 at 01:27:25AM +0100, Philippe Mathieu-Daudé wrote:
> The previous commit removed the mapping code from TYPE_PFLASH_CFI02.
> pflash_cfi02_register() doesn't use the 'nb_mappings' argument
> anymore. Simply remove it to simplify.
>
> Signed-off-by: Philippe Mathieu-Daudé
Revieed-by: David Gibson
> ---
> include/hw/block/flash.h | 1 -
> hw/arm/digic_boards.c| 1 -
> hw/arm/musicpal.c| 1 -
> hw/arm/xilinx_zynq.c | 2 +-
> hw/block/pflash_cfi02.c | 3 +--
> hw/lm32/lm32_boards.c| 4 ++--
> hw/ppc/ppc405_boards.c | 6 +++---
> hw/sh4/r2d.c | 2 +-
> 8 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
> index 7dde0adcee7..0e5dd818a9d 100644
> --- a/include/hw/block/flash.h
> +++ b/include/hw/block/flash.h
> @@ -36,7 +36,6 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
> hwaddr size,
> BlockBackend *blk,
> uint32_t sector_len,
> - int nb_mappings,
> int width,
> uint16_t id0, uint16_t id1,
> uint16_t id2, uint16_t id3,
> diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
> index 293402b1240..eb694c70d4c 100644
> --- a/hw/arm/digic_boards.c
> +++ b/hw/arm/digic_boards.c
> @@ -128,7 +128,6 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s,
> hwaddr addr,
> FLASH_K8P3215UQB_SIZE /
> FLASH_K8P3215UQB_SECTOR_SIZE);
> qdev_prop_set_uint32(dev, "sector-length", FLASH_K8P3215UQB_SECTOR_SIZE);
> qdev_prop_set_uint8(dev, "width", 4); /* 32-bit */
> -qdev_prop_set_uint8(dev, "mappings", 0);
> qdev_prop_set_uint8(dev, "big-endian", 0);
> qdev_prop_set_uint16(dev, "id0", 0x00ec);
> qdev_prop_set_uint16(dev, "id1", 0x007e);
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 7d1f2f3fb3f..e882e11df36 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -1657,7 +1657,6 @@ static void musicpal_init(MachineState *machine)
> qdev_prop_set_uint32(dev, "num-blocks", flash_size / sector_size);
> qdev_prop_set_uint32(dev, "sector-length", sector_size);
> qdev_prop_set_uint8(dev, "width", 2); /* 16-bit */
> -qdev_prop_set_uint8(dev, "mappings", 0);
> qdev_prop_set_uint8(dev, "big-endian", 0);
> qdev_prop_set_uint16(dev, "id0", 0x00bf);
> qdev_prop_set_uint16(dev, "id1", 0x236d);
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 8db6cfd47f5..d12b00e7648 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -220,7 +220,7 @@ static void zynq_init(MachineState *machine)
> pflash_cfi02_register(0xe200, "zynq.pflash", FLASH_SIZE,
>dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
>FLASH_SECTOR_SIZE, 1,
> - 1, 0x0066, 0x0022, 0x, 0x, 0x0555, 0x2aa,
> + 0x0066, 0x0022, 0x, 0x, 0x0555, 0x2aa,
>0);
>
> /* Create the main clock source, and feed slcr with it */
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 6f4b3e3c3fe..2b412402fac 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -968,7 +968,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
> hwaddr size,
> BlockBackend *blk,
> uint32_t sector_len,
> - int nb_mappings, int width,
> + int width,
> uint16_t id0, uint16_t id1,
> uint16_t id2, uint16_t id3,
> uint16_t unlock_addr0,
> @@ -977,7 +977,6 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
> {
> DeviceState *dev = qdev_new(TYPE_PFLASH_CFI02);
>
> -assert(nb_mappings <= 1);
> if (blk) {
> qdev_prop_set_drive(dev, "drive", blk);
> }
> diff --git a/hw/lm32/lm32_boards.c b/hw/lm32/lm32_boards.c
> index b5d97dd53ed..96877ba7cfb 100644
> --- a/hw/lm32/lm32_boards.c
> +++ b/hw/lm32/lm32_boards.c
> @@ -121,7 +121,7 @@ static void lm32_evr_init(MachineState *machine)
> pflash_cfi02_register(flash_base, "lm32_evr.flash", flash_siz
Re: [PATCH-for-6.1 06/10] hw/block/pflash_cfi02: Remove pflash_setup_mappings()
On Fri, Mar 26, 2021 at 01:27:24AM +0100, Philippe Mathieu-Daudé wrote:
> All boards calling pflash_cfi02_register() use nb_mappings=1,
> which does not do any mapping:
>
> $ git grep -wl pflash_cfi02_register hw/
> hw/arm/xilinx_zynq.c
> hw/block/pflash_cfi02.c
> hw/lm32/lm32_boards.c
> hw/ppc/ppc405_boards.c
> hw/sh4/r2d.c
>
> We can remove this now unneeded code.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: David Gibson
> ---
> hw/block/pflash_cfi02.c | 35 ++-
> 1 file changed, 2 insertions(+), 33 deletions(-)
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 02c514fb6e0..6f4b3e3c3fe 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -75,7 +75,6 @@ struct PFlashCFI02 {
> uint32_t nb_blocs[PFLASH_MAX_ERASE_REGIONS];
> uint32_t sector_len[PFLASH_MAX_ERASE_REGIONS];
> uint32_t chip_len;
> -uint8_t mappings;
> uint8_t width;
> uint8_t be;
> int wcycle; /* if 0, the flash is read normally */
> @@ -92,13 +91,6 @@ struct PFlashCFI02 {
> uint16_t unlock_addr1;
> uint8_t cfi_table[0x4d];
> QEMUTimer timer;
> -/*
> - * The device replicates the flash memory across its memory space.
> Emulate
> - * that by having a container (.mem) filled with an array of aliases
> - * (.mem_mappings) pointing to the flash memory (.orig_mem).
> - */
> -MemoryRegion mem;
> -MemoryRegion *mem_mappings;/* array; one per mapping */
> MemoryRegion orig_mem;
> bool rom_mode;
> int read_counter; /* used for lazy switch-back to rom mode */
> @@ -158,23 +150,6 @@ static inline void toggle_dq2(PFlashCFI02 *pfl)
> pfl->status ^= 0x04;
> }
>
> -/*
> - * Set up replicated mappings of the same region.
> - */
> -static void pflash_setup_mappings(PFlashCFI02 *pfl)
> -{
> -unsigned i;
> -hwaddr size = memory_region_size(>orig_mem);
> -
> -memory_region_init(>mem, OBJECT(pfl), "pflash", pfl->mappings *
> size);
> -pfl->mem_mappings = g_new(MemoryRegion, pfl->mappings);
> -for (i = 0; i < pfl->mappings; ++i) {
> -memory_region_init_alias(>mem_mappings[i], OBJECT(pfl),
> - "pflash-alias", >orig_mem, 0, size);
> -memory_region_add_subregion(>mem, i * size,
> >mem_mappings[i]);
> -}
> -}
> -
> static void pflash_reset_state_machine(PFlashCFI02 *pfl)
> {
> trace_pflash_reset(pfl->name);
> @@ -917,12 +892,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error
> **errp)
> pfl->sector_erase_map = bitmap_new(pfl->total_sectors);
>
> pfl->rom_mode = true;
> -if (pfl->mappings > 1) {
> -pflash_setup_mappings(pfl);
> -sysbus_init_mmio(SYS_BUS_DEVICE(dev), >mem);
> -} else {
> -sysbus_init_mmio(SYS_BUS_DEVICE(dev), >orig_mem);
> -}
> +sysbus_init_mmio(SYS_BUS_DEVICE(dev), >orig_mem);
>
> timer_init_ns(>timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> pfl->status = 0;
> @@ -950,7 +920,6 @@ static Property pflash_cfi02_properties[] = {
> DEFINE_PROP_UINT32("num-blocks3", PFlashCFI02, nb_blocs[3], 0),
> DEFINE_PROP_UINT32("sector-length3", PFlashCFI02, sector_len[3], 0),
> DEFINE_PROP_UINT8("width", PFlashCFI02, width, 0),
> -DEFINE_PROP_UINT8("mappings", PFlashCFI02, mappings, 0),
> DEFINE_PROP_UINT8("big-endian", PFlashCFI02, be, 0),
> DEFINE_PROP_UINT16("id0", PFlashCFI02, ident0, 0),
> DEFINE_PROP_UINT16("id1", PFlashCFI02, ident1, 0),
> @@ -1008,6 +977,7 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
> {
> DeviceState *dev = qdev_new(TYPE_PFLASH_CFI02);
>
> +assert(nb_mappings <= 1);
> if (blk) {
> qdev_prop_set_drive(dev, "drive", blk);
> }
> @@ -1015,7 +985,6 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
> qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
> qdev_prop_set_uint32(dev, "sector-length", sector_len);
> qdev_prop_set_uint8(dev, "width", width);
> -qdev_prop_set_uint8(dev, "mappings", nb_mappings);
> qdev_prop_set_uint8(dev, "big-endian", !!be);
> qdev_prop_set_uint16(dev, "id0", id0);
> qdev_prop_set_uint16(dev, "id1", id1);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 2/2] sysemu: Let VMChangeStateHandler take boolean 'running' argument
On Mon, Jan 11, 2021 at 04:20:20PM +0100, Philippe Mathieu-Daudé wrote:
> The 'running' argument from VMChangeStateHandler does not require
> other value than 0 / 1. Make it a plain boolean.
>
> Signed-off-by: Philippe Mathieu-Daudé
ppc parts
Acked-by: David Gibson
> ---
> include/sysemu/runstate.h | 10 --
> target/arm/kvm_arm.h| 2 +-
> target/ppc/cpu-qom.h| 2 +-
> accel/xen/xen-all.c | 2 +-
> audio/audio.c | 2 +-
> block/block-backend.c | 2 +-
> gdbstub.c | 2 +-
> hw/block/pflash_cfi01.c | 2 +-
> hw/block/virtio-blk.c | 2 +-
> hw/display/qxl.c| 2 +-
> hw/i386/kvm/clock.c | 2 +-
> hw/i386/kvm/i8254.c | 2 +-
> hw/i386/kvmvapic.c | 2 +-
> hw/i386/xen/xen-hvm.c | 2 +-
> hw/ide/core.c | 2 +-
> hw/intc/arm_gicv3_its_kvm.c | 2 +-
> hw/intc/arm_gicv3_kvm.c | 2 +-
> hw/intc/spapr_xive_kvm.c| 2 +-
> hw/misc/mac_via.c | 2 +-
> hw/net/e1000e_core.c| 2 +-
> hw/nvram/spapr_nvram.c | 2 +-
> hw/ppc/ppc.c| 2 +-
> hw/ppc/ppc_booke.c | 2 +-
> hw/s390x/tod-kvm.c | 2 +-
> hw/scsi/scsi-bus.c | 2 +-
> hw/usb/hcd-ehci.c | 2 +-
> hw/usb/host-libusb.c| 2 +-
> hw/usb/redirect.c | 2 +-
> hw/vfio/migration.c | 2 +-
> hw/virtio/virtio-rng.c | 2 +-
> hw/virtio/virtio.c | 2 +-
> net/net.c | 2 +-
> softmmu/memory.c| 2 +-
> softmmu/runstate.c | 2 +-
> target/arm/kvm.c| 2 +-
> target/i386/kvm/kvm.c | 2 +-
> target/i386/sev.c | 2 +-
> target/i386/whpx/whpx-all.c | 2 +-
> target/mips/kvm.c | 4 ++--
> ui/gtk.c| 2 +-
> ui/spice-core.c | 2 +-
> 41 files changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
> index 3ab35a039a0..a5356915734 100644
> --- a/include/sysemu/runstate.h
> +++ b/include/sysemu/runstate.h
> @@ -10,7 +10,7 @@ bool runstate_is_running(void);
> bool runstate_needs_reset(void);
> bool runstate_store(char *str, size_t size);
>
> -typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
> +typedef void VMChangeStateHandler(void *opaque, bool running, RunState
> state);
>
> VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler
> *cb,
> void *opaque);
> @@ -20,7 +20,13 @@ VMChangeStateEntry
> *qdev_add_vm_change_state_handler(DeviceState *dev,
> VMChangeStateHandler
> *cb,
> void *opaque);
> void qemu_del_vm_change_state_handler(VMChangeStateEntry *e);
> -void vm_state_notify(int running, RunState state);
> +/**
> + * vm_state_notify: Notify the state of the VM
> + *
> + * @running: whether the VM is running or not.
> + * @state: the #RunState of the VM.
> + */
> +void vm_state_notify(bool running, RunState state);
>
> static inline bool shutdown_caused_by_guest(ShutdownCause cause)
> {
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index eb81b7059eb..68ec970c4f4 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -352,7 +352,7 @@ void kvm_arm_get_virtual_time(CPUState *cs);
> */
> void kvm_arm_put_virtual_time(CPUState *cs);
>
> -void kvm_arm_vm_state_change(void *opaque, int running, RunState state);
> +void kvm_arm_vm_state_change(void *opaque, bool running, RunState state);
>
> int kvm_arm_vgic_probe(void);
>
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index 63b9e8632ca..118baf8d41f 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -218,7 +218,7 @@ extern const VMStateDescription vmstate_ppc_timebase;
> .offset = vmstate_offset_value(_state, _field, PPCTimebase), \
> }
>
> -void cpu_ppc_clock_vm_state_change(void *opaque, int running,
> +void cpu_ppc_clock_vm_state_change(void *opaque, bool running,
> RunState state);
> #endif
>
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 878a4089d97..3756aca27be 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -122,7 +122,7 @@ static void xenstore_record_dm_state(struct xs_handle
> *xs, const char *state)
> }
>
>
> -static void xen_change_state_handler(void *opaque, int running,
> +static void xen_change_state_handler(void *opaque, bool
Re: [PATCH v6 11/11] qapi: Use QAPI_LIST_ADD() where possible
On Tue, Oct 27, 2020 at 12:05:56AM -0500, Eric Blake wrote:
> Anywhere we create a list of just one item or by prepending items
> (typically because order doesn't matter), we can use the now-public
> macro. But places where we must keep the list in order by appending
> remain open-coded.
>
> Signed-off-by: Eric Blake
> ---
> docs/devel/writing-qmp-commands.txt | 13 +++--
> hw/net/rocker/rocker_fp.h | 2 +-
> block/gluster.c | 19 +
> chardev/char.c | 21 +++
> hw/core/machine.c | 6 +
> hw/net/rocker/rocker.c | 8 +++---
> hw/net/rocker/rocker_fp.c | 14 +-
> hw/net/virtio-net.c | 21 +--
> migration/migration.c | 7 ++---
> migration/postcopy-ram.c| 7 ++---
> monitor/hmp-cmds.c | 11
> qemu-img.c | 5 ++--
> qga/commands-posix.c| 13 +++--
> qga/commands-win32.c| 17 +++-
> qga/commands.c | 6 +
> qom/qom-qmp-cmds.c | 29 ++--
> target/arm/helper.c | 6 +
> target/arm/monitor.c| 13 ++---
> target/i386/cpu.c | 6 +
> target/mips/helper.c| 6 +
> target/s390x/cpu_models.c | 12 ++---
> tests/test-clone-visitor.c | 7 ++---
> tests/test-qobject-output-visitor.c | 42 ++---
> tests/test-visitor-serialization.c | 5 +---
> trace/qmp.c | 22 +++
> ui/vnc.c| 21 +--
> util/qemu-config.c | 14 +++---
> target/ppc/translate_init.c.inc | 12 ++---
ppc parts
Acked-by: David Gibson
> 28 files changed, 119 insertions(+), 246 deletions(-)
>
> diff --git a/docs/devel/writing-qmp-commands.txt
> b/docs/devel/writing-qmp-commands.txt
> index 46a6c48683f5..3e11eeaa1893 100644
> --- a/docs/devel/writing-qmp-commands.txt
> +++ b/docs/devel/writing-qmp-commands.txt
> @@ -531,15 +531,10 @@ TimerAlarmMethodList *qmp_query_alarm_methods(Error
> **errp)
> bool current = true;
>
> for (p = alarm_timers; p->name; p++) {
> -TimerAlarmMethodList *info = g_malloc0(sizeof(*info));
> -info->value = g_malloc0(sizeof(*info->value));
> -info->value->method_name = g_strdup(p->name);
> -info->value->current = current;
> -
> -current = false;
> -
> -info->next = method_list;
> -method_list = info;
> + TimerAlarmMethod *value = g_new0(TimerAlarmMethod, 1);
> +value->method_name = g_strdup(p->name);
> +value->current = current;
> +QAPI_LIST_ADD(method_list, value);
> }
>
> return method_list;
> diff --git a/hw/net/rocker/rocker_fp.h b/hw/net/rocker/rocker_fp.h
> index dbe1dd329a4b..4cb0bb9ccf81 100644
> --- a/hw/net/rocker/rocker_fp.h
> +++ b/hw/net/rocker/rocker_fp.h
> @@ -28,7 +28,7 @@ int fp_port_eg(FpPort *port, const struct iovec *iov, int
> iovcnt);
>
> char *fp_port_get_name(FpPort *port);
> bool fp_port_get_link_up(FpPort *port);
> -void fp_port_get_info(FpPort *port, RockerPortList *info);
> +void fp_port_get_info(FpPort *port, RockerPort *info);
> void fp_port_get_macaddr(FpPort *port, MACAddr *macaddr);
> void fp_port_set_macaddr(FpPort *port, MACAddr *macaddr);
> uint8_t fp_port_get_learning(FpPort *port);
> diff --git a/block/gluster.c b/block/gluster.c
> index 4f1448e2bc88..cf446c23f85d 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -359,8 +359,8 @@ static int qemu_gluster_parse_uri(BlockdevOptionsGluster
> *gconf,
> return -EINVAL;
> }
>
> -gconf->server = g_new0(SocketAddressList, 1);
> -gconf->server->value = gsconf = g_new0(SocketAddress, 1);
> +gsconf = g_new0(SocketAddress, 1);
> +QAPI_LIST_ADD(gconf->server, gsconf);
>
> /* transport */
> if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> @@ -514,7 +514,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
> *gconf,
> {
> QemuOpts *opts;
> SocketAddress *gsconf = NULL;
> -SocketAddressList *curr = NULL;
> +SocketAddressList **curr;
> QDict *backing_options = NULL;
> Error *local_err = NULL;
> char *str = NULL;
> @@ -547,6 +547,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster
> *gconf,
> }
> gconf->path = g_strdup(ptr);
> qemu_opts_del(opts);
> +cur
Re: Suspicious QOM types without instance/class size
On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote: > While trying to convert TypeInfo declarations to the new > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > where instance_size or class_size is not set, despite having type > checker macros that use a specific type. > > The ones with "WARNING" are abstract types (maybe not serious if > subclasses set the appropriate sizes). The ones with "ERROR" > don't seem to be abstract types. Comment on the ones within my area: > > WARNING: hw/input/adb.c:310:1: class_size should be set to > sizeof(ADBDeviceClass)? Yeah, that looks like a bug (though we'll get away with it because it's abstract). > WARNING: hw/ppc/pnv_lpc.c:771:1: instance_size should be set to > sizeof(PnvLpcController)? Ditto. Should I make fixes for these, or will you? > ERROR: hw/ppc/spapr_drc.c:771:1: instance_size should be set to > sizeof(SpaprDrc)? I'm confused by this one. I'm not exactly sure which definition is tripping the error, and AFAICT they should all be correctly inheriting instance_size from either TYPE_SPAPR_DR_CONNECTOR or TYPE_SPAPR_DRC_PHSYICAL. If anything, it looks like TYPE_SPAPR_DRC_PHB could drop it's explicit override of instance_size. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH-for-5.1? v2 1/2] qemu/osdep: Make QEMU_VMALLOC_ALIGN unsigned long
On Thu, Jul 30, 2020 at 04:12:44PM +0200, Philippe Mathieu-Daudé wrote: > QEMU_VMALLOC_ALIGN is sometimes expanded to signed type, > other times to unsigned. Unify using unsigned. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: David Gibson > --- > include/qemu/osdep.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index 20872e793e..085df8d508 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -454,10 +454,10 @@ void qemu_anon_ram_free(void *ptr, size_t size); > /* Use 2 MiB alignment so transparent hugepages can be used by KVM. >Valgrind does not support alignments larger than 1 MiB, >therefore we need special code which handles running on Valgrind. */ > -# define QEMU_VMALLOC_ALIGN (512 * 4096) > +# define QEMU_VMALLOC_ALIGN (512 * 4096UL) > #elif defined(__linux__) && defined(__s390x__) > /* Use 1 MiB (segment size) alignment so gmap can be used by KVM. */ > -# define QEMU_VMALLOC_ALIGN (256 * 4096) > +# define QEMU_VMALLOC_ALIGN (256 * 4096UL) > #elif defined(__linux__) && defined(__sparc__) > #include > # define QEMU_VMALLOC_ALIGN MAX(qemu_real_host_page_size, SHMLBA) -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH-for-5.1? v2 2/2] util/pagesize: Make qemu_real_host_page_size of type size_t
On Thu, Jul 30, 2020 at 04:12:45PM +0200, Philippe Mathieu-Daudé wrote:
> We use different types to hold 'qemu_real_host_page_size'.
> Unify picking 'size_t' which seems the best candidate.
>
> Doing so fix a format string issue in hw/virtio/virtio-mem.c
> reported when building with GCC 4.9.4:
>
> hw/virtio/virtio-mem.c: In function ‘virtio_mem_set_block_size’:
> hw/virtio/virtio-mem.c:756:9: error: format ‘%x’ expects argument of type
> ‘unsigned int’, but argument 7 has type ‘uintptr_t’ [-Werror=format=]
> error_setg(errp, "'%s' property has to be at least 0x%" PRIx32, name,
> ^
>
> Fixes: 910b25766b ("virtio-mem: Paravirtualized memory hot(un)plug")
> Reported-by: Bruce Rogers
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: David Gibson
and ppc parts
Acked-by: David Gibson
> ---
> include/exec/ram_addr.h | 4 ++--
> include/qemu/osdep.h | 2 +-
> accel/kvm/kvm-all.c | 3 ++-
> block/qcow2-cache.c | 2 +-
> exec.c | 8
> hw/ppc/spapr_pci.c | 2 +-
> hw/virtio/virtio-mem.c | 2 +-
> migration/migration.c| 2 +-
> migration/postcopy-ram.c | 2 +-
> monitor/misc.c | 2 +-
> util/pagesize.c | 2 +-
> 11 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 3ef729a23c..e07532266e 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -93,8 +93,8 @@ static inline unsigned long int
> ramblock_recv_bitmap_offset(void *host_addr,
>
> bool ramblock_is_pmem(RAMBlock *rb);
>
> -long qemu_minrampagesize(void);
> -long qemu_maxrampagesize(void);
> +size_t qemu_minrampagesize(void);
> +size_t qemu_maxrampagesize(void);
>
> /**
> * qemu_ram_alloc_from_file,
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 085df8d508..77115a8270 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -635,10 +635,10 @@ char *qemu_get_pid_name(pid_t pid);
> */
> pid_t qemu_fork(Error **errp);
>
> +extern size_t qemu_real_host_page_size;
> /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even
> * when intptr_t is 32-bit and we are aligning a long long.
> */
> -extern uintptr_t qemu_real_host_page_size;
> extern intptr_t qemu_real_host_page_mask;
>
> extern int qemu_icache_linesize;
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 63ef6af9a1..59becfbd6c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -674,7 +674,8 @@ static int kvm_log_clear_one_slot(KVMSlot *mem, int
> as_id, uint64_t start,
> KVMState *s = kvm_state;
> uint64_t end, bmap_start, start_delta, bmap_npages;
> struct kvm_clear_dirty_log d;
> -unsigned long *bmap_clear = NULL, psize = qemu_real_host_page_size;
> +unsigned long *bmap_clear = NULL;
> +size_t psize = qemu_real_host_page_size;
> int ret;
>
> /*
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 7444b9c4ab..4ad9f5929f 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -74,7 +74,7 @@ static void qcow2_cache_table_release(Qcow2Cache *c, int i,
> int num_tables)
> /* Using MADV_DONTNEED to discard memory is a Linux-specific feature */
> #ifdef CONFIG_LINUX
> void *t = qcow2_cache_get_table_addr(c, i);
> -int align = qemu_real_host_page_size;
> +size_t align = qemu_real_host_page_size;
> size_t mem_size = (size_t) c->table_size * num_tables;
> size_t offset = QEMU_ALIGN_UP((uintptr_t) t, align) - (uintptr_t) t;
> size_t length = QEMU_ALIGN_DOWN(mem_size - offset, align);
> diff --git a/exec.c b/exec.c
> index 6f381f98e2..4b6d52e01f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1657,7 +1657,7 @@ static int find_max_backend_pagesize(Object *obj, void
> *opaque)
> * TODO: We assume right now that all mapped host memory backends are
> * used as RAM, however some might be used for different purposes.
> */
> -long qemu_minrampagesize(void)
> +size_t qemu_minrampagesize(void)
> {
> long hpsize = LONG_MAX;
> Object *memdev_root = object_resolve_path("/objects", NULL);
> @@ -1666,7 +1666,7 @@ long qemu_minrampagesize(void)
> return hpsize;
> }
>
> -long qemu_maxrampagesize(void)
> +size_t qemu_maxrampagesize(void)
> {
> long pagesize = 0;
> Object *memdev_root = object_resolve_path("/objects", NULL);
> @@ -1675,11 +1675,11 @@ long qemu_maxrampagesize(void)
> return pagesize;
> }
> #else
> -long qemu_minrampagesize(void)
> +size_t qemu_minrampagesize(void)
> {
> return q
Re: [PATCH] trivial: Remove trailing whitespaces
On Mon, Jul 06, 2020 at 06:23:00PM +0200, Christophe de Dinechin wrote: > There are a number of unnecessary trailing whitespaces that have > accumulated over time in the source code. They cause stray changes > in patches if you use tools that automatically remove them. > > Tested by doing a `git diff -w` after the change. > > This could probably be turned into a pre-commit hook. > > Signed-off-by: Christophe de Dinechin ppc parts Acked-by: David Gibson > --- > block/iscsi.c | 2 +- > disas/cris.c | 2 +- > disas/microblaze.c| 80 +++--- > disas/nios2.c | 256 +- > hmp-commands.hx | 2 +- > hw/alpha/typhoon.c| 6 +- > hw/arm/gumstix.c | 6 +- > hw/arm/omap1.c| 2 +- > hw/arm/stellaris.c| 2 +- > hw/char/etraxfs_ser.c | 2 +- > hw/core/ptimer.c | 2 +- > hw/cris/axis_dev88.c | 2 +- > hw/cris/boot.c| 2 +- > hw/display/qxl.c | 2 +- > hw/dma/etraxfs_dma.c | 18 +- > hw/dma/i82374.c | 2 +- > hw/i2c/bitbang_i2c.c | 2 +- > hw/input/tsc2005.c| 2 +- > hw/input/tsc210x.c| 2 +- > hw/intc/etraxfs_pic.c | 8 +- > hw/intc/sh_intc.c | 10 +- > hw/intc/xilinx_intc.c | 2 +- > hw/misc/imx25_ccm.c | 6 +- > hw/misc/imx31_ccm.c | 2 +- > hw/net/vmxnet3.h | 2 +- > hw/net/xilinx_ethlite.c | 2 +- > hw/pci/pcie.c | 2 +- > hw/sd/omap_mmc.c | 2 +- > hw/sh4/shix.c | 2 +- > hw/sparc64/sun4u.c| 2 +- > hw/timer/etraxfs_timer.c | 2 +- > hw/timer/xilinx_timer.c | 4 +- > hw/usb/hcd-musb.c | 10 +- > hw/usb/hcd-ohci.c | 6 +- > hw/usb/hcd-uhci.c | 2 +- > hw/virtio/virtio-pci.c| 2 +- > include/hw/cris/etraxfs_dma.h | 4 +- > include/hw/net/lance.h| 2 +- > include/hw/ppc/spapr.h| 2 +- > include/hw/xen/interface/io/ring.h| 34 +-- > include/qemu/log.h| 2 +- > include/qom/object.h | 4 +- > linux-user/cris/cpu_loop.c| 16 +- > linux-user/microblaze/cpu_loop.c | 16 +- > linux-user/mmap.c | 8 +- > linux-user/sparc/signal.c | 4 +- > linux-user/syscall.c | 24 +- > linux-user/syscall_defs.h | 2 +- > linux-user/uaccess.c | 2 +- > os-posix.c| 2 +- > qapi/qapi-util.c | 2 +- > qemu-img.c| 2 +- > qemu-options.hx | 26 +- > qom/object.c | 2 +- > target/cris/translate.c | 28 +- > target/cris/translate_v10.inc.c | 6 +- > target/i386/hvf/hvf.c | 4 +- > target/i386/hvf/x86.c | 4 +- > target/i386/hvf/x86_decode.c | 20 +- > target/i386/hvf/x86_decode.h | 4 +- > target/i386/hvf/x86_descr.c | 2 +- > target/i386/hvf/x86_emu.c | 2 +- > target/i386/hvf/x86_mmu.c | 6 +- > target/i386/hvf/x86_task.c| 2 +- > target/i386/hvf/x86hvf.c | 42 +-- > target/i386/translate.c | 8 +- > target/microblaze/mmu.c | 2 +- > target/microblaze/translate.c | 2 +- > target/sh4/op_helper.c| 4 +- > target/xtensa/core-de212/core-isa.h | 6 +- > .../xtensa/core-sample_controller/core-isa.h | 6 +- > target/xtensa/core-test_kc705_be/core-isa.h | 2 +- > tcg/sparc/tcg-target.inc.c| 2 +- >
Re: [PATCH 04/46] macio: Tidy up error handling in macio_newworld_realize()
On Wed, Jun 24, 2020 at 06:43:02PM +0200, Markus Armbruster wrote:
> macio_newworld_realize() effectively ignores ns->gpio realization
> errors, leaking the Error object. Fortunately, macio_gpio_realize()
> can't actually fail. Tidy up.
>
> Cc: Mark Cave-Ayland
> Cc: David Gibson
> Signed-off-by: Markus Armbruster
Acked-by: David Gibson
> ---
> hw/misc/macio/macio.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 7cfe357cc4..bedf10e77b 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -329,7 +329,9 @@ static void macio_newworld_realize(PCIDevice *d, Error
> **errp)
> _abort);
> memory_region_add_subregion(>bar, 0x50,
> sysbus_mmio_get_region(sysbus_dev, 0));
> -qdev_realize(DEVICE(>gpio), BUS(>macio_bus), );
> +if (!qdev_realize(DEVICE(>gpio), BUS(>macio_bus), errp)) {
> +return;
> +}
>
> /* PMU */
> object_initialize_child(OBJECT(s), "pmu", >pmu, TYPE_VIA_PMU);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 05/10] exec: Move qemu_minrampagesize/qemu_maxrampagesize to 'qemu-common.h'
On Thu, May 07, 2020 at 07:39:53PM +0200, Philippe Mathieu-Daudé wrote: > Move these generic functions to a more common place, with other > functions related to host page size. Document a little. > > Cc: Alexey Kardashevskiy > Signed-off-by: Philippe Mathieu-Daudé ppc parts Acked-by: David Gibson > --- > include/exec/ram_addr.h| 3 --- > include/qemu-common.h | 10 ++ > hw/ppc/spapr_caps.c| 2 +- > hw/s390x/s390-virtio-ccw.c | 1 + > hw/vfio/spapr.c| 2 +- > 5 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h > index 06096e8c6a..195b67d3c8 100644 > --- a/include/exec/ram_addr.h > +++ b/include/exec/ram_addr.h > @@ -93,9 +93,6 @@ static inline unsigned long int > ramblock_recv_bitmap_offset(void *host_addr, > > bool ramblock_is_pmem(RAMBlock *rb); > > -long qemu_minrampagesize(void); > -long qemu_maxrampagesize(void); > - > /** > * qemu_ram_alloc_from_file, > * qemu_ram_alloc_from_fd: Allocate a ram block from the specified backing > diff --git a/include/qemu-common.h b/include/qemu-common.h > index d0142f29ac..2821a6ef76 100644 > --- a/include/qemu-common.h > +++ b/include/qemu-common.h > @@ -80,6 +80,16 @@ bool set_preferred_target_page_bits(int bits); > */ > void finalize_target_page_bits(void); > > +/** > + * qemu_minrampagesize: > + * qemu_maxrampagesize: > + * > + * If backed via -memdev, return the device page size, > + * else return the host kernel page size. > + */ > +long qemu_minrampagesize(void); > +long qemu_maxrampagesize(void); > + > /** > * Sends a (part of) iovec down a socket, yielding when the socket is full, > or > * Receives data into a (part of) iovec from a socket, > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index eb54f94227..33a802a103 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -23,11 +23,11 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu-common.h" > #include "qemu/error-report.h" > #include "qapi/error.h" > #include "qapi/visitor.h" > #include "sysemu/hw_accel.h" > -#include "exec/ram_addr.h" > #include "target/ppc/cpu.h" > #include "target/ppc/mmu-hash64.h" > #include "cpu-models.h" > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index f660070d22..c009384505 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -12,6 +12,7 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu-common.h" > #include "qapi/error.h" > #include "cpu.h" > #include "hw/boards.h" > diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c > index 2900bd1941..c64db940a7 100644 > --- a/hw/vfio/spapr.c > +++ b/hw/vfio/spapr.c > @@ -9,13 +9,13 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu-common.h" > #include "cpu.h" > #include > #include > > #include "hw/vfio/vfio-common.h" > #include "hw/hw.h" > -#include "exec/ram_addr.h" > #include "qemu/error-report.h" > #include "qapi/error.h" > #include "trace.h" -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH 10/10] exec: Move cpu_physical_memory_* functions to 'exec/memory-internal.h'
On Thu, May 07, 2020 at 07:39:58PM +0200, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
ppc parts
Acked-by: David Gibson
> ---
> include/exec/memory-internal.h | 305 -
> include/exec/ram_addr.h| 303 +---
> accel/tcg/cputlb.c | 1 -
> hw/ppc/spapr.c | 1 -
> hw/ppc/spapr_pci.c | 1 -
> memory.c | 1 -
> 6 files changed, 305 insertions(+), 307 deletions(-)
>
> diff --git a/include/exec/memory-internal.h b/include/exec/memory-internal.h
> index b2b7c1e78a..4abb3bbd85 100644
> --- a/include/exec/memory-internal.h
> +++ b/include/exec/memory-internal.h
> @@ -21,8 +21,13 @@
> #define MEMORY_INTERNAL_H
>
> #include "cpu.h"
> +#include "sysemu/tcg.h"
> +#include "sysemu/xen.h"
> +#include "exec/ramlist.h"
> +#include "exec/ramblock.h"
>
> #ifdef CONFIG_SOFTMMU
> +
> static inline AddressSpaceDispatch *flatview_to_dispatch(FlatView *fv)
> {
> return fv->dispatch;
> @@ -49,5 +54,303 @@ void address_space_dispatch_free(AddressSpaceDispatch *d);
>
> void mtree_print_dispatch(struct AddressSpaceDispatch *d,
>MemoryRegion *root);
> -#endif
> +
> +#define DIRTY_CLIENTS_ALL ((1 << DIRTY_MEMORY_NUM) - 1)
> +#define DIRTY_CLIENTS_NOCODE (DIRTY_CLIENTS_ALL & ~(1 << DIRTY_MEMORY_CODE))
> +
> +static inline bool cpu_physical_memory_get_dirty(ram_addr_t start,
> + ram_addr_t length,
> + unsigned client)
> +{
> +DirtyMemoryBlocks *blocks;
> +unsigned long end, page;
> +unsigned long idx, offset, base;
> +bool dirty = false;
> +
> +assert(client < DIRTY_MEMORY_NUM);
> +
> +end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +page = start >> TARGET_PAGE_BITS;
> +
> +WITH_RCU_READ_LOCK_GUARD() {
> +blocks = atomic_rcu_read(_list.dirty_memory[client]);
> +
> +idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> +offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +base = page - offset;
> +while (page < end) {
> +unsigned long next = MIN(end, base + DIRTY_MEMORY_BLOCK_SIZE);
> +unsigned long num = next - base;
> +unsigned long found = find_next_bit(blocks->blocks[idx],
> +num, offset);
> +if (found < num) {
> +dirty = true;
> +break;
> +}
> +
> +page = next;
> +idx++;
> +offset = 0;
> +base += DIRTY_MEMORY_BLOCK_SIZE;
> +}
> +}
> +
> +return dirty;
> +}
> +
> +static inline bool cpu_physical_memory_all_dirty(ram_addr_t start,
> + ram_addr_t length,
> + unsigned client)
> +{
> +DirtyMemoryBlocks *blocks;
> +unsigned long end, page;
> +unsigned long idx, offset, base;
> +bool dirty = true;
> +
> +assert(client < DIRTY_MEMORY_NUM);
> +
> +end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS;
> +page = start >> TARGET_PAGE_BITS;
> +
> +RCU_READ_LOCK_GUARD();
> +
> +blocks = atomic_rcu_read(_list.dirty_memory[client]);
> +
> +idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> +offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +base = page - offset;
> +while (page < end) {
> +unsigned long next = MIN(end, base + DIRTY_MEMORY_BLOCK_SIZE);
> +unsigned long num = next - base;
> +unsigned long found = find_next_zero_bit(blocks->blocks[idx],
> + num, offset);
> +if (found < num) {
> +dirty = false;
> +break;
> +}
> +
> +page = next;
> +idx++;
> +offset = 0;
> +base += DIRTY_MEMORY_BLOCK_SIZE;
> +}
> +
> +return dirty;
> +}
> +
> +static inline bool cpu_physical_memory_get_dirty_flag(ram_addr_t addr,
> + unsigned client)
> +{
> +return cpu_physical_memory_get_dirty(addr, 1, client);
> +}
> +
> +static inline bool cpu_physical_memory_is_clean(ram_addr_t addr)
> +{
> +bool vga = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_VGA);
> +bool code = cpu_physical_memory_get_dirty_flag(addr, DIRTY_MEMORY_CODE);
> +bool migration = c
Re: [PATCH 07/10] exec: Move all RAMBlock functions to 'exec/ramblock.h'
On Thu, May 07, 2020 at 07:39:55PM +0200, Philippe Mathieu-Daudé wrote:
> The RAMBlock API was dispersed in 3 different headers.
> One of these headers, "exec/ram_addr.h", is restricted
> to target dependent code. However these functions are
> not target specific. Move all functions into a single
> place. Now all these functions can be accessed by
> target-agnostic code.
>
> Signed-off-by: Philippe Mathieu-Daudé
ppc parts
Acked-by: David Gibson
> ---
> include/exec/cpu-common.h| 24 ---
> include/exec/ram_addr.h | 105 ---
> include/exec/ramblock.h | 134 +++
> migration/migration.h| 1 +
> accel/tcg/translate-all.c| 2 -
> hw/block/nvme.c | 2 +-
> hw/s390x/s390-stattrib-kvm.c | 1 -
> hw/s390x/s390-stattrib.c | 1 -
> hw/s390x/s390-virtio-ccw.c | 1 -
> hw/virtio/vhost-user.c | 1 +
> hw/virtio/vhost.c| 1 +
> hw/virtio/virtio-balloon.c | 1 +
> memory.c | 1 +
> migration/migration.c| 1 +
> migration/postcopy-ram.c | 1 +
> migration/savevm.c | 1 +
> stubs/ram-block.c| 2 +-
> target/ppc/kvm.c | 1 -
> target/s390x/kvm.c | 1 -
> util/vfio-helpers.c | 2 +-
> 20 files changed, 145 insertions(+), 139 deletions(-)
>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index b47e5630e7..347ceb603b 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -49,25 +49,6 @@ typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr
> addr);
> void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
> /* This should not be used by devices. */
> ram_addr_t qemu_ram_addr_from_host(void *ptr);
> -RAMBlock *qemu_ram_block_by_name(const char *name);
> -RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
> - ram_addr_t *offset);
> -ram_addr_t qemu_ram_block_host_offset(RAMBlock *rb, void *host);
> -void qemu_ram_set_idstr(RAMBlock *block, const char *name, DeviceState *dev);
> -void qemu_ram_unset_idstr(RAMBlock *block);
> -const char *qemu_ram_get_idstr(RAMBlock *rb);
> -void *qemu_ram_get_host_addr(RAMBlock *rb);
> -ram_addr_t qemu_ram_get_offset(RAMBlock *rb);
> -ram_addr_t qemu_ram_get_used_length(RAMBlock *rb);
> -bool qemu_ram_is_shared(RAMBlock *rb);
> -bool qemu_ram_is_uf_zeroable(RAMBlock *rb);
> -void qemu_ram_set_uf_zeroable(RAMBlock *rb);
> -bool qemu_ram_is_migratable(RAMBlock *rb);
> -void qemu_ram_set_migratable(RAMBlock *rb);
> -void qemu_ram_unset_migratable(RAMBlock *rb);
> -
> -size_t qemu_ram_pagesize(RAMBlock *block);
> -size_t qemu_ram_pagesize_largest(void);
>
> void cpu_physical_memory_rw(hwaddr addr, void *buf,
> hwaddr len, bool is_write);
> @@ -100,11 +81,6 @@ void qemu_flush_coalesced_mmio_buffer(void);
>
> void cpu_flush_icache_range(hwaddr start, hwaddr len);
>
> -typedef int (RAMBlockIterFunc)(RAMBlock *rb, void *opaque);
> -
> -int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque);
> -int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length);
> -
> #endif
>
> #endif /* CPU_COMMON_H */
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index c61d5188f7..0deffad66f 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -26,112 +26,7 @@
> #include "exec/ramlist.h"
> #include "exec/ramblock.h"
>
> -/**
> - * clear_bmap_size: calculate clear bitmap size
> - *
> - * @pages: number of guest pages
> - * @shift: guest page number shift
> - *
> - * Returns: number of bits for the clear bitmap
> - */
> -static inline long clear_bmap_size(uint64_t pages, uint8_t shift)
> -{
> -return DIV_ROUND_UP(pages, 1UL << shift);
> -}
>
> -/**
> - * clear_bmap_set: set clear bitmap for the page range
> - *
> - * @rb: the ramblock to operate on
> - * @start: the start page number
> - * @size: number of pages to set in the bitmap
> - *
> - * Returns: None
> - */
> -static inline void clear_bmap_set(RAMBlock *rb, uint64_t start,
> - uint64_t npages)
> -{
> -uint8_t shift = rb->clear_bmap_shift;
> -
> -bitmap_set_atomic(rb->clear_bmap, start >> shift,
> - clear_bmap_size(npages, shift));
> -}
> -
> -/**
> - * clear_bmap_test_and_clear: test clear bitmap for the page, clear if set
> - *
> - * @rb: the ramblock to operate on
> - * @page: the page number to check
> - *
> - * Returns: true i
Re: [PATCH 5/6] block/nvme: Align block pages queue to host page size
On Mon, May 04, 2020 at 11:46:40AM +0200, Philippe Mathieu-Daudé wrote: > In nvme_create_queue_pair() we create a page list using > qemu_blockalign(), then map it with qemu_vfio_dma_map(): > > q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_QUEUE_SIZE); > r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages, > s->page_size * NVME_QUEUE_SIZE, ...); > > With: > > s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF))); > > The qemu_vfio_dma_map() documentation says "The caller need > to make sure the area is aligned to page size". While we use > multiple s->page_size as alignment, it might be not sufficient > on some hosts. Use the qemu_real_host_page_size value to be > sure the host alignment is respected. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: David Gibson > --- > Cc: Cédric Le Goater > Cc: David Gibson > Cc: Laurent Vivier > --- > block/nvme.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/nvme.c b/block/nvme.c > index 7b7c0cc5d6..bde0d28b39 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -627,7 +627,7 @@ static int nvme_init(BlockDriverState *bs, const char > *device, int namespace, > > s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF))); > s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t); > -bs->bl.opt_mem_alignment = s->page_size; > + bs->bl.opt_mem_alignment = MAX(qemu_real_host_page_size, s->page_size); > timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 3); > > /* Reset device to get a clean state. */ -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH-for-5.1 v3 11/23] hw/pci-host/pnv_phb3: Add missing error-propagation code
On Mon, Apr 13, 2020 at 12:41:32AM +0200, Philippe Mathieu-Daudé wrote:
> Patch created mechanically by running:
>
> $ spatch \
> --macro-file scripts/cocci-macro-file.h --include-headers \
> --sp-file scripts/coccinelle/use-error_propagate-in-realize.cocci \
> --keep-comments --smpl-spacing --in-place --dir hw
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/pci-host/pnv_phb3.c | 24
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 57d717ed23..a9029f5a02 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -1008,7 +1008,11 @@ static void pnv_phb3_realize(DeviceState *dev, Error
> **errp)
>
> /* LSI sources */
> object_property_set_link(OBJECT(>lsis), OBJECT(pnv), "xics",
> - _abort);
> + _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> object_property_set_bool(OBJECT(>lsis), true, "realized",
> _err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -1023,9 +1027,17 @@ static void pnv_phb3_realize(DeviceState *dev, Error
> **errp)
>
> /* MSI sources */
> object_property_set_link(OBJECT(>msis), OBJECT(phb), "phb",
> - _abort);
> + _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> object_property_set_link(OBJECT(>msis), OBJECT(pnv), "xics",
> - _abort);
> + _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> object_property_set_bool(OBJECT(>msis), true, "realized",
> _err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -1034,7 +1046,11 @@ static void pnv_phb3_realize(DeviceState *dev, Error
> **errp)
>
> /* Power Bus Common Queue */
> object_property_set_link(OBJECT(>pbcq), OBJECT(phb), "phb",
> - _abort);
> + _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> object_property_set_bool(OBJECT(>pbcq), true, "realized",
> _err);
> if (local_err) {
> error_propagate(errp, local_err);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH-for-5.1 v3 18/24] hw/pci-host/pnv_phb3: Move some code from realize() to init()
On Mon, Apr 13, 2020 at 12:36:13AM +0200, Philippe Mathieu-Daudé wrote:
> Coccinelle reported:
>
> $ spatch ... --timeout 60 --sp-file \
> scripts/coccinelle/simplify-init-realize-error_propagate.cocci
> HANDLING: ./hw/pci-host/pnv_phb3.c
> >>> possible moves from pnv_phb3_instance_init() to pnv_phb3_realize() in
> ./hw/pci-host/pnv_phb3.c:992
>
> Move the calls using _abort which don't depend on input
> updated before realize() to init().
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> v3: Typo 'depend of' -> 'depend on' (eblake)
> ---
> hw/pci-host/pnv_phb3.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> index 74618fadf0..57d717ed23 100644
> --- a/hw/pci-host/pnv_phb3.c
> +++ b/hw/pci-host/pnv_phb3.c
> @@ -970,6 +970,8 @@ static void pnv_phb3_instance_init(Object *obj)
> /* LSI sources */
> object_initialize_child(obj, "lsi", >lsis, sizeof(phb->lsis),
> TYPE_ICS, _abort, NULL);
> +object_property_set_int(OBJECT(>lsis), PNV_PHB3_NUM_LSI, "nr-irqs",
> +_abort);
>
> /* Default init ... will be fixed by HW inits */
> phb->lsis.offset = 0;
> @@ -977,6 +979,8 @@ static void pnv_phb3_instance_init(Object *obj)
> /* MSI sources */
> object_initialize_child(obj, "msi", >msis, sizeof(phb->msis),
> TYPE_PHB3_MSI, _abort, NULL);
> +object_property_set_int(OBJECT(>msis), PHB3_MAX_MSI, "nr-irqs",
> +_abort);
>
> /* Power Bus Common Queue */
> object_initialize_child(obj, "pbcq", >pbcq, sizeof(phb->pbcq),
> @@ -1005,8 +1009,6 @@ static void pnv_phb3_realize(DeviceState *dev, Error
> **errp)
> /* LSI sources */
> object_property_set_link(OBJECT(>lsis), OBJECT(pnv), "xics",
> _abort);
> -object_property_set_int(OBJECT(>lsis), PNV_PHB3_NUM_LSI, "nr-irqs",
> -_abort);
> object_property_set_bool(OBJECT(>lsis), true, "realized",
> _err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -1024,8 +1026,6 @@ static void pnv_phb3_realize(DeviceState *dev, Error
> **errp)
> _abort);
> object_property_set_link(OBJECT(>msis), OBJECT(pnv), "xics",
> _abort);
> -object_property_set_int(OBJECT(>msis), PHB3_MAX_MSI, "nr-irqs",
> -_abort);
> object_property_set_bool(OBJECT(>msis), true, "realized",
> _err);
> if (local_err) {
> error_propagate(errp, local_err);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH-for-5.1 1/3] target: Remove unnecessary CPU() cast
On Sun, Apr 12, 2020 at 11:09:52PM +0200, Philippe Mathieu-Daudé wrote:
> The CPU() macro is defined as:
>
> #define CPU(obj) ((CPUState *)(obj))
>
> Remove an unnecessary CPU() cast.
>
> Patch created mechanically using spatch with this script:
>
> @@
> typedef CPUState;
> CPUState *s;
> @@
> - CPU(s)
> + s
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> target/ppc/mmu_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 86c667b094..8972714775 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -1820,7 +1820,7 @@ static inline void do_invalidate_BAT(CPUPPCState *env,
> target_ulong BATu,
> if (((end - base) >> TARGET_PAGE_BITS) > 1024) {
> /* Flushing 1024 4K pages is slower than a complete flush */
> LOG_BATS("Flush all BATs\n");
> -tlb_flush(CPU(cs));
> +tlb_flush(cs);
> LOG_BATS("Flush done\n");
> return;
> }
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH-for-5.1 3/3] hw: Remove unnecessary DEVICE() cast
On Sun, Apr 12, 2020 at 11:09:54PM +0200, Philippe Mathieu-Daudé wrote:
> The DEVICE() macro is defined as:
>
> #define DEVICE(obj) OBJECT_CHECK(DeviceState, (obj), TYPE_DEVICE)
>
> Remove unnecessary DEVICE() casts.
>
> Patch created mechanically using spatch with this script:
>
> @@
> typedef DeviceState;
> DeviceState *s;
> @@
> - DEVICE(s)
> + s
>
> Signed-off-by: Philippe Mathieu-Daudé
ppc parts
Acked-by: David Gibson
> ---
> hw/display/artist.c | 2 +-
> hw/display/cg3.c| 2 +-
> hw/display/sm501.c | 2 +-
> hw/display/tcx.c| 4 ++--
> hw/display/vga-isa.c| 2 +-
> hw/i2c/imx_i2c.c| 2 +-
> hw/i2c/mpc_i2c.c| 2 +-
> hw/ide/piix.c | 2 +-
> hw/misc/macio/pmu.c | 2 +-
> hw/net/ftgmac100.c | 3 +--
> hw/net/imx_fec.c| 2 +-
> hw/nubus/nubus-device.c | 2 +-
> hw/pci-host/bonito.c| 2 +-
> hw/ppc/spapr.c | 2 +-
> hw/sh4/sh_pci.c | 2 +-
> hw/xen/xen-legacy-backend.c | 2 +-
> 16 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/hw/display/artist.c b/hw/display/artist.c
> index 753dbb9a77..7e2a4556bd 100644
> --- a/hw/display/artist.c
> +++ b/hw/display/artist.c
> @@ -1353,7 +1353,7 @@ static void artist_realizefn(DeviceState *dev, Error
> **errp)
> s->cursor_height = 32;
> s->cursor_width = 32;
>
> -s->con = graphic_console_init(DEVICE(dev), 0, _ops, s);
> +s->con = graphic_console_init(dev, 0, _ops, s);
> qemu_console_resize(s->con, s->width, s->height);
> }
>
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index a1ede10394..f7f1c199ce 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -321,7 +321,7 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
>
> sysbus_init_irq(sbd, >irq);
>
> -s->con = graphic_console_init(DEVICE(dev), 0, _ops, s);
> +s->con = graphic_console_init(dev, 0, _ops, s);
> qemu_console_resize(s->con, s->width, s->height);
> }
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index de0ab9d977..2a564889bd 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1839,7 +1839,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
> >twoD_engine_region);
>
> /* create qemu graphic console */
> -s->con = graphic_console_init(DEVICE(dev), 0, _ops, s);
> +s->con = graphic_console_init(dev, 0, _ops, s);
> }
>
> static const VMStateDescription vmstate_sm501_state = {
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 76de16e8ea..1fb45b1aab 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -868,9 +868,9 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
> sysbus_init_irq(sbd, >irq);
>
> if (s->depth == 8) {
> -s->con = graphic_console_init(DEVICE(dev), 0, _ops, s);
> +s->con = graphic_console_init(dev, 0, _ops, s);
> } else {
> -s->con = graphic_console_init(DEVICE(dev), 0, _ops, s);
> +s->con = graphic_console_init(dev, 0, _ops, s);
> }
> s->thcmisc = 0;
>
> diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
> index 0633ed382c..3aaeeeca1e 100644
> --- a/hw/display/vga-isa.c
> +++ b/hw/display/vga-isa.c
> @@ -74,7 +74,7 @@ static void vga_isa_realizefn(DeviceState *dev, Error
> **errp)
> 0x000a,
> vga_io_memory, 1);
> memory_region_set_coalescing(vga_io_memory);
> -s->con = graphic_console_init(DEVICE(dev), 0, s->hw_ops, s);
> +s->con = graphic_console_init(dev, 0, s->hw_ops, s);
>
> memory_region_add_subregion(isa_address_space(isadev),
> VBE_DISPI_LFB_PHYSICAL_ADDRESS,
> diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
> index 30b9aea247..2e02e1c4fa 100644
> --- a/hw/i2c/imx_i2c.c
> +++ b/hw/i2c/imx_i2c.c
> @@ -305,7 +305,7 @@ static void imx_i2c_realize(DeviceState *dev, Error
> **errp)
>IMX_I2C_MEM_SIZE);
> sysbus_init_mmio(SYS_BUS_DEVICE(dev), >iomem);
> sysbus_init_irq(SYS_BUS_DEVICE(dev), >irq);
> -s->bus = i2c_init_bus(DEVICE(dev), NULL);
> +s->bus = i2c_init_bus(dev, NULL);
> }
>
> static void imx_i2c_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
> index 0aa1be3ce7..9a724f3a3e 100644
> --- a/hw/i2c/mpc_i2c.c
> +++ b/hw/i2c/mpc_i2c.c
> @@
Re: [PATCH-for-5.1 v3 01/24] various: Remove suspicious '\' character outside of #define in C code
On Mon, Apr 13, 2020 at 12:35:56AM +0200, Philippe Mathieu-Daudé wrote:
> Fixes the following coccinelle warnings:
>
> $ spatch --sp-file --verbose-parsing ... \
> scripts/coccinelle/remove_local_err.cocci
> ...
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/ppc/translate_init.inc.c:5213
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/ppc/translate_init.inc.c:5261
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/microblaze/cpu.c:166
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/microblaze/cpu.c:167
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/microblaze/cpu.c:169
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/microblaze/cpu.c:170
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/microblaze/cpu.c:171
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/microblaze/cpu.c:172
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/microblaze/cpu.c:173
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:5787
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:5789
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:5800
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:5801
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:5802
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:5804
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:5805
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:5806
> SUSPICIOUS: a \ character appears outside of a #define at
> ./target/i386/cpu.c:6329
> SUSPICIOUS: a \ character appears outside of a #define at
> ./hw/sd/sdhci.c:1133
> SUSPICIOUS: a \ character appears outside of a #define at
> ./hw/scsi/scsi-disk.c:3081
> SUSPICIOUS: a \ character appears outside of a #define at
> ./hw/net/virtio-net.c:1529
> SUSPICIOUS: a \ character appears outside of a #define at
> ./hw/riscv/sifive_u.c:468
> SUSPICIOUS: a \ character appears outside of a #define at ./dump/dump.c:1895
> SUSPICIOUS: a \ character appears outside of a #define at
> ./block/vhdx.c:2209
> SUSPICIOUS: a \ character appears outside of a #define at
> ./block/vhdx.c:2215
> SUSPICIOUS: a \ character appears outside of a #define at
> ./block/vhdx.c:2221
> SUSPICIOUS: a \ character appears outside of a #define at
> ./block/vhdx.c:
> SUSPICIOUS: a \ character appears outside of a #define at
> ./block/replication.c:172
> SUSPICIOUS: a \ character appears outside of a #define at
> ./block/replication.c:173
>
> Reviewed-by: Marc-André Lureau
> Signed-off-by: Philippe Mathieu-Daudé
ppc parts
Acked-by: David Gibson
> ---
> block/replication.c | 4 ++--
> block/vhdx.c| 8
> dump/dump.c | 2 +-
> hw/net/virtio-net.c | 2 +-
> hw/riscv/sifive_u.c | 2 +-
> hw/scsi/scsi-disk.c | 2 +-
> hw/sd/sdhci.c | 2 +-
> target/i386/cpu.c | 18 +-
> target/microblaze/cpu.c | 14 +++---
> target/ppc/translate_init.inc.c | 4 ++--
> 10 files changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/block/replication.c b/block/replication.c
> index da013c2041..971f0fe266 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -172,8 +172,8 @@ static void replication_child_perm(BlockDriverState *bs,
> BdrvChild *c,
> if ((bs->open_flags & (BDRV_O_INACTIVE | BDRV_O_RDWR)) == BDRV_O_RDWR) {
> *nperm |= BLK_PERM_WRITE;
> }
> -*nshared = BLK_PERM_CONSISTENT_READ \
> - | BLK_PERM_WRITE \
> +*nshared = BLK_PERM_CONSISTENT_READ
> + | BLK_PERM_WRITE
> | BLK_PERM_WRITE_UNCHANGED;
> return;
> }
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 33e57cd656..e16fdc2f2d 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -2206,20 +2206,20 @@ static QemuOptsList vhdx_create_opts = {
> .name = VHDX_BLOCK_OPT_BLOCK_SIZE,
> .type = QEMU_OPT_SIZE,
> .def_value_str = stringify(0),
> - .help = "Block Size; min 1MB, max 256MB. " \
> + .help = "Block Size; min 1MB, max 256MB. "
> "0 means auto-calculate based on image size."
> },
>
Re: [PATCH-for-5.0 v2 04/11] hw/input/adb-kbd: Remove dead assignment
On Sat, Mar 21, 2020 at 03:41:03PM +0100, Philippe Mathieu-Daudé wrote:
> Since commit 5a1f49718 the 'olen' variable is not really
> used. Remove it to fix a warning reported by Clang static
> code analyzer:
>
> CC hw/input/adb-kbd.o
> hw/input/adb-kbd.c:200:5: warning: Value stored to 'olen' is never read
> olen = 0;
> ^ ~
>
> Fixes: 5a1f49718 (adb: add support for QKeyCode)
> Reported-by: Clang Static Analyzer
> Suggested-by: BALATON Zoltan
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> v2: Remove 'olen' (Zoltan)
> ---
> hw/input/adb-kbd.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/hw/input/adb-kbd.c b/hw/input/adb-kbd.c
> index 0ba8207589..a6d5c9b7c9 100644
> --- a/hw/input/adb-kbd.c
> +++ b/hw/input/adb-kbd.c
> @@ -195,9 +195,7 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
> {
> KBDState *s = ADB_KEYBOARD(d);
> int keycode;
> -int olen;
>
> -olen = 0;
> if (s->count == 0) {
> return 0;
> }
> @@ -216,7 +214,6 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
> if (keycode == 0x7f) {
> obuf[0] = 0x7f;
> obuf[1] = 0x7f;
> -olen = 2;
> } else {
> obuf[0] = keycode;
> /* NOTE: the power key key-up is the two byte sequence 0xff 0xff;
> @@ -224,10 +221,9 @@ static int adb_kbd_poll(ADBDevice *d, uint8_t *obuf)
> * byte, but choose not to bother.
> */
> obuf[1] = 0xff;
> - olen = 2;
> }
>
> -return olen;
> +return 2;
> }
>
> static int adb_kbd_request(ADBDevice *d, uint8_t *obuf,
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH RESEND v2 17/32] hw/ppc/ppc405: Use memory_region_init_rom() with read-only regions
On Mon, Feb 24, 2020 at 09:55:18PM +0100, Philippe Mathieu-Daudé wrote:
> The scripts/coccinelle/memory-region-housekeeping.cocci reported:
> * TODO
> [[view:./hw/ppc/ppc405_boards.c::face=ovl-face1::linb=195::colb=8::cole=30][potential
> use of memory_region_init_rom*() in ./hw/ppc/ppc405_boards.c::195]]
> * TODO
> [[view:./hw/ppc/ppc405_boards.c::face=ovl-face1::linb=464::colb=8::cole=30][potential
> use of memory_region_init_rom*() in ./hw/ppc/ppc405_boards.c::464]]
>
> We can indeed replace the memory_region_init_ram() and
> memory_region_set_readonly() calls by memory_region_init_rom().
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/ppc/ppc405_boards.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 1f721feed6..5afe023253 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -192,7 +192,7 @@ static void ref405ep_init(MachineState *machine)
> #endif
> {
> bios = g_new(MemoryRegion, 1);
> -memory_region_init_ram(bios, NULL, "ef405ep.bios", BIOS_SIZE,
> +memory_region_init_rom(bios, NULL, "ef405ep.bios", BIOS_SIZE,
> _fatal);
>
> if (bios_name == NULL)
> @@ -216,7 +216,6 @@ static void ref405ep_init(MachineState *machine)
> /* Avoid an uninitialized variable warning */
> bios_size = -1;
> }
> -memory_region_set_readonly(bios, true);
> }
> /* Register FPGA */
> ref405ep_fpga_init(sysmem, 0xF030);
> @@ -461,7 +460,7 @@ static void taihu_405ep_init(MachineState *machine)
> if (bios_name == NULL)
> bios_name = BIOS_FILENAME;
> bios = g_new(MemoryRegion, 1);
> -memory_region_init_ram(bios, NULL, "taihu_405ep.bios", BIOS_SIZE,
> +memory_region_init_rom(bios, NULL, "taihu_405ep.bios", BIOS_SIZE,
> _fatal);
> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> if (filename) {
> @@ -479,7 +478,6 @@ static void taihu_405ep_init(MachineState *machine)
> error_report("Could not load PowerPC BIOS '%s'", bios_name);
> exit(1);
> }
> -memory_region_set_readonly(bios, true);
> }
> /* Register Linux flash */
> dinfo = drive_get(IF_PFLASH, 0, fl_idx);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH RESEND v2 10/32] hw/pci-host: Use memory_region_init_rom() with read-only regions
On Mon, Feb 24, 2020 at 09:55:11PM +0100, Philippe Mathieu-Daudé wrote:
> This commit was produced with the Coccinelle script
> scripts/coccinelle/memory-region-housekeeping.cocci.
>
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/pci-host/prep.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 1aff72bec6..1a02e9a670 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -325,9 +325,8 @@ static void raven_realize(PCIDevice *d, Error **errp)
> d->config[0x0D] = 0x10; // latency_timer
> d->config[0x34] = 0x00; // capabilities_pointer
>
> -memory_region_init_ram_nomigrate(>bios, OBJECT(s), "bios", BIOS_SIZE,
> - _fatal);
> -memory_region_set_readonly(>bios, true);
> +memory_region_init_rom_nomigrate(>bios, OBJECT(s), "bios", BIOS_SIZE,
> + _fatal);
> memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE),
> >bios);
> if (s->bios_name) {
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH RESEND v2 11/32] hw/ppc: Use memory_region_init_rom() with read-only regions
On Mon, Feb 24, 2020 at 09:55:12PM +0100, Philippe Mathieu-Daudé wrote: > This commit was produced with the Coccinelle script > scripts/coccinelle/memory-region-housekeeping.cocci. > > Signed-off-by: Philippe Mathieu-Daudé Acked-by: David Gibson > --- > hw/ppc/mac_newworld.c | 3 +-- > hw/ppc/mac_oldworld.c | 3 +-- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c > index 464d012103..566413e479 100644 > --- a/hw/ppc/mac_newworld.c > +++ b/hw/ppc/mac_newworld.c > @@ -156,13 +156,12 @@ static void ppc_core99_init(MachineState *machine) > memory_region_add_subregion(get_system_memory(), 0, ram); > > /* allocate and load BIOS */ > -memory_region_init_ram(bios, NULL, "ppc_core99.bios", BIOS_SIZE, > +memory_region_init_rom(bios, NULL, "ppc_core99.bios", BIOS_SIZE, > _fatal); > > if (bios_name == NULL) > bios_name = PROM_FILENAME; > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > -memory_region_set_readonly(bios, true); > memory_region_add_subregion(get_system_memory(), PROM_ADDR, bios); > > /* Load OpenBIOS (ELF) */ > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c > index 7318d7e9b4..8b22ff60b8 100644 > --- a/hw/ppc/mac_oldworld.c > +++ b/hw/ppc/mac_oldworld.c > @@ -132,13 +132,12 @@ static void ppc_heathrow_init(MachineState *machine) > memory_region_add_subregion(sysmem, 0, ram); > > /* allocate and load BIOS */ > -memory_region_init_ram(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE, > +memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE, > _fatal); > > if (bios_name == NULL) > bios_name = PROM_FILENAME; > filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); > -memory_region_set_readonly(bios, true); > memory_region_add_subregion(sysmem, PROM_ADDR, bios); > > /* Load OpenBIOS (ELF) */ -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH v3 19/20] Let cpu_[physical]_memory() calls pass a boolean 'is_write' argument
On Thu, Feb 20, 2020 at 02:05:47PM +0100, Philippe Mathieu-Daudé wrote:
> Use an explicit boolean type.
>
> This commit was produced with the included Coccinelle script
> scripts/coccinelle/exec_rw_const.
>
> Signed-off-by: Philippe Mathieu-Daudé
ppc parts
Acked-by: David Gibson
> ---
> scripts/coccinelle/exec_rw_const.cocci | 14 ++
> include/exec/cpu-common.h | 4 ++--
> hw/display/exynos4210_fimd.c | 3 ++-
> hw/display/milkymist-tmu2.c| 8
> hw/display/omap_dss.c | 2 +-
> hw/display/ramfb.c | 2 +-
> hw/misc/pc-testdev.c | 2 +-
> hw/nvram/spapr_nvram.c | 4 ++--
> hw/ppc/ppc440_uc.c | 6 --
> hw/ppc/spapr_hcall.c | 4 ++--
> hw/s390x/ipl.c | 2 +-
> hw/s390x/s390-pci-bus.c| 2 +-
> hw/s390x/virtio-ccw.c | 2 +-
> hw/xen/xen_pt_graphics.c | 2 +-
> target/i386/hax-all.c | 4 ++--
> target/s390x/excp_helper.c | 2 +-
> target/s390x/helper.c | 6 +++---
> 17 files changed, 43 insertions(+), 26 deletions(-)
>
> diff --git a/scripts/coccinelle/exec_rw_const.cocci
> b/scripts/coccinelle/exec_rw_const.cocci
> index ee98ce988e..54b1cab8cd 100644
> --- a/scripts/coccinelle/exec_rw_const.cocci
> +++ b/scripts/coccinelle/exec_rw_const.cocci
> @@ -11,6 +11,20 @@ expression E1, E2, E3, E4, E5;
> |
> - address_space_rw(E1, E2, E3, E4, E5, 1)
> + address_space_rw(E1, E2, E3, E4, E5, true)
> +|
> +
> +- cpu_physical_memory_rw(E1, E2, E3, 0)
> ++ cpu_physical_memory_rw(E1, E2, E3, false)
> +|
> +- cpu_physical_memory_rw(E1, E2, E3, 1)
> ++ cpu_physical_memory_rw(E1, E2, E3, true)
> +|
> +
> +- cpu_physical_memory_map(E1, E2, 0)
> ++ cpu_physical_memory_map(E1, E2, false)
> +|
> +- cpu_physical_memory_map(E1, E2, 1)
> ++ cpu_physical_memory_map(E1, E2, true)
> )
>
> // Use address_space_write instead of casting to non-const
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 6bfe201779..e7fd5781ea 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -74,12 +74,12 @@ void cpu_physical_memory_rw(hwaddr addr, void *buf,
> static inline void cpu_physical_memory_read(hwaddr addr,
> void *buf, hwaddr len)
> {
> -cpu_physical_memory_rw(addr, buf, len, 0);
> +cpu_physical_memory_rw(addr, buf, len, false);
> }
> static inline void cpu_physical_memory_write(hwaddr addr,
> const void *buf, hwaddr len)
> {
> -cpu_physical_memory_rw(addr, (void *)buf, len, 1);
> +cpu_physical_memory_rw(addr, (void *)buf, len, true);
> }
> void *cpu_physical_memory_map(hwaddr addr,
>hwaddr *plen,
> diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
> index c1071ecd46..ec6776680e 100644
> --- a/hw/display/exynos4210_fimd.c
> +++ b/hw/display/exynos4210_fimd.c
> @@ -1164,7 +1164,8 @@ static void
> fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
> goto error_return;
> }
>
> -w->host_fb_addr = cpu_physical_memory_map(fb_start_addr, _mapped_len,
> 0);
> +w->host_fb_addr = cpu_physical_memory_map(fb_start_addr, _mapped_len,
> + false);
> if (!w->host_fb_addr) {
> DPRINT_ERROR("Failed to map window %u framebuffer\n", win);
> goto error_return;
> diff --git a/hw/display/milkymist-tmu2.c b/hw/display/milkymist-tmu2.c
> index 199f1227e7..513c0d5bab 100644
> --- a/hw/display/milkymist-tmu2.c
> +++ b/hw/display/milkymist-tmu2.c
> @@ -218,7 +218,7 @@ static void tmu2_start(MilkymistTMU2State *s)
> glGenTextures(1, );
> glBindTexture(GL_TEXTURE_2D, texture);
> fb_len = 2ULL * s->regs[R_TEXHRES] * s->regs[R_TEXVRES];
> -fb = cpu_physical_memory_map(s->regs[R_TEXFBUF], _len, 0);
> +fb = cpu_physical_memory_map(s->regs[R_TEXFBUF], _len, false);
> if (fb == NULL) {
> glDeleteTextures(1, );
> glXMakeContextCurrent(s->dpy, None, None, NULL);
> @@ -262,7 +262,7 @@ static void tmu2_start(MilkymistTMU2State *s)
>
> /* Read the QEMU dest. framebuffer into the OpenGL framebuffer */
> fb_len = 2ULL * s->regs[R_DSTHRES] * s->regs[R_DSTVRES];
> -fb = cpu_physical_memory_map(s->regs[R_DSTFBUF], _len, 0);
> +fb = cpu_physical_memory_map(s->regs[R_DSTFBUF], _len, false);
> if (fb == NULL) {
>
Re: [PATCH v2 7/7] travis.yml: Enable builds on arm64, ppc64le and s390x
On Wed, Dec 04, 2019 at 04:46:18PM +0100, Thomas Huth wrote:
> Travis recently added the possibility to test on these architectures,
> too, so let's enable them in our travis.yml file to extend our test
> coverage.
>
> Unfortunately, the libssh in this Ubuntu version (bionic) is in a pretty
> unusable Frankenstein state and libspice-server-dev is not available here,
> so we can not use the global list of packages to install, but have to
> provide individual package lists instead.
>
> Also, some of the iotests crash when using "dist: bionic" on arm64
> and ppc64le, thus these two builders have to use "dist: xenial" until
> the problem is understood / fixed.
>
> Signed-off-by: Thomas Huth
Acked-by: David Gibson
> ---
> .travis.yml | 86 +
> 1 file changed, 86 insertions(+)
>
> diff --git a/.travis.yml b/.travis.yml
> index 445b0646c1..0e6458b0af 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -354,6 +354,92 @@ matrix:
> - TEST_CMD="make -j3 check-tcg V=1"
> - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
>
> +- arch: arm64
> + dist: xenial
> + addons:
> +apt_packages:
> + - libaio-dev
> + - libattr1-dev
> + - libbrlapi-dev
> + - libcap-ng-dev
> + - libgcrypt20-dev
> + - libgnutls28-dev
> + - libgtk-3-dev
> + - libiscsi-dev
> + - liblttng-ust-dev
> + - libncurses5-dev
> + - libnfs-dev
> + - libnss3-dev
> + - libpixman-1-dev
> + - libpng-dev
> + - librados-dev
> + - libsdl2-dev
> + - libseccomp-dev
> + - liburcu-dev
> + - libusb-1.0-0-dev
> + - libvdeplug-dev
> + - libvte-2.91-dev
> + env:
> +- TEST_CMD="make check check-tcg V=1"
> +- CONFIG="--disable-containers --target-list=${MAIN_SOFTMMU_TARGETS}"
> +
> +- arch: ppc64le
> + dist: xenial
> + addons:
> +apt_packages:
> + - libaio-dev
> + - libattr1-dev
> + - libbrlapi-dev
> + - libcap-ng-dev
> + - libgcrypt20-dev
> + - libgnutls28-dev
> + - libgtk-3-dev
> + - libiscsi-dev
> + - liblttng-ust-dev
> + - libncurses5-dev
> + - libnfs-dev
> + - libnss3-dev
> + - libpixman-1-dev
> + - libpng-dev
> + - librados-dev
> + - libsdl2-dev
> + - libseccomp-dev
> + - liburcu-dev
> + - libusb-1.0-0-dev
> + - libvdeplug-dev
> + - libvte-2.91-dev
> + env:
> +- TEST_CMD="make check check-tcg V=1"
> +- CONFIG="--disable-containers
> --target-list=${MAIN_SOFTMMU_TARGETS},ppc64le-linux-user"
> +
> +- arch: s390x
> + dist: bionic
> + addons:
> +apt_packages:
> + - libaio-dev
> + - libattr1-dev
> + - libbrlapi-dev
> + - libcap-ng-dev
> + - libgcrypt20-dev
> + - libgnutls28-dev
> + - libgtk-3-dev
> + - libiscsi-dev
> + - liblttng-ust-dev
> + - libncurses5-dev
> + - libnfs-dev
> + - libnss3-dev
> + - libpixman-1-dev
> + - libpng-dev
> + - librados-dev
> + - libsdl2-dev
> + - libseccomp-dev
> + - liburcu-dev
> + - libusb-1.0-0-dev
> + - libvdeplug-dev
> + - libvte-2.91-dev
> + env:
> +- TEST_CMD="make check check-tcg V=1"
> +- CONFIG="--disable-containers
> --target-list=${MAIN_SOFTMMU_TARGETS},s390x-linux-user"
>
> # Release builds
> # The make-release script expect a QEMU version, so our tag must start
> with a 'v'.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 11:56:35AM -0400, Richard Henderson wrote: > On 10/12/19 10:11 PM, Wei Yang wrote: > > Use ROUND_UP() to define, which is a little bit easy to read. > > > > Signed-off-by: Wei Yang > > --- > > include/exec/cpu-all.h | 7 +++ > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > > index ad9ab85eb3..255bb186ac 100644 > > --- a/include/exec/cpu-all.h > > +++ b/include/exec/cpu-all.h > > @@ -220,7 +220,7 @@ extern int target_page_bits; > > > > #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) > > #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) > > -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & > > TARGET_PAGE_MASK) > > +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) > > > > /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even > > * when intptr_t is 32-bit and we are aligning a long long. > > @@ -228,9 +228,8 @@ extern int target_page_bits; > > extern uintptr_t qemu_host_page_size; > > extern intptr_t qemu_host_page_mask; > > > > -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & > > qemu_host_page_mask) > > -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - > > 1) & \ > > -qemu_real_host_page_mask) > > +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) > > +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), > > qemu_real_host_page_size) > > > No, please. > > (1) The compiler does not know that qemu_*host_page_size is a power of 2, and > will generate a real division at runtime. The same is true for > TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. Ouch, good point, I didn't think of that when I gave an R-b. > (2) The first hunk conflicts with an in-flight patch of mine: > > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html > > > r~ > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 10:11:44AM +0800, Wei Yang wrote: > Use ROUND_UP() to define, which is a little bit easy to read. > > Signed-off-by: Wei Yang Reviewed-by: David Gibson > --- > include/exec/cpu-all.h | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index ad9ab85eb3..255bb186ac 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -220,7 +220,7 @@ extern int target_page_bits; > > #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) > #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) > -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & > TARGET_PAGE_MASK) > +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) > > /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even > * when intptr_t is 32-bit and we are aligning a long long. > @@ -228,9 +228,8 @@ extern int target_page_bits; > extern uintptr_t qemu_host_page_size; > extern intptr_t qemu_host_page_mask; > > -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & > qemu_host_page_mask) > -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) > & \ > -qemu_real_host_page_mask) > +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) > +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size) > > /* same as PROT_xxx */ > #define PAGE_READ 0x0001 -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On Sun, Oct 13, 2019 at 10:11:45AM +0800, Wei Yang wrote: > There are three page size in qemu: > > real host page size > host page size > target page size > > All of them have dedicate variable to represent. For the last two, we > use the same form in the whole qemu project, while for the first one we > use two forms: qemu_real_host_page_size and getpagesize(). > > qemu_real_host_page_size is defined to be a replacement of > getpagesize(), so let it serve the role. > > [Note] Not fully tested for some arch or device. > > Signed-off-by: Wei Yang Reviewed-by: David Gibson Although the chances of someone messing this up again are almost 100%. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v4 01/10] add device_legacy_reset function to prepare for reset api change
On Wed, Aug 21, 2019 at 06:33:32PM +0200, Damien Hedde wrote:
> Provide a temporary device_legacy_reset function doing what
> device_reset does to prepare for the transition with Resettable
> API.
>
> All occurrence of device_reset in the code tree are also replaced
> by device_legacy_reset.
>
> The new resettable API has different prototype and semantics
> (resetting child buses as well as the specified device). Subsequent
> commits will make the changeover for each call site individually; once
> that is complete device_legacy_reset() will be removed.
>
> Signed-off-by: Damien Hedde
> Reviewed-by: Peter Maydell
ppc parts
Acked-by: David Gibson
> ---
> Cc: Gerd Hoffmann
> Cc: Paolo Bonzini
> Cc: "Daniel P. Berrangé"
> Cc: Eduardo Habkost
> Cc: Richard Henderson
> Cc: "Michael S. Tsirkin"
> Cc: Marcel Apfelbaum
> Cc: John Snow
> Cc: "Cédric Le Goater"
> Cc: Collin Walling
> Cc: Cornelia Huck
> Cc: David Hildenbrand
> Cc: Halil Pasic
> Cc: Christian Borntraeger
> Cc: Dmitry Fleytman
> Cc: Fam Zheng
> Cc: qemu-block@nongnu.org
> Cc: qemu-...@nongnu.org
> Cc: qemu-s3...@nongnu.org
> Cc: qemu-...@nongnu.org
> ---
> hw/audio/intel-hda.c | 2 +-
> hw/core/qdev.c | 6 +++---
> hw/hyperv/hyperv.c | 2 +-
> hw/i386/pc.c | 2 +-
> hw/ide/microdrive.c | 8
> hw/intc/spapr_xive.c | 2 +-
> hw/ppc/pnv_psi.c | 2 +-
> hw/ppc/spapr_pci.c | 2 +-
> hw/ppc/spapr_vio.c | 2 +-
> hw/s390x/s390-pci-inst.c | 2 +-
> hw/scsi/vmw_pvscsi.c | 2 +-
> hw/sd/omap_mmc.c | 2 +-
> hw/sd/pl181.c| 2 +-
> include/hw/qdev-core.h | 4 ++--
> 14 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
> index 6ecd383540..27b71c57cf 100644
> --- a/hw/audio/intel-hda.c
> +++ b/hw/audio/intel-hda.c
> @@ -1087,7 +1087,7 @@ static void intel_hda_reset(DeviceState *dev)
> QTAILQ_FOREACH(kid, >codecs.qbus.children, sibling) {
> DeviceState *qdev = kid->child;
> cdev = HDA_CODEC_DEVICE(qdev);
> -device_reset(DEVICE(cdev));
> +device_legacy_reset(DEVICE(cdev));
> d->state_sts |= (1 << cdev->cad);
> }
> intel_hda_update_irq(d);
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 60d66c2f39..a95d4fa87d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -257,7 +257,7 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>
> static int qdev_reset_one(DeviceState *dev, void *opaque)
> {
> -device_reset(dev);
> +device_legacy_reset(dev);
>
> return 0;
> }
> @@ -865,7 +865,7 @@ static void device_set_realized(Object *obj, bool value,
> Error **errp)
> }
> }
> if (dev->hotplugged) {
> -device_reset(dev);
> +device_legacy_reset(dev);
> }
> dev->pending_deleted_event = false;
>
> @@ -1087,7 +1087,7 @@ void device_class_set_parent_unrealize(DeviceClass *dc,
> dc->unrealize = dev_unrealize;
> }
>
> -void device_reset(DeviceState *dev)
> +void device_legacy_reset(DeviceState *dev)
> {
> DeviceClass *klass = DEVICE_GET_CLASS(dev);
>
> diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
> index 6ebf31c310..cd9db3cb5c 100644
> --- a/hw/hyperv/hyperv.c
> +++ b/hw/hyperv/hyperv.c
> @@ -140,7 +140,7 @@ void hyperv_synic_reset(CPUState *cs)
> SynICState *synic = get_synic(cs);
>
> if (synic) {
> -device_reset(DEVICE(synic));
> +device_legacy_reset(DEVICE(synic));
> }
> }
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 3ab4bcb3ca..f33a8de42f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2826,7 +2826,7 @@ static void pc_machine_reset(MachineState *machine)
> cpu = X86_CPU(cs);
>
> if (cpu->apic_state) {
> -device_reset(cpu->apic_state);
> +device_legacy_reset(cpu->apic_state);
> }
> }
> }
> diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
> index b0272ea14b..6b30e36ed8 100644
> --- a/hw/ide/microdrive.c
> +++ b/hw/ide/microdrive.c
> @@ -173,7 +173,7 @@ static void md_attr_write(PCMCIACardState *card, uint32_t
> at, uint8_t value)
> case 0x00: /* Configuration Option Register */
> s->opt = value & 0xcf;
> if (value & OPT_SRESET) {
> -device_reset(DEVICE(s));
> +device_legacy_reset(DEVICE(s));
> }
> md_interrupt_update(s);
> break;
> @@ -316
Re: [Qemu-block] [Qemu-devel] [PATCH v7 00/42] Invert Endian bit in SPARCv9 MMU TTE
On Fri, Aug 16, 2019 at 11:58:05AM +0200, Philippe Mathieu-Daudé wrote: > Hi Tony, > > On 8/16/19 8:28 AM, tony.ngu...@bt.com wrote: > > This patchset implements the IE (Invert Endian) bit in SPARCv9 MMU TTE. > > > > v7: > [...] > > - Re-declared many native endian devices as little or big endian. This is > > why > > v7 has +16 patches. > > Why are you doing that? What is the rational? > > Anyhow if this not required by your series, you should split it out of > it, and send it on your principal changes are merged. > I'm worried because this these new patches involve many subsystems (thus > maintainers) and reviewing them will now take a fair amount of time. > > > For each device declared with DEVICE_NATIVE_ENDIAN, find the set of > > targets from the set of target/hw/*/device.o. > > > > If the set of targets are all little or all big endian, re-declare > > the device endianness as DEVICE_LITTLE_ENDIAN or DEVICE_BIG_ENDIAN > > respectively. > > If only little endian targets use a device, that doesn't mean the device > is designed in little endian... > > Then if a big endian target plan to use this device, it will require > more work and you might have introduced regressions... Uh.. only if they make the version of the device on a big endian target big endian. Which is a terrible idea - if you know a hardware designer planning to do this, please slap them. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus
On Fri, Aug 09, 2019 at 12:08:43PM +0100, Peter Maydell wrote:
> On Fri, 9 Aug 2019 at 01:10, David Gibson wrote:
> >
> > On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 7/31/19 11:29 AM, Damien Hedde wrote:
> > > > On 7/31/19 8:05 AM, David Gibson wrote:
> > > >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
> > > >>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool
> > > >>> value, Error **errp)
> > > >>> }
> > > >>> }
> > > >>> if (dev->hotplugged) {
> > > >>> -device_legacy_reset(dev);
> > > >>> +device_reset(dev, true);
> > > >>
> > > >> So.. is this change in the device_reset() signature really necessary?
> > > >> Even if there are compelling reasons to handle warm reset in the new
> > > >> API, that doesn't been you need to change device_reset() itself from
> > > >> its established meaning of a cold (i.e. as per power cycle) reset.
>
> So, I don't think that actually is the established meaning of
> device_reset(). I think our current semantics for this function are
> "reset of some sort, probably cold, but in practice called in
> lots of places which really wanted some other kind of reset,
> because our current reset semantics are not well-defined".
>
> For instance in s390-pci-inst.c the handling of CLP_SET_DISABLE_PCI_FN
> calls device_reset() on a device. I bet that's not really intentionally
> trying to model "we powered it off and on again".
> hw/scsi/vmw_pvscsi.c uses device_reset() in its handling of
> the guest "reset the SCSI bus" command. I know that isn't literally
> powering off the SCSI disks and powering them on again.
>
> The advantage of an actual API change here is that it means we go
> and look at all the call sites and find out what the semantics
> they actually wanted were, and hopefully that then feeds into the
> design of the new API and we make sure we can handle those
> semantics in a non-confused way.
That's a fair point.
> > > >> Warm resets are generally called in rather more specific circumstances
> > > >> (often under guest software direction) so it seems likely that users
> > > >> would want to engage with the new reset API directly. Or we could
> > > >> just create a device_warm_reset() wrapper. That would also avoid the
> > > >> bare boolean parameter, which is not great for readability (you have
> > > >> to look up the signature to have any idea what it means).
> > >
> > > If the boolean is not meaningful, we can use an enum...
> >
> > That's certainly better, but I'm not seeing a compelling reason not to
> > have separate function names. It's just as clear and means less churn.
>
> One advantage of an enum is that we have an extendable API,
> so we could say something like "all devices support reset types
> 'cold' and 'warm', but individual devices or families of devices
> can also support other kinds of reset". So the relevant s390
> devices could directly support the kinds of reset currently
> enumerated by the enum s390_reset, and then if you know you're
> dealing with an s390 device you can ask it to reset with the
> right semantics rather than fudging it with one of the generic ones.
> Or devices with "if I'm reset by the guest writing to a register then
> I reset less stuff than a reset via external reset line" have a
> way to model that.
Ok, I can see the value in that. I guess the way I'd prefer to
approach it though, is to start out with just a single-value enum for
(roughly) a cold reset. Then when we find a subset of devices for
which we can consistently define a warm reset of some type, we add an
enum value for it.
I guess we'd also need some way of introspecting what reset types are
supported by a device.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v3 05/33] Switch to new api in qdev/bus
On Fri, Aug 09, 2019 at 01:39:46PM +0200, Cédric Le Goater wrote: > > >>> So.. is this change in the device_reset() signature really necessary? > >>> Even if there are compelling reasons to handle warm reset in the new > >>> API, that doesn't been you need to change device_reset() itself from > >>> its established meaning of a cold (i.e. as per power cycle) reset. > >>> Warm resets are generally called in rather more specific circumstances > >>> (often under guest software direction) so it seems likely that users > >>> would want to engage with the new reset API directly. Or we could > >>> just create a device_warm_reset() wrapper. That would also avoid the > >>> bare boolean parameter, which is not great for readability (you have > >>> to look up the signature to have any idea what it means). > >> > >> I've added device_reset_cold/warm wrapper functions to avoid having to > >> pass the boolean parameter. it seems I forgot to use them in qdev.c > >> I suppose, like you said, we could live with > >> + no function with the boolean parameter > >> + device_reset doing cold reset > >> + device_reset_warm (or device_warm_reset) for the warm version > > > > Ok, good. > > > > I'm afraid the whole series still makes me pretty uncomfortable, > > though, since the whole "warm reset" concept still seems way to vague > > to me. > > Isn't the reset after the CAS negotiation sequence between the hypervisor > and the pseries machine some sort of warm reset driven by SW ? Yes.. and? The fact that something as messy as CAS can come under the category of warm reset only re-inforces that what a warm reset is isn't really well defined. [That said, in the case of CAS, I'd really like it if we can change things to avoid the pseudo-reset and just rewrite the dt instead. The sorta-reboot causes us problems with -no-reboot and with disabling the SLOF autoboot flag] -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable
On Wed, Aug 07, 2019 at 09:55:13AM +0200, Damien Hedde wrote:
>
>
> On 8/6/19 2:35 AM, David Gibson wrote:
> > On Wed, Jul 31, 2019 at 11:09:05AM +0200, Damien Hedde wrote:
> >>
> >>
> >> On 7/31/19 7:56 AM, David Gibson wrote:
> >>> On Mon, Jul 29, 2019 at 04:56:25PM +0200, Damien Hedde wrote:
> >>>> This add Resettable interface implementation for both Bus and Device.
> >>>>
> >>>> *resetting* counter and *reset_is_cold* flag are added in DeviceState
> >>>> and BusState.
> >>>>
> >>>> Compatibility with existing code base is ensured.
> >>>> The legacy bus or device reset method is called in the new exit phase
> >>>> and the other 2 phases are let empty. Using the exit phase guarantee that
> >>>> legacy resets are called in the "post" order (ie: children then parent)
> >>>> in hierarchical reset. That is the same order as legacy qdev_reset_all
> >>>> or qbus_reset_all were using.
> >>>>
> >>>> New *device_reset* and *bus_reset* function are proposed with an
> >>>> additional boolean argument telling whether the reset is cold or warm.
> >>>> Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
> >>>> are defined also as helpers.
> >>>>
> >>>> Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
> >>>> functions telling respectively whether the object is currently under
> >>>> reset and
> >>>> if the current reset is cold or not.
> >>>>
> >>>> Signed-off-by: Damien Hedde
> >>>> ---
> >>>> hw/core/bus.c | 85 ++
> >>>> hw/core/qdev.c | 82
> >>>> include/hw/qdev-core.h | 84 ++---
> >>>> tests/Makefile.include | 1 +
> >>>> 4 files changed, 247 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/hw/core/bus.c b/hw/core/bus.c
> >>>> index 17bc1edcde..08a97addb6 100644
> >>>> --- a/hw/core/bus.c
> >>>> +++ b/hw/core/bus.c
> >>>> @@ -22,6 +22,7 @@
> >>>> #include "qemu/module.h"
> >>>> #include "hw/qdev.h"
> >>>> #include "qapi/error.h"
> >>>> +#include "hw/resettable.h"
> >>>>
> >>>> void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error
> >>>> **errp)
> >>>> {
> >>>> @@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +void bus_reset(BusState *bus, bool cold)
> >>>> +{
> >>>> +resettable_reset(OBJECT(bus), cold);
> >>>> +}
> >>>> +
> >>>> +bool bus_is_resetting(BusState *bus)
> >>>> +{
> >>>> +return (bus->resetting != 0);
> >>>> +}
> >>>> +
> >>>> +bool bus_is_reset_cold(BusState *bus)
> >>>> +{
> >>>> +return bus->reset_is_cold;
> >>>> +}
> >>>> +
> >>>> +static uint32_t bus_get_reset_count(Object *obj)
> >>>> +{
> >>>> +BusState *bus = BUS(obj);
> >>>> +return bus->resetting;
> >>>> +}
> >>>> +
> >>>> +static uint32_t bus_increment_reset_count(Object *obj)
> >>>> +{
> >>>> +BusState *bus = BUS(obj);
> >>>> +return ++bus->resetting;
> >>>> +}
> >>>> +
> >>>> +static uint32_t bus_decrement_reset_count(Object *obj)
> >>>> +{
> >>>> +BusState *bus = BUS(obj);
> >>>> +return --bus->resetting;
> >>>> +}
> >>>> +
> >>>> +static bool bus_set_reset_cold(Object *obj, bool cold)
> >>>> +{
> >>>> +BusState *bus = BUS(obj);
> >>>> +bool old = bus->reset_is_cold;
> >>>> +bus->reset_is_cold = cold;
> >>>> +return old;
> >>>> +}
> >>>> +
> >>>> +static bool bus_set_hold_needed(Object *obj, bool hold_needed)
> >>>> +{
> >>>> +BusState *bus = BUS(obj);
> &
Re: [Qemu-block] [Qemu-devel] [PATCH v3 01/33] Create Resettable QOM interface
On Thu, Aug 01, 2019 at 11:35:20AM +0200, Damien Hedde wrote: > > > On 7/31/19 7:46 AM, David Gibson wrote: > > On Tue, Jul 30, 2019 at 04:08:59PM +0200, Damien Hedde wrote: > >> > >> On 7/30/19 3:59 PM, Peter Maydell wrote: > >>> On Tue, 30 Jul 2019 at 14:56, Cornelia Huck wrote: > >>>> > >>>> On Tue, 30 Jul 2019 14:44:21 +0100 > >>>> Peter Maydell wrote: > >>>> > >>>>> On Tue, 30 Jul 2019 at 14:42, Cornelia Huck wrote: > >>>>>> I'm having a hard time figuring out what a 'cold' or a 'warm' reset is > >>>>>> supposed to be... can you add a definition/guideline somewhere? > >>>>> > >>>>> Generally "cold" reset is "power on" and "warm" is "we were already > >>>>> powered-on, but somebody flipped a reset line somewhere". > >>>> > >>>> Ok, that makes sense... my main concern is to distinguish that in a > >>>> generic way, as it is a generic interface. What about adding something > >>>> like: > >>>> > >>>> "A 'cold' reset means that the object to be reset is initially reset; a > >>>> 'warm' > >>>> reset means that the object to be reset has already been initialized." > >>>> > >>>> Or is that again too generic? > >>> > >>> I think it doesn't quite capture the idea -- an object can have already > >>> been reset and then get a 'cold' reset: this is like having a powered-on > >>> machine and then power-cycling it. > >>> > >>> The 'warm' reset is the vaguer one, because the specific behaviour > >>> is somewhat device-dependent (many devices might not have any > >>> difference from 'cold' reset, for those that do the exact detail > >>> of what doesn't get reset on warm-reset will vary). But every > >>> device should have some kind of "as if you power-cycled it" (or > >>> for QEMU, "go back to the same state as if you just started QEMU on the > >>> command line"). Our current "reset" method is really cold-reset. > >>> > >> > >> Exactly. In the following patches, I've tried to replace existing reset > >> calls by cold or warm reset depending on whether: > >> + it is called through the main system reset -> cold > >> + it is called during normal life-time -> warm > >> > >> But I definitely can add some docs/comments to better explain that. > > > > Hrm, that helps, but it still seems pretty vague to me. > > > > It's not really my call, but building the concept of warm versus cold > > resets into such a generic interface seems pretty dubios to me. While > > it's moderately common for things to have a notion of warm versus cold > > reset it's certainly not universal. There are many devices where > > there's no meaningful difference between the two. There are some > > devices where there are > 2 different types of reset suitable for > > various different situations. > > > > Is there any way this could work with it usually just presenting the > > cold reset (which is the closest to a universal concept), and any warm > > or additional resets are handled buy a different instance of the > > Resettable interface? > > > > In my current implementation, cold/warm is only a question of setting a > flag before calling the reset methods. I rely and the reset methods to > check that flag if necessary. The feature can be added/removed without > pain (if we stick with device_reset to do a cold one). So if it's > better, I can put it aside for now. > > IMO handling warm reset with another interface is probably not a good > idea because it will need another load of methods. I was thinking of a different instance of the same interface, not a new interface. But on consideration that probably means dummy objects so that's also ugly. Here's another way to look at things though: I can see why a common interface for cold resets is useful; we use it during system resets and bus resets and so forth. But AIUI, in order to do a warm reset whatever is initiating it is going to need to know what a warm reset means for this device - in which case it doesn't really need a uniform interface for it. So, would we be better off with helper functions to make implementing a multi-phase reset easy for devices that do have one or more notions of a warm reset. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v3 09/33] add doc about Resettable interface
On Wed, Aug 07, 2019 at 05:01:42PM +0100, Peter Maydell wrote: > On Wed, 31 Jul 2019 at 07:33, David Gibson > wrote: > > > > On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote: > > > Signed-off-by: Damien Hedde > > > +For Devices and Buses there is also the corresponding helpers: > > > +void device_reset(Device *dev, bool cold) > > > +void bus_reset(Device *dev, bool cold) > > > > What's the semantic difference between resetting a bus and resetting > > the bridge device which owns it? > > We should definitely explain this in the documentation, but > consider for instance a SCSI controller. Resetting the > SCSI controller puts all its registers back into whatever > the reset state is for the device, as well as resetting > everything on the SCSI bus. Resetting just the SCSI bus > resets the disks and so on on the bus, but doesn't change > the state of the controller itself, which remains programmed > with whatever state the guest has set up. > > PCI has a similar distinction between resetting the controller > and resetting the bus. > > Note that we have this distinction in the current APIs too: > qbus_reset_all() vs qdev_reset_all(). Yeah, sorry, I didn't express my concern very well... and now I've kind of forgotten the details of it. I think the oddities you also pointed out with the state saving made me think the two were sorta equivalent in this patchset, but the interface suggested otherwise. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs
On Fri, Aug 09, 2019 at 10:45:43AM +0200, Damien Hedde wrote: > > > On 8/9/19 7:51 AM, David Gibson wrote: > > On Wed, Aug 07, 2019 at 11:37:51AM +0100, Peter Maydell wrote: > >> On Wed, 31 Jul 2019 at 07:33, David Gibson > >> wrote: > >>> > >>> On Mon, Jul 29, 2019 at 04:56:29PM +0200, Damien Hedde wrote: > >>>> It adds the possibility to add 2 gpios to control the warm and cold > >>>> reset. > >>>> With theses ios, the reset can be maintained during some time. > >>>> Each io is associated with a state to detect level changes. > >>>> > >>>> Vmstate subsections are also added to the existsing device_reset > >>>> subsection. > >>> > >>> This doesn't seem like a thing that should be present on every single > >>> DeviceState. > >> > >> It's a facility that's going to be useful to multiple different > >> subclasses of DeviceState, so it seems to me cleaner to > >> have base class support for the common feature rather than > >> to reimplement it entirely from scratch in every subclass > >> that wants it. > > > > Hm, I suppose so. Would it really have to be from scratch, though? > > Couldn't some suitable helper functions make adding such GPIOs to a > > device pretty straightforward? > > > > This patch does that. A device does have to use the helper to add the > gpio. Either qdev_init_warm_reset_gpio(...) or > qdev_init_cold_reset_gpio(...) , like any another gpio. Ah, sorry, I misunderstood. That seems ok then. > > The mechanics to control the reset with gpio change is done in the base > class and there is some state pre-allocated (and associated vmstate > description) to it. > > If that's a problem I can only provide helpers and let devices handle > state allocation and vmstate addition. > > Damien > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs
On Wed, Aug 07, 2019 at 11:37:51AM +0100, Peter Maydell wrote: > On Wed, 31 Jul 2019 at 07:33, David Gibson > wrote: > > > > On Mon, Jul 29, 2019 at 04:56:29PM +0200, Damien Hedde wrote: > > > It adds the possibility to add 2 gpios to control the warm and cold reset. > > > With theses ios, the reset can be maintained during some time. > > > Each io is associated with a state to detect level changes. > > > > > > Vmstate subsections are also added to the existsing device_reset > > > subsection. > > > > This doesn't seem like a thing that should be present on every single > > DeviceState. > > It's a facility that's going to be useful to multiple different > subclasses of DeviceState, so it seems to me cleaner to > have base class support for the common feature rather than > to reimplement it entirely from scratch in every subclass > that wants it. Hm, I suppose so. Would it really have to be from scratch, though? Couldn't some suitable helper functions make adding such GPIOs to a device pretty straightforward? -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v3 05/33] Switch to new api in qdev/bus
On Wed, Jul 31, 2019 at 11:29:36AM +0200, Damien Hedde wrote:
>
>
> On 7/31/19 8:05 AM, David Gibson wrote:
> > On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
> >> Deprecate old reset apis and make them use the new one while they
> >> are still used somewhere.
> >>
> >> Signed-off-by: Damien Hedde
> >> ---
> >> hw/core/qdev.c | 22 +++---
> >> include/hw/qdev-core.h | 28 ++--
> >> 2 files changed, 25 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index 559ced070d..e9e5f2d5f9 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj,
> >> void (*func)(Object *))
> >> }
> >> }
> >>
> >> -static int qdev_reset_one(DeviceState *dev, void *opaque)
> >> -{
> >> -device_legacy_reset(dev);
> >> -
> >> -return 0;
> >> -}
> >> -
> >> -static int qbus_reset_one(BusState *bus, void *opaque)
> >> -{
> >> -BusClass *bc = BUS_GET_CLASS(bus);
> >> -if (bc->reset) {
> >> -bc->reset(bus);
> >> -}
> >> -return 0;
> >> -}
> >> -
> >> void qdev_reset_all(DeviceState *dev)
> >> {
> >> -qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one,
> >> NULL);
> >> +device_reset(dev, false);
> >> }
> >>
> >> void qdev_reset_all_fn(void *opaque)
> >> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
> >>
> >> void qbus_reset_all(BusState *bus)
> >> {
> >> -qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one,
> >> NULL);
> >> +bus_reset(bus, false);
> >> }
> >>
> >> void qbus_reset_all_fn(void *opaque)
> >> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool
> >> value, Error **errp)
> >> }
> >> }
> >> if (dev->hotplugged) {
> >> -device_legacy_reset(dev);
> >> +device_reset(dev, true);
> >
> > So.. is this change in the device_reset() signature really necessary?
> > Even if there are compelling reasons to handle warm reset in the new
> > API, that doesn't been you need to change device_reset() itself from
> > its established meaning of a cold (i.e. as per power cycle) reset.
> > Warm resets are generally called in rather more specific circumstances
> > (often under guest software direction) so it seems likely that users
> > would want to engage with the new reset API directly. Or we could
> > just create a device_warm_reset() wrapper. That would also avoid the
> > bare boolean parameter, which is not great for readability (you have
> > to look up the signature to have any idea what it means).
>
> I've added device_reset_cold/warm wrapper functions to avoid having to
> pass the boolean parameter. it seems I forgot to use them in qdev.c
> I suppose, like you said, we could live with
> + no function with the boolean parameter
> + device_reset doing cold reset
> + device_reset_warm (or device_warm_reset) for the warm version
Ok, good.
I'm afraid the whole series still makes me pretty uncomfortable,
though, since the whole "warm reset" concept still seems way to vague
to me.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v3 09/33] add doc about Resettable interface
On Wed, Aug 07, 2019 at 11:34:41AM +0100, Peter Maydell wrote: > On Wed, 31 Jul 2019 at 07:33, David Gibson > wrote: > > > > On Mon, Jul 29, 2019 at 04:56:30PM +0200, Damien Hedde wrote: > > > +The function *resettable_reset* is used to trigger a reset on a given > > > +object. > > > +void resettable_reset(Object *obj, bool cold) > > > + > > > +The parameter *obj* must implement the Resettable interface. > > > > And what happens if it doesn't? This function has no way to report an > > error. > > Trying to reset an object that isn't actually resettable would > be a programming error -- I think asserting is a reasonable > response to that. Yeah, fair enough. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3 05/33] Switch to new api in qdev/bus
On Wed, Jul 31, 2019 at 01:31:28PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/31/19 11:29 AM, Damien Hedde wrote:
> > On 7/31/19 8:05 AM, David Gibson wrote:
> >> On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
> >>> Deprecate old reset apis and make them use the new one while they
> >>> are still used somewhere.
> >>>
> >>> Signed-off-by: Damien Hedde
> >>> ---
> >>> hw/core/qdev.c | 22 +++---
> >>> include/hw/qdev-core.h | 28 ++--
> >>> 2 files changed, 25 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>> index 559ced070d..e9e5f2d5f9 100644
> >>> --- a/hw/core/qdev.c
> >>> +++ b/hw/core/qdev.c
> >>> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj,
> >>> void (*func)(Object *))
> >>> }
> >>> }
> >>>
> >>> -static int qdev_reset_one(DeviceState *dev, void *opaque)
> >>> -{
> >>> -device_legacy_reset(dev);
> >>> -
> >>> -return 0;
> >>> -}
> >>> -
> >>> -static int qbus_reset_one(BusState *bus, void *opaque)
> >>> -{
> >>> -BusClass *bc = BUS_GET_CLASS(bus);
> >>> -if (bc->reset) {
> >>> -bc->reset(bus);
> >>> -}
> >>> -return 0;
> >>> -}
> >>> -
> >>> void qdev_reset_all(DeviceState *dev)
> >>> {
> >>> -qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one,
> >>> NULL);
> >>> +device_reset(dev, false);
> >>> }
> >>>
> >>> void qdev_reset_all_fn(void *opaque)
> >>> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
> >>>
> >>> void qbus_reset_all(BusState *bus)
> >>> {
> >>> -qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one,
> >>> NULL);
> >>> +bus_reset(bus, false);
> >>> }
> >>>
> >>> void qbus_reset_all_fn(void *opaque)
> >>> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool
> >>> value, Error **errp)
> >>> }
> >>> }
> >>> if (dev->hotplugged) {
> >>> -device_legacy_reset(dev);
> >>> +device_reset(dev, true);
> >>
> >> So.. is this change in the device_reset() signature really necessary?
> >> Even if there are compelling reasons to handle warm reset in the new
> >> API, that doesn't been you need to change device_reset() itself from
> >> its established meaning of a cold (i.e. as per power cycle) reset.
> >> Warm resets are generally called in rather more specific circumstances
> >> (often under guest software direction) so it seems likely that users
> >> would want to engage with the new reset API directly. Or we could
> >> just create a device_warm_reset() wrapper. That would also avoid the
> >> bare boolean parameter, which is not great for readability (you have
> >> to look up the signature to have any idea what it means).
>
> If the boolean is not meaningful, we can use an enum...
That's certainly better, but I'm not seeing a compelling reason not to
have separate function names. It's just as clear and means less churn.
>
> > I've added device_reset_cold/warm wrapper functions to avoid having to
> > pass the boolean parameter. it seems I forgot to use them in qdev.c
> > I suppose, like you said, we could live with
> > + no function with the boolean parameter
> > + device_reset doing cold reset
> > + device_reset_warm (or device_warm_reset) for the warm version
> >
> > Damien
> >
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable
On Wed, Jul 31, 2019 at 11:09:05AM +0200, Damien Hedde wrote:
>
>
> On 7/31/19 7:56 AM, David Gibson wrote:
> > On Mon, Jul 29, 2019 at 04:56:25PM +0200, Damien Hedde wrote:
> >> This add Resettable interface implementation for both Bus and Device.
> >>
> >> *resetting* counter and *reset_is_cold* flag are added in DeviceState
> >> and BusState.
> >>
> >> Compatibility with existing code base is ensured.
> >> The legacy bus or device reset method is called in the new exit phase
> >> and the other 2 phases are let empty. Using the exit phase guarantee that
> >> legacy resets are called in the "post" order (ie: children then parent)
> >> in hierarchical reset. That is the same order as legacy qdev_reset_all
> >> or qbus_reset_all were using.
> >>
> >> New *device_reset* and *bus_reset* function are proposed with an
> >> additional boolean argument telling whether the reset is cold or warm.
> >> Helper functions *device_reset_[warm|cold]* and *bus_reset_[warm|cold]*
> >> are defined also as helpers.
> >>
> >> Also add a [device|bus]_is_resetting and [device|bus]_is_reset_cold
> >> functions telling respectively whether the object is currently under reset
> >> and
> >> if the current reset is cold or not.
> >>
> >> Signed-off-by: Damien Hedde
> >> ---
> >> hw/core/bus.c | 85 ++
> >> hw/core/qdev.c | 82
> >> include/hw/qdev-core.h | 84 ++---
> >> tests/Makefile.include | 1 +
> >> 4 files changed, 247 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/core/bus.c b/hw/core/bus.c
> >> index 17bc1edcde..08a97addb6 100644
> >> --- a/hw/core/bus.c
> >> +++ b/hw/core/bus.c
> >> @@ -22,6 +22,7 @@
> >> #include "qemu/module.h"
> >> #include "hw/qdev.h"
> >> #include "qapi/error.h"
> >> +#include "hw/resettable.h"
> >>
> >> void qbus_set_hotplug_handler(BusState *bus, Object *handler, Error
> >> **errp)
> >> {
> >> @@ -68,6 +69,75 @@ int qbus_walk_children(BusState *bus,
> >> return 0;
> >> }
> >>
> >> +void bus_reset(BusState *bus, bool cold)
> >> +{
> >> +resettable_reset(OBJECT(bus), cold);
> >> +}
> >> +
> >> +bool bus_is_resetting(BusState *bus)
> >> +{
> >> +return (bus->resetting != 0);
> >> +}
> >> +
> >> +bool bus_is_reset_cold(BusState *bus)
> >> +{
> >> +return bus->reset_is_cold;
> >> +}
> >> +
> >> +static uint32_t bus_get_reset_count(Object *obj)
> >> +{
> >> +BusState *bus = BUS(obj);
> >> +return bus->resetting;
> >> +}
> >> +
> >> +static uint32_t bus_increment_reset_count(Object *obj)
> >> +{
> >> +BusState *bus = BUS(obj);
> >> +return ++bus->resetting;
> >> +}
> >> +
> >> +static uint32_t bus_decrement_reset_count(Object *obj)
> >> +{
> >> +BusState *bus = BUS(obj);
> >> +return --bus->resetting;
> >> +}
> >> +
> >> +static bool bus_set_reset_cold(Object *obj, bool cold)
> >> +{
> >> +BusState *bus = BUS(obj);
> >> +bool old = bus->reset_is_cold;
> >> +bus->reset_is_cold = cold;
> >> +return old;
> >> +}
> >> +
> >> +static bool bus_set_hold_needed(Object *obj, bool hold_needed)
> >> +{
> >> +BusState *bus = BUS(obj);
> >> +bool old = bus->reset_hold_needed;
> >> +bus->reset_hold_needed = hold_needed;
> >> +return old;
> >> +}
> >> +
> >> +static void bus_foreach_reset_child(Object *obj, void (*func)(Object *))
> >> +{
> >> +BusState *bus = BUS(obj);
> >> +BusChild *kid;
> >> +
> >> +QTAILQ_FOREACH(kid, >children, sibling) {
> >> +func(OBJECT(kid->child));
> >> +}
> >> +}
> >
> > IIUC, every resettable class would need more or less identical
> > implementations of the above. That seems like an awful lot of
> > boilerplate.
>
> Do you mean the get/increment_count/decrement_count, set_cold/hold part ?
> True, but it's limited to the base classes.
> Since Resettable is an interface, we have no state there to store what
> we need. Only alternative is to have some kind of single
> get_resettable_state method returning a pointer to the state (allowing
> us to keep the functions in the interface code).
> Beyond Device and Bus, which are done here, there is probably not so
> many class candidates for the Resettable interface.
Right. I can't immediately see a better way to handle this, but it
still seems like a mild code smell.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v3 09/33] add doc about Resettable interface
ld,
> + ResettableExitPhase exit,
> +
> +For Devices and Buses, some helper exists to know if a device/bus is under
> +reset and what type of reset it is:
> +```
> +bool device_is_resetting(DeviceState *dev);
> +bool device_is_reset_cold(DeviceState *dev);
It's not really clear to me when *_is_reset_cold() would be useful.
> +bool bus_is_resetting(BusState *bus);
> +bool bus_is_reset_cold(BusState *bus);
> +```
> +
> +
> +Implementing the base Resettable behavior : Re-entrance, Hierarchy and
> Cold/Warm
> +
> +
> +There is five others methods in the interface to handle the base mechanics
> +of the Resettable interface. The methods should be implemented in object
> +base class. DeviceClass and BusClass implement them.
> +
> +```
> +typedef bool (*ResettableSetCold)(Object *obj, bool cold);
> +typedef bool (*ResettableSetHoldNeeded)(Object *obj, bool hold_needed);
> +typedef uint32_t (*ResettableGetCount)(Object *obj);
> +typedef uint32_t (*ResettableIncrementCount)(Object *obj);
> +typedef uint32_t (*ResettableDecrementCount)(Object *obj);
> +typedef void (*ResettableForeachChild)(Object *obj, void (*visitor)(Object
> *));
> +typedef struct ResettableClass {
> +InterfaceClass parent_class;
> +
> +[...]
> +
> +ResettableSetCold set_cold;
> +ResettableSetHoldNeeded set_hold_needed;
> +ResettableGetCount get_count;
> +ResettableIncrementCount increment_count;
> +ResettableDecrementCount decrement_count;
> +ResettableForeachChild foreach_child;
> +} ResettableClass;
> +```
> +
> +*set_cold* is used when entering reset, before calling the init phase, to
> +indicate the reset type.
> +
> +*set_hold_needed* is used to set/clear and retrieve an "hold_needed" flag.
> +This flag allows to omly execute the hold pahse when required.
> +
> +As stated above, several reset procedures can be concurrent on an object.
> +This is handled with the three methods *get_count*, *increment_count* and
> +*decrement_count*. An object is in reset state if the count is non-zero.
> +
> +The reset hierarchy is handled using the *foreach_child* method. This method
> +executes a given function on every reset "child".
> +
> +In DeviceClass and BusClass the base behavior is to mimic the legacy qdev
> +reset. Reset hierarchy follows the qdev/qbus tree.
> +
> +Reset control through GPIO
> +--
> +
> +For devices, two reset inputs can be added: one for the cold, one the warm
> +reset. This is done using the following function.
> +```
> +typedef enum DeviceResetActiveType {
> +DEVICE_RESET_ACTIVE_LOW,
> +DEVICE_RESET_ACTIVE_HIGH,
> +} DeviceResetActiveType;
> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
> + bool cold, DeviceResetActiveType type);
> +```
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v3 05/33] Switch to new api in qdev/bus
On Mon, Jul 29, 2019 at 04:56:26PM +0200, Damien Hedde wrote:
> Deprecate old reset apis and make them use the new one while they
> are still used somewhere.
>
> Signed-off-by: Damien Hedde
> ---
> hw/core/qdev.c | 22 +++---
> include/hw/qdev-core.h | 28 ++--
> 2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 559ced070d..e9e5f2d5f9 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -312,25 +312,9 @@ static void device_foreach_reset_child(Object *obj, void
> (*func)(Object *))
> }
> }
>
> -static int qdev_reset_one(DeviceState *dev, void *opaque)
> -{
> -device_legacy_reset(dev);
> -
> -return 0;
> -}
> -
> -static int qbus_reset_one(BusState *bus, void *opaque)
> -{
> -BusClass *bc = BUS_GET_CLASS(bus);
> -if (bc->reset) {
> -bc->reset(bus);
> -}
> -return 0;
> -}
> -
> void qdev_reset_all(DeviceState *dev)
> {
> -qdev_walk_children(dev, NULL, NULL, qdev_reset_one, qbus_reset_one,
> NULL);
> +device_reset(dev, false);
> }
>
> void qdev_reset_all_fn(void *opaque)
> @@ -340,7 +324,7 @@ void qdev_reset_all_fn(void *opaque)
>
> void qbus_reset_all(BusState *bus)
> {
> -qbus_walk_children(bus, NULL, NULL, qdev_reset_one, qbus_reset_one,
> NULL);
> +bus_reset(bus, false);
> }
>
> void qbus_reset_all_fn(void *opaque)
> @@ -922,7 +906,7 @@ static void device_set_realized(Object *obj, bool value,
> Error **errp)
> }
> }
> if (dev->hotplugged) {
> -device_legacy_reset(dev);
> +device_reset(dev, true);
So.. is this change in the device_reset() signature really necessary?
Even if there are compelling reasons to handle warm reset in the new
API, that doesn't been you need to change device_reset() itself from
its established meaning of a cold (i.e. as per power cycle) reset.
Warm resets are generally called in rather more specific circumstances
(often under guest software direction) so it seems likely that users
would want to engage with the new reset API directly. Or we could
just create a device_warm_reset() wrapper. That would also avoid the
bare boolean parameter, which is not great for readability (you have
to look up the signature to have any idea what it means).
> }
> dev->pending_deleted_event = false;
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index eeb75611c8..1670ae41bb 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -109,6 +109,11 @@ typedef struct DeviceClass {
> bool hotpluggable;
>
> /* callbacks */
> +/*
> + * Reset method here is deprecated and replaced by methods in the
> + * resettable class interface to implement a multi-phase reset.
> + * TODO: remove once every reset callback is unused
> + */
> DeviceReset reset;
> DeviceRealize realize;
> DeviceUnrealize unrealize;
> @@ -455,19 +460,22 @@ bool bus_is_resetting(BusState *bus);
> */
> bool bus_is_reset_cold(BusState *bus);
>
> -void qdev_reset_all(DeviceState *dev);
> -void qdev_reset_all_fn(void *opaque);
> -
> /**
> - * @qbus_reset_all:
> - * @bus: Bus to be reset.
> + * qbus/qdev_reset_all:
> + * @bus/dev: Bus/Device to be reset.
> *
> - * Reset @bus and perform a bus-level ("hard") reset of all devices connected
> + * Reset @bus/dev and perform a bus-level reset of all devices/buses
> connected
> * to it, including recursive processing of all buses below @bus itself. A
> * hard reset means that qbus_reset_all will reset all state of the device.
> * For PCI devices, for example, this will include the base address registers
> * or configuration space.
> + *
> + * Theses functions are deprecated, please use device/bus_reset or
> + * resettable_reset_* instead
> + * TODO: remove them when all occurence are removed
> */
> +void qdev_reset_all(DeviceState *dev);
> +void qdev_reset_all_fn(void *opaque);
> void qbus_reset_all(BusState *bus);
> void qbus_reset_all_fn(void *opaque);
>
> @@ -489,9 +497,17 @@ void qdev_machine_init(void);
> * device_legacy_reset:
> *
> * Reset a single device (by calling the reset method).
> + *
> + * This function is deprecated, please use device_reset() instead.
> + * TODO: remove the function when all occurences are removed.
> */
> void device_legacy_reset(DeviceState *dev);
>
> +/**
> + * device_class_set_parent_reset:
> + * TODO: remove the function when DeviceClass's reset method
> + * is not used anymore.
> + */
> void device
Re: [Qemu-block] [PATCH v3 04/33] make Device and Bus Resettable
quot;
>
> enum {
> DEV_NVECTORS_UNSPECIFIED = -1,
> @@ -132,6 +133,10 @@ struct NamedGPIOList {
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> + * @resetting: Indicates whether the device is under reset. Also
> + * used to count how many times reset has been initiated on the device.
> + * @reset_is_cold: If the device is under reset, indicates whether it is cold
> + * or warm.
> *
> * This structure should not be accessed directly. We declare it here
> * so that it can be embedded in individual device state structures.
> @@ -153,6 +158,9 @@ struct DeviceState {
> int num_child_bus;
> int instance_id_alias;
> int alias_required_for_version;
> +uint32_t resetting;
> +bool reset_is_cold;
> +bool reset_hold_needed;
> };
>
> struct DeviceListener {
> @@ -199,6 +207,10 @@ typedef struct BusChild {
> /**
> * BusState:
> * @hotplug_handler: link to a hotplug handler associated with bus.
> + * @resetting: Indicates whether the bus is under reset. Also
> + * used to count how many times reset has been initiated on the bus.
> + * @reset_is_cold: If the bus is under reset, indicates whether it is cold
> + * or warm.
> */
> struct BusState {
> Object obj;
> @@ -210,6 +222,9 @@ struct BusState {
> int num_children;
> QTAILQ_HEAD(, BusChild) children;
> QLIST_ENTRY(BusState) sibling;
> +uint32_t resetting;
> +bool reset_is_cold;
> +bool reset_hold_needed;
> };
>
> /**
> @@ -376,6 +391,70 @@ int qdev_walk_children(DeviceState *dev,
> qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
> void *opaque);
>
> +/**
> + * device_reset:
> + * Resets the device @dev, @cold tell whether to do a cold or warm reset.
> + * Uses the ressetable interface.
> + * Base behavior is to reset the device and its qdev/qbus subtree.
> + */
> +void device_reset(DeviceState *dev, bool cold);
> +
> +static inline void device_reset_warm(DeviceState *dev)
> +{
> +device_reset(dev, false);
> +}
> +
> +static inline void device_reset_cold(DeviceState *dev)
> +{
> +device_reset(dev, true);
> +}
> +
> +/**
> + * bus_reset:
> + * Resets the bus @bus, @cold tell whether to do a cold or warm reset.
> + * Uses the ressetable interface.
> + * Base behavior is to reset the bus and its qdev/qbus subtree.
> + */
> +void bus_reset(BusState *bus, bool cold);
> +
> +static inline void bus_reset_warm(BusState *bus)
> +{
> +bus_reset(bus, false);
> +}
> +
> +static inline void bus_reset_cold(BusState *bus)
> +{
> +bus_reset(bus, true);
> +}
> +
> +/**
> + * device_is_resetting:
> + * Tell whether the device @dev is currently under reset.
> + */
> +bool device_is_resetting(DeviceState *dev);
> +
> +/**
> + * device_is_reset_cold:
> + * Tell whether the device @dev is currently under reset cold or warm reset.
> + *
> + * Note: only valid when device_is_resetting returns true.
> + */
> +bool device_is_reset_cold(DeviceState *dev);
> +
> +/**
> + * bus_is_resetting:
> + * Tell whether the bus @bus is currently under reset.
> + */
> +bool bus_is_resetting(BusState *bus);
> +
> +/**
> + * bus_is_reset_cold:
> + * Tell whether the bus @bus is currently under reset cold or warm reset.
> + *
> + * Note: only valid when bus_is_resetting returns true.
> + */
> +bool bus_is_reset_cold(BusState *bus);
> +
> void qdev_reset_all(DeviceState *dev);
> void qdev_reset_all_fn(void *opaque);
>
> @@ -413,11 +492,6 @@ void qdev_machine_init(void);
> */
> void device_legacy_reset(DeviceState *dev);
>
> -static inline void device_reset(DeviceState *dev)
> -{
> -device_legacy_reset(dev);
> -}
> -
> void device_class_set_parent_reset(DeviceClass *dc,
> DeviceReset dev_reset,
> DeviceReset *parent_reset);
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index fd7fdb8658..1c0a5345b9 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -561,6 +561,7 @@ tests/fp/%:
>
> tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
> hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
> + hw/core/resettable.o \
> hw/core/bus.o \
> hw/core/irq.o \
> hw/core/fw-path-provider.o \
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v3 06/33] add the vmstate description for device reset state
On Mon, Jul 29, 2019 at 04:56:27PM +0200, Damien Hedde wrote:
> It contains the resetting counter and cold flag status.
>
> At this point, migration of bus reset related state (counter and cold/warm
> flag) is handled by parent device. This done using the post_load
> function in the vmsd subsection.
>
> This is last point allow to add an initial support of migration with part of
> qdev/qbus tree in reset state under the following condition:
> + time-lasting reset are asserted on Device only
>
> Note that if this condition is not respected, migration will succeed and
> no failure will occurs. The only impact is that the resetting counter
> of a bus may lower afer a migration.
>
> Signed-off-by: Damien Hedde
> ---
> hw/core/Makefile.objs | 1 +
> hw/core/qdev-vmstate.c | 45 ++
> 2 files changed, 46 insertions(+)
> create mode 100644 hw/core/qdev-vmstate.c
>
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index d9234aa98a..49e9be0228 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -4,6 +4,7 @@ common-obj-y += bus.o reset.o
> common-obj-y += resettable.o
> common-obj-$(CONFIG_SOFTMMU) += qdev-fw.o
> common-obj-$(CONFIG_SOFTMMU) += fw-path-provider.o
> +common-obj-$(CONFIG_SOFTMMU) += qdev-vmstate.o
> # irq.o needed for qdev GPIO handling:
> common-obj-y += irq.o
> common-obj-y += hotplug.o
> diff --git a/hw/core/qdev-vmstate.c b/hw/core/qdev-vmstate.c
> new file mode 100644
> index 00..07b010811f
> --- /dev/null
> +++ b/hw/core/qdev-vmstate.c
> @@ -0,0 +1,45 @@
> +/*
> + * Device vmstate
> + *
> + * Copyright (c) 2019 GreenSocs
> + *
> + * Authors:
> + * Damien Hedde
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev.h"
> +#include "migration/vmstate.h"
> +
> +static bool device_vmstate_reset_needed(void *opaque)
> +{
> +DeviceState *dev = (DeviceState *) opaque;
> +return dev->resetting != 0;
> +}
> +
> +static int device_vmstate_reset_post_load(void *opaque, int version_id)
> +{
> +DeviceState *dev = (DeviceState *) opaque;
> +BusState *bus;
> +QLIST_FOREACH(bus, >child_bus, sibling) {
> +bus->resetting = dev->resetting;
Having redundant copies of the resetting bit in the bridge and every
bus instance seems kind of bogus.
> +bus->reset_is_cold = dev->reset_is_cold;
> +}
> +return 0;
> +}
> +
> +const struct VMStateDescription device_vmstate_reset = {
> +.name = "device_reset",
> +.version_id = 0,
> +.minimum_version_id = 0,
> +.needed = device_vmstate_reset_needed,
> +.post_load = device_vmstate_reset_post_load,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(resetting, DeviceState),
> +VMSTATE_BOOL(reset_is_cold, DeviceState),
> +VMSTATE_END_OF_LIST()
> +},
> +};
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v3 08/33] Add function to control reset with gpio inputs
roperty_add_bool(obj, "realized",
> device_get_realized, device_set_realized, NULL);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 926d4bbcb1..f724ddc8f4 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -136,6 +136,23 @@ struct NamedGPIOList {
> QLIST_ENTRY(NamedGPIOList) node;
> };
>
> +typedef enum DeviceResetActiveType {
> +DEVICE_RESET_ACTIVE_LOW,
> +DEVICE_RESET_ACTIVE_HIGH,
> +} DeviceResetActiveType;
> +
> +/**
> + * DeviceResetInputState:
> + * @exists: tell if io exists
> + * @type: tell whether the io active low or high
> + * @state: true if reset is currently active
> + */
> +typedef struct DeviceResetInputState {
> +bool exists;
> +DeviceResetActiveType type;
> +bool state;
> +} DeviceResetInputState;
> +
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> @@ -143,6 +160,8 @@ struct NamedGPIOList {
> * used to count how many times reset has been initiated on the device.
> * @reset_is_cold: If the device is under reset, indicates whether it is cold
> * or warm.
> + * @cold_reset_input: state data for cold reset io
> + * @warm_reset_input: state data for warm reset io
> *
> * This structure should not be accessed directly. We declare it here
> * so that it can be embedded in individual device state structures.
> @@ -167,6 +186,8 @@ struct DeviceState {
> uint32_t resetting;
> bool reset_is_cold;
> bool reset_hold_needed;
> +DeviceResetInputState cold_reset_input;
> +DeviceResetInputState warm_reset_input;
> };
>
> struct DeviceListener {
> @@ -372,6 +393,42 @@ static inline void qdev_init_gpio_in_named(DeviceState
> *dev,
> void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
> const char *name);
>
> +/**
> + * qdev_init_reset_gpio_in_named:
> + * Create a gpio controlling the warm or cold reset of the device.
> + *
> + * @cold: specify whether it triggers cold or warm reset
> + * @type: what kind of reset io it is
> + *
> + * Note: the io is considered created in its inactive state. No reset
> + * is started by this function.
> + */
> +void qdev_init_reset_gpio_in_named(DeviceState *dev, const char *name,
> + bool cold, DeviceResetActiveType type);
> +
> +/**
> + * qdev_init_warm_reset_gpio:
> + * Create the input to control the device warm reset.
> + */
> +static inline void qdev_init_warm_reset_gpio(DeviceState *dev,
> + const char *name,
> + DeviceResetActiveType type)
> +{
> +qdev_init_reset_gpio_in_named(dev, name, false, type);
> +}
> +
> +/**
> + * qdev_init_cold_reset_gpio:
> + * Create the input to control the device cold reset.
> + * It can also be used as a power gate control.
> + */
> +static inline void qdev_init_cold_reset_gpio(DeviceState *dev,
> + const char *name,
> + DeviceResetActiveType type)
> +{
> +qdev_init_reset_gpio_in_named(dev, name, true, type);
> +}
> +
> BusState *qdev_get_parent_bus(DeviceState *dev);
>
> /*** BUS API. ***/
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v3 03/33] Replace all call to device_reset by call to device_legacy_reset
void
> *opaque)
> DeviceState *dev = (DeviceState *) object_dynamic_cast(child,
> TYPE_DEVICE);
>
> if (dev) {
> -device_reset(dev);
> +device_legacy_reset(dev);
> }
>
> return 0;
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 583c13deda..5a0b5cc35c 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -306,7 +306,7 @@ int spapr_vio_send_crq(SpaprVioDevice *dev, uint8_t *crq)
> static void spapr_vio_quiesce_one(SpaprVioDevice *dev)
> {
> if (dev->tcet) {
> -device_reset(DEVICE(dev->tcet));
> +device_legacy_reset(DEVICE(dev->tcet));
> }
> free_crq(dev);
> }
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 00235148be..93cda37c27 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -242,7 +242,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t
> ra)
> stw_p(>hdr.rsp, CLP_RC_SETPCIFN_FHOP);
> goto out;
> }
> -device_reset(DEVICE(pbdev));
> +device_legacy_reset(DEVICE(pbdev));
> pbdev->fh &= ~FH_MASK_ENABLE;
> pbdev->state = ZPCI_FS_DISABLED;
> stl_p(>fh, pbdev->fh);
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index 14641df1c8..cda3fc96a0 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -835,7 +835,7 @@ pvscsi_on_cmd_reset_device(PVSCSIState *s)
>
> if (sdev != NULL) {
> s->resetting++;
> -device_reset(>qdev);
> +device_legacy_reset(>qdev);
> s->resetting--;
> return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
> }
> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
> index d0c98ca021..24a1edc149 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -317,7 +317,7 @@ void omap_mmc_reset(struct omap_mmc_s *host)
> * into any bus, and we must reset it manually. When omap_mmc is
> * QOMified this must move into the QOM reset function.
> */
> -device_reset(DEVICE(host->card));
> +device_legacy_reset(DEVICE(host->card));
> }
>
> static uint64_t omap_mmc_read(void *opaque, hwaddr offset,
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index 81b406dbf0..15b4aaa67f 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -480,7 +480,7 @@ static void pl181_reset(DeviceState *d)
> /* Since we're still using the legacy SD API the card is not plugged
> * into any bus, and we must reset it manually.
> */
> -device_reset(DEVICE(s->card));
> +device_legacy_reset(DEVICE(s->card));
> }
>
> static void pl181_init(Object *obj)
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH-for-4.1? 7/7] spapr_events: Rewrite a fall through comment
On Fri, Jul 19, 2019 at 03:14:25PM +0200, Philippe Mathieu-Daudé wrote:
> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
>
> CC ppc64-softmmu/hw/ppc/spapr_rtc.o
> hw/ppc/spapr_events.c: In function ‘rtas_event_log_to_source’:
> hw/ppc/spapr_events.c:312:12: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
> 312 | if (spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT)) {
> |^
> hw/ppc/spapr_events.c:317:5: note: here
> 317 | case RTAS_LOG_TYPE_EPOW:
> | ^~~~
> cc1: all warnings being treated as errors
>
> Rewrite the comment using 'fall through' which is recognized by
> GCC and static analyzers.
>
> Reported-by: Stefan Weil
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> hw/ppc/spapr_events.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index ae0f093f59..0a98894ad6 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -313,7 +313,7 @@ rtas_event_log_to_source(SpaprMachineState *spapr, int
> log_type)
> g_assert(source->enabled);
> break;
> }
> -/* fall back to epow for legacy hotplug interrupt source */
> +/* fall through back to epow for legacy hotplug interrupt source */
> case RTAS_LOG_TYPE_EPOW:
> source = spapr_event_sources_get_source(spapr->event_sources,
> EVENT_CLASS_EPOW);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH-for-4.1? 5/7] target/ppc: Rewrite a fall through comment
On Fri, Jul 19, 2019 at 03:14:23PM +0200, Philippe Mathieu-Daudé wrote:
> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
>
> target/ppc/mmu_helper.c: In function ‘dump_mmu’:
> target/ppc/mmu_helper.c:1349:12: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
>1349 | if (ppc64_v3_radix(env_archcpu(env))) {
> |^
> target/ppc/mmu_helper.c:1356:5: note: here
>1356 | default:
> | ^~~
> cc1: all warnings being treated as errors
>
> Rewrite the comment using 'fall through' which is recognized by
> GCC and static analyzers.
>
> Reported-by: Stefan Weil
> Signed-off-by: Philippe Mathieu-Daudé
Acked-by: David Gibson
> ---
> target/ppc/mmu_helper.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 261a8fe707..862824b073 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -98,7 +98,7 @@ static int pp_check(int key, int pp, int nx)
> case 0x1:
> case 0x2:
> access |= PAGE_WRITE;
> -/* No break here */
> +/* fall through */
> case 0x3:
> access |= PAGE_READ;
> break;
> @@ -706,7 +706,7 @@ static int mmu40x_get_physical_address(CPUPPCState *env,
> mmu_ctx_t *ctx,
> if (pr != 0) {
> goto check_perms;
> }
> -/* No break here */
> +/* fall through */
> case 0x3:
> /* All accesses granted */
> ctx->prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> @@ -720,7 +720,7 @@ static int mmu40x_get_physical_address(CPUPPCState *env,
> mmu_ctx_t *ctx,
> ret = -2;
> break;
> }
> - /* No break here */
> +/* fall through */
> case 0x1:
> check_perms:
> /* Check from TLB entry */
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] ppc405_boards: Don't size flash memory to match backing image
On Fri, Mar 08, 2019 at 08:24:04AM +0100, Markus Armbruster wrote: > David Gibson writes: > > > On Thu, Mar 07, 2019 at 02:03:16PM +0100, Markus Armbruster wrote: > >> Machine "ref405ep" maps its flash memory at address 2^32 - image size. > >> Image size is rounded up to the next multiple of 64KiB. Useless, > >> because pflash_cfi02_realize() fails with "failed to read the initial > >> flash content" unless the rounding is a no-op. > >> > >> If the image size exceeds 0x8 Bytes, we overlap first SRAM, then > >> other stuff. No idea how that would play out, but useful outcomes > >> seem unlikely. > >> > >> Map the flash memory at fixed address 0xFFF8 with size 512KiB, > >> regardless of image size, to match the physical hardware. > >> > >> Machine "taihu" maps its boot flash memory similarly. The code even > >> has a comment /* XXX: should check that size is 2MB */, followed by > >> disabled code to adjust the size to 2MiB regardless of image size. > >> > >> Its code to map its application flash memory looks the same, except > >> there the XXX comment asks for 32MiB, and the code to adjust the size > >> isn't disabled. Note that pflash_cfi02_realize() fails with "failed > >> to read the initial flash content" for images smaller than 32MiB. > >> > >> Map the boot flash memory at fixed address 0xFFE0 with size 2MiB, > >> to match the physical hardware. Delete dead code from application > >> flash mapping, and simplify some. > >> > >> Cc: David Gibson > >> Signed-off-by: Markus Armbruster > >> Acked-by: David Gibson > >> Reviewed-by: Alex Bennée > > > > I'm assuming because this is in a series I'm not otherwise CCed on > > that this is going in through someone else's tree. Let me know if you > > want me take it through mine. > > I intend to take the complete series through my tree unless a maintainer > objects. No objection here. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH v2 07/11] ppc405_boards: Don't size flash memory to match backing image
On Thu, Mar 07, 2019 at 08:01:35AM +0100, Markus Armbruster wrote: > Alex Bennée writes: > > > Markus Armbruster writes: > > > >> Machine "ref405ep" maps its flash memory at address 2^32 - image size. > >> Image size is rounded up to the next multiple of 64KiB. Useless, > >> because pflash_cfi02_realize() fails with "failed to read the initial > >> flash content" unless the rounding is a no-op. > >> > >> If the image size exceeds 0x8 Bytes, we overlap first SRAM, then > >> other stuff. No idea how that would play out, but a useful outcomes > >> seem unlikely. > >> > >> Map the flash memory at fixed address 0xFFF8 with size 512KiB, > >> regardless of image size, to match the physical hardware. > >> > >> Machine "taihu" maps its boot flash memory similarly. The code even > >> has a comment /* XXX: should check that size is 2MB */, followed by > >> disabled code to adjust the size to 2MiB regardless of image size. > >> > >> Its code to map its application flash memory looks the same, except > >> there the XXX comment asks for 32MiB, and the code to adjust the size > >> isn't disabled. Note that pflash_cfi02_realize() fails with "failed > >> to read the initial flash content" for images smaller than 32MiB. > >> > >> Map the boot flash memory at fixed address 0xFFE0 with size 2MiB, > >> to match the physical hardware. Delete dead code from application > >> flash mapping, and simplify some. > > > > It seems to me the DEBUG_BOARD_INIT code is probably out of date cruft > > that could be excised all together. But that doesn't stop this being > > useful: > > David, would you like me to excise DEBUG_BOARD_INIT? If you have the chance to look at it, that would be great. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v3 07/14] ppc405_boards: Don't size flash memory to match backing image
On Thu, Mar 07, 2019 at 02:03:16PM +0100, Markus Armbruster wrote:
> Machine "ref405ep" maps its flash memory at address 2^32 - image size.
> Image size is rounded up to the next multiple of 64KiB. Useless,
> because pflash_cfi02_realize() fails with "failed to read the initial
> flash content" unless the rounding is a no-op.
>
> If the image size exceeds 0x8 Bytes, we overlap first SRAM, then
> other stuff. No idea how that would play out, but useful outcomes
> seem unlikely.
>
> Map the flash memory at fixed address 0xFFF8 with size 512KiB,
> regardless of image size, to match the physical hardware.
>
> Machine "taihu" maps its boot flash memory similarly. The code even
> has a comment /* XXX: should check that size is 2MB */, followed by
> disabled code to adjust the size to 2MiB regardless of image size.
>
> Its code to map its application flash memory looks the same, except
> there the XXX comment asks for 32MiB, and the code to adjust the size
> isn't disabled. Note that pflash_cfi02_realize() fails with "failed
> to read the initial flash content" for images smaller than 32MiB.
>
> Map the boot flash memory at fixed address 0xFFE0 with size 2MiB,
> to match the physical hardware. Delete dead code from application
> flash mapping, and simplify some.
>
> Cc: David Gibson
> Signed-off-by: Markus Armbruster
> Acked-by: David Gibson
> Reviewed-by: Alex Bennée
I'm assuming because this is in a series I'm not otherwise CCed on
that this is going in through someone else's tree. Let me know if you
want me take it through mine.
> ---
> hw/ppc/ppc405_boards.c | 51 +-
> 1 file changed, 15 insertions(+), 36 deletions(-)
>
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index f47b15f10e..672717ef1b 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -158,7 +158,7 @@ static void ref405ep_init(MachineState *machine)
> target_ulong kernel_base, initrd_base;
> long kernel_size, initrd_size;
> int linux_boot;
> -int fl_idx, fl_sectors, len;
> +int len;
> DriveInfo *dinfo;
> MemoryRegion *sysmem = get_system_memory();
>
> @@ -185,26 +185,19 @@ static void ref405ep_init(MachineState *machine)
> #ifdef DEBUG_BOARD_INIT
> printf("%s: register BIOS\n", __func__);
> #endif
> -fl_idx = 0;
> #ifdef USE_FLASH_BIOS
> -dinfo = drive_get(IF_PFLASH, 0, fl_idx);
> +dinfo = drive_get(IF_PFLASH, 0, 0);
> if (dinfo) {
> -BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> -
> -bios_size = blk_getlength(blk);
> -fl_sectors = (bios_size + 65535) >> 16;
> #ifdef DEBUG_BOARD_INIT
> -printf("Register parallel flash %d size %lx"
> - " at addr %lx '%s' %d\n",
> - fl_idx, bios_size, -bios_size,
> - blk_name(blk), fl_sectors);
> +printf("Register parallel flash\n");
> #endif
> +bios_size = 8 * MiB;
> pflash_cfi02_register((uint32_t)(-bios_size),
>NULL, "ef405ep.bios", bios_size,
> - blk, 65536, fl_sectors, 1,
> + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> + 64 * KiB, bios_size / (64 * KiB), 1,
>2, 0x0001, 0x22DA, 0x, 0x, 0x555,
> 0x2AA,
>1);
> -fl_idx++;
> } else
> #endif
> {
> @@ -455,7 +448,7 @@ static void taihu_405ep_init(MachineState *machine)
> target_ulong kernel_base, initrd_base;
> long kernel_size, initrd_size;
> int linux_boot;
> -int fl_idx, fl_sectors;
> +int fl_idx;
> DriveInfo *dinfo;
>
> /* RAM is soldered to the board so the size cannot be changed */
> @@ -486,21 +479,14 @@ static void taihu_405ep_init(MachineState *machine)
> #if defined(USE_FLASH_BIOS)
> dinfo = drive_get(IF_PFLASH, 0, fl_idx);
> if (dinfo) {
> -BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> -
> -bios_size = blk_getlength(blk);
> -/* XXX: should check that size is 2MB */
> -//bios_size = 2 * 1024 * 1024;
> -fl_sectors = (bios_size + 65535) >> 16;
> #ifdef DEBUG_BOARD_INIT
> -printf("Register parallel flash %d size %lx"
> - " at addr %lx '%s' %d\n",
> - fl_idx, bios_size, -bios_size,
> - blk_name(blk), fl_sectors);
> +printf("Register boot flash\n");
> #endif
> -pflash_cfi02_re
Re: [Qemu-block] [Qemu-devel] [PATCH 05/10] ppc405_boards: Don't size flash memory to match backing image
On Thu, Feb 21, 2019 at 05:31:30PM +0100, Markus Armbruster wrote:
> Alex Bennée writes:
>
> > Markus Armbruster writes:
> >
> >> Machine "ref405ep" maps its flash memory at address 2^32 - image size.
> >> Image size is rounded up to the next multiple of 64KiB. Useless,
> >> because pflash_cfi02_realize() fails with "failed to read the initial
> >> flash content" unless the rounding is a no-op.
> >>
> >> If the image size exceeds 0x8 Bytes, we overlap first SRAM, then
> >> other stuff. No idea how that would play out, but a useful outcomes
> >> seem unlikely.
> >>
> >> Map the flash memory at fixed address 0xFFF8 with size 512KiB,
> >> regardless of image size, to match the physical hardware.
> >>
> >> Machine "taihu" maps its boot flash memory similarly. The code even
> >> has a comment /* XXX: should check that size is 2MB */, followed by
> >> disabled code to adjust the size to 2MiB regardless of image size.
> >>
> >> Its code to map its application flash memory looks the same, except
> >> there the XXX comment asks for 32MiB, and the code to adjust the size
> >> isn't disabled. Note that pflash_cfi02_realize() fails with "failed
> >> to read the initial flash content" for images smaller than 32MiB.
> >>
> >> Map the boot flash memory at fixed address 0xFFE0 with size 2MiB,
> >> to match the physical hardware. Delete dead code from application
> >> flash mapping, and simplify some.
> >>
> >> Cc: David Gibson
> >> Signed-off-by: Markus Armbruster
> >> ---
> >> hw/ppc/ppc405_boards.c | 53 +-
> >> 1 file changed, 16 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> >> index f47b15f10e..728154aebb 100644
> >> --- a/hw/ppc/ppc405_boards.c
> >> +++ b/hw/ppc/ppc405_boards.c
> >> @@ -158,7 +158,7 @@ static void ref405ep_init(MachineState *machine)
> >> target_ulong kernel_base, initrd_base;
> >> long kernel_size, initrd_size;
> >> int linux_boot;
> >> -int fl_idx, fl_sectors, len;
> >> +int len;
> >> DriveInfo *dinfo;
> >> MemoryRegion *sysmem = get_system_memory();
> >>
> >> @@ -185,26 +185,19 @@ static void ref405ep_init(MachineState *machine)
> >> #ifdef DEBUG_BOARD_INIT
> >> printf("%s: register BIOS\n", __func__);
> >> #endif
> >> -fl_idx = 0;
> >> #ifdef USE_FLASH_BIOS
> >> -dinfo = drive_get(IF_PFLASH, 0, fl_idx);
> >> +dinfo = drive_get(IF_PFLASH, 0, 0);
> >> if (dinfo) {
> >> -BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
> >> -
> >> -bios_size = blk_getlength(blk);
> >> -fl_sectors = (bios_size + 65535) >> 16;
> >> #ifdef DEBUG_BOARD_INIT
> >> -printf("Register parallel flash %d size %lx"
> >> - " at addr %lx '%s' %d\n",
> >> - fl_idx, bios_size, -bios_size,
> >> - blk_name(blk), fl_sectors);
> >> +printf("Register parallel flash\n");
> >> #endif
> >> -pflash_cfi02_register((uint32_t)(-bios_size),
> >> +bios_size = 0x8;
> >
> > bios_size = 8 * MiB?
>
> The next line has base address 0xFFF8. I picked 0x8 to make
> 0xFFF8 + 0x8 == 0 mod 2^32 more obvious.
>
> If I change 0x8 to 8 * MiB, the size is more obvious, but "at end of
> 32 bit address space" less so.
>
> If I additionally change the base address back to ((uint32_t)-bios_size,
> "at end of 32 bit address space" is obvious again, but the actual base
> address less so.
I have a weak preference for ((uint32_t)-bios_size), with bios_size =
8 * MiB.
>
> I don't really care myself. David, you're the maintainer, do you have a
> preference?
>
> >> +pflash_cfi02_register(0xFFF8,
> >>NULL, "ef405ep.bios", bios_size,
> >> - blk, 65536, fl_sectors, 1,
> >> + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> >> + 65536, bios_size / 65536, 1,
> >
> > 64 * KiB?
>
> David, same question (two additional instances below).
Here I think 64 * KiB would be nice in each of those places. Again,
only a weak preference.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH 2/2] avoid TABs in files that only contain a few
On Thu, Dec 13, 2018 at 11:37:37PM +0100, Paolo Bonzini wrote: > Most files that have TABs only contain a handful of them. Change > them to spaces so that we don't confuse people. > > disas, standard-headers, linux-headers and libdecnumber are imported > from other projects and probably should be exempted from the check. > Outside those, after this patch the following files still contain both > 8-space and TAB sequences at the beginning of the line. Many of them > have a majority of TABs, or were initially committed with all tabs. > > bsd-user/i386/target_syscall.h > bsd-user/x86_64/target_syscall.h > crypto/aes.c > hw/audio/fmopl.c > hw/audio/fmopl.h > hw/block/tc58128.c > hw/display/cirrus_vga.c > hw/display/xenfb.c > hw/dma/etraxfs_dma.c > hw/intc/sh_intc.c > hw/misc/mst_fpga.c > hw/net/pcnet.c > hw/sh4/sh7750.c > hw/timer/m48t59.c > hw/timer/sh_timer.c > include/crypto/aes.h > include/disas/bfd.h > include/hw/sh4/sh.h > libdecnumber/decNumber.c > linux-headers/asm-generic/unistd.h > linux-headers/linux/kvm.h > linux-user/alpha/target_syscall.h > linux-user/arm/nwfpe/double_cpdo.c > linux-user/arm/nwfpe/fpa11_cpdt.c > linux-user/arm/nwfpe/fpa11_cprt.c > linux-user/arm/nwfpe/fpa11.h > linux-user/flat.h > linux-user/flatload.c > linux-user/i386/target_syscall.h > linux-user/ppc/target_syscall.h > linux-user/sparc/target_syscall.h > linux-user/syscall.c > linux-user/syscall_defs.h > linux-user/x86_64/target_syscall.h > slirp/cksum.c > slirp/if.c > slirp/ip.h > slirp/ip_icmp.c > slirp/ip_icmp.h > slirp/ip_input.c > slirp/ip_output.c > slirp/mbuf.c > slirp/misc.c > slirp/sbuf.c > slirp/socket.c > slirp/socket.h > slirp/tcp_input.c > slirp/tcpip.h > slirp/tcp_output.c > slirp/tcp_subr.c > slirp/tcp_timer.c > slirp/tftp.c > slirp/udp.c > slirp/udp.h > target/cris/cpu.h > target/cris/mmu.c > target/cris/op_helper.c > target/sh4/helper.c > target/sh4/op_helper.c > target/sh4/translate.c > tcg/sparc/tcg-target.inc.c > tests/tcg/cris/check_addo.c > tests/tcg/cris/check_moveq.c > tests/tcg/cris/check_swap.c > tests/tcg/multiarch/test-mmap.c > ui/vnc-enc-hextile-template.h > ui/vnc-enc-zywrle.h > util/envlist.c > util/readline.c > > The following have only TABs: > > bsd-user/i386/target_signal.h > bsd-user/sparc64/target_signal.h > bsd-user/sparc64/target_syscall.h > bsd-user/sparc/target_signal.h > bsd-user/sparc/target_syscall.h > bsd-user/x86_64/target_signal.h > crypto/desrfb.c > hw/audio/intel-hda-defs.h > hw/core/uboot_image.h > hw/sh4/sh7750_regnames.c > hw/sh4/sh7750_regs.h > include/hw/cris/etraxfs_dma.h > linux-user/alpha/termbits.h > linux-user/arm/nwfpe/fpopcode.h > linux-user/arm/nwfpe/fpsr.h > linux-user/arm/syscall_nr.h > linux-user/arm/target_signal.h > linux-user/cris/target_signal.h > linux-user/i386/target_signal.h > linux-user/linux_loop.h > linux-user/m68k/target_signal.h > linux-user/microblaze/target_signal.h > linux-user/mips64/target_signal.h > linux-user/mips/target_signal.h > linux-user/mips/target_syscall.h > linux-user/mips/termbits.h > linux-user/ppc/target_signal.h > linux-user/sh4/target_signal.h > linux-user/sh4/termbits.h > linux-user/sparc64/target_syscall.h > linux-user/sparc/target_signal.h > linux-user/x86_64/target_signal.h > linux-user/x86_64/termbits.h > pc-bios/optionrom/optionrom.h > slirp/mbuf.h > slirp/misc.h > slirp/sbuf.h > slirp/tcp.h > slirp/tcp_timer.h > slirp/tcp_var.h > target/i386/svm.h > target/sparc/asi.h > target/xtensa/core-dc232b/xtensa-modules.inc.c > target/xtensa/core-dc233c/xtensa-modules.inc.c > target/xtensa/core-de212/core-isa.h > target/xtensa/core-de212/xtensa-modules.inc.c > target/xtensa/core-fsf/xtensa-modules.inc.c > target/xtensa/core-sample_controller/core-isa.h > target/xtensa/core-sample_controller/xtensa-modules.inc.c > target/xtensa/core-test_kc705_be/core-isa.h > target/xtensa/core-test_kc705_be/xtensa-modules.inc.c > tests/tcg/cris/check_abs.c > tests/tcg/cris/check_addc.c > tests/tcg/cris/check_addcm.c > tests/tcg/cris/check_addoq.c > tests/tcg/cris/check_bound.c > tests/tcg/cris/check_ftag.c > tests/tcg/cris/check_
Re: [Qemu-block] [PATCH 02/10] hw/ppc/ppc405_boards: Don't use load_image()
On Fri, Nov 30, 2018 at 03:17:04PM +, Peter Maydell wrote:
> The load_image() function is deprecated, as it does not let the
> caller specify how large the buffer to read the file into is.
> Instead use load_image_size().
>
> Signed-off-by: Peter Maydell
Acked-by: David Gibson
> ---
> hw/ppc/ppc405_boards.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 3be3fe4432b..1b0a0a8ba3a 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -219,9 +219,11 @@ static void ref405ep_init(MachineState *machine)
> bios_name = BIOS_FILENAME;
> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> if (filename) {
> -bios_size = load_image(filename,
> memory_region_get_ram_ptr(bios));
> +bios_size = load_image_size(filename,
> +memory_region_get_ram_ptr(bios),
> +BIOS_SIZE);
> g_free(filename);
> -if (bios_size < 0 || bios_size > BIOS_SIZE) {
> +if (bios_size < 0) {
> error_report("Could not load PowerPC BIOS '%s'", bios_name);
> exit(1);
> }
> @@ -515,9 +517,11 @@ static void taihu_405ep_init(MachineState *machine)
> _fatal);
> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> if (filename) {
> -bios_size = load_image(filename,
> memory_region_get_ram_ptr(bios));
> +bios_size = load_image_size(filename,
> +memory_region_get_ram_ptr(bios),
> +BIOS_SIZE);
> g_free(filename);
> -if (bios_size < 0 || bios_size > BIOS_SIZE) {
> +if (bios_size < 0) {
> error_report("Could not load PowerPC BIOS '%s'", bios_name);
> exit(1);
> }
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH 01/10] hw/ppc/mac_newworld, mac_oldworld: Don't use load_image()
On Fri, Nov 30, 2018 at 03:17:03PM +, Peter Maydell wrote:
> The load_image() function is deprecated, as it does not let the
> caller specify how large the buffer to read the file into is.
> Use the glib g_file_get_contents() function instead, which does
> the whole "allocate memory for the file and read it in" operation.
>
> Signed-off-by: Peter Maydell
Acked-by: David Gibson
> ---
> hw/ppc/mac_newworld.c | 10 --
> hw/ppc/mac_oldworld.c | 10 --
> 2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 14273a123e5..7e45afae7c5 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -127,8 +127,7 @@ static void ppc_core99_init(MachineState *machine)
> MACIOIDEState *macio_ide;
> BusState *adb_bus;
> MacIONVRAMState *nvr;
> -int bios_size, ndrv_size;
> -uint8_t *ndrv_file;
> +int bios_size;
> int ppc_boot_device;
> DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> void *fw_cfg;
> @@ -510,11 +509,10 @@ static void ppc_core99_init(MachineState *machine)
> /* MacOS NDRV VGA driver */
> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, NDRV_VGA_FILENAME);
> if (filename) {
> -ndrv_size = get_image_size(filename);
> -if (ndrv_size != -1) {
> -ndrv_file = g_malloc(ndrv_size);
> -ndrv_size = load_image(filename, ndrv_file);
> +gchar *ndrv_file;
> +gsize ndrv_size;
>
> +if (g_file_get_contents(filename, _file, _size, NULL)) {
> fw_cfg_add_file(fw_cfg, "ndrv/qemu_vga.ndrv", ndrv_file,
> ndrv_size);
> }
> g_free(filename);
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 9891c325a9b..817f70e52cf 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -99,8 +99,7 @@ static void ppc_heathrow_init(MachineState *machine)
> SysBusDevice *s;
> DeviceState *dev, *pic_dev;
> BusState *adb_bus;
> -int bios_size, ndrv_size;
> -uint8_t *ndrv_file;
> +int bios_size;
> uint16_t ppc_boot_device;
> DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> void *fw_cfg;
> @@ -361,11 +360,10 @@ static void ppc_heathrow_init(MachineState *machine)
> /* MacOS NDRV VGA driver */
> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, NDRV_VGA_FILENAME);
> if (filename) {
> -ndrv_size = get_image_size(filename);
> -if (ndrv_size != -1) {
> -ndrv_file = g_malloc(ndrv_size);
> -ndrv_size = load_image(filename, ndrv_file);
> +gchar *ndrv_file;
> +gsize ndrv_size;
>
> +if (g_file_get_contents(filename, _file, _size, NULL)) {
> fw_cfg_add_file(fw_cfg, "ndrv/qemu_vga.ndrv", ndrv_file,
> ndrv_size);
> }
> g_free(filename);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v2 5/5] Remove unnecessary variables for function return value
On Fri, Mar 23, 2018 at 03:32:02PM +0100, Laurent Vivier wrote:
> Re-run Coccinelle script scripts/coccinelle/return_directly.cocci
>
> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
ppc part
Acked-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> accel/tcg/translate-all.c | 5 +-
> block/quorum.c | 6 +--
> hw/arm/exynos4210.c| 6 +--
> hw/block/vhost-user-blk.c | 5 +-
> hw/hppa/dino.c | 5 +-
> hw/misc/mos6522.c | 8 +---
> hw/net/ftgmac100.c | 5 +-
> hw/ppc/pnv_lpc.c | 16 ++-
> io/net-listener.c | 6 +--
> target/i386/hax-darwin.c | 10 ++--
> target/mips/dsp_helper.c | 15 ++
> target/xtensa/core-dc232b/xtensa-modules.c | 56
> ++
> target/xtensa/core-dc233c/xtensa-modules.c | 56
> ++
> target/xtensa/core-de212/xtensa-modules.c | 48 +--
> target/xtensa/core-fsf/xtensa-modules.c| 32 -
> .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
> target/xtensa/translate.c | 7 +--
> tests/m48t59-test.c| 6 +--
> tests/test-thread-pool.c | 6 +--
> util/uri.c | 5 +-
> 20 files changed, 79 insertions(+), 248 deletions(-)
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 5ad1b919bc..55d822d410 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -644,11 +644,8 @@ static inline void *alloc_code_gen_buffer(void)
> static inline void *alloc_code_gen_buffer(void)
> {
> size_t size = tcg_ctx->code_gen_buffer_size;
> -void *buf;
> -
> -buf = VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
> +return VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
> PAGE_EXECUTE_READWRITE);
> -return buf;
> }
> #else
> static inline void *alloc_code_gen_buffer(void)
> diff --git a/block/quorum.c b/block/quorum.c
> index 14333c18aa..304442ef65 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -608,7 +608,7 @@ static void read_quorum_children_entry(void *opaque)
> static int read_quorum_children(QuorumAIOCB *acb)
> {
> BDRVQuorumState *s = acb->bs->opaque;
> -int i, ret;
> +int i;
>
> acb->children_read = s->num_children;
> for (i = 0; i < s->num_children; i++) {
> @@ -643,9 +643,7 @@ static int read_quorum_children(QuorumAIOCB *acb)
> qemu_coroutine_yield();
> }
>
> -ret = acb->vote_ret;
> -
> -return ret;
> +return acb->vote_ret;
> }
>
> static int read_fifo_child(QuorumAIOCB *acb)
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index 06f9d1ffa4..b7463a71ec 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -156,12 +156,8 @@ void exynos4210_write_secondary(ARMCPU *cpu,
>
> static uint64_t exynos4210_calc_affinity(int cpu)
> {
> -uint64_t mp_affinity;
> -
> /* Exynos4210 has 0x9 as cluster ID */
> -mp_affinity = (0x9 << ARM_AFF1_SHIFT) | cpu;
> -
> -return mp_affinity;
> +return (0x9 << ARM_AFF1_SHIFT) | cpu;
> }
>
> Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index f840f07dfe..3f41ca9e26 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -196,7 +196,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
> *vdev,
> Error **errp)
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -uint64_t get_features;
>
> /* Turn on pre-defined features */
> virtio_add_feature(, VIRTIO_BLK_F_SEG_MAX);
> @@ -215,9 +214,7 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
> *vdev,
> virtio_add_feature(, VIRTIO_BLK_F_MQ);
> }
>
> -get_features = vhost_get_features(>dev, user_feature_bits, features);
> -
> -return get_features;
> +return vhost_get_features(>dev, user_feature_bits, features);
> }
>
> static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
> index 15aefde09c..c5dcf3104d 100644
> --- a/hw/hppa/dino.c
> +++ b/hw
Re: [Qemu-block] [PATCH 4/4] Remove unnecessary variables for function return value
On Thu, Mar 22, 2018 at 05:12:26PM +0100, Laurent Vivier wrote:
> Re-run Coccinelle script scripts/coccinelle/return_directly.cocci
>
> Signed-off-by: Laurent Vivier <lviv...@redhat.com>
> ---
> accel/tcg/translate-all.c | 5 +-
> block/quorum.c | 6 +--
> hw/arm/exynos4210.c| 7 +--
> hw/block/vhost-user-blk.c | 5 +-
> hw/hppa/dino.c | 5 +-
> hw/misc/mos6522.c | 6 +--
> hw/net/ftgmac100.c | 5 +-
> hw/ppc/pnv_lpc.c | 14 +-
> io/net-listener.c | 6 +--
> target/i386/hax-darwin.c | 10 ++--
> target/mips/dsp_helper.c | 15 ++
> target/xtensa/core-dc232b/xtensa-modules.c | 56
> ++
> target/xtensa/core-dc233c/xtensa-modules.c | 56
> ++
> target/xtensa/core-de212/xtensa-modules.c | 48 +--
> target/xtensa/core-fsf/xtensa-modules.c| 32 -
> .../xtensa/core-sample_controller/xtensa-modules.c | 24 +++---
> target/xtensa/translate.c | 5 +-
> tests/m48t59-test.c| 6 +--
> tests/test-thread-pool.c | 6 +--
> util/uri.c | 5 +-
> 20 files changed, 75 insertions(+), 247 deletions(-)
ppc part
Acked-by: David Gibson <da...@gibson.dropbear.id.au>
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 5ad1b919bc..55d822d410 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -644,11 +644,8 @@ static inline void *alloc_code_gen_buffer(void)
> static inline void *alloc_code_gen_buffer(void)
> {
> size_t size = tcg_ctx->code_gen_buffer_size;
> -void *buf;
> -
> -buf = VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
> +return VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT,
> PAGE_EXECUTE_READWRITE);
> -return buf;
> }
> #else
> static inline void *alloc_code_gen_buffer(void)
> diff --git a/block/quorum.c b/block/quorum.c
> index 14333c18aa..304442ef65 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -608,7 +608,7 @@ static void read_quorum_children_entry(void *opaque)
> static int read_quorum_children(QuorumAIOCB *acb)
> {
> BDRVQuorumState *s = acb->bs->opaque;
> -int i, ret;
> +int i;
>
> acb->children_read = s->num_children;
> for (i = 0; i < s->num_children; i++) {
> @@ -643,9 +643,7 @@ static int read_quorum_children(QuorumAIOCB *acb)
> qemu_coroutine_yield();
> }
>
> -ret = acb->vote_ret;
> -
> -return ret;
> +return acb->vote_ret;
> }
>
> static int read_fifo_child(QuorumAIOCB *acb)
> diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> index 06f9d1ffa4..d5ce275b21 100644
> --- a/hw/arm/exynos4210.c
> +++ b/hw/arm/exynos4210.c
> @@ -156,12 +156,7 @@ void exynos4210_write_secondary(ARMCPU *cpu,
>
> static uint64_t exynos4210_calc_affinity(int cpu)
> {
> -uint64_t mp_affinity;
> -
> -/* Exynos4210 has 0x9 as cluster ID */
> -mp_affinity = (0x9 << ARM_AFF1_SHIFT) | cpu;
> -
> -return mp_affinity;
> +return (0x9 << ARM_AFF1_SHIFT) | cpu;
> }
>
> Exynos4210State *exynos4210_init(MemoryRegion *system_mem)
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index f840f07dfe..3f41ca9e26 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -196,7 +196,6 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
> *vdev,
> Error **errp)
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -uint64_t get_features;
>
> /* Turn on pre-defined features */
> virtio_add_feature(, VIRTIO_BLK_F_SEG_MAX);
> @@ -215,9 +214,7 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice
> *vdev,
> virtio_add_feature(, VIRTIO_BLK_F_MQ);
> }
>
> -get_features = vhost_get_features(>dev, user_feature_bits, features);
> -
> -return get_features;
> +return vhost_get_features(>dev, user_feature_bits, features);
> }
>
> static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
> index 15aefde09c..c5dcf3104d 100644
> --- a/hw/hppa/dino.c
> +++ b/hw
Re: [Qemu-block] [Qemu-ppc] [PATCH 3/3] maint: Fix macros with broken 'do/while(0); ' usage
On Thu, Nov 30, 2017 at 07:41:59AM -0600, Eric Blake wrote:
> The point of writing a macro embedded in a 'do { ... } while (0)'
> loop is so that the macro can be used as a drop-in statement with
> the caller supplying the trailing ';'. Although our coding style
> frowns on brace-less 'if':
> if (cond)
> statement;
> else
> something else;
> the use of do/while (0) in a macro is absolutely essential for the
> purpose of avoiding a syntax error on the 'else' - but it only works
> if there is no trailing ';' in the macro (as the ';' in the code
> calling the macro would then be a second statement and cause the
> 'else' to not pair to the 'if').
>
> Many of the places touched in this code are examples of the ugly
> bit-rotting debug print statements; cleaning those up is left as
> a bite-sized task for another day.
>
> Found mechanically via: $ git grep -B1 'while (0);' | grep -A1
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
ppc related portions
Acked-by: David Gibson <da...@gibson.dropbear.id.au>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v2 3/5] pci: Add INTERFACE_PCIE_DEVICE to all PCIe devices
On Wed, Sep 27, 2017 at 04:56:33PM -0300, Eduardo Habkost wrote:
> Change all devices that set is_express=1 to implement
> INTERFACE_PCIE_DEVICE.
>
> Cc: Keith Busch <keith.bu...@intel.com>
> Cc: Kevin Wolf <kw...@redhat.com>
> Cc: Max Reitz <mre...@redhat.com>
> Cc: Dmitry Fleytman <dmi...@daynix.com>
> Cc: Jason Wang <jasow...@redhat.com>
> Cc: "Michael S. Tsirkin" <m...@redhat.com>
> Cc: Marcel Apfelbaum <mar...@redhat.com>
> Cc: Paul Burton <paul.bur...@imgtec.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Hannes Reinecke <h...@suse.com>
> Cc: qemu-block@nongnu.org
> Reviewed-by: Alistair Francis <alistair.fran...@xilinx.com>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> Changes v1 -> v2:
> * base-xhci is marked as hybrid, now (in another patch)
> * Included pcie-pci-bridge
> ---
> hw/block/nvme.c| 4
> hw/net/e1000e.c| 4
> hw/pci-bridge/pcie_pci_bridge.c| 1 +
> hw/pci-bridge/pcie_root_port.c | 4
> hw/pci-bridge/xio3130_downstream.c | 4
> hw/pci-bridge/xio3130_upstream.c | 4
> hw/pci-host/xilinx-pcie.c | 4
> hw/scsi/megasas.c | 6 ++
> 8 files changed, 31 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 9aa32692a3..441e21ed1f 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1110,6 +1110,10 @@ static const TypeInfo nvme_info = {
> .instance_size = sizeof(NvmeCtrl),
> .class_init= nvme_class_init,
> .instance_init = nvme_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
> };
>
> static void nvme_register_types(void)
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index 6c42b4478c..81f7934a59 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -708,6 +708,10 @@ static const TypeInfo e1000e_info = {
> .instance_size = sizeof(E1000EState),
> .class_init = e1000e_class_init,
> .instance_init = e1000e_instance_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
> };
>
> static void e1000e_register_types(void)
> diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
> index 9aa5cc3e45..88db143633 100644
> --- a/hw/pci-bridge/pcie_pci_bridge.c
> +++ b/hw/pci-bridge/pcie_pci_bridge.c
> @@ -180,6 +180,7 @@ static const TypeInfo pcie_pci_bridge_info = {
> .class_init = pcie_pci_bridge_class_init,
> .interfaces = (InterfaceInfo[]) {
> { TYPE_HOTPLUG_HANDLER },
> +{ INTERFACE_PCIE_DEVICE },
> { },
> }
> };
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 4d588cb22e..9b6e4ce512 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -161,6 +161,10 @@ static const TypeInfo rp_info = {
> .class_init= rp_class_init,
> .abstract = true,
> .class_size = sizeof(PCIERootPortClass),
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
> };
>
> static void rp_register_types(void)
> diff --git a/hw/pci-bridge/xio3130_downstream.c
> b/hw/pci-bridge/xio3130_downstream.c
> index e706f36cb7..7d2f7629c1 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -195,6 +195,10 @@ static const TypeInfo xio3130_downstream_info = {
> .name = "xio3130-downstream",
> .parent= TYPE_PCIE_SLOT,
> .class_init= xio3130_downstream_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
> };
>
> static void xio3130_downstream_register_types(void)
> diff --git a/hw/pci-bridge/xio3130_upstream.c
> b/hw/pci-bridge/xio3130_upstream.c
> index a052224bbf..227997ce46 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -166,6 +166,10 @@ static const TypeInfo xio3130_upstream_info = {
> .name = "x3130-upstream",
> .parent= TYPE_PCIE_PORT,
> .class_init= xio3130_upstream_class_init,
> +.interfaces = (InterfaceInfo[]) {
> +{ INTERFACE_PCIE_DEVICE },
> +{ }
> +},
> };
>
> static void xio3130_upstream_register_types(void)
> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> index 46
Re: [Qemu-block] [PATCH v2 4/5] pci: Add INTERFACE_CONVENTIONAL_PCI_DEVICE to Conventional PCI devices
On Wed, Sep 27, 2017 at 04:56:34PM -0300, Eduardo Habkost wrote: > Add INTERFACE_CONVENTIONAL_PCI_DEVICE to all direct subtypes of > TYPE_PCI_DEVICE, except: > > 1) The ones that already have INTERFACE_PCIE_DEVICE set: > > * base-xhci > * e1000e > * nvme > * pvscsi > * vfio-pci > * virtio-pci > * vmxnet3 > > 2) base-pci-bridge > > Not all PCI bridges are Conventional PCI devices, so > INTERFACE_CONVENTIONAL_PCI_DEVICE is added only to the subtypes > that are actually Conventional PCI: > > * dec-21154-p2p-bridge > * i82801b11-bridge > * pbm-bridge > * pci-bridge > > The direct subtypes of base-pci-bridge not touched by this patch > are: > > * xilinx-pcie-root: Already marked as PCIe-only. > * pcie-pci-bridge: Already marked as PCIe-only. > * pcie-port: all non-abstract subtypes of pcie-port are already > marked as PCIe-only devices. > > 3) megasas-base > > Not all megasas devices are Conventional PCI devices, so the > interface names are added to the subclasses registered by > megasas_register_types(), according to information in the > megasas_devices[] array. > > "megasas-gen2" already implements INTERFACE_PCIE_DEVICE, so add > INTERFACE_CONVENTIONAL_PCI_DEVICE only to "megasas". > > Acked-by: Alberto Garcia <be...@igalia.com> > Acked-by: John Snow <js...@redhat.com> > Acked-by: Anthony PERARD <anthony.per...@citrix.com> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> Reviewed-by: David Gibson <da...@gibson.dropbear.id.au> and for the ppc devices Acked-by: David Gibson <da...@gibson.dropbear.id.au> > --- > Changes v1 -> v2: > * s/legacy/conventional/ > * Suggested-by: Alex Williamson <alex.william...@redhat.com> > * Note about pcie-pci-bridge on commit message. > * New devices: sungem, sunhme > > Cc: "Michael S. Tsirkin" <m...@redhat.com> > Cc: Igor Mammedov <imamm...@redhat.com> > Cc: Gerd Hoffmann <kra...@redhat.com> > Cc: Paolo Bonzini <pbonz...@redhat.com> > Cc: Richard Henderson <r...@twiddle.net> > Cc: Eduardo Habkost <ehabk...@redhat.com> > Cc: Stefano Stabellini <sstabell...@kernel.org> > Cc: Anthony Perard <anthony.per...@citrix.com> > Cc: John Snow <js...@redhat.com> > Cc: Alberto Garcia <be...@igalia.com> > Cc: Aurelien Jarno <aurel...@aurel32.net> > Cc: Yongbok Kim <yongbok@imgtec.com> > Cc: Jiri Slaby <jsl...@suse.cz> > Cc: Alexander Graf <ag...@suse.de> > Cc: Marcel Apfelbaum <mar...@redhat.com> > Cc: Jason Wang <jasow...@redhat.com> > Cc: Jiri Pirko <j...@resnulli.us> > Cc: "Hervé Poussineau" <hpous...@reactos.org> > Cc: Peter Maydell <peter.mayd...@linaro.org> > Cc: David Gibson <da...@gibson.dropbear.id.au> > Cc: Hannes Reinecke <h...@suse.com> > Cc: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> > Cc: Artyom Tarasenko <atar4q...@gmail.com> > Cc: Alex Williamson <alex.william...@redhat.com> > Cc: qemu-de...@nongnu.org > Cc: xen-de...@lists.xenproject.org > Cc: qemu-block@nongnu.org > Cc: qemu-...@nongnu.org > Cc: qemu-...@nongnu.org > --- > hw/acpi/piix4.c | 1 + > hw/audio/ac97.c | 4 > hw/audio/es1370.c | 4 > hw/audio/intel-hda.c| 4 > hw/char/serial-pci.c| 12 > hw/display/cirrus_vga.c | 4 > hw/display/qxl.c| 4 > hw/display/sm501.c | 4 > hw/display/vga-pci.c| 4 > hw/display/vmware_vga.c | 4 > hw/i2c/smbus_ich9.c | 4 > hw/i386/amd_iommu.c | 4 > hw/i386/kvm/pci-assign.c| 4 > hw/i386/pc_piix.c | 4 > hw/i386/xen/xen_platform.c | 4 > hw/i386/xen/xen_pvdevice.c | 4 > hw/ide/ich.c| 4 > hw/ide/pci.c| 4 > hw/ipack/tpci200.c | 4 > hw/isa/i82378.c | 4 > hw/isa/lpc_ich9.c | 1 + > hw/isa/piix4.c | 4 > hw/isa/vt82c686.c | 16 > hw/mips/gt64xxx_pci.c | 4 > hw/misc/edu.c | 5 + > hw/misc/ivshmem.c | 4 > hw/misc/macio/macio.c | 4 > hw/misc/pci-testdev.c | 4 > hw/net/e1000.c | 4 > hw/net/eepro100.c | 4 >
Re: [Qemu-block] [Qemu-devel] [PATCH v5 2/5] util: Use new error_report_fatal/abort instead of error_setg(_fatal/abort)
On Fri, Jan 29, 2016 at 02:33:08PM +0100, Lluís Vilanova wrote: > David Gibson writes: > > > On Thu, Jan 28, 2016 at 10:53:43PM +0100, Lluís Vilanova wrote: > >> Replaces all direct uses of 'error_setg(_fatal/abort)' with > >> 'error_report_fatal/abort'. Also reimplements the former on top of the > >> latter. > >> > >> Signed-off-by: Lluís Vilanova <vilan...@ac.upc.edu> > > > I think the spapr parts of this will be obsoleted by the cleanups to > > error handling included in the pull request I sent today. > > Ok, then I'll rebase once merged. Is there an ETA for the merge? Well, I'd been hoping a couple of days ago, but there were some mistakes in the last pull request, so.. sometime this week, I hope? -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v5 2/5] util: Use new error_report_fatal/abort instead of error_setg(_fatal/abort)
On Thu, Jan 28, 2016 at 10:53:43PM +0100, Lluís Vilanova wrote: > Replaces all direct uses of 'error_setg(_fatal/abort)' with > 'error_report_fatal/abort'. Also reimplements the former on top of the > latter. > > Signed-off-by: Lluís Vilanova <vilan...@ac.upc.edu> I think the spapr parts of this will be obsoleted by the cleanups to error handling included in the pull request I sent today. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-block] [RHEL-7.2.z qemu-kvm-rhev PATCH] virtio-blk: Fix double completion for werror=stop
On Wed, 18 Nov 2015 11:19:45 +0800
Fam Zheng <f...@redhat.com> wrote:
> BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1277922
> Brew: http://brewweb.devel.redhat.com/brew/taskinfo?taskID=10113752
>
> When a request R is absorbed by request M, it is appended to the
> "mr_next" queue led by M, and is completed together with the completion
> of M, in virtio_blk_rw_complete.
>
> During DMA restart in virtio_blk_dma_restart_bh, requests in s->rq are
> parsed and submitted again, possibly with a stale req->mr_next. It could
> be a problem if the request merging in virtio_blk_handle_request hasn't
> refreshed every mr_next pointer, in which case, virtio_blk_rw_complete
> could walk through unexpected requests following the stale pointers.
>
> Fix this by unsetting the pointer in virtio_blk_rw_complete. It is safe
> because this req is either completed and freed right away, or it will be
> restarted and parsed from scratch out of the vq later.
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> (cherry picked from commit 10f5a72f70862d299ddbdf226d6dc71fa4ae34dd)
> Signed-off-by: Fam Zheng <f...@redhat.com>
ACK
> ---
> hw/block/virtio-blk.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f9301ae..fe146ad 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -112,6 +112,10 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
> * happen on the other side of the migration).
> */
> if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
> +/* Break the link in case the next request is added to the
> + * restart queue and is going to be parsed from the ring
> again.
> + */
> +req->mr_next = NULL;
> continue;
> }
> }
> --
> 2.4.3
>
--
David Gibson <dgib...@redhat.com>
Senior Software Engineer, Virtualization, Red Hat
pgp36fNh69GNB.pgp
Description: OpenPGP digital signature
