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.
