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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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
>
> Jan
>