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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Nov 03, 2021 at 11:02:37AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 03.11.21 13:01, Roger Pau Monné wrote:
> > On Wed, Nov 03, 2021 at 10:36:36AM +0000, Oleksandr Andrushchenko wrote:
> >>
> >> On 03.11.21 12:34, Jan Beulich wrote:
> >>> On 03.11.2021 11:24, Oleksandr Andrushchenko wrote:
> >>>> On 03.11.21 11:49, Jan Beulich wrote:
> >>>>> 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?
> >>> Something along these lines, yes. I'd omit the outer if() for clarity /
> >>> brevity.
> >> Sure, thank you!
> >> @Roger are you ok with this approach?
> > Sure, I would even do:
> >
> > #ifdef CONFIG_HAS_PCI_MSI
> > if ( !(cmd & PCI_COMMAND_INTX_DISABLE) && pdev->vpci->msi->enabled )
> > {
> >      /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X 
> > enabled. */
> >      cmd |= PCI_COMMAND_INTX_DISABLE;
> > }
> > #endif
> >
> > There's no need for the outer check if there's no support for MSI.
> Ok, sounds good!
> Thank you both!!

In fact you could even remove the check for !(cmd &
PCI_COMMAND_INTX_DISABLE) and always set PCI_COMMAND_INTX_DISABLE if
MSI is enabled, which I think is what Jan was pointing to in his
previous reply.

Regards, Roger.