Re: [PATCH v13 10/14] vpci/header: emulate PCI_COMMAND register for guests

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 2/14/24 10:41, Jan Beulich wrote:
> On 02.02.2024 22:33, Stewart Hildebrand wrote:
>> @@ -836,9 +870,20 @@ static int cf_check init_header(struct pci_dev *pdev)
>>      if ( pdev->ignore_bars )
>>          return 0;
>>  
>> -    /* Disable memory decoding before sizing. */
>>      cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
>> -    if ( 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.
> 
> To achieve this, fiddling with PCI_COMMAND_IO isn't necessary. Which isn't
> to say its clearing should go away; quite the other way around: Why would
> we leave e.g. PCI_COMMAND_MASTER enabled? In fact wasn't it in an earlier
> version of the series that the guest view simply started out as zero? The
> patch description still says so.

Yep, clearing PCI_COMMAND_MASTER too for domUs makes sense to me, I'll
make this change in v14. I'll also try to improve the comment.

Roger suggested at [1] that we should reflect the state of the hardware
in the command register. I'll update the patch description accordingly.

Archaeology/notes/references follow, primarily for my own reference:

Note that the rsvdp_mask will be applied to the guest_cmd value before
being returned to the guest, so no need to apply masks here.

Clearing both PCI_COMMAND_MEMORY and PCI_COMMAND_IO for domUs was
suggested by Roger at [2] and [3]. It is currently problematic for
devices assigned to domUs to have memory decoding enabled at this stage
because we don't yet have a good/generic way to initialize
bar.guest_addr taking the domU memory layout into account.

Reminder that we want to leave the PCI_COMMAND_{MASTER,MEMORY,IO} bits
unchanged for devices assigned to dom0. A description of why can be
found in the commit message of:

53d9133638c3 ("pci: do not disable memory decoding for devices").

[1] 
https://lore.kernel.org/xen-devel/ZLqI65gmNj1XDBm4@MacBook-Air-de-Roger.local/
[2] https://lore.kernel.org/xen-devel/ZRquRcRz-K43WeMc@MacBookPdeRoger/
[3] https://lore.kernel.org/xen-devel/ZVy73iJ3E8nJHvgf@macbook.local/

>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -70,6 +70,13 @@ 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. */
>> +        if ( !is_hardware_domain(pdev->domain) )
>> +        {
>> +            pci_intx(pdev, false);
>> +            pdev->vpci->header.guest_cmd |= PCI_COMMAND_INTX_DISABLE;
>> +        }
> 
> While here we're inside "if ( new_enabled )", ...
> 
>> --- 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;
>> +    }
> 
> .. here you further check that it's actually a 0->1 transition? Why
> not alike for MSI?

Good catch, we should similarly check for a 0->1 transition for MSI.
I'll fix it.