[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v13 10/14] vpci/header: emulate PCI_COMMAND register for guests
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |