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.
