Re: [PATCH 5/9] vpci/header: Implement guest BAR register handlers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -400,12 +400,72 @@ static void bar_write(const struct pci_dev *pdev, 
> unsigned int reg,
>  static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
>                              uint32_t val, void *data)
>  {
> +    struct vpci_bar *bar = data;
> +    bool hi = false;
> +
> +    if ( bar->type == VPCI_BAR_MEM64_HI )
> +    {
> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> +        bar--;
> +        hi = true;
> +    }
> +    else
> +        val &= PCI_BASE_ADDRESS_MEM_MASK;
> +    bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
> +    bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);

What you store here is not the address that's going to be used, as
you don't mask off the low bits (to account for the BAR's size).
When a BAR gets written with all ones, all writable bits get these
ones stored. The address of the BAR, aiui, really changes to
(typically) close below 4Gb (in the case of a 32-bit BAR), which
is why memory / I/O decoding should be off while sizing BARs.
Therefore you shouldn't look for the specific "all writable bits
are ones" pattern (or worse, as you presently do, the "all bits
outside of the type specifier are ones" one) on the read path.
Instead mask the value appropriately here, and simply return back
the stored value from the read path.

>  }
>  
>  static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
>                                 void *data)
>  {
> -    return 0xffffffff;
> +    struct vpci_bar *bar = data;
> +    uint32_t val;
> +    bool hi = false;
> +
> +    switch ( bar->type )
> +    {
> +    case VPCI_BAR_MEM64_HI:
> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> +        bar--;
> +        hi = true;
> +        /* fallthrough */
> +    case VPCI_BAR_MEM64_LO:
> +    {

Please don't add braces to case blocks when they're not needed.

> +        if ( hi )
> +            val = bar->guest_addr >> 32;
> +        else
> +            val = bar->guest_addr & 0xffffffff;
> +        if ( (val & PCI_BASE_ADDRESS_MEM_MASK_32) ==  
> PCI_BASE_ADDRESS_MEM_MASK_32 )

This is wrong when falling through to here from VPCI_BAR_MEM64_HI:
All 32 bits need to be looked at. Yet as per the comment further
up I think it isn't right anyway to apply the mask here.

Also: Stray double blanks.

> +        {
> +            /* Guests detects BAR's properties and sizes. */
> +            if ( hi )
> +                val = bar->size >> 32;
> +            else
> +                val = 0xffffffff & ~(bar->size - 1);
> +        }
> +        if ( !hi )
> +        {
> +            val |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> +            val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0;
> +        }
> +        bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
> +        bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
> +        break;
> +    }
> +    case VPCI_BAR_MEM32:

Please separate non-fall-through case blocks by a blank line.

> @@ -522,6 +582,13 @@ static int add_bar_handlers(struct pci_dev *pdev, bool 
> is_hwdom)
>              if ( rc )
>                  return rc;
>          }
> +        /*
> +         * It is neither safe nor secure to initialize guest's view of the 
> BARs
> +         * with real values which are used by the hardware domain, so assign
> +         * all zeros to guest's view of the BARs, so the guest can perform
> +         * proper PCI device enumeration and assign BARs on its own.
> +         */
> +        bars[i].guest_addr = 0;

I'm afraid I don't understand the comment: Without memory decoding
enabled, the BARs are simple registers (with a few r/o bits).

> --- a/xen/include/xen/pci_regs.h
> +++ b/xen/include/xen/pci_regs.h
> @@ -103,6 +103,7 @@
>  #define  PCI_BASE_ADDRESS_MEM_TYPE_64        0x04    /* 64 bit address */
>  #define  PCI_BASE_ADDRESS_MEM_PREFETCH       0x08    /* prefetchable? */
>  #define  PCI_BASE_ADDRESS_MEM_MASK   (~0x0fUL)
> +#define  PCI_BASE_ADDRESS_MEM_MASK_32        (~0x0fU)

Please don't introduce an identical constant that's merely of
different type. (uint32_t)PCI_BASE_ADDRESS_MEM_MASK at the use
site (if actually still needed as per the comment above) would
seem more clear to me.

Jan