Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler

From: David Gibson <david@gibson.dropbear.id.au>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	qemu-block@nongnu.org, "Markus Armbruster" <armbru@redhat.com>,
	"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
	"Greg Kurz" <groug@kaod.org>,
	qemu-devel@nongnu.org, qemu-arm@nongnu.org,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	qemu-ppc@nongnu.org, "Max Reitz" <mreitz@redhat.com>
Subject: Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler
Date: Tue, 27 Apr 2021 11:45:07 +1000	[thread overview]
Message-ID: <YIdso1hf+bXwZHmG@yekko.fritz.box> (raw)
In-Reply-To: <20210424162229.3312116-2-f4bug@amsat.org>

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

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é <f4bug@amsat.org>
> ---
>  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, &vmstate_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), &vmstate_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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]