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:
>>>>> On Mon, Jan 27, 2025 at 03:20:40PM +0100, Jan Beulich wrote:
>>>>>> On 23.01.2025 04:50, Jiqian Chen wrote:
>>>>>>> v5->v6 changes:
>>>>>>> * Changed "1UL" to "1ULL" in PCI_REBAR_CTRL_SIZE idefinition for 32 bit
>>>>>>> architecture.
>>>>>>> * In rebar_ctrl_write used "bar - pdev->vpci->header.bars" to get index
>>>>>>> instead of reading
>>>>>>> from register.
>>>>>>> * Added the index of BAR to error messages.
>>>>>>> * Changed to "continue" instead of "return an error" when
>>>>>>> vpci_add_register failed.
>>>>>>
>>>>>> I'm not convinced this was a good change to make. While ...
>>>>>>
>>>>>>> +static int cf_check init_rebar(struct pci_dev *pdev)
>>>>>>> +{
>>>>>>> + uint32_t ctrl;
>>>>>>> + unsigned int nbars;
>>>>>>> + unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
>>>>>>> +
>>>>>>> PCI_EXT_CAP_ID_REBAR);
>>>>>>> +
>>>>>>> + if ( !rebar_offset )
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + if ( !is_hardware_domain(pdev->domain) )
>>>>>>> + {
>>>>>>> + printk(XENLOG_ERR "%pp: resizable BARs unsupported for unpriv
>>>>>>> %pd\n",
>>>>>>> + &pdev->sbdf, pdev->domain);
>>>>>>> + return -EOPNOTSUPP;
>>>>>>> + }
>>>>>>> +
>>>>>>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset +
>>>>>>> PCI_REBAR_CTRL(0));
>>>>>>> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>>>>>>> + for ( unsigned int i = 0; i < nbars; i++ )
>>>>>>> + {
>>>>>>> + int rc;
>>>>>>> + struct vpci_bar *bar;
>>>>>>> + unsigned int index;
>>>>>>> +
>>>>>>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset +
>>>>>>> PCI_REBAR_CTRL(i));
>>>>>>> + index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
>>>>>>> + if ( index >= PCI_HEADER_NORMAL_NR_BARS )
>>>>>>> + {
>>>>>>> + printk(XENLOG_ERR "%pd %pp: too big BAR number %u in
>>>>>>> REBAR_CTRL\n",
>>>>>>> + pdev->domain, &pdev->sbdf, index);
>>>>>>> + continue;
>>>>>>> + }
>>>>>>> +
>>>>>>> + bar = &pdev->vpci->header.bars[index];
>>>>>>> + if ( bar->type != VPCI_BAR_MEM64_LO && bar->type !=
>>>>>>> VPCI_BAR_MEM32 )
>>>>>>> + {
>>>>>>> + printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory
>>>>>>> space\n",
>>>>>>> + pdev->domain, &pdev->sbdf, index);
>>>>>>> + continue;
>>>>>>> + }
>>>>>>
>>>>>> ... for these two cases we can permit Dom0 direct access because the BAR
>>>>>> isn't going to work anyway (as far as we can tell), ...
>>>>>>
>>>>>>> + rc = vpci_add_register(pdev->vpci,
>>>>>>> vpci_hw_read32vpci_hw_read32, vpci_hw_write32,
>>>>>>> + rebar_offset + PCI_REBAR_CAP(i), 4,
>>>>>>> NULL);
>>>>>>> + if ( rc )
>>>>>>> + {
>>>>>>> + /*
>>>>>>> + * TODO: for failed pathes, need to hide ReBar capability
>>>>>>> + * from hardware domain instead of returning an error.
>>>>>>> + */
>>>>>>> + printk(XENLOG_ERR "%pd %pp: BAR%u fail to add reg of
>>>>>>> REBAR_CAP rc=%d\n",
>>>>>>> + pdev->domain, &pdev->sbdf, index, rc);
>>>>>>> + continue;
>>>>>>> + }
>>>>>>> +
>>>>>>> + 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);
>>>>>>> + continue;
>>>>>>> + }
>>>>>>
>>>>>> ... in these two cases we had an issue internally, and would hence
>>>>>> wrongly
>>>>>> allow Dom0 direct access (and in case it's the 2nd one that failed, in
>>>>>> fact
>>>>>> only partially direct access, with who knows what resulting
>>>>>> inconsistencies).
>>>>>>
>>>>>> Only with this particular change undone:
>>>>> R> Reviewed-by: Jan Beulich <jbeul...@suse.com>
>>>>>>
>>>>>> Otherwise you and Roger (who needs to at least ack the change anyway)
>>>>>> will
>>>>>> need to sort that out, with me merely watching.
>>>>>
>>>>> 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"
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
