Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Jason Gunthorpe <jgg@ziepe.ca>
Cc:     Shannon Nelson <shannon.nelson@amd.com>,
        alex.williamson@redhat.com, kevin.tian@intel.com,
        reinette.chatre@intel.com, kvm@vger.kernel.org,
        brett.creeley@amd.com, linux-kernel@vger.kernel.org,
        Leon Romanovsky <leonro@nvidia.com>
Subject: Re: [PATCH vfio] vfio/pci: remove msi domain on msi disable
In-Reply-To: <20230919000215.GQ13795@ziepe.ca>
References: <20230914191406.54656-1-shannon.nelson@amd.com>
 <20230918141705.GE13795@ziepe.ca> <87led3xqye.ffs@tglx>
 <20230918233735.GP13795@ziepe.ca> <87a5tjxcva.ffs@tglx>
 <20230919000215.GQ13795@ziepe.ca>
Date:   Tue, 19 Sep 2023 02:25:32 +0200
Message-ID: <874jjrxb43.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Mon, Sep 18 2023 at 21:02, Jason Gunthorpe wrote:
> On Tue, Sep 19, 2023 at 01:47:37AM +0200, Thomas Gleixner wrote:
>> >> > The intree way to alter the MSI configuration is via
>> >> > sriov_set_msix_vec_count, and there is only one in-tree driver that
>> >> > uses it right now.
>> >> 
>> >> Right, but that only addresses the driver specific issues.
>> >
>> > Sort of.. sriov_vf_msix_count_store() is intended to be the entry
>> > point for this and if the kernel grows places that cache the value or
>> > something then this function should flush those caches too.
>> 
>> Sorry. What I wanted to say is that the driver callback is not the right
>> place to reload the MSI domains after the change.
>
> Oh, that isn't even what Shannon's patch does, it patched VFIO's main
> PCI driver - not a sriov_set_msix_vec_count() callback :( Shannon's
> scenario doesn't even use sriov_vf_msix_count_store() at all - the AMD
> device just randomly changes its MSI count whenever it likes.

Ooops. When real hardware changes things behind the kernels back we
consider it a hardware bug. The same applies to virtualization muck.

So all we should do is add some code which yells when the "hardware"
plays silly buggers.

>> > I suppose flushing happens implicitly because Shannon reports that
>> > things work fine if the driver is rebound. Since
>> > sriov_vf_msix_count_store() ensures there is no driver bound before
>> > proceeding it probe/unprobe must be flushing out everything?
>> 
>> Correct. So sriov_set_msix_vec_count() could just do:
>> 
>> 	ret = pdev->driver->sriov_set_msix_vec_count(vf_dev, val);
>>         if (!ret)
>>         	teardown_msi_domain(pdev);
>>
>> Right?
>
> It subtly isn't needed, sriov_vf_msix_count_store() already requires
> no driver is associated with the device and this:
>
> int msi_setup_device_data(struct device *dev)
> {
> 	struct msi_device_data *md;
> 	int ret, i;
>
> 	if (dev->msi.data)
> 		return 0;
>
> 	md = devres_alloc(msi_device_data_release, sizeof(*md), GFP_KERNEL);
> 	if (!md)
> 		return -ENOMEM;
>
> Already ensured that msi_remove_device_irq_domain() was called via
> msi_device_data_release() triggering as part of the devm shutdown of
> the bound driver.

Indeed.

> So, the intree mechanism to change the MSI vector size works. The
> crazy mechanism where the device just changes its value without
> synchronizing to the OS does not.
>
> I don't think we need to try and fix that..

We might want to detect it and yell about it, right?

Thanks,

        tglx