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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>>>>> 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 <jbeulich@xxxxxxxx>
>>>>>>>
>>>>>>> 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"

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.

Jan