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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2022 15:26, Oleksandr Andrushchenko wrote:
> 
> 
> 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?

Again: Only one of INTX, MSI, or MSI-X may be enabled at a time.
This needs to be checked whenever any one of the three is about
to change state. Since failing config space writes isn't really
an option (there's no error code to hand back and raising an
exception is nothing real hardware would do), adjusting state to
be sane behind the back of the guest looks to be the least bad
option.

> This seems to be the only place in vPCI which touches PCI_COMMAND register.

How is this relevant?

Jan