Re: [PATCH v12 10/15] vpci/header: emulate PCI_COMMAND register for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.01.2024 22:51, Stewart Hildebrand wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -168,6 +168,9 @@ static void modify_decoding(const struct pci_dev *pdev, 
> uint16_t cmd,
>      if ( !rom_only )
>      {
>          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> +        /* Show DomU that we updated P2M */
> +        header->guest_cmd &= ~PCI_COMMAND_MEMORY;
> +        header->guest_cmd |= cmd & PCI_COMMAND_MEMORY;
>          header->bars_mapped = map;
>      }

I don't follow what the comment means to say. The bit in question has no
real connection to the P2M, and the guest also may have no notion of the
underlying hypervisor's internals. Likely connected to ...

> @@ -524,9 +527,26 @@ static void cf_check cmd_write(
>  {
>      struct vpci_header *header = data;
>  
> +    if ( !is_hardware_domain(pdev->domain) )
> +    {
> +        const struct vpci *vpci = pdev->vpci;
> +
> +        if ( (vpci->msi && vpci->msi->enabled) ||
> +             (vpci->msix && vpci->msix->enabled) )
> +            cmd |= PCI_COMMAND_INTX_DISABLE;
> +
> +        /*
> +         * Do not show change to PCI_COMMAND_MEMORY bit until we finish
> +         * modifying P2M mappings.
> +         */
> +        header->guest_cmd = (cmd & ~PCI_COMMAND_MEMORY) |
> +                            (header->guest_cmd & PCI_COMMAND_MEMORY);
> +    }

... the comment here, but then shouldn't it be that the guest can't even
issue a 2nd cfg space access until the present write has been carried out?
Otherwise I'd be inclined to claim that such a partial update is unlikely
to be spec-conformant.

> @@ -843,6 +885,15 @@ static int cf_check init_header(struct pci_dev *pdev)
>      if ( cmd & PCI_COMMAND_MEMORY )
>          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);
>  
> +    /*
> +     * Clear PCI_COMMAND_MEMORY and PCI_COMMAND_IO for DomUs, so they will
> +     * always start with memory decoding disabled and to ensure that we will 
> not
> +     * call modify_bars() at the end of this function.
> +     */
> +    if ( !is_hwdom )
> +        cmd &= ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
> +    header->guest_cmd = cmd;

With PCI_COMMAND_MEMORY clear, the hw reg won't further be written on the
success return path. Yet wouldn't we better clear PCI_COMMAND_IO also in
hardware (until we properly support it)?

I also think the insertion point for the new code isn't well chosen: The
comment just out of context indicates that the code in context above is
connected to the subsequent code. Whereas the addition is not.

> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -70,6 +70,15 @@ static void cf_check control_write(
>  
>          if ( vpci_msi_arch_enable(msi, pdev, vectors) )
>              return;
> +
> +        /*
> +         * Make sure domU doesn't enable INTx while enabling MSI.
> +         */

Nit: This ought to be a single line comment, just like ...

> +        if ( !is_hardware_domain(pdev->domain) )
> +        {
> +            pci_intx(pdev, false);
> +            pdev->vpci->header.guest_cmd |= PCI_COMMAND_INTX_DISABLE;
> +        }
>      }
>      else
>          vpci_msi_arch_disable(msi, pdev);
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -135,6 +135,13 @@ static void cf_check control_write(
>          }
>      }
>  
> +    /* Make sure domU doesn't enable INTx while enabling MSI-X. */
> +    if ( new_enabled && !msix->enabled && !is_hardware_domain(pdev->domain) )
> +    {
> +        pci_intx(pdev, false);
> +        pdev->vpci->header.guest_cmd |= PCI_COMMAND_INTX_DISABLE;
> +    }

... the similar code here has it.

In both cases, is it really appropriate to set the bit in guest view?

Jan