Re: [PATCH v3 08/11] vpci/header: Emulate PCI_COMMAND register for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.11.2021 10:30, Oleksandr Andrushchenko wrote:
> 
> 
> On 03.11.21 11:24, Jan Beulich wrote:
>> On 03.11.2021 10:18, Oleksandr Andrushchenko wrote:
>>>
>>> On 03.11.21 11:11, Jan Beulich wrote:
>>>> On 03.11.2021 09:53, Oleksandr Andrushchenko wrote:
>>>>> On 02.11.21 16:10, Oleksandr Andrushchenko wrote:
>>>>>> On 02.11.21 15:54, Jan Beulich wrote:
>>>>>>> On 02.11.2021 12:50, Roger Pau Monné wrote:
>>>>>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote:
>>>>>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote:
>>>>>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko 
>>>>>>>>>> wrote:
>>>>>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>>>>>> @@ -451,6 +451,32 @@ 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. */
>>>>>>>>>>> +
>>>>>>>>>>> +    if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 )
>>>>>>>>>>> +    {
>>>>>>>>>>> +        /*
>>>>>>>>>>> +         * Guest wants to enable INTx. It can't be enabled if:
>>>>>>>>>>> +         *  - host has INTx disabled
>>>>>>>>>>> +         *  - MSI/MSI-X enabled
>>>>>>>>>>> +         */
>>>>>>>>>>> +        if ( pdev->vpci->msi->enabled )
>>>>>>>>>>> +            cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>>>>>>>> +        else
>>>>>>>>>>> +        {
>>>>>>>>>>> +            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, 
>>>>>>>>>>> reg);
>>>>>>>>>>> +
>>>>>>>>>>> +            if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
>>>>>>>>>>> +                cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>>>>>>>> +        }
>>>>>>>>>> This last part should be Arm specific. On other architectures we
>>>>>>>>>> likely want the guest to modify INTx disable in order to select the
>>>>>>>>>> interrupt delivery mode for the device.
>>>>>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X
>>>>>>>>> enabled - only one of the three is supposed to be active at a time.
>>>>>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when
>>>>>>>>> the bit is clear.)
>>>>>>>> Sure, but this code is making the bit sticky, by not allowing
>>>>>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on
>>>>>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs
>>>>>>>> to be Arm only.
>>>>>>> Isn't the "else" part questionable even on Arm?
>>>>>> It is. Once fixed I can't see anything Arm specific here
>>>>> Well, I have looked at the code one more time and everything seems to
>>>>> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and
>>>>> guest_cmd_write. The former is used for the hardware domain and has
>>>>> *no restrictions* on writing PCI_COMMAND register contents and the later
>>>>> is only used for guests and which does have restrictions applied in
>>>>> emulate_cmd_reg function.
>>>>>
>>>>> So, for the hardware domain, there is no "sticky" bit possible and for the
>>>>> guest domains if the physical contents of the PCI_COMMAND register
>>>>> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to
>>>>> use PCI_COMMAND_INTX_DISABLE bit set.
>>>>>
>>>>> So, from hardware domain POV, this should not be a problem, but from
>>>>> guests view it can. Let's imagine that the hardware domain can handle
>>>>> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware
>>>>> domain can decide what can be used for the interrupt source (again, no
>>>>> restriction here) and program PCI_COMMAND accordingly.
>>>>> Guest domains need to align with this configuration, e.g. if INTx was 
>>>>> disabled
>>>>> by the hardware domain then INTx cannot be enabled for guests
>>>> Why? It's the DomU that's in control of the device, so it ought to
>>>> be able to pick any of the three. I don't think Dom0 is involved in
>>>> handling of interrupts from the device, and hence its own "dislike"
>>>> of INTx ought to only extend to the period of time where Dom0 is
>>>> controlling the device. This would be different if Xen's view was
>>>> different, but as we seem to agree Xen's role here is solely to
>>>> prevent invalid combinations getting established in hardware.
>>> On top of a PCI device there is a physical host bridge and
>>> physical bus topology which may impose restrictions from
>>> Dom0 POV on that particular device.
>> Well, such physical restrictions may mean INTx doesn't actually work,
>> but this won't mean the DomU isn't free in choosing the bit's setting.
>> The bit merely controls whether the device is allowed to assert its
>> interrupt pin. Hence ...
>>
>>> So, every PCI device
>>> being passed through to a DomU may have different INTx
>>> settings which do depend on Dom0 in our case.
>> ... I'm still unconvinced of this.
> Ok, so I can accept any suggestion how to solve this. It seems that
> we already have number of no go scenarios here, but still it is not
> clear to me what could be an acceptable approach here. Namely:
> what do we do with INTx bit for guests?
> 1. I can leave it as is in the patch
> 2. I can remove INTx emulation and let the guest decide and program INTx
> 3. What else can I do?

Aiui you want to prevent the guest from clearing the bit if either
MSI or MSI-X are in use. Symmetrically, when the guest enables MSI
or MSI-X, you will want to force the bit set (which may well be in
a separate, future patch).

Jan