Re: [PATCH v5 09/14] vpci/header: emulate PCI_COMMAND register for guests

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 02.02.22 16:18, Jan Beulich wrote:
> On 02.02.2022 14:47, Oleksandr Andrushchenko wrote:
>>> On 02.02.2022 13:49, Oleksandr Andrushchenko wrote:
>>>> On 13.01.22 12:50, Roger Pau Monné wrote:
>>>>> On Thu, Nov 25, 2021 at 01:02:46PM +0200, Oleksandr Andrushchenko wrote:
>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>> @@ -491,6 +491,22 @@ static void cmd_write(const struct pci_dev *pdev, 
>>>>>> unsigned int reg,
>>>>>>             pci_conf_write16(pdev->sbdf, reg, cmd);
>>>>>>     }
>>>>>>     
>>>>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int 
>>>>>> reg,
>>>>>> +                            uint32_t cmd, void *data)
>>>>>> +{
>>>>>> +    /* TODO: Add proper emulation for all bits of the command register. 
>>>>>> */
>>>>>> +
>>>>>> +#ifdef CONFIG_HAS_PCI_MSI
>>>>>> +    if ( pdev->vpci->msi->enabled )
>>>>> You need to check for MSI-X also, pdev->vpci->msix->enabled.
>>>> Indeed, thank you
>>>>>> +    {
>>>>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X 
>>>>>> enabled. */
>>>>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>> You will also need to make sure PCI_COMMAND_INTX_DISABLE is set in the
>>>>> command register when attempting to enable MSI or MSIX capabilities.
>>>> Isn't it enough that we just check above if MSI/MSI-X enabled then make
>>>> sure INTX disabled? I am not following you here on what else needs to
>>>> be done.
>>> No, you need to deal with the potentially bad combination on both
>>> paths - command register writes (here) and MSI/MSI-X control register
>>> writes (which is what Roger points you at). I would like to suggest
>>> to consider simply forcing INTX_DISABLE on behind the guest's back
>>> for those other two paths.
>> Do you suggest that we need to have some code which will
>> write PCI_COMMAND while we write MSI/MSI-X control register
>> for that kind of consistency? E.g. control register handler will
>> need to write to PCI_COMMAND and go through emulation for
>> guests?
> Either check or write, yes. Since you're setting the bit here behind
> the guest's back, setting it on the other paths as well would only
> look consistent to me.
I can't find any access to PCI_COMMAND register from vMSI/vMSI-X
code, so what's the concern? This seems to be the only place in vPCI
which touches PCI_COMMAND register.
>
>> If so, why didn't we have that before?
> Because we assume Dom0 to be behaving itself.
ok...
>
> Jan
>
Thank you,
Oleksandr