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

[Jan Beulich <jbeulich@xxxxxxxx>]

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").

Jan