On Tue, 2023-12-19 at 15:02 +0100, Eugenio Perez Martin wrote: > On Tue, Dec 19, 2023 at 12:16 PM Dragos Tatulea <dtatu...@nvidia.com> wrote: > > > > On Tue, 2023-12-19 at 08:24 +0100, Eugenio Perez Martin wrote: > > > On Mon, Dec 18, 2023 at 2:58 PM Dragos Tatulea <dtatu...@nvidia.com> > > > wrote: > > > > > > > > On Mon, 2023-12-18 at 13:06 +0100, Eugenio Perez Martin wrote: > > > > > On Mon, Dec 18, 2023 at 11:52 AM Dragos Tatulea <dtatu...@nvidia.com> > > > > > wrote: > > > > > > > > > > > > On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote: > > > > > > > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea > > > > > > > <dtatu...@nvidia.com> wrote: > > > > > > > > > > > > > > > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > > > > > > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea > > > > > > > > > <dtatu...@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > > > > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin > > > > > > > > > > > wrote: > > > > > > > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea > > > > > > > > > > > > <dtatu...@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos > > > > > > > > > > > > > > Tatulea wrote: > > > > > > > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos > > > > > > > > > > > > > > > > > Tatulea <dtatu...@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq > > > > > > > > > > > > > > > > > > addresses will be updated on > > > > > > > > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea > > > > > > > > > > > > > > > > > > <dtatu...@nvidia.com> > > > > > > > > > > > > > > > > > > Reviewed-by: Gal Pressman <g...@nvidia.com> > > > > > > > > > > > > > > > > > > Acked-by: Eugenio Pérez > > > > > > > > > > > > > > > > > > <epere...@redhat.com> > > > > > > > > > > > > > > > > > I'm kind of ok with this patch and the next > > > > > > > > > > > > > > > > > one about state, but I > > > > > > > > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to > > > > > > > > > > > > > > > > > change the vq address after > > > > > > > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only > > > > > > > > > > > > > > > > > memory maps are ok to > > > > > > > > > > > > > > > > > change at this moment. I'm not sure about vq > > > > > > > > > > > > > > > > > state in vDPA, but vhost > > > > > > > > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this > > > > > > > > > > > > > > > > > moment though, so maybe it is > > > > > > > > > > > > > > > > > ok to decide that all of these parameters may > > > > > > > > > > > > > > > > > change during suspend. > > > > > > > > > > > > > > > > > Maybe the best thing is to protect this with > > > > > > > > > > > > > > > > > a vDPA feature flag. > > > > > > > > > > > > > > > > I think protect with vDPA feature flag could > > > > > > > > > > > > > > > > work, while on the other > > > > > > > > > > > > > > > > hand vDPA means vendor specific optimization is > > > > > > > > > > > > > > > > possible around suspend > > > > > > > > > > > > > > > > and resume (in case it helps performance), > > > > > > > > > > > > > > > > which doesn't have to be > > > > > > > > > > > > > > > > backed by virtio spec. Same applies to vhost > > > > > > > > > > > > > > > > user backend features, > > > > > > > > > > > > > > > > variations there were not backed by spec > > > > > > > > > > > > > > > > either. Of course, we should > > > > > > > > > > > > > > > > try best to make the default behavior backward > > > > > > > > > > > > > > > > compatible with > > > > > > > > > > > > > > > > virtio-based backend, but that circles back to > > > > > > > > > > > > > > > > no suspend definition in > > > > > > > > > > > > > > > > the current virtio spec, for which I hope we > > > > > > > > > > > > > > > > don't cease development on > > > > > > > > > > > > > > > > vDPA indefinitely. After all, the virtio based > > > > > > > > > > > > > > > > vdap backend can well > > > > > > > > > > > > > > > > define its own feature flag to describe (minor > > > > > > > > > > > > > > > > difference in) the > > > > > > > > > > > > > > > > suspend behavior based on the later spec once > > > > > > > > > > > > > > > > it is formed in future. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So what is the way forward here? From what I > > > > > > > > > > > > > > > understand the options are: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device > > > > > > > > > > > > > > > properties while suspended. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. > > > > > > > > > > > > > > > Not sure if this makes sense as > > > > > > > > > > > > > > > this. But then Si-Wei's qemu device > > > > > > > > > > > > > > > suspend/resume poc [0] that exercises this > > > > > > > > > > > > > > > code won't work anymore. This means the series > > > > > > > > > > > > > > > would be less well tested. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Are there other possible options? What do you > > > > > > > > > > > > > > > think? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [0] > > > > > > > > > > > > > > > https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > > > > > > > > > > > > > > > How about allowing the change only under the > > > > > > > > > > > > > following conditions: > > > > > > > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > > > > > > > I think the best option by far is 1, as there is no > > > > > > > > > > > > hint in the > > > > > > > > > > > > combination of these 3 indicating that you can change > > > > > > > > > > > > device > > > > > > > > > > > > properties in the suspended state. > > > > > > > > > > > > > > > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > > > > > > > > > > > > > > > Another series can implement option 2 and add these 2 > > > > > > > > > > > patches on top. > > > > > > > > > > Hmm...I misunderstood your statement and sent a erroneus > > > > > > > > > > v3. You said that > > > > > > > > > > having a feature flag is the best option. > > > > > > > > > > > > > > > > > > > > Will add a feature flag in v4: is this similar to the > > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Right, it should be easy to return it from > > > > > > > > > .get_backend_features op if > > > > > > > > > the FW returns that capability, isn't it? > > > > > > > > > > > > > > > > > Yes, that's easy. But I wonder if we need one feature bit for > > > > > > > > each type of > > > > > > > > change: > > > > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > > > > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > > > > > > > > > > > > > > > > > > > > > > I'd say yes. Although we could configure SVQ initial state in > > > > > > > userland > > > > > > > as different than 0 for this first step, it would be needed in the > > > > > > > long term. > > > > > > > > > > > > > > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND > > > > > > > > suffice? > > > > > > > > > > > > > > > > > > > > > > I'd say "reconfig vq" is not valid as mlx driver doesn't allow > > > > > > > changing queue sizes, for example, isn't it? > > > > > > > > > > > > > Modifying the queue size for a vq is indeed not supported by the > > > > > > mlx device. > > > > > > > > > > > > > To define that it is > > > > > > > valid to change "all parameters" seems very confident. > > > > > > > > > > > > > Ack > > > > > > > > > > > > > > To me having individual feature bits makes sense. But it could > > > > > > > > also takes too > > > > > > > > many bits if more changes are required. > > > > > > > > > > > > > > > > > > > > > > Yes, that's a good point. Maybe it is valid to define a subset of > > > > > > > features that can be changed., but I think it is way clearer to > > > > > > > just > > > > > > > check for individual feature bits. > > > > > > > > > > > > > I will prepare extra patches with the 2 feature bits approach. > > > > > > > > > > > > Is it necessary to add checks in the vdpa core that block changing > > > > > > these > > > > > > properties if the state is driver ok and the device doesn't support > > > > > > the feature? > > > > > > > > > > > > > > > > Yes, I think it is better to protect for changes in vdpa core. > > > > > > > > > Hmmm... there is no suspended state available. I would only add checks > > > > for the > > > > DRIVER_OK state of the device because adding a is_suspended state/op > > > > seems out > > > > of scope for this series. Any thoughts? > > > > > > > > > > I can develop it so you can include it in your series for sure, I will > > > send it ASAP. > > > > > If it's a matter of: > > - Adding a suspended state to struct vhost_vdpa. > > - Setting it to true on successful device suspend. > > - Clearing it on successful device resume and device reset. > > > > I can add this patch. I'm just not sure about the locking part. But maybe I > > can > > send it and we can debate on the code. > > > > Yes, that was the plan basically. > > I think vhost_vdpa_suspend / vhost_vdpa_resume are already called from > vhost_vdpa_unlocked_ioctl with the lock acquired, is that what you > mean? > Yes, that's what I wanted to make sure that is correct. I will send the v4 soon.
Thanks, Dragos
