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(-)

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;

base-commit: 1645bb7997cb1eccb45235ab350872733c74b305
-- 
2.47.1

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.

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