From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 13603C433F5 for ; Mon, 27 Sep 2021 22:47:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EF4EC60EE7 for ; Mon, 27 Sep 2021 22:47:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237860AbhI0Wsi (ORCPT ); Mon, 27 Sep 2021 18:48:38 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:37247 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237780AbhI0Wsa (ORCPT ); Mon, 27 Sep 2021 18:48:30 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1632782812; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8F+8mdvFnjyvb27t/Z7nCLuVODAh1SKK2P1DV+cBzVQ=; b=T8rl9bJniw4vopVQ7mtIaOb48I3cuFVPEbhpGi+9B/WGYumaEZSxPdI3yPReDQgDkWaAI2 n9lDoWkfWL9fazmKwT11LSAv5xpOnP5rtAkEzqlHhgHKfB8LxYXCEjSTcb+QLzCiEldJJm rnak6LdxAQKREuTWXfkkUHizZbpSBe0= Received: from mail-oi1-f200.google.com (mail-oi1-f200.google.com [209.85.167.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-103-T886Z7R9NJaq_35ckmEkBQ-1; Mon, 27 Sep 2021 18:46:50 -0400 X-MC-Unique: T886Z7R9NJaq_35ckmEkBQ-1 Received: by mail-oi1-f200.google.com with SMTP id w26-20020a056808091a00b0027630e0f24aso5592663oih.0 for ; Mon, 27 Sep 2021 15:46:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:organization:mime-version:content-transfer-encoding; bh=8F+8mdvFnjyvb27t/Z7nCLuVODAh1SKK2P1DV+cBzVQ=; b=G5cI5mGPPAJIZT4LtpObcMCZbj/mmvY3x26Dendl3/9KK8QBk5GTiGAQoGfQGoGb1M F+vJiiklvvcRIrC+wi9vREZG7nY9T0HrQUGcKaX5za6mDKUinzXUmjgqQi4Ozdht2oNA 7+YleZI62e2nr4tQGiFbFjycnshmlP6X6CIpcBS1mGr83W0lW2Px9ajJyaddKn6Io+m4 qH3/1vt/EUK1Y9uZYWQQt6qhTk8/RooE8nswjWMENzXLWqml98Qp9Om/yhxslkwnvQtC QKC/HTJa9L/Hi0ybGg7Ah1TyKoNzrh8ANMgEKp3Virf4ao84mGH1Ciyt0qO4jbXzp7es e3oQ== X-Gm-Message-State: AOAM533NroHLmbkciTIeckgLcvsHtu/CPboID8nUyEqMPcB59d73FwCa gb7GjjRcfDiAtjSbXeZMIvBZ2BNW3M1ZFtWJCB2taZyPG8w9AE4ytPrM60g/w3o3Hk2k3m1JFF2 HQnOeWS7Y9kIYcYe7dFDByg/v X-Received: by 2002:a9d:3e4b:: with SMTP id h11mr2287814otg.294.1632782810026; Mon, 27 Sep 2021 15:46:50 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz1jND2vrOW39yVryOFHax2Bs9KlJre1+KCsNJKeV9Tr2lKgH+ZgnhOc6Tb6usJYsbl04jIhQ== X-Received: by 2002:a9d:3e4b:: with SMTP id h11mr2287802otg.294.1632782809801; Mon, 27 Sep 2021 15:46:49 -0700 (PDT) Received: from redhat.com ([198.99.80.109]) by smtp.gmail.com with ESMTPSA id y9sm4403779ooe.10.2021.09.27.15.46.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Sep 2021 15:46:49 -0700 (PDT) Date: Mon, 27 Sep 2021 16:46:48 -0600 From: Alex Williamson To: Leon Romanovsky Cc: Doug Ledford , Jason Gunthorpe , Yishai Hadas , Bjorn Helgaas , "David S. Miller" , Jakub Kicinski , Kirti Wankhede , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-rdma@vger.kernel.org, netdev@vger.kernel.org, Saeed Mahameed , Cornelia Huck Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity Message-ID: <20210927164648.1e2d49ac.alex.williamson@redhat.com> In-Reply-To: References: Organization: Red Hat X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 22 Sep 2021 13:38:51 +0300 Leon Romanovsky wrote: > From: Yishai Hadas > > Add an API in the core layer to check migration state transition validity > as part of a migration flow. > > The valid transitions follow the expected usage as described in > uapi/vfio.h and triggered by QEMU. > > This ensures that all migration implementations follow a consistent > migration state machine. > > Signed-off-by: Yishai Hadas > Reviewed-by: Kirti Wankhede > Signed-off-by: Jason Gunthorpe > Signed-off-by: Leon Romanovsky > --- > drivers/vfio/vfio.c | 41 +++++++++++++++++++++++++++++++++++++++++ > include/linux/vfio.h | 1 + > 2 files changed, 42 insertions(+) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 3c034fe14ccb..c3ca33e513c8 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1664,6 +1664,47 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep) > return 0; > } > > +/** > + * vfio_change_migration_state_allowed - Checks whether a migration state > + * transition is valid. > + * @new_state: The new state to move to. > + * @old_state: The old state. > + * Return: true if the transition is valid. > + */ > +bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state) > +{ > + enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING }; > + static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = { > + [VFIO_DEVICE_STATE_STOP] = { > + [VFIO_DEVICE_STATE_RUNNING] = 1, > + [VFIO_DEVICE_STATE_RESUMING] = 1, > + }, Our state transition diagram is pretty weak on reachable transitions out of the _STOP state, why do we select only these two as valid? Consistent behavior to userspace is of course nice, but I wonder if we were expecting a device reset to get us back to _RUNNING, or if the drivers would make use of the protocol through which a driver can nak (write error, no state change) or fault (_ERROR device state) a state change. There does need to be a way to get back to _RUNNING to support a migration failure without a reset, but would that be from _SAVING or from _STOP and what's our rationale for the excluded states? I'll see if I can dig through emails to find what was intended to be reachable from _STOP. Kirti or Connie, do you recall? Also, I think the _ERROR state is implicitly handled correctly here, its value is >MAX_STATE so we can't transition into or out of it, but a comment to indicate that it's been considered for this would be nice. > + [VFIO_DEVICE_STATE_RUNNING] = { > + [VFIO_DEVICE_STATE_STOP] = 1, > + [VFIO_DEVICE_STATE_SAVING] = 1, > + [VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = 1, > + }, Shameer's comment is correct here, _RESUMING is a valid next state since the default state is _RUNNING. > + [VFIO_DEVICE_STATE_SAVING] = { > + [VFIO_DEVICE_STATE_STOP] = 1, > + [VFIO_DEVICE_STATE_RUNNING] = 1, > + }, What's the rationale that we can't return to _SAVING|_RUNNING here? > + [VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RUNNING] = { > + [VFIO_DEVICE_STATE_RUNNING] = 1, > + [VFIO_DEVICE_STATE_SAVING] = 1, > + }, Can't we always _STOP the device at any point? > + [VFIO_DEVICE_STATE_RESUMING] = { > + [VFIO_DEVICE_STATE_RUNNING] = 1, > + [VFIO_DEVICE_STATE_STOP] = 1, > + }, Couldn't it be possible to switch immediately to _RUNNING|_SAVING for tracing purposes? Or _SAVING, perhaps to validate the restored state without starting the device? Thanks, Alex > + }; > + > + if (new_state > MAX_STATE || old_state > MAX_STATE) > + return false; > + > + return vfio_from_state_table[old_state][new_state]; > +} > +EXPORT_SYMBOL_GPL(vfio_change_migration_state_allowed); > + > static long vfio_device_fops_unl_ioctl(struct file *filep, > unsigned int cmd, unsigned long arg) > { > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index b53a9557884a..e65137a708f1 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -83,6 +83,7 @@ extern struct vfio_device *vfio_device_get_from_dev(struct device *dev); > extern void vfio_device_put(struct vfio_device *device); > > int vfio_assign_device_set(struct vfio_device *device, void *set_id); > +bool vfio_change_migration_state_allowed(u32 new_state, u32 old_state); > > /* events for the backend driver notify callback */ > enum vfio_iommu_notify_type {