On 23.05.2023 12:59, Roger Pau Monné wrote:
> On Tue, May 23, 2023 at 08:44:48AM +0200, Jan Beulich wrote:
>> On 23.05.2023 00:20, Stefano Stabellini wrote:
>>> On Sat, 20 May 2023, Roger Pau Monné wrote:
>>>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>>>> index ec2e978a4e6b..0ff8e940fa8d 100644
>>>> --- a/xen/drivers/vpci/header.c
>>>> +++ b/xen/drivers/vpci/header.c
>>>> @@ -289,6 +289,13 @@ static int modify_bars(const struct pci_dev *pdev,
>>>> uint16_t cmd, bool rom_only)
>>>> */
>>>> for_each_pdev ( pdev->domain, tmp )
>>>> {
>>>> + if ( !tmp->vpci )
>>>> + {
>>>> + printk(XENLOG_G_WARNING "%pp: not handled by vPCI for %pd\n",
>>>> + &tmp->sbdf, pdev->domain);
>>>> + continue;
>>>> + }
>>>> +
>>>> if ( tmp == pdev )
>>>> {
>>>> /*
>>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>>> index 652807a4a454..0baef3a8d3a1 100644
>>>> --- a/xen/drivers/vpci/vpci.c
>>>> +++ b/xen/drivers/vpci/vpci.c
>>>> @@ -72,7 +72,12 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>>> unsigned int i;
>>>> int rc = 0;
>>>>
>>>> - if ( !has_vpci(pdev->domain) )
>>>> + if ( !has_vpci(pdev->domain) ||
>>>> + /*
>>>> + * Ignore RO and hidden devices, those are in use by Xen and vPCI
>>>> + * won't work on them.
>>>> + */
>>>> + pci_get_pdev(dom_xen, pdev->sbdf) )
>>>> return 0;
>>>>
>>>> /* We should not get here twice for the same device. */
>>>
>>>
>>> Now this patch works! Thank you!! :-)
>>>
>>> You can check the full logs here
>>> https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/4329259080
>>>
>>> Is the patch ready to be upstreamed aside from the commit message?
>>
>> I don't think so. vPCI ought to work on "r/o" devices. Out of curiosity,
>> have you also tried my (hackish and hence RFC) patch [1]?
>
> For r/o devices there should be no need of vPCI handlers, reading the
> config space of such devices can be done directly.
>
> There's some work to be done for hidden devices, as for those dom0 has
> write access to the config space and thus needs vPCI to be setup
> properly.
But then isn't it going to complicate things when dealing with r/o and
hidden devices differently?
> The change to modify_bars() in order to handle devices without vpci
> populated is a bugfix, as it's already possible to have devices
> assigned to a domain that don't have vpci setup, if the call to
> vpci_add_handlers() in setup_one_hwdom_device() fails. That one could
> go in separately of the rest of the work in order to enable support
> for hidden devices.
You saying "assigned to a domain" makes this sound more problematic
than it probably is: If it really was any domain other than Dom0, I
think there would be a security concern. Yet even for Dom0 I wonder
what good can come out of there not being proper vPCI setup for a
device.
Jan
