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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan