Re: [PATCH v6] vpci: Add resizable bar support

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/2/5 17:58, Jan Beulich wrote:
> On 05.02.2025 10:12, Chen, Jiqian wrote:
>> On 2025/2/5 16:56, Roger Pau Monné wrote:
>>> On Wed, Feb 05, 2025 at 03:42:30AM +0000, Chen, Jiqian wrote:
>>>> On 2025/1/27 23:08, Roger Pau Monné wrote:
>>>>> On Mon, Jan 27, 2025 at 03:52:31PM +0100, Jan Beulich wrote:
>>>>>> On 27.01.2025 15:41, Roger Pau Monné wrote:
>>>>>>> Ideally errors here should be dealt with by masking the capability.
>>>>>>> However Xen doesn't yet have that support.  The usage of continue is
>>>>>>> to merely attempt to keep any possible setup hooks working (header,
>>>>>>> MSI, MSI-X). Returning failure from init_rebar() will cause all
>>>>>>> vPCI hooks to be removed, and thus the hardware domain to have
>>>>>>> unmediated access to the device, which is likely worse than just
>>>>>>> continuing here.
>>>>>>
>>>>>> Hmm, true. Maybe with the exception of the case where the first reg
>>>>>> registration works, but the 2nd fails. Since CTRL is writable but
>>>>>> CAP is r/o (and data there is simply being handed through) I wonder
>>>>>> whether we need to intercept CAP at all, and if we do, whether we
>>>>>> wouldn't better try to register CTRL first.
>>>>>
>>>>> Indeed, Jiqian is that a leftover from a previous version when writes
>>>>> to CAP where ignored for being a read-only register?
>>>> Sorry to reply late, I just came back from an annual leave.
>>>> Did you mean: why I added handler vpci_hw_write32 for CAP?
>>>> If so, this is a change since V2, that you suggested to add it because 
>>>> there is no write limitation for dom0.
>>>
>>> Indeed, if there's no write limitation, you can just drop the addition
>>> of the traps, as the hardware domain by default gets read and write
>>> access to all PCI config space.  IOW: there's no need for a
>>> vpci_add_register() call for PCI_REBAR_CAP if the handlers are just
>>> vpci_hw_{read,write}32().
>> OK, I think so.
>>
>> Hi Jan, can this change meet your opinion?
>> Not to add register for CAP, and if fail to add register for CTRL, then 
>> "continue"
> 
> Well, Roger as the maintainer has indicated to go that route. That's okay
> with me. My only request then is to add a comment there, summarizing what
> he said earlier on.
Thanks.
How about adding below comments near adding register for CTRL?

        /*
         * Here not to add register for PCI_REBAR_CAP since it is read-only
         * register without other specific operations, and hardware domain
         * has no limitation of read/write access to all PCI config space.
         */
        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
                               rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
        if ( rc )
        {
            printk(XENLOG_ERR "%pd %pp: BAR%u fail to add reg of REBAR_CTRL 
rc=%d\n",
                   pdev->domain, &pdev->sbdf, index, rc);
            /*
             * The reason of using continue here is that ideally failing here
             * should hide ReBar capability, but Xen doesn't yet support that,
             * and using continue can keep any possible hooks working, instead,
             * returning failure will cause all vPCI hooks down and hardware
             * domain has unmediated access to devices, which is worse.
             */
            continue;
        }

> 
> Jan

-- 
Best regards,
Jiqian Chen.