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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 03.11.21 11:49, Jan Beulich wrote:
> 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).
static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd)
{
     /* 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 MSI/MSI-X 
enabled. */
#ifdef CONFIG_HAS_PCI_MSI
         if ( pdev->vpci->msi->enabled )
             cmd |= PCI_COMMAND_INTX_DISABLE;
#endif
     }

     return cmd;
}

Is this what you mean?
> Jan
>
Thank you,
Oleksandr