On Tue, Aug 29, 2023 at 11:19:43PM +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko <[email protected]>
>
> Add relevant vpci register handlers when assigning PCI device to a domain
> and remove those when de-assigning. This allows having different
> handlers for different domains, e.g. hwdom and other guests.
>
> Emulate guest BAR register values: this allows creating a guest view
> of the registers and emulates size and properties probe as it is done
> during PCI device enumeration by the guest.
>
> All empty, IO and ROM BARs for guests are emulated by returning 0 on
> reads and ignoring writes: this BARs are special with this respect as
> their lower bits have special meaning, so returning default ~0 on read
> may confuse guest OS.
>
> Signed-off-by: Oleksandr Andrushchenko <[email protected]>
> ---
> Since v9:
> - factored-out "fail" label introduction in init_bars()
> - replaced #ifdef CONFIG_X86 with IS_ENABLED()
> - do not pass bars[i] to empty_bar_read() handler
> - store guest's BAR address instead of guests BAR register view
> Since v6:
> - unify the writing of the PCI_COMMAND register on the
> error path into a label
> - do not introduce bar_ignore_access helper and open code
> - s/guest_bar_ignore_read/empty_bar_read
> - update error message in guest_bar_write
> - only setup empty_bar_read for IO if !x86
> Since v5:
> - make sure that the guest set address has the same page offset
> as the physical address on the host
> - remove guest_rom_{read|write} as those just implement the default
> behaviour of the registers not being handled
> - adjusted comment for struct vpci.addr field
> - add guest handlers for BARs which are not handled and will otherwise
> return ~0 on read and ignore writes. The BARs are special with this
> respect as their lower bits have special meaning, so returning ~0
> doesn't seem to be right
> Since v4:
> - updated commit message
> - s/guest_addr/guest_reg
> Since v3:
> - squashed two patches: dynamic add/remove handlers and guest BAR
> handler implementation
> - fix guest BAR read of the high part of a 64bit BAR (Roger)
> - add error handling to vpci_assign_device
> - s/dom%pd/%pd
> - blank line before return
> Since v2:
> - remove unneeded ifdefs for CONFIG_HAS_VPCI_GUEST_SUPPORT as more code
> has been eliminated from being built on x86
> Since v1:
> - constify struct pci_dev where possible
> - do not open code is_system_domain()
> - simplify some code3. simplify
> - use gdprintk + error code instead of gprintk
> - gate vpci_bar_{add|remove}_handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT,
> so these do not get compiled for x86
> - removed unneeded is_system_domain check
> - re-work guest read/write to be much simpler and do more work on write
> than read which is expected to be called more frequently
> - removed one too obvious comment
> ---
> xen/drivers/vpci/header.c | 131 +++++++++++++++++++++++++++++++++-----
> xen/include/xen/vpci.h | 3 +
> 2 files changed, 118 insertions(+), 16 deletions(-)
>
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index e58bbdf68d..e96d7b2b37 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -477,6 +477,72 @@ 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;
> +
> + if ( bar->type == VPCI_BAR_MEM64_HI )
> + {
> + ASSERT(reg > PCI_BASE_ADDRESS_0);
> + bar--;
> + hi = true;
> + }
> + else
> + {
> + val &= PCI_BASE_ADDRESS_MEM_MASK;
> + }
> +
> + guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
> + guest_addr |= (uint64_t)val << (hi ? 32 : 0);
> +
> + guest_addr &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK;
I don't think you need to mask out PCI_BASE_ADDRESS_MEM_MASK here, as
you already do it if bar->type != VPCI_BAR_MEM64_HI for val.
> +
> + /*
> + * 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 & (~PAGE_MASK)) != (bar->addr & ~PAGE_MASK) )
PAGE_OFFSET() would be easier to read.
> + {
> + 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;
> +}
> +
> +static uint32_t cf_check guest_bar_read(const struct pci_dev *pdev,
> + unsigned int reg, void *data)
> +{
> + const struct vpci_bar *bar = data;
> + uint32_t reg_val;
> +
> + if ( bar->type == VPCI_BAR_MEM64_HI )
> + {
> + ASSERT(reg > PCI_BASE_ADDRESS_0);
> + bar--;
> + return bar->guest_addr >> 32;
> + }
> +
> + reg_val = bar->guest_addr;
> + reg_val |= bar->type == VPCI_BAR_MEM32 ? PCI_BASE_ADDRESS_MEM_TYPE_32 :
> + PCI_BASE_ADDRESS_MEM_TYPE_64;
> + reg_val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0;
> +
> + return reg_val;
> +}
> +
> +static uint32_t cf_check empty_bar_read(const struct pci_dev *pdev,
> + unsigned int reg, void *data)
> +{
> + return 0;
> +}
If we are going to gain a lot of helpers that return a fixed value it
might be worthwhile to introduce a helper that returns what gets
passed as 'data'. Let's leave it as you propose for now.
> +
> static void cf_check rom_write(
> const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
> {
> @@ -537,6 +603,7 @@ static int cf_check init_bars(struct pci_dev *pdev)
> struct vpci_header *header = &pdev->vpci->header;
> struct vpci_bar *bars = header->bars;
> int rc;
> + bool is_hwdom = is_hardware_domain(pdev->domain);
>
> ASSERT(rw_is_locked(&pdev->domain->pci_lock));
>
> @@ -578,8 +645,10 @@ static int cf_check init_bars(struct pci_dev *pdev)
> if ( i && bars[i - 1].type == VPCI_BAR_MEM64_LO )
> {
> bars[i].type = VPCI_BAR_MEM64_HI;
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write,
> reg,
> - 4, &bars[i]);
> + rc = vpci_add_register(pdev->vpci,
> + is_hwdom ? vpci_hw_read32 :
> guest_bar_read,
> + is_hwdom ? bar_write : guest_bar_write,
> + reg, 4, &bars[i]);
> if ( rc )
> goto fail;
> continue;
> @@ -589,6 +658,15 @@ static int cf_check init_bars(struct pci_dev *pdev)
> if ( (val & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO )
> {
> bars[i].type = VPCI_BAR_IO;
> +
> + if ( !IS_ENABLED(CONFIG_X86) && !is_hwdom )
> + {
> + rc = vpci_add_register(pdev->vpci, empty_bar_read, NULL,
> + reg, 4, NULL);
> + if ( rc )
> + goto fail;
For consistency you should also set bars[i].type = VPCI_BAR_EMPTY
here.
> + }
> +
> continue;
> }
> if ( (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> @@ -605,6 +683,15 @@ static int cf_check init_bars(struct pci_dev *pdev)
> if ( size == 0 )
> {
> bars[i].type = VPCI_BAR_EMPTY;
> +
> + if ( !is_hwdom )
> + {
> + rc = vpci_add_register(pdev->vpci, empty_bar_read, NULL,
> + reg, 4, NULL);
> + if ( rc )
> + goto fail;
> + }
> +
> continue;
> }
>
> @@ -612,28 +699,40 @@ static int cf_check init_bars(struct pci_dev *pdev)
> bars[i].size = size;
> bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
>
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, 4,
> - &bars[i]);
> + rc = vpci_add_register(pdev->vpci,
> + is_hwdom ? vpci_hw_read32 : guest_bar_read,
> + is_hwdom ? bar_write : guest_bar_write,
> + reg, 4, &bars[i]);
> if ( rc )
> goto fail;
> }
>
> - /* Check expansion ROM. */
> - rc = pci_size_mem_bar(pdev->sbdf, rom_reg, &addr, &size, PCI_BAR_ROM);
> - if ( rc > 0 && size )
> + /* TODO: Check expansion ROM, we do not handle ROM for guests for now. */
> + if ( is_hwdom )
> {
> - struct vpci_bar *rom = &header->bars[num_bars];
> + rc = pci_size_mem_bar(pdev->sbdf, rom_reg, &addr, &size,
> PCI_BAR_ROM);
> + if ( rc > 0 && size )
> + {
> + struct vpci_bar *rom = &header->bars[num_bars];
>
> - rom->type = VPCI_BAR_ROM;
> - rom->size = size;
> - rom->addr = addr;
> - header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) &
> - PCI_ROM_ADDRESS_ENABLE;
> + rom->type = VPCI_BAR_ROM;
> + rom->size = size;
> + rom->addr = addr;
> + header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) &
> + PCI_ROM_ADDRESS_ENABLE;
>
> - rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> rom_reg,
> - 4, rom);
> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> + rom_reg, 4, rom);
> + if ( rc )
> + rom->type = VPCI_BAR_EMPTY;
> + }
> + }
> + else
> + {
> + rc = vpci_add_register(pdev->vpci, empty_bar_read, NULL,
> + rom_reg, 4, NULL);
You should set the BAR type to VPCI_BAR_EMPTY here.
Thanks, Roger.
