On 19.11.21 13:58, Jan Beulich wrote:
> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>
>> 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.
>>
>> ROM BAR is only handled for the hardware domain and for guest domains
>> there is a stub: at the moment PCI expansion ROM is x86 only, so it
>> might not be used by other architectures without emulating x86. Other
>> use-cases may include using that expansion ROM before Xen boots, hence
>> no emulation is needed in Xen itself. Or when a guest wants to use the
>> ROM code which seems to be rare.
> At least in the initial days of EFI there was the concept of EFI byte
> code, for ROM code to be compiled to such that it would be arch-
> independent. While I don't mean this to be an argument against leaving
> out ROM BAR handling for now, this may want mentioning here to avoid
> giving the impression that it's only x86 which might be affected by
> this deliberate omission.
I can put:
at the moment PCI expansion ROM handling is supported for x86 only
and it might not be used by other architectures without emulating x86.
>
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -408,6 +408,48 @@ static void bar_write(const struct pci_dev *pdev,
>> unsigned int reg,
>> pci_conf_write32(pdev->sbdf, reg, val);
>> }
>>
>> +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;
>> + val |= bar->type == VPCI_BAR_MEM32 ? PCI_BASE_ADDRESS_MEM_TYPE_32
>> + : 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);
>> +
>> + bar->guest_addr &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK;
>> +}
>> +
>> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
>> + void *data)
>> +{
>> + const struct vpci_bar *bar = data;
>> + bool hi = false;
>> +
>> + if ( bar->type == VPCI_BAR_MEM64_HI )
>> + {
>> + ASSERT(reg > PCI_BASE_ADDRESS_0);
>> + bar--;
>> + hi = true;
>> + }
>> +
>> + return bar->guest_addr >> (hi ? 32 : 0);
> I'm afraid "guest_addr" then isn't the best name; maybe "guest_val"?
> This would make more obvious that there is a meaningful difference
> from "addr" besides the guest vs host aspect.
I am not sure I can agree here:
bar->addr and bar->guest_addr make it clear what are these while
bar->addr and bar->guest_val would make someone go look for
additional information about what that val is for.
>
> Jan
>
Thank you,
Oleksandr
