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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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. So, every PCI device
being passed through to a DomU may have different INTx
settings which do depend on Dom0 in our case.
>
> Jan
>
>