| Message ID | 20241210162546.403882-1-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/vpci: fix memory type in guest_mem_bar_read() | expand |
On 10.12.2024 17:25, Stewart Hildebrand wrote: > Currently, if bar->type is anything other than VPCI_BAR_MEM32, the > memory type bits get set to PCI_BASE_ADDRESS_MEM_TYPE_64 in the returned > value. This leads to the wrong memory type for, e.g. VPCI_BAR_EMPTY. > Only set PCI_BASE_ADDRESS_MEM_TYPE_64 when the bar type is > VPCI_BAR_MEM64_LO. Yet would guest_mem_bar_read() ever be installed as a handler for an empty BAR? If so, that's the mistake, I think. Jan
On Tue, Dec 10, 2024 at 11:25:44AM -0500, Stewart Hildebrand wrote: > Currently, if bar->type is anything other than VPCI_BAR_MEM32, the > memory type bits get set to PCI_BASE_ADDRESS_MEM_TYPE_64 in the returned > value. This leads to the wrong memory type for, e.g. VPCI_BAR_EMPTY. > Only set PCI_BASE_ADDRESS_MEM_TYPE_64 when the bar type is > VPCI_BAR_MEM64_LO. I'm confused, VPCI_BAR_EMPTY shouldn't use guest_mem_bar_read() in the first place, as its read handler should be vpci_read_val() instead. Is there something I'm missing from init_header()? if ( size == 0 ) { bars[i].type = VPCI_BAR_EMPTY; if ( !is_hwdom ) { rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, reg, 4, (void *)0); if ( rc ) goto fail; } continue; } AFAICT guest_mem_bar_read() should only handle BAR types that are either VPCI_BAR_MEM32, VPCI_BAR_MEM64_HI or VPCI_BAR_MEM64_LO, and that seems to be correctly handled? Thanks, Roger.
On 12/10/24 11:37, Roger Pau Monné wrote: > On Tue, Dec 10, 2024 at 11:25:44AM -0500, Stewart Hildebrand wrote: >> Currently, if bar->type is anything other than VPCI_BAR_MEM32, the >> memory type bits get set to PCI_BASE_ADDRESS_MEM_TYPE_64 in the returned >> value. This leads to the wrong memory type for, e.g. VPCI_BAR_EMPTY. >> Only set PCI_BASE_ADDRESS_MEM_TYPE_64 when the bar type is >> VPCI_BAR_MEM64_LO. > > I'm confused, VPCI_BAR_EMPTY shouldn't use guest_mem_bar_read() in the > first place, as its read handler should be vpci_read_val() instead. > > Is there something I'm missing from init_header()? > > if ( size == 0 ) > { > bars[i].type = VPCI_BAR_EMPTY; > > if ( !is_hwdom ) > { > rc = vpci_add_register(pdev->vpci, vpci_read_val, NULL, > reg, 4, (void *)0); > if ( rc ) > goto fail; > } > > continue; > } > > AFAICT guest_mem_bar_read() should only handle BAR types that are > either VPCI_BAR_MEM32, VPCI_BAR_MEM64_HI or VPCI_BAR_MEM64_LO, and > that seems to be correctly handled? Ah, you're right, sorry. I pulled this patch out of another series that I'm working on, but failed to realize that guest_mem_bar_read shouldn't be used for VPCI_BAR_EMPTY. Please disregard.
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index ef6c965c081c..493ca5de032d 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -668,8 +668,10 @@ static uint32_t cf_check guest_mem_bar_read(const struct pci_dev *pdev, } reg_val = bar->guest_addr; - reg_val |= bar->type == VPCI_BAR_MEM32 ? PCI_BASE_ADDRESS_MEM_TYPE_32 : - PCI_BASE_ADDRESS_MEM_TYPE_64; + if ( bar->type == VPCI_BAR_MEM32 ) + reg_val |= PCI_BASE_ADDRESS_MEM_TYPE_32; + else if ( bar->type == VPCI_BAR_MEM64_LO ) + reg_val |= PCI_BASE_ADDRESS_MEM_TYPE_64; reg_val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0; return reg_val;
Currently, if bar->type is anything other than VPCI_BAR_MEM32, the memory type bits get set to PCI_BASE_ADDRESS_MEM_TYPE_64 in the returned value. This leads to the wrong memory type for, e.g. VPCI_BAR_EMPTY. Only set PCI_BASE_ADDRESS_MEM_TYPE_64 when the bar type is VPCI_BAR_MEM64_LO. Fixes: 8c5bca70742c ("vpci/header: implement guest BAR register handlers") Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- xen/drivers/vpci/header.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) base-commit: 1645bb7997cb1eccb45235ab350872733c74b305