Re: [PATCH v2 6/8] vpci: add SR-IOV support for PVH Dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.03.2026 12:08, Mykyta Poturai wrote:
> +static void cf_check control_write(const struct pci_dev *pdev, unsigned int 
> reg,
> +                                   uint32_t val, void *data)
> +{
> +    unsigned int sriov_pos = reg - PCI_SRIOV_CTRL;
> +    struct vpci_sriov *sriov = pdev->vpci->sriov;
> +    struct callback_data *cb = NULL;
> +    uint16_t control = pci_conf_read16(pdev->sbdf, reg);
> +    bool mem_enabled = control & PCI_SRIOV_CTRL_MSE;
> +    bool new_mem_enabled = val & PCI_SRIOV_CTRL_MSE;
> +    bool enabled = control & PCI_SRIOV_CTRL_VFE;
> +    bool new_enabled = val & PCI_SRIOV_CTRL_VFE;
> +
> +    ASSERT(!pdev->info.is_virtfn);
> +
> +    if ( new_enabled == enabled && new_mem_enabled == mem_enabled )
> +    {
> +        pci_conf_write16(pdev->sbdf, reg, val);
> +        return;
> +    }
> +
> +    cb = xzalloc(struct callback_data);
> +
> +    if ( !cb )
> +    {
> +        gprintk(XENLOG_ERR,
> +                "%pp: Unable to allocate memory for SR-IOV enable\n",
> +                pdev);
> +        return;
> +    }
> +
> +    cb->pdev = pdev;
> +    cb->pos = sriov_pos;
> +    cb->value = val;
> +    cb->map = new_mem_enabled && !mem_enabled;
> +    cb->unmap = !new_mem_enabled && mem_enabled;
> +    cb->enable = new_enabled && !enabled;
> +    cb->disable = !new_enabled && enabled;
> +
> +    current->vpci.task = WAIT;
> +    current->vpci.wait.callback = control_write_cb;
> +    current->vpci.wait.data = cb;
> +    current->vpci.wait.end = NOW();
> +
> +    if ( cb->enable )
> +    {
> +        size_vf_bars((struct pci_dev *)pdev, sriov_pos);

No casting away of const, please. See Misra rule 11.8.

> +        /*
> +         * Only update the number of active VFs when enabling, when
> +         * disabling use the cached value in order to always remove the same
> +         * number of VFs that were active.
> +         */
> +        sriov->num_vfs = pci_conf_read16(pdev->sbdf,
> +                                         sriov_pos + PCI_SRIOV_NUM_VF);
> +        /*
> +         * NB: VFE needs to be enabled before calling pci_add_device so Xen
> +         * can access the config space of VFs. FIXME casting away const-ness
> +         * to modify vf_rlen
> +         */
> +        pci_conf_write16(pdev->sbdf, reg, control | PCI_SRIOV_CTRL_VFE);
> +        /*
> +         * The spec states that the software must wait at least 100ms before
> +         * attempting to access VF registers when enabling virtual functions
> +         * on the PF.
> +         */
> +
> +        current->vpci.wait.end = NOW() + MILLISECS(100);

Aren't you effectively busy-waiting for these 100ms, by simply returning "true"
from vpci_process_pending() until the time has passed? This imo is a no-go. You
want to set a timer and put the vCPU to sleep, to wake it up again when the
timer has expired. That'll then eliminate the need for the not-so-nice patch 4.

Question is whether we need to actually go this far (right away). I expect you
don't mean to hand PFs to DomU-s. As long as we keep them in the hardware
domain, can't we trust it to set things up correctly, just like we trust it in
a number of other aspects?

Jan