Re: [PATCH v10 07/17] vpci/header: implement guest BAR register handlers

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 10/12/23 18:09, Volodymyr Babchuk wrote:
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 33db58580c..40d1a07ada 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -477,6 +477,74 @@ static void cf_check bar_write(
>      pci_conf_write32(pdev->sbdf, reg, val);
>  }
> 
> +static void cf_check guest_bar_write(const struct pci_dev *pdev,
> +                                     unsigned int reg, uint32_t val, void 
> *data)
> +{
> +    struct vpci_bar *bar = data;
> +    bool hi = false;
> +    uint64_t guest_addr = bar->guest_addr;

This initialization is using the initial value of bar ...

> +
> +    if ( bar->type == VPCI_BAR_MEM64_HI )
> +    {
> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> +        bar--;

... but here bar is decremented ...

> +        hi = true;
> +    }
> +    else
> +    {
> +        val &= PCI_BASE_ADDRESS_MEM_MASK;
> +    }
> +

... so to ensure a consistent state, the initialization should be moved here.

> +    guest_addr &= ~(0xffffffffULL << (hi ? 32 : 0));
> +    guest_addr |= (uint64_t)val << (hi ? 32 : 0);
> +
> +    /* Allow guest to size BAR correctly */
> +    guest_addr &= ~(bar->size - 1);
> +
> +    /*
> +     * Make sure that the guest set address has the same page offset
> +     * as the physical address on the host or otherwise things won't work as
> +     * expected.
> +     */
> +    if ( guest_addr != ~(bar->size -1 )  &&

Should this sizing check only apply to the lower 32 bits, or take "hi" into 
account?

For reference, it may be helpful to see an example sequence of a Linux domU 
sizing a 64 bit BAR. I instrumented guest_bar_write() to print the raw/initial 
val argument, and guest_bar_read() to print the final reg_val:
(XEN) drivers/vpci/header.c:guest_bar_read  d1 0000:01:00.0 reg 0x10 val 
0xe0100004
(XEN) drivers/vpci/header.c:guest_bar_write d1 0000:01:00.0 reg 0x10 val 
0xffffffff
(XEN) drivers/vpci/header.c:guest_bar_read  d1 0000:01:00.0 reg 0x10 val 
0xffffc004
(XEN) drivers/vpci/header.c:guest_bar_write d1 0000:01:00.0 reg 0x10 val 
0xe0100004
(XEN) drivers/vpci/header.c:guest_bar_read  d1 0000:01:00.0 reg 0x14 val 0x0 
(hi)
(XEN) drivers/vpci/header.c:guest_bar_write d1 0000:01:00.0 reg 0x14 val 
0xffffffff (hi)
(XEN) drivers/vpci/header.c:guest_bar_read  d1 0000:01:00.0 reg 0x14 val 
0xffffffff (hi)
(XEN) drivers/vpci/header.c:guest_bar_write d1 0000:01:00.0 reg 0x14 val 0x0 
(hi)
(XEN) drivers/vpci/header.c:guest_bar_write d1 0000:01:00.0 reg 0x10 val 
0x23000004
(XEN) drivers/vpci/header.c:guest_bar_read  d1 0000:01:00.0 reg 0x10 val 
0x23000004
(XEN) drivers/vpci/header.c:guest_bar_write d1 0000:01:00.0 reg 0x14 val 0x0 
(hi)
(XEN) drivers/vpci/header.c:guest_bar_read  d1 0000:01:00.0 reg 0x14 val 0x0 
(hi)

> +         PAGE_OFFSET(guest_addr) != PAGE_OFFSET(bar->addr) )
> +    {
> +        gprintk(XENLOG_WARNING,
> +                "%pp: ignored BAR %zu write attempting to change page 
> offset\n",
> +                &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
> +        return;
> +    }
> +
> +    bar->guest_addr = guest_addr;
> +}