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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 02.02.22 17:08, Jan Beulich wrote:
> On 02.02.2022 16:04, Oleksandr Andrushchenko wrote:
>>
>> On 02.02.22 16:31, Jan Beulich wrote:
>>> 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 is clear and I don't question that
>>> 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.
>> Would it be enough if I read PCI_MSIX_FLAGS_ENABLE and
>> PCI_MSI_FLAGS_ENABLE in guest_cmd_write to make a
>> decision on INTX?
>>
>> On the other hand msi->enabled and msix->enabled
>> already have this information if I understand the
>> MSI/MSI-X code correctly.
>>
>> Or do we want some additional code in MSI/MSI-X's control_write
>> functions to set INTX bit there as well?
> Well, yes, this is what Roger and I have been asking you to add.
Do we only want this for !is_hardware_domain(d) or unconditionally?
>
>> I mean that in this guest_cmd_write handler we can only see
>> if we write a consistent wrt MSI/MSI-X PCI_COMMAND value
>>
>> If we want some more checks when we alter PCI_MSIX_FLAGS_ENABLE
>> and/or PCI_MSI_FLAGS_ENABLE bits, this means we need a relevant
>> PCI_COMMAND write there to be added (which doesn't exist now)
>> to make sure INTX bit is set.
> Exactly.
Ok
>
> Jan
>
Thank you,
Oleksandr