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

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

On 2025/2/5 18:40, Jan Beulich wrote:
> On 05.02.2025 11:31, Chen, Jiqian wrote:
>> 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;
>>         }
> 
> I consider this too verbose. How about you start with "Ideally we would hide
> the ReBar capability here, but code for doing so still needs to be written."
> Later in the long sentence there's then "will" which I think you mean to be
> "would". The "unmediated" otoh, needs further qualifying: It's not "devices"
> aiui (but just the one device we're dealing with here), and I think you mean
> "entirely unmediated" (as opposed to "unmediated access to just this one
> register").
Thank you!
After modifying, comments are:

        /*
         * 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);
            /*
             * Ideally we would hide the ReBar capability here, but code
             * for doing so still needs to be written. And using continue
             * can keep any possible hooks working, instead, returning
             * failure would cause all vPCI hooks down and hardware domain
             * has entirely unmediated access to the device, which is worse.
             */
            continue;
        }

> 
> Jan
> 

-- 
Best regards,
Jiqian Chen.